From 9dc26c2ffb88fa900ea92a649d7f6cc7438d1a14 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Fri, 18 Jan 2019 17:53:01 +0700 Subject: [PATCH] ConditionalCanBeOptionalInspection: support effectively unqualified fields --- .../ConditionalCanBeOptionalInspection.java | 57 ++++++++++++------- .../conditionalCanBeOptional/afterField.java | 11 ++++ .../conditionalCanBeOptional/afterField2.java | 11 ++++ .../afterFieldOtherAndThis.java | 11 ++++ .../conditionalCanBeOptional/beforeField.java | 9 +++ .../beforeField2.java | 9 +++ .../beforeFieldOther.java | 9 +++ .../beforeFieldOtherAndThis.java | 9 +++ 8 files changed, 105 insertions(+), 21 deletions(-) create mode 100644 java/java-tests/testData/inspection/conditionalCanBeOptional/afterField.java create mode 100644 java/java-tests/testData/inspection/conditionalCanBeOptional/afterField2.java create mode 100644 java/java-tests/testData/inspection/conditionalCanBeOptional/afterFieldOtherAndThis.java create mode 100644 java/java-tests/testData/inspection/conditionalCanBeOptional/beforeField.java create mode 100644 java/java-tests/testData/inspection/conditionalCanBeOptional/beforeField2.java create mode 100644 java/java-tests/testData/inspection/conditionalCanBeOptional/beforeFieldOther.java create mode 100644 java/java-tests/testData/inspection/conditionalCanBeOptional/beforeFieldOtherAndThis.java diff --git a/java/java-impl/src/com/intellij/codeInspection/ConditionalCanBeOptionalInspection.java b/java/java-impl/src/com/intellij/codeInspection/ConditionalCanBeOptionalInspection.java index 94c4a6767029..cf1d4fb824f5 100644 --- a/java/java-impl/src/com/intellij/codeInspection/ConditionalCanBeOptionalInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/ConditionalCanBeOptionalInspection.java @@ -1,6 +1,7 @@ // Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file. package com.intellij.codeInspection; +import com.intellij.codeInsight.ExpressionUtil; import com.intellij.codeInsight.Nullability; import com.intellij.codeInspection.dataFlow.NullabilityUtil; import com.intellij.codeInspection.util.LambdaGenerationUtil; @@ -9,21 +10,20 @@ import com.intellij.codeInspection.util.OptionalUtil; import com.intellij.openapi.project.Project; import com.intellij.psi.*; import com.intellij.psi.codeStyle.JavaCodeStyleManager; -import com.intellij.psi.search.LocalSearchScope; -import com.intellij.psi.search.searches.ReferencesSearch; +import com.intellij.psi.codeStyle.VariableKind; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.psi.util.PsiUtil; -import com.siyeh.ig.psiutils.CommentTracker; -import com.siyeh.ig.psiutils.ExpressionUtils; -import com.siyeh.ig.psiutils.TypeUtils; -import com.siyeh.ig.psiutils.VariableAccessUtils; +import com.siyeh.ig.psiutils.*; import org.jetbrains.annotations.Contract; import org.jetbrains.annotations.Nls; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import java.util.List; import java.util.Objects; +import static com.intellij.util.ObjectUtils.tryCast; + public class ConditionalCanBeOptionalInspection extends AbstractBaseJavaLocalInspectionTool { @NotNull @Override @@ -42,8 +42,12 @@ public class ConditionalCanBeOptionalInspection extends AbstractBaseJavaLocalIns if (!ExpressionUtils.isSafelyRecomputableExpression(nullBranch) && !LambdaGenerationUtil.canBeUncheckedLambda(nullBranch, variable::equals)) { return; } - if (!VariableAccessUtils.variableIsUsed(variable, notNullBranch) || - !LambdaGenerationUtil.canBeUncheckedLambda(notNullBranch, variable::equals)) { + List references = VariableAccessUtils.getVariableReferences(variable, notNullBranch); + if (references.isEmpty() || + variable instanceof PsiField && references.stream().noneMatch(ExpressionUtil::isEffectivelyUnqualified)) { + return; + } + if (!LambdaGenerationUtil.canBeUncheckedLambda(notNullBranch, variable::equals)) { return; } if (!areTypesCompatible(nullBranch, notNullBranch)) return; @@ -97,30 +101,39 @@ public class ConditionalCanBeOptionalInspection extends AbstractBaseJavaLocalIns PsiVariable variable = ternaryNullCheck.myVariable; String name = variable.getName(); if (name == null) return; - String inLambdaName = JavaCodeStyleManager.getInstance(project).suggestUniqueVariableName(name, ternary, true); + String inLambdaName = + new VariableNameGenerator(ternary, VariableKind.PARAMETER).byName(name).byType(variable.getType()).generate(true); PsiExpression nullBranch = ternaryNullCheck.myNullBranch; PsiExpression notNullBranch = ternaryNullCheck.myNotNullBranch; PsiElementFactory factory = JavaPsiFacade.getElementFactory(project); - for (PsiReference reference : ReferencesSearch.search(variable, new LocalSearchScope(nullBranch)).findAll()) { - if (reference instanceof PsiReferenceExpression) { - PsiElement result = ((PsiReferenceExpression)reference).replace(factory.createExpressionFromText("null", ternary)); + CommentTracker ct = new CommentTracker(); + for (PsiReferenceExpression reference : VariableAccessUtils.getVariableReferences(variable, nullBranch)) { + if (ExpressionUtil.isEffectivelyUnqualified(reference)) { + PsiElement result = ct.replace(reference, "null"); if (nullBranch == reference) { nullBranch = (PsiExpression)result; } } } - for (PsiReference reference : ReferencesSearch.search(variable, new LocalSearchScope(notNullBranch)).findAll()) { - if (reference instanceof PsiReferenceExpression) { - ExpressionUtils.bindReferenceTo((PsiReferenceExpression)reference, inLambdaName); + String origExpression = null; + for (PsiReferenceExpression reference : VariableAccessUtils.getVariableReferences(variable, notNullBranch)) { + PsiExpression qualifier = reference.getQualifierExpression(); + if (qualifier != null) { + if (!ExpressionUtil.isEffectivelyUnqualified(reference)) continue; + if (origExpression == null) { + origExpression = ct.text(reference); + } + ct.delete(qualifier); } + ExpressionUtils.bindReferenceTo(reference, inLambdaName); } - CommentTracker ct = new CommentTracker(); PsiLambdaExpression trueLambda = (PsiLambdaExpression)factory.createExpressionFromText("(" + variable.getType().getCanonicalText() + " " + inLambdaName + ")->" + ct.text(notNullBranch), ternary); PsiParameter lambdaParameter = trueLambda.getParameterList().getParameters()[0]; PsiExpression trueBody = Objects.requireNonNull((PsiExpression)trueLambda.getBody()); - String replacement = OptionalRefactoringUtil.generateOptionalUnwrap(CommonClassNames.JAVA_UTIL_OPTIONAL + ".ofNullable(" + name + ")", + String ofNullableText = CommonClassNames.JAVA_UTIL_OPTIONAL + ".ofNullable(" + (origExpression == null ? name : origExpression) + ")"; + String replacement = OptionalRefactoringUtil.generateOptionalUnwrap(ofNullableText, lambdaParameter, trueBody, ct.markUnchanged(nullBranch), ternary.getType(), !ExpressionUtils.isSafelyRecomputableExpression(nullBranch)); PsiElement result = ct.replaceAndRestoreComments(ternary, replacement); @@ -146,12 +159,14 @@ public class ConditionalCanBeOptionalInspection extends AbstractBaseJavaLocalIns if (ternary == null) return null; PsiExpression condition = ternary.getCondition(); boolean isNull = true; - PsiVariable variable = ExpressionUtils.getVariableFromNullComparison(condition, true); - if (variable == null) { + PsiReferenceExpression ref = ExpressionUtils.getReferenceExpressionFromNullComparison(condition, true); + if (ref == null) { isNull = false; - variable = ExpressionUtils.getVariableFromNullComparison(condition, false); + ref = ExpressionUtils.getReferenceExpressionFromNullComparison(condition, false); } - if (variable == null || variable instanceof PsiField) return null; + if (ref == null) return null; + PsiVariable variable = tryCast(ref.resolve(), PsiVariable.class); + if (variable == null || (variable instanceof PsiField && !ExpressionUtil.isEffectivelyUnqualified(ref))) return null; PsiExpression nullBranch = isNull ? ternary.getThenExpression() : ternary.getElseExpression(); PsiExpression notNullBranch = isNull ? ternary.getElseExpression() : ternary.getThenExpression(); if (nullBranch == null || notNullBranch == null) { diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/afterField.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterField.java new file mode 100644 index 000000000000..991e7c981005 --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterField.java @@ -0,0 +1,11 @@ +// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING" + +import java.util.Optional; + +class Test { + String foo; + + String select() { + return Optional.ofNullable(this.foo).map(String::trim).orElse(""); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/afterField2.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterField2.java new file mode 100644 index 000000000000..78bb11a8edcb --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterField2.java @@ -0,0 +1,11 @@ +// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING" + +import java.util.Optional; + +class Test { + String foo; + + String select() { + return Optional.ofNullable(foo).map(String::trim).orElse(""); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/afterFieldOtherAndThis.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterFieldOtherAndThis.java new file mode 100644 index 000000000000..399cabf38172 --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterFieldOtherAndThis.java @@ -0,0 +1,11 @@ +// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING" + +import java.util.Optional; + +class Test { + String foo; + + String select(Test other) { + return Optional.ofNullable(foo).map(s -> other.foo.trim() + s.trim()).orElseGet(() -> other.foo + null); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeField.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeField.java new file mode 100644 index 000000000000..3f1663a9af14 --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeField.java @@ -0,0 +1,9 @@ +// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING" + +class Test { + String foo; + + String select() { + return foo != null ? this.foo.trim() : ""; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeField2.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeField2.java new file mode 100644 index 000000000000..692e6cd7147b --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeField2.java @@ -0,0 +1,9 @@ +// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING" + +class Test { + String foo; + + String select() { + return this.foo != null ? foo.trim() : ""; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeFieldOther.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeFieldOther.java new file mode 100644 index 000000000000..cc8e3c1d0800 --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeFieldOther.java @@ -0,0 +1,9 @@ +// "Replace with Optional.ofNullable() chain" "false" + +class Test { + String foo; + + String select(Test other) { + return this.foo != null ? other.foo.trim() : ""; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeFieldOtherAndThis.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeFieldOtherAndThis.java new file mode 100644 index 000000000000..dc95de40f74f --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeFieldOtherAndThis.java @@ -0,0 +1,9 @@ +// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING" + +class Test { + String foo; + + String select(Test other) { + return this.foo != null ? other.foo.trim() + foo.trim() : other.foo+this.foo; + } +} \ No newline at end of file