From f3cb948613e1b678d8ab98f3b192f45ae99d1e50 Mon Sep 17 00:00:00 2001 From: Peter Gromov Date: Fri, 6 Mar 2020 19:19:08 +0100 Subject: [PATCH] IDEA-231906 IDEA should check violations of array/collection component nullability in overridden methods GitOrigin-RevId: e57066f48674a0b3491e1a57bbfa3f4cab4bd60d --- .../messages/JavaAnalysisBundle.properties | 2 + .../nullable/NullableStuffInspectionBase.java | 164 +++++++++++------- ...erridingNotNullCollectionWithNullable.java | 30 ++++ .../NullableStuffInspectionTest.java | 5 + 4 files changed, 137 insertions(+), 64 deletions(-) create mode 100644 java/java-tests/testData/inspection/nullableProblems/OverridingNotNullCollectionWithNullable.java diff --git a/java/java-analysis-api/resources/messages/JavaAnalysisBundle.properties b/java/java-analysis-api/resources/messages/JavaAnalysisBundle.properties index 64c6b14c0bc3..3f5839380936 100644 --- a/java/java-analysis-api/resources/messages/JavaAnalysisBundle.properties +++ b/java/java-analysis-api/resources/messages/JavaAnalysisBundle.properties @@ -155,6 +155,8 @@ annotate.overridden.methods.parameters=Annotate overridden method parameters as anonymous.ref.loc.can.be.replaced.with.0=Anonymous #ref #loc can be replaced with {0} anonymous.ref.loc.can.be.replaced.with.lambda=Anonymous #ref #loc can be replaced with lambda assigning.a.collection.of.nullable.elements=Assigning a collection of nullable elements into a collection of non-null elements +nullable.stuff.error.overriding.nullable.with.notnull=Overriding a collection of nullable elements with a collection of non-null elements +nullable.stuff.error.overriding.notnull.with.nullable=Overriding a collection of not-null elements with a collection of nullable elements comparision.between.object.and.primitive=Comparision between Object and primitive is illegal and is accepted in java 7 only custom.exception.class.should.have.a.constructor=Custom exception class should have a constructor with a single message parameter of String type delimiters.argument.contains.duplicated.characters=Delimiters argument contains duplicated characters 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 e6243305f66a..7cad54cb617b 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 @@ -278,7 +278,7 @@ public class NullableStuffInspectionBase extends AbstractBaseJavaLocalInspection private void checkCollectionNullityOnAssignment(@NotNull PsiElement errorElement, @Nullable PsiType expectedType, @Nullable PsiType assignedType) { - if (isNullableNotNullCollectionConflict(expectedType, assignedType, new HashSet<>())) { + if (isNullableNotNullCollectionConflict(expectedType, assignedType, file, new HashSet<>())) { holder.registerProblem(errorElement, JavaAnalysisBundle .message("assigning.a.collection.of.nullable.elements"), @@ -287,35 +287,37 @@ public class NullableStuffInspectionBase extends AbstractBaseJavaLocalInspection } } - private boolean isNullableNotNullCollectionConflict(@Nullable PsiType expectedType, - @Nullable PsiType assignedType, - @NotNull Set> visited) { - if (!visited.add(Couple.of(expectedType, assignedType))) return false; - - GlobalSearchScope scope = holder.getFile().getResolveScope(); - if (isNullityConflict(JavaGenericsUtil.getCollectionItemType(expectedType, scope), - JavaGenericsUtil.getCollectionItemType(assignedType, scope))) { - 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) || - expectedArg != null && assignedArg != null && isNullableNotNullCollectionConflict(expectedArg, assignedArg, visited)) { - return true; - } - } - - return false; - } - - private boolean isNullityConflict(PsiType expected, PsiType assigned) { - return DfaPsiUtil.getTypeNullability(expected) == Nullability.NOT_NULL && DfaPsiUtil.getTypeNullability(assigned) == Nullability.NULLABLE; - } }; } + private static boolean isNullableNotNullCollectionConflict(@Nullable PsiType expectedType, + @Nullable PsiType assignedType, + @NotNull PsiFile place, + @NotNull Set> visited) { + if (!visited.add(Couple.of(expectedType, assignedType))) return false; + + GlobalSearchScope scope = place.getResolveScope(); + if (isNullityConflict(JavaGenericsUtil.getCollectionItemType(expectedType, scope), + JavaGenericsUtil.getCollectionItemType(assignedType, scope))) { + 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) || + expectedArg != null && assignedArg != null && isNullableNotNullCollectionConflict(expectedArg, assignedArg, place, visited)) { + return true; + } + } + + return false; + } + + private static boolean isNullityConflict(PsiType expected, PsiType assigned) { + return DfaPsiUtil.getTypeNullability(expected) == Nullability.NOT_NULL && DfaPsiUtil.getTypeNullability(assigned) == Nullability.NULLABLE; + } + @Nullable private String checkIndirectInheritance(PsiElement psiClass, PsiClass intf) { for (PsiMethod intfMethod : intf.getAllMethods()) { @@ -681,6 +683,15 @@ public class NullableStuffInspectionBase extends AbstractBaseJavaLocalInspection createFixForNonAnnotatedOverridesNotNull(method, superMethod)); break; } + + PsiTypeElement returnTypeElement = method.getReturnTypeElement(); + if (returnTypeElement != null && + isNullableNotNullCollectionConflict(superMethod.getReturnType(), method.getReturnType(), holder.getFile(), new HashSet<>())) { + holder.registerProblem(returnTypeElement, + JavaAnalysisBundle.message("nullable.stuff.error.overriding.notnull.with.nullable"), + ProblemHighlightType.GENERIC_ERROR_OR_WARNING); + break; + } } } @@ -724,49 +735,74 @@ public class NullableStuffInspectionBase extends AbstractBaseJavaLocalInspection PsiParameter parameter = parameters[i]; if (parameter.getType() instanceof PsiPrimitiveType) continue; - List superParameters = new ArrayList<>(); - for (PsiMethod superMethod : superMethods) { - PsiParameter[] _superParameters = superMethod.getParameterList().getParameters(); - if (_superParameters.length == parameters.length) { - superParameters.add(_superParameters[i]); - } - } + List superParameters = getSuperParameters(superMethods, parameters, i); - PsiParameter nullableSuper = findNullableSuperForNotNullParameter(parameter, superParameters); - if (nullableSuper != null) { - PsiAnnotation annotation = AnnotationUtil.findAnnotation(parameter, nullableManager.getNotNulls(), true); - holder.registerProblem(annotation != null ? annotation : parameter.getNameIdentifier(), - JavaAnalysisBundle.message("inspection.nullable.problems.NotNull.parameter.overrides.Nullable", - getPresentableAnnoName(parameter), - getPresentableAnnoName(nullableSuper)), - ProblemHighlightType.GENERIC_ERROR_OR_WARNING); - } - PsiParameter notNullSuper = findNotNullSuperForNonAnnotatedParameter(nullableManager, parameter, superParameters); - if (notNullSuper != null) { - LocalQuickFix fix = AnnotationUtil.isAnnotatingApplicable(parameter, nullableManager.getDefaultNotNull()) - ? AddAnnotationPsiFix.createAddNotNullFix(parameter) - : createChangeDefaultNotNullFix(nullableManager, notNullSuper); - holder.registerProblem(parameter.getNameIdentifier(), - JavaAnalysisBundle.message("inspection.nullable.problems.parameter.overrides.NotNull", getPresentableAnnoName(notNullSuper)), - ProblemHighlightType.GENERIC_ERROR_OR_WARNING, - fix); - } - if (isNotNullParameterOverridingNonAnnotated(nullableManager, parameter, superParameters)) { - NullabilityAnnotationInfo info = nullableManager.findOwnNullabilityInfo(parameter); - assert info != null; - PsiAnnotation notNullAnnotation = info.getAnnotation(); - boolean physical = PsiTreeUtil.isAncestor(parameter, notNullAnnotation, true); - final LocalQuickFix fix = physical ? new RemoveAnnotationQuickFix(notNullAnnotation, parameter) : null; - holder.registerProblem(physical ? notNullAnnotation : parameter.getNameIdentifier(), - JavaAnalysisBundle.message("inspection.nullable.problems.NotNull.parameter.overrides.not.annotated", getPresentableAnnoName(parameter)), - ProblemHighlightType.GENERIC_ERROR_OR_WARNING, - fix); - } + checkSuperParameterAnnotations(holder, nullableManager, parameter, superParameters); checkNullLiteralArgumentOfNotNullParameterUsages(method, holder, nullableManager, isOnFly, i, parameter); } } + @NotNull + private static List getSuperParameters(List superMethods, PsiParameter[] parameters, int i) { + List superParameters = new ArrayList<>(); + for (PsiMethod superMethod : superMethods) { + PsiParameter[] _superParameters = superMethod.getParameterList().getParameters(); + if (_superParameters.length == parameters.length) { + superParameters.add(_superParameters[i]); + } + } + return superParameters; + } + + private void checkSuperParameterAnnotations(ProblemsHolder holder, + NullableNotNullManager nullableManager, + PsiParameter parameter, + List superParameters) { + PsiParameter nullableSuper = findNullableSuperForNotNullParameter(parameter, superParameters); + if (nullableSuper != null) { + PsiAnnotation annotation = AnnotationUtil.findAnnotation(parameter, nullableManager.getNotNulls(), true); + holder.registerProblem(annotation != null ? annotation : parameter.getNameIdentifier(), + JavaAnalysisBundle.message("inspection.nullable.problems.NotNull.parameter.overrides.Nullable", + getPresentableAnnoName(parameter), + getPresentableAnnoName(nullableSuper)), + ProblemHighlightType.GENERIC_ERROR_OR_WARNING); + } + PsiParameter notNullSuper = findNotNullSuperForNonAnnotatedParameter(nullableManager, parameter, superParameters); + if (notNullSuper != null) { + LocalQuickFix fix = AnnotationUtil.isAnnotatingApplicable(parameter, nullableManager.getDefaultNotNull()) + ? AddAnnotationPsiFix.createAddNotNullFix(parameter) + : createChangeDefaultNotNullFix(nullableManager, notNullSuper); + holder.registerProblem(parameter.getNameIdentifier(), + JavaAnalysisBundle.message("inspection.nullable.problems.parameter.overrides.NotNull", getPresentableAnnoName(notNullSuper)), + ProblemHighlightType.GENERIC_ERROR_OR_WARNING, + fix); + } + if (isNotNullParameterOverridingNonAnnotated(nullableManager, parameter, superParameters)) { + NullabilityAnnotationInfo info = nullableManager.findOwnNullabilityInfo(parameter); + assert info != null; + PsiAnnotation notNullAnnotation = info.getAnnotation(); + boolean physical = PsiTreeUtil.isAncestor(parameter, notNullAnnotation, true); + final LocalQuickFix fix = physical ? new RemoveAnnotationQuickFix(notNullAnnotation, parameter) : null; + holder.registerProblem(physical ? notNullAnnotation : parameter.getNameIdentifier(), + JavaAnalysisBundle.message("inspection.nullable.problems.NotNull.parameter.overrides.not.annotated", getPresentableAnnoName(parameter)), + ProblemHighlightType.GENERIC_ERROR_OR_WARNING, + fix); + } + + PsiTypeElement typeElement = parameter.getTypeElement(); + if (typeElement != null) { + for (PsiParameter superParameter : superParameters) { + if (isNullableNotNullCollectionConflict(parameter.getType(), superParameter.getType(), holder.getFile(), new HashSet<>())) { + holder.registerProblem(typeElement, + JavaAnalysisBundle.message("nullable.stuff.error.overriding.nullable.with.notnull"), + ProblemHighlightType.GENERIC_ERROR_OR_WARNING); + break; + } + } + } + } + @Nullable private PsiParameter findNotNullSuperForNonAnnotatedParameter(NullableNotNullManager nullableManager, PsiParameter parameter, diff --git a/java/java-tests/testData/inspection/nullableProblems/OverridingNotNullCollectionWithNullable.java b/java/java-tests/testData/inspection/nullableProblems/OverridingNotNullCollectionWithNullable.java new file mode 100644 index 000000000000..2b58f1b37d0f --- /dev/null +++ b/java/java-tests/testData/inspection/nullableProblems/OverridingNotNullCollectionWithNullable.java @@ -0,0 +1,30 @@ +import typeUse.*; +import java.util.*; + +abstract class Parent { + + abstract @NotNull String @NotNull [] getStrings(); + + abstract @NotNull List<@NotNull String> getStringList(); + + abstract void foo(@Nullable String @NotNull [] p1, + @NotNull List<@Nullable String> p2); +} + +class Child extends Parent { + + @Override + @Nullable String @NotNull [] getStrings() { + throw new UnsupportedOperationException(); + } + + @Override + @NotNull List<@Nullable String> getStringList() { + throw new UnsupportedOperationException(); + } + + void foo(@NotNull String @NotNull [] p1, + @NotNull List<@NotNull String> p2) { + + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/NullableStuffInspectionTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/NullableStuffInspectionTest.java index 9efbd5278142..4df0ecf7cf25 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/NullableStuffInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/NullableStuffInspectionTest.java @@ -254,6 +254,11 @@ public class NullableStuffInspectionTest extends LightJavaCodeInsightFixtureTest doTest(); } + public void testOverridingNotNullCollectionWithNullable() { + DataFlowInspection8Test.setupTypeUseAnnotations("typeUse", myFixture); + doTest(); + } + public void testNotNullCollectionItemWithNullableSuperType() { DataFlowInspection8Test.setupTypeUseAnnotations("typeUse", myFixture); doTest();