From 582f0f95711af5f0e094939f1d6873601facd54c Mon Sep 17 00:00:00 2001 From: peter Date: Mon, 15 May 2017 16:15:21 +0200 Subject: [PATCH] IDEA-CR-20904 fixes after review (IDEA-123301 Show a warning if 'List<@Nullable X>' is passed to a place where 'List<@NotNull X'> is expected) supported maps, inheritance, returns, functional expressions --- .../codeInspection/dataFlow/DfaPsiUtil.java | 30 ++++-- .../nullable/NullableStuffInspectionBase.java | 92 ++++++++++++++++++- .../util/RefactoringHierarchyUtil.java | 15 +-- .../impl/analysis/JavaGenericsUtil.java | 6 +- .../intellij/psi/util/InheritanceUtil.java | 16 ++++ ...llCollectionItemWithNullableSuperType.java | 8 ++ ...lableCollectionWhereNotNullIsExpected.java | 37 ++++++-- ...singNullableMapWhereNotNullIsExpected.java | 14 +++ .../NullableStuffInspectionTest.java | 10 ++ 9 files changed, 193 insertions(+), 35 deletions(-) create mode 100644 java/java-tests/testData/inspection/nullableProblems/NotNullCollectionItemWithNullableSuperType.java create mode 100644 java/java-tests/testData/inspection/nullableProblems/PassingNullableMapWhereNotNullIsExpected.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaPsiUtil.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaPsiUtil.java index 7dad0ab29502..0d95a08575b8 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaPsiUtil.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaPsiUtil.java @@ -102,16 +102,26 @@ public class DfaPsiUtil { @NotNull public static Nullness getTypeNullability(@Nullable PsiType type) { - if (type != null) { - for (PsiAnnotation annotation : type.getAnnotations()) { - String qualifiedName = annotation.getQualifiedName(); - NullableNotNullManager nnn = NullableNotNullManager.getInstance(annotation.getProject()); - if (nnn.getNullables().contains(qualifiedName)) { - return Nullness.NULLABLE; - } - if (nnn.getNotNulls().contains(qualifiedName)) { - return Nullness.NOT_NULL; - } + if (type == null) return Nullness.UNKNOWN; + + Ref result = Ref.create(Nullness.UNKNOWN); + InheritanceUtil.processSuperTypes(type, true, eachType -> { + result.set(getTypeOwnNullability(result, eachType)); + return result.get() == Nullness.UNKNOWN; + }); + return result.get(); + } + + @NotNull + private static Nullness getTypeOwnNullability(Ref result, PsiType eachType) { + for (PsiAnnotation annotation : eachType.getAnnotations()) { + String qualifiedName = annotation.getQualifiedName(); + NullableNotNullManager nnn = NullableNotNullManager.getInstance(annotation.getProject()); + if (nnn.getNullables().contains(qualifiedName)) { + return Nullness.NULLABLE; + } + if (nnn.getNotNulls().contains(qualifiedName)) { + return Nullness.NOT_NULL; } } return Nullness.UNKNOWN; diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/nullable/NullableStuffInspectionBase.java b/java/java-analysis-impl/src/com/intellij/codeInspection/nullable/NullableStuffInspectionBase.java index f3a9f97fe0b6..c54e265f197f 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/nullable/NullableStuffInspectionBase.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/nullable/NullableStuffInspectionBase.java @@ -100,6 +100,14 @@ public class NullableStuffInspectionBase extends BaseJavaBatchLocalInspectionToo @Override public void visitMethodReferenceExpression(PsiMethodReferenceExpression expression) { checkMethodReference(expression, holder); + + JavaResolveResult result = expression.advancedResolve(false); + PsiElement target = result.getElement(); + if (target instanceof PsiMethod) { + checkCollectionNullityOnAssignment(expression, + LambdaUtil.getFunctionalInterfaceReturnType(expression), + result.getSubstitutor().substitute(((PsiMethod)target).getReturnType())); + } } @Override @@ -160,6 +168,28 @@ public class NullableStuffInspectionBase extends BaseJavaBatchLocalInspectionToo return false; } + @Override + public void visitReferenceElement(PsiJavaCodeReferenceElement reference) { + super.visitReferenceElement(reference); + + checkNullableNotNullInstantiationConflict(reference); + } + + private void checkNullableNotNullInstantiationConflict(PsiJavaCodeReferenceElement reference) { + PsiElement element = reference.resolve(); + if (element instanceof PsiClass && ((PsiClass)element).getTypeParameters().length > 0) { + PsiElementFactory factory = JavaPsiFacade.getElementFactory(element.getProject()); + if (isNullableNotNullCollectionConflict(reference, + factory.createType((PsiClass)element, PsiSubstitutor.EMPTY), + factory.createType(reference))) { + holder.registerProblem(reference, + "Nullable type arguments where non-null ones are expected", + ProblemHighlightType.GENERIC_ERROR_OR_WARNING); + + } + } + } + @Override public void visitAssignmentExpression(PsiAssignmentExpression expression) { checkCollectionNullityOnAssignment(expression.getOperationSign(), expression.getLExpression().getType(), expression.getRExpression()); @@ -173,6 +203,28 @@ public class NullableStuffInspectionBase extends BaseJavaBatchLocalInspectionToo } } + @Override + public void visitReturnStatement(PsiReturnStatement statement) { + PsiExpression returnValue = statement.getReturnValue(); + if (returnValue == null) return; + + PsiElement element = PsiTreeUtil.getParentOfType(statement, PsiMethod.class, PsiLambdaExpression.class); + if (element == null) return; + + PsiType returnType = element instanceof PsiMethod ? ((PsiMethod)element).getReturnType() : LambdaUtil.getFunctionalInterfaceReturnType((PsiFunctionalExpression)element); + + checkCollectionNullityOnAssignment(statement.getReturnValue(), returnType, returnValue); + } + + @Override + public void visitLambdaExpression(PsiLambdaExpression lambda) { + super.visitLambdaExpression(lambda); + PsiElement body = lambda.getBody(); + if (body instanceof PsiExpression) { + checkCollectionNullityOnAssignment(body, LambdaUtil.getFunctionalInterfaceReturnType(lambda), (PsiExpression)body); + } + } + @Override public void visitCallExpression(PsiCallExpression callExpression) { PsiExpressionList argList = callExpression.getArgumentList(); @@ -192,18 +244,48 @@ public class NullableStuffInspectionBase extends BaseJavaBatchLocalInspectionToo } } - private void checkCollectionNullityOnAssignment(@NotNull PsiElement errorElement, PsiType expectedType, PsiExpression assignedExpression) { - PsiType lItemType = JavaGenericsUtil.getCollectionItemType(expectedType, errorElement.getResolveScope()); - PsiType rItemType = assignedExpression == null ? null : JavaGenericsUtil.getCollectionItemType(assignedExpression); + private void checkCollectionNullityOnAssignment(@NotNull PsiElement errorElement, + @Nullable PsiType expectedType, + @Nullable PsiExpression assignedExpression) { + if (assignedExpression == null) return; - if (DfaPsiUtil.getTypeNullability(lItemType) == Nullness.NOT_NULL && - DfaPsiUtil.getTypeNullability(rItemType) == Nullness.NULLABLE) { + checkCollectionNullityOnAssignment(errorElement, expectedType, assignedExpression.getType()); + } + + private void checkCollectionNullityOnAssignment(@NotNull PsiElement errorElement, + @Nullable PsiType expectedType, + @Nullable PsiType assignedType) { + if (isNullableNotNullCollectionConflict(errorElement, expectedType, assignedType)) { holder.registerProblem(errorElement, "Assigning a collection of nullable elements into a collection of non-null elements", ProblemHighlightType.GENERIC_ERROR_OR_WARNING); } } + + private boolean isNullableNotNullCollectionConflict(PsiElement place, + @Nullable PsiType expectedType, + @Nullable PsiType assignedType) { + + if (isNullityConflict(JavaGenericsUtil.getCollectionItemType(expectedType, place.getResolveScope()), + JavaGenericsUtil.getCollectionItemType(assignedType, place.getResolveScope()))) { + return true; + } + + for (int i = 0; i <= 1; i++) { + PsiType expectedArg = PsiUtil.substituteTypeParameter(expectedType, CommonClassNames.JAVA_UTIL_MAP, i, false); + PsiType assignedArg = PsiUtil.substituteTypeParameter(assignedType, CommonClassNames.JAVA_UTIL_MAP, i, false); + if (isNullityConflict(expectedArg, assignedArg)) { + return true; + } + } + + return false; + } + + private boolean isNullityConflict(PsiType expected, PsiType assigned) { + return DfaPsiUtil.getTypeNullability(expected) == Nullness.NOT_NULL && DfaPsiUtil.getTypeNullability(assigned) == Nullness.NULLABLE; + } }; } diff --git a/java/java-impl/src/com/intellij/refactoring/util/RefactoringHierarchyUtil.java b/java/java-impl/src/com/intellij/refactoring/util/RefactoringHierarchyUtil.java index 4bd191569dcc..2cc40d11352b 100644 --- a/java/java-impl/src/com/intellij/refactoring/util/RefactoringHierarchyUtil.java +++ b/java/java-impl/src/com/intellij/refactoring/util/RefactoringHierarchyUtil.java @@ -45,6 +45,7 @@ public class RefactoringHierarchyUtil { boolean includeSubclasses) { PsiElement parent = place; while (parent != null) { + //noinspection SuspiciousMethodCalls if (membersToMove.contains(parent)) return true; if (parent instanceof PsiModifierList) return false; //see IDEADEV-12448 if (parent instanceof PsiClass && targetClass != null) { @@ -162,11 +163,6 @@ public class RefactoringHierarchyUtil { } public static void processSuperTypes(PsiType type, SuperTypeVisitor visitor) { - processSuperTypes(type, visitor, new HashSet<>()); - } - private static void processSuperTypes(PsiType type, SuperTypeVisitor visitor, Set visited) { - if (visited.contains(type)) return; - visited.add(type); if (type instanceof PsiPrimitiveType) { int index = PRIMITIVE_TYPES.indexOf(type); if (index >= 0) { @@ -176,11 +172,10 @@ public class RefactoringHierarchyUtil { } } else { - final PsiType[] superTypes = type.getSuperTypes(); - for (PsiType superType : superTypes) { - visitor.visitType(superType); - processSuperTypes(superType, visitor, visited); - } + InheritanceUtil.processSuperTypes(type, false, aType -> { + visitor.visitType(aType); + return true; + }); } } diff --git a/java/java-psi-api/src/com/intellij/codeInsight/daemon/impl/analysis/JavaGenericsUtil.java b/java/java-psi-api/src/com/intellij/codeInsight/daemon/impl/analysis/JavaGenericsUtil.java index ef8845ae0a57..819bb81ac8e8 100644 --- a/java/java-psi-api/src/com/intellij/codeInsight/daemon/impl/analysis/JavaGenericsUtil.java +++ b/java/java-psi-api/src/com/intellij/codeInsight/daemon/impl/analysis/JavaGenericsUtil.java @@ -270,13 +270,11 @@ public class JavaGenericsUtil { @Nullable public static PsiType getCollectionItemType(@NotNull PsiExpression expression) { - final PsiType type = expression.getType(); - if (type == null) return null; - return getCollectionItemType(type, expression.getResolveScope()); + return getCollectionItemType(expression.getType(), expression.getResolveScope()); } @Nullable - public static PsiType getCollectionItemType(final PsiType type, final GlobalSearchScope scope) { + public static PsiType getCollectionItemType(@Nullable PsiType type, @NotNull GlobalSearchScope scope) { if (type instanceof PsiArrayType) { return ((PsiArrayType)type).getComponentType(); } diff --git a/java/java-psi-api/src/com/intellij/psi/util/InheritanceUtil.java b/java/java-psi-api/src/com/intellij/psi/util/InheritanceUtil.java index c273e6433e35..e16283471ab2 100644 --- a/java/java-psi-api/src/com/intellij/psi/util/InheritanceUtil.java +++ b/java/java-psi-api/src/com/intellij/psi/util/InheritanceUtil.java @@ -18,6 +18,7 @@ package com.intellij.psi.util; import com.intellij.openapi.util.Condition; import com.intellij.psi.*; import com.intellij.util.Processor; +import com.intellij.util.containers.HashSet; import gnu.trove.THashSet; import org.jetbrains.annotations.Contract; import org.jetbrains.annotations.NonNls; @@ -165,4 +166,19 @@ public class InheritanceUtil { } return place == aClass; } + + public static boolean processSuperTypes(@NotNull PsiType type, boolean includeSelf, @NotNull Processor processor) { + if (includeSelf && !processor.process(type)) return false; + return processSuperTypes(type, processor, new HashSet<>()); + } + + private static boolean processSuperTypes(PsiType type, Processor processor, Set visited) { + if (!visited.add(type)) return true; + for (PsiType superType : type.getSuperTypes()) { + if (!processor.process(superType)) return false; + processSuperTypes(superType, processor, visited); + } + return true; + } + } diff --git a/java/java-tests/testData/inspection/nullableProblems/NotNullCollectionItemWithNullableSuperType.java b/java/java-tests/testData/inspection/nullableProblems/NotNullCollectionItemWithNullableSuperType.java new file mode 100644 index 000000000000..5e5e2ffaaf6a --- /dev/null +++ b/java/java-tests/testData/inspection/nullableProblems/NotNullCollectionItemWithNullableSuperType.java @@ -0,0 +1,8 @@ +import typeUse.*; +import java.util.*; + +class MyList extends ArrayList {} + +class SubList extends MyList<@Nullable Integer> { + MyList<@Nullable Integer> myList; +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/nullableProblems/PassingNullableCollectionWhereNotNullIsExpected.java b/java/java-tests/testData/inspection/nullableProblems/PassingNullableCollectionWhereNotNullIsExpected.java index fecc38e39a94..0b37349104d2 100644 --- a/java/java-tests/testData/inspection/nullableProblems/PassingNullableCollectionWhereNotNullIsExpected.java +++ b/java/java-tests/testData/inspection/nullableProblems/PassingNullableCollectionWhereNotNullIsExpected.java @@ -1,21 +1,46 @@ import typeUse.*; import java.util.*; +import java.util.function.*; class JC { - public static void main(String[] args) { - List<@Nullable String> list = new ArrayList<>(); - print(list); + void testList() { + List<@Nullable String> nullableList = new ArrayList<>(); + print(nullableList); - List<@NotNull String> list2 = list; + List<@NotNull String> list2 = nullableList; List<@NotNull String> list3; - list2 = list; - } + list2 = nullableList; + List list4 = nullableList; + List list5 = nullableList; + } + private static void print(List<@NotNull String> list) { for (String s : list) { System.out.println(s.length()); } } + + List<@Nullable String> getNullableList() {return new ArrayList<>();} + + List<@NotNull String> testReturnValue() { + List<@Nullable String> list = new ArrayList<>(); + + Supplier> supplier = () -> list; + Supplier> supplierRef = this::getNullableList; + + Supplier> supplier3 = () -> { return list;}; + + return list; + } + +} + +class Test { + public void test() { + List nullableList = new ArrayList<>(); + List list2 = nullableList; + } } \ No newline at end of file diff --git a/java/java-tests/testData/inspection/nullableProblems/PassingNullableMapWhereNotNullIsExpected.java b/java/java-tests/testData/inspection/nullableProblems/PassingNullableMapWhereNotNullIsExpected.java new file mode 100644 index 000000000000..71166267356b --- /dev/null +++ b/java/java-tests/testData/inspection/nullableProblems/PassingNullableMapWhereNotNullIsExpected.java @@ -0,0 +1,14 @@ +import typeUse.*; +import java.util.*; + +class JC { + + void testMap() { + Map<@NotNull String, @NotNull String> m1 = new HashMap<@Nullable String, String>(); + m1 = new HashMap(); + m1 = new HashMap(); + + Map<@NotNull String, ? extends @NotNull String> m2 = new HashMap<@Nullable String, String>(); + } + +} diff --git a/java/java-tests/testSrc/com/intellij/codeInspection/NullableStuffInspectionTest.java b/java/java-tests/testSrc/com/intellij/codeInspection/NullableStuffInspectionTest.java index de55d9d82f4b..312970470cc0 100644 --- a/java/java-tests/testSrc/com/intellij/codeInspection/NullableStuffInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/codeInspection/NullableStuffInspectionTest.java @@ -207,4 +207,14 @@ public class NullableStuffInspectionTest extends LightCodeInsightFixtureTestCase doTest(); } + public void testPassingNullableMapWhereNotNullIsExpected() { + DataFlowInspection8Test.setupTypeUseAnnotations("typeUse", myFixture); + doTest(); + } + + public void testNotNullCollectionItemWithNullableSuperType() { + DataFlowInspection8Test.setupTypeUseAnnotations("typeUse", myFixture); + doTest(); + } + } \ No newline at end of file