From 973ebe7d60d21b298a762bcd963b671fd56a0557 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Mon, 20 Dec 2021 11:37:06 +0700 Subject: [PATCH] [java-dfa] Check mutability for inlined Map methods Fixes IDEA-285183 Enhance dfa: Add check mutabilaty for Map.merge and Map.compute Also fix the lattice for Mutability Tests from PR#1877 Co-authored-by: pyltsin-m GitOrigin-RevId: 9b29b2ccf3d56092e0ef5ecf1a00243cfeb1e493 --- .../codeInspection/dataFlow/Mutability.java | 35 ++++++++++++++----- .../dataFlow/inference/inferenceResults.kt | 2 +- .../java/inliner/MapUpdateInliner.java | 5 +++ .../dataFlow/types/DfGenericObjectType.java | 9 ++--- .../types/DfReferenceConstantType.java | 2 +- .../dataFlow/fixture/MutabilityJdk9.java | 25 +++++++++++++ 6 files changed, 63 insertions(+), 15 deletions(-) diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/Mutability.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/Mutability.java index b9041c4a3238..7119938575e9 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/Mutability.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/Mutability.java @@ -35,9 +35,21 @@ import org.jetbrains.annotations.PropertyKey; import java.util.Collections; import java.util.List; +/** + * Lattice: + * UNKNOWN + * | \ + * MUTABLE MUST_NOT_MODIFY + * | | + * | UNMODIFIABLE_VIEW + * | | + * | UNMODIFIABLE + * \ / + * BOTTOM (null) + */ public enum Mutability { /** - * Mutability is not known; probably value can be mutated + * Mutability is not known; probably value can be mutated; TOP */ UNKNOWN("mutability.unknown", null), /** @@ -96,23 +108,28 @@ public enum Mutability { } @NotNull - public Mutability unite(Mutability other) { + public Mutability join(@NotNull Mutability other) { if (this == other) return this; if (this == UNKNOWN || other == UNKNOWN) return UNKNOWN; - if (this == MUTABLE || other == MUTABLE) return MUTABLE; + if (this == MUTABLE || other == MUTABLE) return UNKNOWN; if (this == MUST_NOT_MODIFY || other == MUST_NOT_MODIFY) return MUST_NOT_MODIFY; if (this == UNMODIFIABLE_VIEW || other == UNMODIFIABLE_VIEW) return UNMODIFIABLE_VIEW; return UNMODIFIABLE; } - @NotNull - public Mutability intersect(Mutability other) { + /** + * @param other mutability to meet + * @return resulting mutability; null if bottom + */ + @Nullable + public Mutability meet(@NotNull Mutability other) { if (this == other) return this; + if (this == UNKNOWN) return other; + if (other == UNKNOWN) return this; + if (this == MUTABLE || other == MUTABLE) return null; if (this == UNMODIFIABLE || other == UNMODIFIABLE) return UNMODIFIABLE; if (this == UNMODIFIABLE_VIEW || other == UNMODIFIABLE_VIEW) return UNMODIFIABLE_VIEW; - if (this == MUST_NOT_MODIFY || other == MUST_NOT_MODIFY) return MUST_NOT_MODIFY; - if (this == MUTABLE || other == MUTABLE) return MUTABLE; - return UNKNOWN; + return MUST_NOT_MODIFY; } @Nullable @@ -197,7 +214,7 @@ public enum Mutability { newMutability = method == null ? UNKNOWN : getMutability(method); } } - mutability = mutability.unite(newMutability); + mutability = mutability.join(newMutability); if (!mutability.isUnmodifiable()) break; } return mutability; diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inference/inferenceResults.kt b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inference/inferenceResults.kt index e465418e3246..64311eb6386a 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inference/inferenceResults.kt +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inference/inferenceResults.kt @@ -128,7 +128,7 @@ interface MethodReturnInferenceResult { return Mutability.UNKNOWN } return delegateCalls.stream().map { range -> getDelegateMutability(method, range, body()) }.reduce( - Mutability::unite).orElse( + Mutability::join).orElse( Mutability.UNKNOWN) } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/java/inliner/MapUpdateInliner.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/java/inliner/MapUpdateInliner.java index 542db853a74a..3a7517056a73 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/java/inliner/MapUpdateInliner.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/java/inliner/MapUpdateInliner.java @@ -4,9 +4,12 @@ package com.intellij.codeInspection.dataFlow.java.inliner; import com.intellij.codeInsight.Nullability; +import com.intellij.codeInspection.dataFlow.Mutability; import com.intellij.codeInspection.dataFlow.java.CFGBuilder; import com.intellij.codeInspection.dataFlow.jvm.SpecialField; +import com.intellij.codeInspection.dataFlow.jvm.problems.MutabilityProblem; import com.intellij.codeInspection.dataFlow.types.DfTypes; +import com.intellij.codeInspection.dataFlow.value.RelationType; import com.intellij.psi.CommonClassNames; import com.intellij.psi.PsiExpression; import com.intellij.psi.PsiMethodCallExpression; @@ -35,6 +38,7 @@ public class MapUpdateInliner implements CallInliner { PsiExpression function = args[1]; builder .pushExpression(qualifier) // stack: .. qualifier + .ensure(RelationType.IS, Mutability.MUTABLE.asDfType(), new MutabilityProblem(call, true), null) .pushExpression(key) // stack: .. qualifier; key .evaluateFunction(function); String name = Objects.requireNonNull(call.getMethodExpression().getReferenceName()); @@ -65,6 +69,7 @@ public class MapUpdateInliner implements CallInliner { PsiExpression function = args[2]; builder .pushExpression(qualifier) // stack: .. qualifier + .ensure(RelationType.IS, Mutability.MUTABLE.asDfType(), new MutabilityProblem(call, true), null) .pushExpression(key) // stack: .. qualifier; key .pop() // stack: .. qualifier .pushExpression(value) // stack: .. qualifier; value diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfGenericObjectType.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfGenericObjectType.java index bb6c20813f09..4c50225476e9 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfGenericObjectType.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfGenericObjectType.java @@ -163,7 +163,7 @@ final class DfGenericObjectType extends DfAntiConstantType implements Df if (type.getNullability() != getNullability() && getNullability() != DfaNullability.UNKNOWN && type.getNullability() != DfaNullability.NOT_NULL) return false; if (!getConstraint().isSuperConstraintOf(type.getConstraint())) return false; - if (getMutability().ordinal() > type.getMutability().ordinal()) return false; + if (getMutability().join(type.getMutability()) != getMutability()) return false; SpecialField sf = getSpecialField(); if (sf != null) { if (sf != type.getSpecialField()) return false; @@ -197,7 +197,7 @@ final class DfGenericObjectType extends DfAntiConstantType implements Df throw new AssertionError("Join failed: " + this + " | " + other); } DfaNullability nullability = getNullability().unite(type.getNullability()); - Mutability mutability = getMutability().unite(type.getMutability()); + Mutability mutability = getMutability().join(type.getMutability()); boolean locality = isLocal() && type.isLocal(); SpecialField sf = Objects.equals(getSpecialField(), type.getSpecialField()) ? getSpecialField() : null; DfType sfType = sf == null ? BOTTOM : getSpecialFieldType().join(type.getSpecialFieldType()); @@ -266,7 +266,7 @@ final class DfGenericObjectType extends DfAntiConstantType implements Df mySpecialField, mySpecialFieldType, myLocal); } case MUTABILITY: - return new DfGenericObjectType(myNotValues, myConstraint, myNullability, myMutability.unite(otherMutability), + return new DfGenericObjectType(myNotValues, myConstraint, myNullability, myMutability.join(otherMutability), mySpecialField, mySpecialFieldType, myLocal); case NULLABILITY: return new DfGenericObjectType(myNotValues, myConstraint, myNullability.unite(otherNullability), myMutability, @@ -304,7 +304,8 @@ final class DfGenericObjectType extends DfAntiConstantType implements Df } DfaNullability nullability = getNullability().intersect(type.getNullability()); if (nullability == null) return BOTTOM; - Mutability mutability = getMutability().intersect(type.getMutability()); + Mutability mutability = getMutability().meet(type.getMutability()); + if (mutability == null) return BOTTOM; boolean locality = isLocal() || type.isLocal(); SpecialField sf; DfType sfType; diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfReferenceConstantType.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfReferenceConstantType.java index b55bbca22ec4..2b1b0f88764d 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfReferenceConstantType.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfReferenceConstantType.java @@ -108,7 +108,7 @@ public class DfReferenceConstantType extends DfConstantType implements D DfReferenceType type = (DfReferenceType)other; TypeConstraint constraint = getConstraint().join(type.getConstraint()); DfaNullability nullability = getNullability().unite(type.getNullability()); - Mutability mutability = getMutability().unite(type.getMutability()); + Mutability mutability = getMutability().join(type.getMutability()); boolean locality = isLocal() && type.isLocal(); SpecialField sf = Objects.equals(getSpecialField(), type.getSpecialField()) ? getSpecialField() : null; DfType sfType = sf == null ? BOTTOM : getSpecialFieldType().join(type.getSpecialFieldType()); diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/MutabilityJdk9.java b/java/java-tests/testData/inspection/dataFlow/fixture/MutabilityJdk9.java index ebc20cd8fe25..127492133246 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/MutabilityJdk9.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/MutabilityJdk9.java @@ -54,4 +54,29 @@ public class MutabilityJdk9 { list.add("foo"); } } + + void testMapOfEntriesMerge() { + Map map = Map.ofEntries(Map.entry("x", 1), Map.entry("y", 2)); + map.merge("a", 1, (x, y)->y); + } + + void testMapCompute() { + Map map = Map.of("a", 1, "b", 2, "c", 3, "d", 4, "e", 5); + map.compute("a", (x, y)->y); + } + + void testMapComputeIfPresent() { + Map map = Map.of("a", 1, "b", 2, "c", 3, "d", 4, "e", 5); + map.computeIfPresent("a", (x, y)->y); + } + + void testMapComputeIfAbsent() { + Map map = Map.of("a", 1, "b", 2, "c", 3, "d", 4, "e", 5); + map.computeIfAbsent("a", x->1); + } + + void testMapMerge() { + Map map = Map.of("a", 1, "b", 2, "c", 3, "d", 4, "e", 5); + map.merge("a", 1, (x, y)->y); + } }