diff --git a/java/java-impl/src/META-INF/JavaPlugin.xml b/java/java-impl/src/META-INF/JavaPlugin.xml index 298f82d2043c..cfd131f7f73a 100644 --- a/java/java-impl/src/META-INF/JavaPlugin.xml +++ b/java/java-impl/src/META-INF/JavaPlugin.xml @@ -516,6 +516,11 @@ groupKey="group.names.code.style.issues" enabledByDefault="true" level="WARNING" implementationClass="com.intellij.codeInspection.OptionalAssignedToNullInspection" displayName="Null value for Optional type"/> + " + ct.text(notNullBranch), ternary); + PsiParameter lambdaParameter = trueLambda.getParameterList().getParameters()[0]; + PsiExpression trueBody = (PsiExpression)trueLambda.getBody(); + String replacement = OptionalUtil.generateOptionalUnwrap(CommonClassNames.JAVA_UTIL_OPTIONAL + ".ofNullable(" + name + ")", + lambdaParameter, trueBody, ct.markUnchanged(nullBranch), ternary.getType(), + !ExpressionUtils.isSimpleExpression(nullBranch)); + PsiElement result = ct.replaceAndRestoreComments(ternary, replacement); + JavaCodeStyleManager.getInstance(project).shortenClassReferences(result); + LambdaCanBeMethodReferenceInspection.replaceAllLambdasWithMethodReferences(result); + } + } + + private static class TernaryNullCheck { + final PsiVariable myVariable; + final PsiExpression myNullBranch; + final PsiExpression myNotNullBranch; + + public TernaryNullCheck(PsiVariable variable, PsiExpression nullBranch, PsiExpression notNullBranch) { + myVariable = variable; + myNullBranch = nullBranch; + myNotNullBranch = notNullBranch; + } + + @Contract("null -> null") + @Nullable + public static TernaryNullCheck from(@Nullable PsiConditionalExpression ternary) { + if (ternary == null) return null; + PsiExpression condition = ternary.getCondition(); + boolean isNull = true; + PsiVariable variable = ExpressionUtils.getVariableFromNullComparison(condition, true); + if (variable == null) { + isNull = false; + variable = ExpressionUtils.getVariableFromNullComparison(condition, false); + } + if (variable == null || variable instanceof PsiField) return null; + PsiExpression nullBranch = isNull ? ternary.getThenExpression() : ternary.getElseExpression(); + PsiExpression notNullBranch = isNull ? ternary.getElseExpression() : ternary.getThenExpression(); + if (nullBranch == null || notNullBranch == null) { + return null; + } + return new TernaryNullCheck(variable, nullBranch, notNullBranch); + } + } +} diff --git a/java/java-impl/src/com/intellij/codeInspection/util/OptionalUtil.java b/java/java-impl/src/com/intellij/codeInspection/util/OptionalUtil.java index 0cd064b8fed2..cbd3319b0aa2 100644 --- a/java/java-impl/src/com/intellij/codeInspection/util/OptionalUtil.java +++ b/java/java-impl/src/com/intellij/codeInspection/util/OptionalUtil.java @@ -237,7 +237,7 @@ public class OptionalUtil { TypeConversionUtil.isAssignable(type, exprType)) { if (falseExpression == null) return ""; PsiType falseType = falseExpression.getType(); - if (falseType != null && falseType.isAssignableFrom(exprType)) return ""; + if (falseType != null && (falseType.isAssignableFrom(exprType) || falseType.equals(PsiType.NULL))) return ""; } return "<" + type.getCanonicalText() + ">"; } diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/afterMapping.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterMapping.java new file mode 100644 index 000000000000..2147b5e04f89 --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterMapping.java @@ -0,0 +1,9 @@ +// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING" + +import java.util.Optional; + +class Test { + String select(String foo) { + return Optional.ofNullable(foo).map(String::trim).orElse(""); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/afterMappingNullable.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterMappingNullable.java new file mode 100644 index 000000000000..cd2e1168292e --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterMappingNullable.java @@ -0,0 +1,13 @@ +// "Replace with Optional.ofNullable() chain (may change semantics)" "INFORMATION" + +import java.util.Optional; + +class Test { + String trim(String s) { + return s.isEmpty() ? null : s.trim(); + } + + String select(String foo) { + return Optional.ofNullable(foo).map(this::trim).orElse(""); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/afterMappingNullableOk.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterMappingNullableOk.java new file mode 100644 index 000000000000..c1e7988893dd --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterMappingNullableOk.java @@ -0,0 +1,13 @@ +// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING" + +import java.util.Optional; + +class Test { + String trim(String s) { + return s.isEmpty() ? null : s.trim(); + } + + String select(String foo) { + return Optional.ofNullable(foo).map(this::trim).orElse(null); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/afterOptionalReturn.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterOptionalReturn.java new file mode 100644 index 000000000000..bd7e65c9639a --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterOptionalReturn.java @@ -0,0 +1,16 @@ +// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING" + +import java.util.Optional; + +class Test { + interface V {} + + interface Type { + V getValue(); + } + + // IDEA-179273 + public Optional foo(Type arg) { + return Optional.ofNullable(arg).map(Type::getValue); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/afterOrElseGet.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterOrElseGet.java new file mode 100644 index 000000000000..aa6891f78e9b --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterOrElseGet.java @@ -0,0 +1,13 @@ +// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING" + +import java.util.Optional; + +class Test { + String getDefault() { + return ""; + } + + String select(String foo) { + return Optional.ofNullable(foo).map(String::trim).orElseGet(this::getDefault); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/afterSimple.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterSimple.java new file mode 100644 index 000000000000..87aef5ee62cc --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/afterSimple.java @@ -0,0 +1,9 @@ +// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING" + +import java.util.Optional; + +class Test { + String select(String foo, String bar) { + return Optional.ofNullable(foo).orElse(bar); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeMapping.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeMapping.java new file mode 100644 index 000000000000..5ec2c2ee429d --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeMapping.java @@ -0,0 +1,7 @@ +// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING" + +class Test { + String select(String foo) { + return foo != null ? foo.trim() : ""; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeMappingNullable.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeMappingNullable.java new file mode 100644 index 000000000000..add0b83cc76e --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeMappingNullable.java @@ -0,0 +1,11 @@ +// "Replace with Optional.ofNullable() chain (may change semantics)" "INFORMATION" + +class Test { + String trim(String s) { + return s.isEmpty() ? null : s.trim(); + } + + String select(String foo) { + return foo != null ? trim(foo) : ""; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeMappingNullableOk.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeMappingNullableOk.java new file mode 100644 index 000000000000..5397a3e43c0b --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeMappingNullableOk.java @@ -0,0 +1,11 @@ +// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING" + +class Test { + String trim(String s) { + return s.isEmpty() ? null : s.trim(); + } + + String select(String foo) { + return foo != null ? trim(foo) : null; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeOptionalReturn.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeOptionalReturn.java new file mode 100644 index 000000000000..5b779d5091e2 --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeOptionalReturn.java @@ -0,0 +1,16 @@ +// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING" + +import java.util.Optional; + +class Test { + interface V {} + + interface Type { + V getValue(); + } + + // IDEA-179273 + public Optional foo(Type arg) { + return arg == null ? Optional.empty() : Optional.of(arg.getValue()); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeOrElseGet.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeOrElseGet.java new file mode 100644 index 000000000000..71559426922a --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeOrElseGet.java @@ -0,0 +1,11 @@ +// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING" + +class Test { + String getDefault() { + return ""; + } + + String select(String foo) { + return foo == null ? getDefault() : foo.trim(); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeSimple.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeSimple.java new file mode 100644 index 000000000000..2ab73eca4fcc --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeSimple.java @@ -0,0 +1,7 @@ +// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING" + +class Test { + String select(String foo, String bar) { + return foo == null ? bar : foo; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeUnused.java b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeUnused.java new file mode 100644 index 000000000000..d57d916e9003 --- /dev/null +++ b/java/java-tests/testData/inspection/conditionalCanBeOptional/beforeUnused.java @@ -0,0 +1,7 @@ +// "Replace with Optional.ofNullable() chain" "false" + +class Test { + String select(String foo, String bar) { + return foo == null ? bar : bar.trim(); + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/ConditionalCanBeOptionalInspectionTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/ConditionalCanBeOptionalInspectionTest.java new file mode 100644 index 000000000000..5c64f875d084 --- /dev/null +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/ConditionalCanBeOptionalInspectionTest.java @@ -0,0 +1,27 @@ +// 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.java.codeInspection; + +import com.intellij.codeInsight.daemon.quickFix.LightQuickFixParameterizedTestCase; +import com.intellij.codeInspection.ConditionalCanBeOptionalInspection; +import com.intellij.codeInspection.LocalInspectionTool; +import org.jetbrains.annotations.NotNull; + +/** + * @author Tagir Valeev + */ +public class ConditionalCanBeOptionalInspectionTest extends LightQuickFixParameterizedTestCase { + @NotNull + @Override + protected LocalInspectionTool[] configureLocalInspectionTools() { + return new LocalInspectionTool[]{new ConditionalCanBeOptionalInspection()}; + } + + public void test() { + doAllTests(); + } + + @Override + protected String getBasePath() { + return "/inspection/conditionalCanBeOptional/"; + } +} diff --git a/resources-en/src/inspectionDescriptions/ConditionalCanBeOptional.html b/resources-en/src/inspectionDescriptions/ConditionalCanBeOptional.html new file mode 100644 index 000000000000..a2fccd9d1dc1 --- /dev/null +++ b/resources-en/src/inspectionDescriptions/ConditionalCanBeOptional.html @@ -0,0 +1,16 @@ + + +Suggests to replace a null-check condition with an Optional chain. E.g. +
return str == null ? "" : str.trim();
+Could be rewritten as +
return Optional.ofNullable(str).map(String::trim).orElse("");
+

While the replacement is not always shorter, this could be a helpful step for further refactoring + (e.g. changing the method return value to an Optional).

+

Note that when not-null branch of the condition returns null, the corresponding mapping step will produce an empty Optional +possibly changing the semantics. If it cannot be statically proven that semantics will be preserved, quick-fix action name +will contain "(may change semantics)" notice and inspection highlighting will be turned off.

+ +

This inspection only reports if the project or module is configured to use a language level of 8 or higher.

+

New in 2018.1

+ + \ No newline at end of file