From 34ae3706ab3e80003b49fa7edae6f3a548016bc1 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Mon, 8 Jan 2018 12:12:26 +0700 Subject: [PATCH] ReadOnly -> Unmodifiable; introduced UnmodifiableView distinguish between unmodifiable and unmodifiable view --- .../dataFlow/ControlFlowAnalyzer.java | 6 +-- .../codeInspection/dataFlow/DfaFactType.java | 12 ++--- .../dataFlow/DfaMemoryStateImpl.java | 2 +- .../codeInspection/dataFlow/Mutability.java | 50 +++++++++++++++++++ .../dataFlow/MutationSignature.java | 28 ----------- .../dataFlow/StandardInstructionVisitor.java | 16 +++--- .../inliner/CollectionFactoryInliner.java | 3 +- .../dataFlow/fixture/MutabilityBasics.java | 10 ++-- .../dataFlow/fixture/MutabilityJdk.java | 14 ++++++ .../DataFlowInspection8Test.java | 2 +- java/jdkAnnotations/java/util/annotations.xml | 28 +++++------ .../org/jetbrains/annotations/ReadOnly.java | 34 ------------- .../jetbrains/annotations/Unmodifiable.java | 23 +++++++++ .../annotations/UnmodifiableView.java | 24 +++++++++ 14 files changed, 151 insertions(+), 101 deletions(-) create mode 100644 java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/Mutability.java delete mode 100644 platform/util/src/org/jetbrains/annotations/ReadOnly.java create mode 100644 platform/util/src/org/jetbrains/annotations/Unmodifiable.java create mode 100644 platform/util/src/org/jetbrains/annotations/UnmodifiableView.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java index 1f97d064e6bb..a98a63696648 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java @@ -1473,7 +1473,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { qualifierExpression.accept(this); } else if (myThisReadOnly) { - addInstruction(new PushInstruction(myFactory.getFactValue(DfaFactType.MUTABLE, false), null)); + addInstruction(new PushInstruction(myFactory.getFactValue(DfaFactType.MUTABILITY, Mutability.UNMODIFIABLE), null)); } else { pushUnknown(); } @@ -1553,7 +1553,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { } } return CachedValueProvider.Result - .create(Collections.emptyList(), method, PsiModificationTracker.JAVA_STRUCTURE_MODIFICATION_COUNT); + .create(Collections.emptyList(), method, PsiModificationTracker.JAVA_STRUCTURE_MODIFICATION_COUNT); }); } @@ -1768,7 +1768,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { startElement(expression); DfaValue value = myFactory.createTypeValue(expression.getType(), Nullness.NOT_NULL); if (myThisReadOnly) { - value = myFactory.withFact(value, DfaFactType.MUTABLE, false); + value = myFactory.withFact(value, DfaFactType.MUTABILITY, Mutability.UNMODIFIABLE); } addInstruction(new PushInstruction(value, expression)); finishElement(expression); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaFactType.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaFactType.java index 0fb847de5ebf..a526f6de4f48 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaFactType.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaFactType.java @@ -74,17 +74,17 @@ public abstract class DfaFactType extends Key { } }; - public static final DfaFactType MUTABLE = new DfaFactType("Mutable") { + public static final DfaFactType MUTABILITY = new DfaFactType("Mutable") { @Override - String toString(@NotNull Boolean fact) { - return fact ? "Mutable" : "ReadOnly"; + boolean isUnknown(@NotNull Mutability fact) { + return fact == Mutability.UNKNOWN; } - @Nullable + @NotNull @Override - Boolean calcFromVariable(@NotNull DfaVariableValue value) { + Mutability calcFromVariable(@NotNull DfaVariableValue value) { PsiModifierListOwner variable = value.getPsiVariable(); - return MutationSignature.getMutabilityFact(variable); + return Mutability.getMutability(variable); } }; diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java index 6b59fe978b01..5ebf4b598d98 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java @@ -1222,7 +1222,7 @@ public class DfaMemoryStateImpl implements DfaMemoryState { } for (DfaVariableValue value : vars) { if (value.isFlushableByCalls() && (value.getQualifier() == null || - !Boolean.FALSE.equals(getValueFact(value.getQualifier(), DfaFactType.MUTABLE)))) { + getValueFact(value.getQualifier(), DfaFactType.MUTABILITY) != Mutability.UNMODIFIABLE)) { doFlush(value, shouldMarkUnknown(value)); } } 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 new file mode 100644 index 000000000000..32a682de9334 --- /dev/null +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/Mutability.java @@ -0,0 +1,50 @@ +/* + * 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.dataFlow; + +import com.intellij.codeInsight.AnnotationUtil; +import com.intellij.psi.*; +import com.intellij.psi.util.PsiTreeUtil; +import com.intellij.util.ObjectUtils; +import org.jetbrains.annotations.NotNull; + +import java.util.Collections; + +public enum Mutability { + UNKNOWN, MUTABLE, UNMODIFIABLE, UNMODIFIABLE_VIEW; + + @NotNull + static Mutability getMutability(PsiModifierListOwner owner) { + if (owner instanceof PsiParameter && owner.getParent() instanceof PsiParameterList) { + PsiParameterList list = (PsiParameterList)owner.getParent(); + PsiMethod method = ObjectUtils.tryCast(list.getParent(), PsiMethod.class); + if (method != null) { + int index = list.getParameterIndex((PsiParameter)owner); + MutationSignature signature = MutationSignature.fromMethod(method); + if (signature.mutatesArg(index)) { + return MUTABLE; + } else if (signature.preservesArg(index) && + PsiTreeUtil.findChildOfAnyType(method.getBody(), PsiLambdaExpression.class, PsiClass.class) == null) { + // If method preserves argument, it still may return a lambda which captures an argument and changes it + // TODO: more precise check (at least differentiate parameters which are captured by lambdas or not) + return UNMODIFIABLE_VIEW; + } + return UNKNOWN; + } + } + if (AnnotationUtil.isAnnotated(owner, Collections.singleton("org.jetbrains.annotations.Unmodifiable"), + AnnotationUtil.CHECK_HIERARCHY | + AnnotationUtil.CHECK_EXTERNAL | + AnnotationUtil.CHECK_INFERRED)) { + return UNMODIFIABLE; + } + if (AnnotationUtil.isAnnotated(owner, Collections.singleton("org.jetbrains.annotations.UnmodifiableView"), + AnnotationUtil.CHECK_HIERARCHY | + AnnotationUtil.CHECK_EXTERNAL | + AnnotationUtil.CHECK_INFERRED)) { + return UNMODIFIABLE_VIEW; + } + return UNKNOWN; + } +} diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MutationSignature.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MutationSignature.java index 29d30723dcc7..05569a7e9198 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MutationSignature.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MutationSignature.java @@ -3,13 +3,10 @@ package com.intellij.codeInspection.dataFlow; import com.intellij.codeInsight.AnnotationUtil; import com.intellij.psi.*; -import com.intellij.psi.util.PsiTreeUtil; -import com.intellij.util.ObjectUtils; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import java.util.Arrays; -import java.util.Collections; public class MutationSignature { private static final String ATTR_MUTATES = "mutates"; @@ -121,29 +118,4 @@ public class MutationSignature { } return UNKNOWN; } - - @Nullable - static Boolean getMutabilityFact(PsiModifierListOwner owner) { - if (owner instanceof PsiParameter && owner.getParent() instanceof PsiParameterList) { - PsiParameterList list = (PsiParameterList)owner.getParent(); - PsiMethod method = ObjectUtils.tryCast(list.getParent(), PsiMethod.class); - if (method != null) { - int index = list.getParameterIndex((PsiParameter)owner); - MutationSignature signature = fromMethod(method); - if (signature.mutatesArg(index)) { - return Boolean.TRUE; - } else if (signature.preservesArg(index) && - PsiTreeUtil.findChildOfAnyType(method.getBody(), PsiLambdaExpression.class, PsiClass.class) == null) { - // If method preserves argument, it still may return a lambda which captures an argument and changes it - // TODO: more precise check (at least differentiate parameters which are captured by lambdas or not) - return Boolean.FALSE; - } - return null; - } - } - return AnnotationUtil.isAnnotated(owner, Collections.singleton("org.jetbrains.annotations.ReadOnly"), - AnnotationUtil.CHECK_HIERARCHY | - AnnotationUtil.CHECK_EXTERNAL | - AnnotationUtil.CHECK_INFERRED) ? Boolean.FALSE : null; - } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java index cc9238c2d5ac..7773102170fe 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java @@ -368,10 +368,10 @@ public class StandardInstructionVisitor extends InstructionVisitor { else if (requiredNullability == Nullness.UNKNOWN) { checkNotNullable(memState, arg, NullabilityProblemKind.passingNullableArgumentToNonAnnotatedParameter.problem(anchor)); } - if (sig.mutatesArg(paramIndex) && !memState.applyFact(arg, DfaFactType.MUTABLE, true)) { + if (sig.mutatesArg(paramIndex) && !memState.applyFact(arg, DfaFactType.MUTABILITY, Mutability.MUTABLE)) { reportMutabilityViolation(false, anchor); if (arg instanceof DfaVariableValue) { - memState.forceVariableFact((DfaVariableValue)arg, DfaFactType.MUTABLE, true); + memState.forceVariableFact((DfaVariableValue)arg, DfaFactType.MUTABILITY, Mutability.MUTABLE); } } if (argValues != null && (paramIndex < argValues.length - 1 || !varargCall)) { @@ -388,10 +388,10 @@ public class StandardInstructionVisitor extends InstructionVisitor { DfaMemoryState memState, MutationSignature sig) { DfaValue value = dereference(memState, memState.pop(), instruction.getQualifierNullabilityProblem()); - if (sig.mutatesThis() && !memState.applyFact(value, DfaFactType.MUTABLE, true)) { + if (sig.mutatesThis() && !memState.applyFact(value, DfaFactType.MUTABILITY, Mutability.MUTABLE)) { reportMutabilityViolation(true, instruction.getContext()); if (value instanceof DfaVariableValue) { - memState.forceVariableFact((DfaVariableValue)value, DfaFactType.MUTABLE, true); + memState.forceVariableFact((DfaVariableValue)value, DfaFactType.MUTABILITY, Mutability.MUTABLE); } } return value; @@ -511,13 +511,13 @@ public class StandardInstructionVisitor extends InstructionVisitor { if (type != null && !(type instanceof PsiPrimitiveType)) { Nullness nullability = instruction.getReturnNullability(); PsiMethod targetMethod = instruction.getTargetMethod(); - Boolean mutable = null; + Mutability mutable = Mutability.UNKNOWN; if (targetMethod != null) { - mutable = MutationSignature.getMutabilityFact(targetMethod); + mutable = Mutability.getMutability(targetMethod); PsiMethod realMethod = findSpecificMethod(targetMethod, state, qualifierValue); if (realMethod != targetMethod) { nullability = DfaPsiUtil.getElementNullability(type, realMethod); - mutable = MutationSignature.getMutabilityFact(realMethod); + mutable = Mutability.getMutability(realMethod); PsiType returnType = realMethod.getReturnType(); if (returnType != null && TypeConversionUtil.erasure(type).isAssignableFrom(returnType)) { // possibly covariant return type @@ -529,7 +529,7 @@ public class StandardInstructionVisitor extends InstructionVisitor { } } DfaValue value = factory.createTypeValue(type, nullability); - return factory.withFact(value, DfaFactType.MUTABLE, mutable); + return factory.withFact(value, DfaFactType.MUTABILITY, mutable); } LongRangeSet range = LongRangeSet.fromType(type); if (range != null) { diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/CollectionFactoryInliner.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/CollectionFactoryInliner.java index 9288728d11f5..7376dd943686 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/CollectionFactoryInliner.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/CollectionFactoryInliner.java @@ -99,7 +99,8 @@ public class CollectionFactoryInliner implements CallInliner { builder.pop(); } DfaValueFactory factory = builder.getFactory(); - DfaValue result = factory.withFact(factory.createTypeValue(call.getType(), Nullness.NOT_NULL), DfaFactType.MUTABLE, false); + DfaValue result = + factory.withFact(factory.createTypeValue(call.getType(), Nullness.NOT_NULL), DfaFactType.MUTABILITY, Mutability.UNMODIFIABLE); if (factoryInfo.mySize == -1) { builder.push(result); } else { diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/MutabilityBasics.java b/java/java-tests/testData/inspection/dataFlow/fixture/MutabilityBasics.java index c61458a8f1e2..c0b802f4bb70 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/MutabilityBasics.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/MutabilityBasics.java @@ -1,5 +1,5 @@ import org.jetbrains.annotations.Contract; -import org.jetbrains.annotations.ReadOnly; +import org.jetbrains.annotations.Unmodifiable; import java.util.Arrays; import java.util.Collection; @@ -7,7 +7,7 @@ import java.util.Collections; import java.util.List; public class MutabilityBasics { - @ReadOnly + @Unmodifiable static List emptyList() { return Collections.emptyList(); } @@ -43,7 +43,7 @@ public class MutabilityBasics { } } - @ReadOnly + @Unmodifiable static Point getZero() { return new Point() { @Override @@ -59,12 +59,12 @@ public class MutabilityBasics { } // Differs from getZero as getZero() is considered as getter with predefined value - @ReadOnly + @Unmodifiable static Point zero() { return getZero(); } - @ReadOnly List list = Arrays.asList("foo", "bar", "baz"); + @Unmodifiable List list = Arrays.asList("foo", "bar", "baz"); void test() { List collection = emptyList(); diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/MutabilityJdk.java b/java/java-tests/testData/inspection/dataFlow/fixture/MutabilityJdk.java index 86d4621e6854..7f2ee69162f9 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/MutabilityJdk.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/MutabilityJdk.java @@ -59,4 +59,18 @@ public class MutabilityJdk { } return result; } + + void testNoFlush() { + List list1 = Collections.emptyList(); + List list2 = new ArrayList<>(); + List list3 = Collections.unmodifiableList(list2); + if(list1.isEmpty()) System.out.println("ok"); + if(!list3.isEmpty()) return; + if(!list3.isEmpty()) return; + list2.add("foo"); + // list1 size is not flushed (UNMODIFIABLE) + if(list1.isEmpty()) System.out.println("ok"); + // list3 size is flushed (UNMODIFIABLE_VIEW) + if(!list3.isEmpty()) return; + } } diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspection8Test.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspection8Test.java index 8454ac204318..94bdd47b19e8 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspection8Test.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspection8Test.java @@ -209,7 +209,7 @@ public class DataFlowInspection8Test extends DataFlowInspectionTestCase { public void testCastInstanceOf() { doTest(); } public void testMutabilityBasics() { - myFixture.addClass("package org.jetbrains.annotations;public @interface ReadOnly {}"); + myFixture.addClass("package org.jetbrains.annotations;public @interface Unmodifiable {}"); doTest(); } diff --git a/java/jdkAnnotations/java/util/annotations.xml b/java/jdkAnnotations/java/util/annotations.xml index aafc80c9d680..dd8d2ec39dae 100644 --- a/java/jdkAnnotations/java/util/annotations.xml +++ b/java/jdkAnnotations/java/util/annotations.xml @@ -1126,7 +1126,7 @@ - + @@ -1160,22 +1160,22 @@ - + - + - + - + @@ -1194,18 +1194,18 @@ - + - + - + @@ -1235,18 +1235,18 @@ - + - + - + @@ -1265,7 +1265,7 @@ - + @@ -1278,14 +1278,14 @@ - + - + diff --git a/platform/util/src/org/jetbrains/annotations/ReadOnly.java b/platform/util/src/org/jetbrains/annotations/ReadOnly.java deleted file mode 100644 index 33eddcbf0861..000000000000 --- a/platform/util/src/org/jetbrains/annotations/ReadOnly.java +++ /dev/null @@ -1,34 +0,0 @@ -// Copyright 2000-2017 JetBrains s.r.o. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -package org.jetbrains.annotations; - -import java.lang.annotation.*; - -/** - * An annotation which depicts that method returns a read-only value or a variable - * contains a read-only value. Read-only value means that calling methods which may - * mutate this value (alter visible behavior) either don't have any effect or throw - * an exception. This does not mean that value cannot be altered at all. For example, - * a value could be a read-only wrapper over a mutable value. - *

- * This annotation is experimental and may be changed/removed in future - * without additional notice! - *

- */ -@Documented -@Retention(RetentionPolicy.CLASS) -@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE}) -@ApiStatus.Experimental -public @interface ReadOnly { -} \ No newline at end of file diff --git a/platform/util/src/org/jetbrains/annotations/Unmodifiable.java b/platform/util/src/org/jetbrains/annotations/Unmodifiable.java new file mode 100644 index 000000000000..39d989bb6dc9 --- /dev/null +++ b/platform/util/src/org/jetbrains/annotations/Unmodifiable.java @@ -0,0 +1,23 @@ +/* + * 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 org.jetbrains.annotations; + +import java.lang.annotation.*; + +/** + * An annotation which depicts that method returns an unmodifiable value or a variable + * contains an unmodifiable value. Unmodifiable value means that calling methods which may + * mutate this value (alter visible behavior) either don't have any effect or throw + * an exception. + *

+ * This annotation is experimental and may be changed/removed in future + * without additional notice! + *

+ */ +@Documented +@Retention(RetentionPolicy.CLASS) +@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE}) +@ApiStatus.Experimental +public @interface Unmodifiable { +} \ No newline at end of file diff --git a/platform/util/src/org/jetbrains/annotations/UnmodifiableView.java b/platform/util/src/org/jetbrains/annotations/UnmodifiableView.java new file mode 100644 index 000000000000..fd679013f028 --- /dev/null +++ b/platform/util/src/org/jetbrains/annotations/UnmodifiableView.java @@ -0,0 +1,24 @@ +/* + * 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 org.jetbrains.annotations; + +import java.lang.annotation.*; + +/** + * An annotation which depicts that method returns an unmodifiable value or a variable + * contains an unmodifiable view. Unmodifiable view means that calling methods which may + * mutate this value (alter visible behavior) either don't have any effect or throw + * an exception. However this value could be modified by third-party, thus methods reading + * the object content might return different result. + *

+ * This annotation is experimental and may be changed/removed in future + * without additional notice! + *

+ */ +@Documented +@Retention(RetentionPolicy.CLASS) +@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE}) +@ApiStatus.Experimental +public @interface UnmodifiableView { +} \ No newline at end of file