From ba33ae8bcf862537dbc92afa645b4c52fc2d693c Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Thu, 11 Feb 2021 12:10:58 +0700 Subject: [PATCH] [java] JSpecify: initial strict mode support GitOrigin-RevId: a387d37ac8924b8e63847eb54458424f5f690d9a --- .../messages/JavaAnalysisBundle.properties | 1 + .../dataFlow/DataFlowInstructionVisitor.java | 2 +- .../dataFlow/NullabilityProblemKind.java | 39 ++++++++++++++++--- .../FinalFieldNotDuringInitialization.java | 6 +-- .../dataFlow/fixture/ParanoidMode.java | 6 +-- .../JSpecifyAnnotationTest.java | 34 ++++++++-------- 6 files changed, 59 insertions(+), 29 deletions(-) diff --git a/java/java-analysis-api/resources/messages/JavaAnalysisBundle.properties b/java/java-analysis-api/resources/messages/JavaAnalysisBundle.properties index 3be5341d19ff..82c86fec1378 100644 --- a/java/java-analysis-api/resources/messages/JavaAnalysisBundle.properties +++ b/java/java-analysis-api/resources/messages/JavaAnalysisBundle.properties @@ -85,6 +85,7 @@ dataflow.message.unreachable.switch.label=Switch label #ref #loc is dataflow.message.constant.expression=Result of #ref #loc is always ''{0}'' dataflow.message.constant.value=Value #ref #loc is always ''{0}'' dataflow.method.fails.with.null.argument=Method will throw an exception when parameter is null +dataflow.message.unknown.nullability=\ (unknown nullability) dataflow.not.precise={0} is complex: data flow results could be imprecise dataflow.too.complex={0} is too complex to analyze by data flow algorithm diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java index 8a4be253cb3a..4ee75a753564 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java @@ -153,7 +153,7 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { } StreamEx> problems() { - return StreamEx.ofKeys(myStateInfos, StateInfo::shouldReport); + return EntryStream.of(myStateInfos).filterValues(StateInfo::shouldReport).mapKeyValue((np, si) -> si.unknown ? np.makeUnknown() : np); } public Map> getArrayStoreProblems() { diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullabilityProblemKind.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullabilityProblemKind.java index 526f58f4c8e9..e4ea6efd4706 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullabilityProblemKind.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullabilityProblemKind.java @@ -117,7 +117,7 @@ public final class NullabilityProblemKind { @Contract("null, _ -> null") @Nullable public final NullabilityProblem problem(@Nullable T anchor, @Nullable PsiExpression expression) { - return anchor == null || this == noProblem ? null : new NullabilityProblem<>(this, anchor, expression); + return anchor == null || this == noProblem ? null : new NullabilityProblem<>(this, anchor, expression, false); } /** @@ -430,7 +430,12 @@ public final class NullabilityProblemKind { if (innerClassNPE == kind || callNPE == kind || arrayAccessNPE == kind || fieldAccessNPE == kind) { // Qualifier-problems are reported on top-expression level for now as it's rare case to have // something complex in qualifier and we highlight not the qualifier itself, but something else (e.g. called method name) - unchanged.add(problem.withExpression(findTopExpression(expression))); + boolean unknown = problem.hasUnknownNullability(); + problem = problem.withExpression(findTopExpression(expression)); + if (unknown) { + problem = problem.makeUnknown(); + } + unchanged.add(problem); continue; } // Merge ternary problems reported for both branches into single problem @@ -450,7 +455,11 @@ public final class NullabilityProblemKind { NullabilityProblem otherBranchProblem = expressionToProblem.remove(otherBranch); if (otherBranchProblem != null) { expression = ternary; + boolean unknown = problem.hasUnknownNullability() && otherBranchProblem.hasUnknownNullability(); problem = problem.withExpression(ternary); + if (unknown) { + problem = problem.makeUnknown(); + } continue; } } @@ -499,11 +508,16 @@ public final class NullabilityProblemKind { private final @NotNull NullabilityProblemKind myKind; private final @NotNull T myAnchor; private final @Nullable PsiExpression myDereferencedExpression; + private final boolean myFromUnknown; - NullabilityProblem(@NotNull NullabilityProblemKind kind, @NotNull T anchor, @Nullable PsiExpression dereferencedExpression) { + NullabilityProblem(@NotNull NullabilityProblemKind kind, + @NotNull T anchor, + @Nullable PsiExpression dereferencedExpression, + boolean unknown) { myKind = kind; myAnchor = anchor; myDereferencedExpression = dereferencedExpression; + myFromUnknown = unknown; } @NotNull @@ -528,18 +542,27 @@ public final class NullabilityProblemKind { return myDereferencedExpression; } + /** + * @return true if dereferenced expression has unknown nullability + * (reported in {@link DataFlowInspectionBase#TREAT_UNKNOWN_MEMBERS_AS_NULLABLE} mode). + */ + public boolean hasUnknownNullability() { + return myFromUnknown; + } + @NotNull public @InspectionMessage String getMessage(Map expressions) { if (myKind.myAlwaysNullMessage == null || myKind.myNormalMessage == null) { throw new IllegalStateException("This problem kind has no message associated: " + myKind); } + String suffix = myFromUnknown ? JavaAnalysisBundle.message("dataflow.message.unknown.nullability") : ""; PsiExpression expression = PsiUtil.skipParenthesizedExprDown(getDereferencedExpression()); if (expression != null) { if (ExpressionUtils.isNullLiteral(expression) || expressions.get(expression) == DataFlowInspectionBase.ConstantResult.NULL) { - return myKind.myAlwaysNullMessage.get(); + return myKind.myAlwaysNullMessage.get() + suffix; } } - return myKind.myNormalMessage.get(); + return myKind.myNormalMessage.get() + suffix; } @NotNull @@ -567,7 +590,11 @@ public final class NullabilityProblemKind { } public NullabilityProblem withExpression(PsiExpression expression) { - return expression == myDereferencedExpression ? this : new NullabilityProblem<>(myKind, myAnchor, expression); + return expression == myDereferencedExpression ? this : new NullabilityProblem<>(myKind, myAnchor, expression, false); + } + + public NullabilityProblem makeUnknown() { + return new NullabilityProblem<>(myKind, myAnchor, myDereferencedExpression, true); } } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/FinalFieldNotDuringInitialization.java b/java/java-tests/testData/inspection/dataFlow/fixture/FinalFieldNotDuringInitialization.java index b7354ef69ddc..6adf82977826 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/FinalFieldNotDuringInitialization.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/FinalFieldNotDuringInitialization.java @@ -95,7 +95,7 @@ class Test6 extends BadSuper { } public Integer someLength() { - return something.length(); + return something.length(); } protected void overrideableMethod() { @@ -107,7 +107,7 @@ class Test7 extends BadSuper { private final String something = new String("something"); protected void overrideableMethod() { - something.length(); + something.length(); } } @@ -124,6 +124,6 @@ class Test8 { void other() { System.out.println(s.hashCode()); - System.out.println(s2.hashCode()); + System.out.println(s2.hashCode()); } } \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/ParanoidMode.java b/java/java-tests/testData/inspection/dataFlow/fixture/ParanoidMode.java index c359b1760fc2..e514a4be5ca8 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/ParanoidMode.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/ParanoidMode.java @@ -4,15 +4,15 @@ class Test { Object o; void field() { - o.hashCode(); + o.hashCode(); } void parameter(Object o) { - o.hashCode(); + o.hashCode(); } void callUnknownMethod() { - unknownObject().hashCode(); + unknownObject().hashCode(); } void callNotNullMethod() { diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/JSpecifyAnnotationTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/JSpecifyAnnotationTest.java index 9f47a501a79a..883ddfdd1fc6 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/JSpecifyAnnotationTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/JSpecifyAnnotationTest.java @@ -130,6 +130,7 @@ public class JSpecifyAnnotationTest extends LightJavaCodeInsightFixtureTestCase Map actual = new LinkedHashMap<>(); var dfaInspection = new JSpecifyDataFlowInspection(actual); + dfaInspection.TREAT_UNKNOWN_MEMBERS_AS_NULLABLE = true; var nullableStuffInspection = new JSpecifyNullableStuffInspection(actual); var notNullFieldNotInitializedInspection = new JSpecifyNotNullFieldNotInitializedInspection(actual); List inspections = List.of(dfaInspection, nullableStuffInspection, notNullFieldNotInitializedInspection); @@ -208,24 +209,25 @@ public class JSpecifyAnnotationTest extends LightJavaCodeInsightFixtureTestCase List> problems, Map expressions) { for (NullabilityProblemKind.NullabilityProblem problem : problems) { - PsiExpression expression = problem.getDereferencedExpression(); - if (expression != null) { - if (problem.getKind() == NullabilityProblemKind.nullableReturn) { - PsiType returnType = PsiTypesUtil.getMethodReturnType(expression); - Nullability nullability = DfaPsiUtil.getTypeNullability(returnType); - if (nullability == Nullability.UNKNOWN) { - warnings.put(expression, "jspecify_nullness_not_enough_information"); - } - if (nullability == Nullability.NOT_NULL) { - warnings.put(expression, "jspecify_nullness_mismatch"); - } - continue; - } - else if (problem.getKind() == NullabilityProblemKind.passingToNonAnnotatedParameter) continue; - warnings.put(expression, "jspecify_nullness_mismatch"); + String warning = getJSpecifyWarning(problem); + if (warning != null) { + warnings.put(problem.getDereferencedExpression(), warning); } } } + + private static @Nullable String getJSpecifyWarning(NullabilityProblemKind.NullabilityProblem problem) { + PsiExpression expression = problem.getDereferencedExpression(); + if (expression == null) return null; + if (problem.getKind() == NullabilityProblemKind.passingToNonAnnotatedParameter) return null; + if (problem.getKind() == NullabilityProblemKind.nullableReturn) { + PsiType returnType = PsiTypesUtil.getMethodReturnType(expression); + Nullability nullability = DfaPsiUtil.getTypeNullability(returnType); + if (nullability == Nullability.NULLABLE) return null; + if (nullability == Nullability.UNKNOWN) return "jspecify_nullness_not_enough_information"; + } + return problem.hasUnknownNullability() ? "jspecify_nullness_not_enough_information" : "jspecify_nullness_mismatch"; + } } String getActualText(Map actual, String stripped) { @@ -236,7 +238,7 @@ public class JSpecifyAnnotationTest extends LightJavaCodeInsightFixtureTestCase .grouping(TreeMap::new, Collectors.toList()); for (String str : stripped.split("\n", -1)) { int endPos = pos + str.length() + 1; - String warnings = StreamEx.of(map.subMap(pos, endPos).values()).flatMap(List::stream).joining(" & "); + String warnings = StreamEx.of(map.subMap(pos, endPos).values()).flatMap(List::stream).distinct().joining(" & "); if (!warnings.isEmpty()) { sb.append("// ").append(warnings); }