From c22dda7b02e12c14daf4984b34396bb49e8a64cf Mon Sep 17 00:00:00 2001 From: peter Date: Mon, 28 May 2018 20:57:51 +0200 Subject: [PATCH] IDEA-192527 warning persists on field when overriding @Nonnull custom annotation with a @Nonnull(When.Maybe) annotation 1. understand When.Maybe 2. stop at the first default annotation, of any nullity --- .../InferredAnnotationsManagerImpl.java | 3 +- .../NullableNotNullManagerImpl.java | 45 +++++++++++- .../CheckerFrameworkNullityUtil.java | 36 +++++---- .../codeInsight/NullableNotNullManager.java | 73 ++++++++----------- .../intellij/codeInsight/NullityDefault.java | 18 +++++ ...lableDefaultOnClassVsNonnullOnPackage.java | 29 ++++++++ .../NullableStuffInspectionTest.java | 10 +++ 7 files changed, 154 insertions(+), 60 deletions(-) create mode 100644 java/java-psi-api/src/com/intellij/codeInsight/NullityDefault.java create mode 100644 java/java-tests/testData/inspection/nullableProblems/NullableDefaultOnClassVsNonnullOnPackage.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInsight/InferredAnnotationsManagerImpl.java b/java/java-analysis-impl/src/com/intellij/codeInsight/InferredAnnotationsManagerImpl.java index 2e48a916eadd..1cfbf3b94d0c 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInsight/InferredAnnotationsManagerImpl.java +++ b/java/java-analysis-impl/src/com/intellij/codeInsight/InferredAnnotationsManagerImpl.java @@ -131,8 +131,7 @@ public class InferredAnnotationsManagerImpl extends InferredAnnotationsManager { return null; } - if (NullableNotNullManager.findNullabilityDefaultInHierarchy(method, true) != null || - NullableNotNullManager.findNullabilityDefaultInHierarchy(method, false) != null) { + if (manager.findNullityDefaultInHierarchy(method) != null) { return null; } diff --git a/java/java-impl/src/com/intellij/codeInsight/NullableNotNullManagerImpl.java b/java/java-impl/src/com/intellij/codeInsight/NullableNotNullManagerImpl.java index cf6248e23c8e..fbdbed46d538 100644 --- a/java/java-impl/src/com/intellij/codeInsight/NullableNotNullManagerImpl.java +++ b/java/java-impl/src/com/intellij/codeInsight/NullableNotNullManagerImpl.java @@ -20,12 +20,10 @@ import com.intellij.util.containers.ContainerUtil; import one.util.streamex.StreamEx; import org.jdom.Element; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import org.jetbrains.jps.model.serialization.java.compiler.JpsJavaCompilerNotNullableSerializer; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.List; +import java.util.*; @State(name = "NullableNotNullManager") public class NullableNotNullManagerImpl extends NullableNotNullManager implements PersistentStateComponent { @@ -117,6 +115,45 @@ public class NullableNotNullManagerImpl extends NullableNotNullManager implement return result; } + @Override + protected NullityDefault isJsr305Default(@NotNull PsiAnnotation annotation, @NotNull PsiAnnotation.TargetType[] placeTargetTypes) { + PsiClass declaration = resolveAnnotationType(annotation); + PsiModifierList modList = declaration == null ? null : declaration.getModifierList(); + if (modList == null) return null; + + PsiAnnotation tqDefault = AnnotationUtil.findAnnotation(declaration, true, "javax.annotation.meta.TypeQualifierDefault"); + if (tqDefault == null) return null; + + Set required = AnnotationTargetUtil.extractRequiredAnnotationTargets(tqDefault.findAttributeValue(null)); + if (required == null || (!required.isEmpty() && !ContainerUtil.intersects(required, Arrays.asList(placeTargetTypes)))) return null; + + for (PsiAnnotation qualifier : modList.getAnnotations()) { + Nullness nullness = getJsr305QualifierNullness(qualifier); + if (nullness != null) { + return new NullityDefault(annotation, nullness == Nullness.NULLABLE); + } + } + return null; + } + + @Nullable + private static PsiClass resolveAnnotationType(@NotNull PsiAnnotation annotation) { + PsiJavaCodeReferenceElement element = annotation.getNameReferenceElement(); + PsiElement declaration = element == null ? null : element.resolve(); + if (!(declaration instanceof PsiClass) || !((PsiClass)declaration).isAnnotationType()) return null; + return (PsiClass)declaration; + } + + @Nullable + private Nullness getJsr305QualifierNullness(@NotNull PsiAnnotation qualifier) { + String qName = qualifier.getQualifiedName(); + if (qName == null || !qName.startsWith("javax.annotation.")) return null; + + if (qName.equals(JAVAX_ANNOTATION_NULLABLE) && getNullables().contains(qName)) return Nullness.NULLABLE; + if (qName.equals(JAVAX_ANNOTATION_NONNULL)) return extractNullityFromWhenValue(qualifier); + return null; + } + private static boolean isNullabilityNickName(@NotNull PsiClass candidate) { String qname = candidate.getQualifiedName(); if (qname == null || qname.startsWith("javax.annotation.")) return false; diff --git a/java/java-psi-api/src/com/intellij/codeInsight/CheckerFrameworkNullityUtil.java b/java/java-psi-api/src/com/intellij/codeInsight/CheckerFrameworkNullityUtil.java index 5a519ba29c9e..a2df95d8be94 100644 --- a/java/java-psi-api/src/com/intellij/codeInsight/CheckerFrameworkNullityUtil.java +++ b/java/java-psi-api/src/com/intellij/codeInsight/CheckerFrameworkNullityUtil.java @@ -4,6 +4,7 @@ package com.intellij.codeInsight; import com.intellij.psi.*; import com.intellij.psi.util.PsiUtil; import com.intellij.util.containers.ContainerUtil; +import org.jetbrains.annotations.Nullable; import java.util.Set; @@ -14,30 +15,39 @@ class CheckerFrameworkNullityUtil { private static final String DEFAULT_QUALIFIER = "org.checkerframework.framework.qual.DefaultQualifier"; private static final String DEFAULT_QUALIFIERS = "org.checkerframework.framework.qual.DefaultQualifiers"; - static boolean isCheckerDefault(PsiAnnotation anno, boolean nullable, PsiAnnotation.TargetType[] types) { + @Nullable + static NullityDefault isCheckerDefault(PsiAnnotation anno, PsiAnnotation.TargetType[] types) { String qName = anno.getQualifiedName(); if (DEFAULT_QUALIFIER.equals(qName)) { PsiAnnotationMemberValue value = anno.findAttributeValue(PsiAnnotation.DEFAULT_REFERENCED_METHOD_NAME); - return value instanceof PsiClassObjectAccessExpression && - isNullityAnnotationReference(nullable, (PsiClassObjectAccessExpression)value) && - hasAppropriateTarget(types, anno.findAttributeValue("locations")); + if (value instanceof PsiClassObjectAccessExpression && + hasAppropriateTarget(types, anno.findAttributeValue("locations"))) { + PsiClass valueClass = PsiUtil.resolveClassInClassTypeOnly(((PsiClassObjectAccessExpression)value).getOperand().getType()); + if (valueClass != null) { + NullableNotNullManager instance = NullableNotNullManager.getInstance(value.getProject()); + if (instance.getNullables().contains(valueClass.getQualifiedName())) { + return new NullityDefault(anno, true); + } + if (instance.getNotNulls().contains(valueClass.getQualifiedName())) { + return new NullityDefault(anno, true); + } + } + } + return null; } if (DEFAULT_QUALIFIERS.equals(qName)) { PsiAnnotationMemberValue value = anno.findAttributeValue(PsiAnnotation.DEFAULT_REFERENCED_METHOD_NAME); for (PsiAnnotationMemberValue initializer : AnnotationUtil.arrayAttributeValues(value)) { - if (initializer instanceof PsiAnnotation && isCheckerDefault((PsiAnnotation)initializer, nullable, types)) { - return true; + if (initializer instanceof PsiAnnotation) { + NullityDefault result = isCheckerDefault((PsiAnnotation)initializer, types); + if (result != null) { + return result; + } } } } - return false; - } - - private static boolean isNullityAnnotationReference(boolean nullable, PsiClassObjectAccessExpression value) { - PsiClass valueClass = PsiUtil.resolveClassInClassTypeOnly(value.getOperand().getType()); - NullableNotNullManager instance = NullableNotNullManager.getInstance(value.getProject()); - return valueClass != null && (nullable ? instance.getNullables() : instance.getNotNulls()).contains(valueClass.getQualifiedName()); + return null; } private static boolean hasAppropriateTarget(PsiAnnotation.TargetType[] types, PsiAnnotationMemberValue locations) { diff --git a/java/java-psi-api/src/com/intellij/codeInsight/NullableNotNullManager.java b/java/java-psi-api/src/com/intellij/codeInsight/NullableNotNullManager.java index 1f0901ae27aa..44f713bd9589 100644 --- a/java/java-psi-api/src/com/intellij/codeInsight/NullableNotNullManager.java +++ b/java/java-psi-api/src/com/intellij/codeInsight/NullableNotNullManager.java @@ -29,7 +29,7 @@ public abstract class NullableNotNullManager { @SuppressWarnings("deprecation") public final JDOMExternalizableStringList myNullables = new JDOMExternalizableStringList(); @SuppressWarnings("deprecation") public final JDOMExternalizableStringList myNotNulls = new JDOMExternalizableStringList(); - private static final String JAVAX_ANNOTATION_NULLABLE = "javax.annotation.Nullable"; + protected static final String JAVAX_ANNOTATION_NULLABLE = "javax.annotation.Nullable"; protected static final String JAVAX_ANNOTATION_NONNULL = "javax.annotation.Nonnull"; static final String[] DEFAULT_NULLABLES = { @@ -109,12 +109,12 @@ public abstract class NullableNotNullManager { @Nullable public PsiAnnotation getNullableAnnotation(@NotNull PsiModifierListOwner owner, boolean checkBases) { - return findNullabilityAnnotationWithDefault(owner, checkBases, true); + return findNullityAnnotationWithDefault(owner, checkBases, true); } public boolean isContainerAnnotation(@NotNull PsiAnnotation anno) { PsiAnnotation.TargetType[] acceptAnyTarget = PsiAnnotation.TargetType.values(); - return isNullabilityDefault(anno, true, acceptAnyTarget, false) || isNullabilityDefault(anno, false, acceptAnyTarget, false); + return checkNullityDefault(anno, acceptAnyTarget, false) != null; } public void setDefaultNullable(@NotNull String defaultNullable) { @@ -129,7 +129,7 @@ public abstract class NullableNotNullManager { @Nullable public PsiAnnotation getNotNullAnnotation(@NotNull PsiModifierListOwner owner, boolean checkBases) { - return findNullabilityAnnotationWithDefault(owner, checkBases, false); + return findNullityAnnotationWithDefault(owner, checkBases, false); } @Nullable @@ -192,8 +192,8 @@ public abstract class NullableNotNullManager { } @Nullable - private PsiAnnotation findNullabilityAnnotationWithDefault(@NotNull PsiModifierListOwner owner, boolean checkBases, boolean nullable) { - PsiAnnotation annotation = findPlainNullabilityAnnotation(owner, checkBases); + private PsiAnnotation findNullityAnnotationWithDefault(@NotNull PsiModifierListOwner owner, boolean checkBases, boolean nullable) { + PsiAnnotation annotation = findPlainNullityAnnotation(owner, checkBases); if (annotation != null) { String qName = annotation.getQualifiedName(); if (qName == null) return null; @@ -225,20 +225,21 @@ public abstract class NullableNotNullManager { } } - return findNullabilityDefaultInHierarchy(owner, nullable); + NullityDefault nullityDefault = findNullityDefaultInHierarchy(owner); + return nullityDefault != null && nullityDefault.isNullableDefault == nullable ? nullityDefault.annotation : null; } private PsiAnnotation takeAnnotationFromSuperParameters(@NotNull PsiParameter owner, final List superOwners) { return RecursionManager.doPreventingRecursion(owner, true, () -> { for (PsiParameter superOwner : superOwners) { - PsiAnnotation anno = findNullabilityAnnotationWithDefault(superOwner, false, false); + PsiAnnotation anno = findNullityAnnotationWithDefault(superOwner, false, false); if (anno != null) return anno; } return null; }); } - private PsiAnnotation findPlainNullabilityAnnotation(@NotNull PsiModifierListOwner owner, boolean checkBases) { + private PsiAnnotation findPlainNullityAnnotation(@NotNull PsiModifierListOwner owner, boolean checkBases) { Set qNames = ContainerUtil.newHashSet(getNullablesWithNickNames()); qNames.addAll(getNotNullsWithNickNames()); PsiAnnotation memberAnno = checkBases && owner instanceof PsiMethod @@ -292,30 +293,30 @@ public abstract class NullableNotNullManager { } public boolean isNullable(@NotNull PsiModifierListOwner owner, boolean checkBases) { - return findNullabilityAnnotationWithDefault(owner, checkBases, true) != null; + return findNullityAnnotationWithDefault(owner, checkBases, true) != null; } public boolean isNotNull(@NotNull PsiModifierListOwner owner, boolean checkBases) { - return findNullabilityAnnotationWithDefault(owner, checkBases, false) != null; + return findNullityAnnotationWithDefault(owner, checkBases, false) != null; } @Nullable - static PsiAnnotation findNullabilityDefaultInHierarchy(PsiModifierListOwner owner, boolean nullable) { + NullityDefault findNullityDefaultInHierarchy(@NotNull PsiModifierListOwner owner) { PsiAnnotation.TargetType[] placeTargetTypes = AnnotationTargetUtil.getTargetsForLocation(owner.getModifierList()); PsiElement element = owner.getParent(); while (element != null) { if (element instanceof PsiModifierListOwner) { - PsiAnnotation annotation = getNullabilityDefault((PsiModifierListOwner)element, nullable, placeTargetTypes, false); - if (annotation != null) { - return annotation; + NullityDefault result = getNullityDefault((PsiModifierListOwner)element, placeTargetTypes, false); + if (result != null) { + return result; } } if (element instanceof PsiClassOwner) { String packageName = ((PsiClassOwner)element).getPackageName(); - return findNullabilityDefaultOnPackage(nullable, placeTargetTypes, - JavaPsiFacade.getInstance(element.getProject()).findPackage(packageName)); + return findNullityDefaultOnPackage(placeTargetTypes, + JavaPsiFacade.getInstance(element.getProject()).findPackage(packageName)); } element = element.getContext(); @@ -323,12 +324,11 @@ public abstract class NullableNotNullManager { return null; } - private static PsiAnnotation findNullabilityDefaultOnPackage(boolean nullable, - PsiAnnotation.TargetType[] placeTargetTypes, - @Nullable PsiPackage psiPackage) { + @Nullable + private NullityDefault findNullityDefaultOnPackage(PsiAnnotation.TargetType[] placeTargetTypes, @Nullable PsiPackage psiPackage) { boolean superPackage = false; while (psiPackage != null) { - PsiAnnotation onPkg = getNullabilityDefault(psiPackage, nullable, placeTargetTypes, superPackage); + NullityDefault onPkg = getNullityDefault(psiPackage, placeTargetTypes, superPackage); if (onPkg != null) return onPkg; superPackage = true; psiPackage = psiPackage.getParentPackage(); @@ -336,36 +336,27 @@ public abstract class NullableNotNullManager { return null; } - private static PsiAnnotation getNullabilityDefault(@NotNull PsiModifierListOwner container, boolean nullable, PsiAnnotation.TargetType[] placeTargetTypes, boolean superPackage) { + @Nullable + private NullityDefault getNullityDefault(PsiModifierListOwner container, PsiAnnotation.TargetType[] placeTargetTypes, boolean superPackage) { PsiModifierList modifierList = container.getModifierList(); if (modifierList == null) return null; for (PsiAnnotation annotation : modifierList.getAnnotations()) { - if (isNullabilityDefault(annotation, nullable, placeTargetTypes, superPackage)) { - return annotation; + NullityDefault result = checkNullityDefault(annotation, placeTargetTypes, superPackage); + if (result != null) { + return result; } } return null; } - private static boolean isNullabilityDefault(@NotNull PsiAnnotation annotation, boolean nullable, PsiAnnotation.TargetType[] placeTargetTypes, boolean superPackage) { - if (!superPackage && isJsr305Default(annotation, nullable, placeTargetTypes)) return true; - return CheckerFrameworkNullityUtil.isCheckerDefault(annotation, nullable, placeTargetTypes); + @Nullable + private NullityDefault checkNullityDefault(PsiAnnotation annotation, PsiAnnotation.TargetType[] placeTargetTypes, boolean superPackage) { + NullityDefault jsr = superPackage ? null : isJsr305Default(annotation, placeTargetTypes); + return jsr != null ? jsr : CheckerFrameworkNullityUtil.isCheckerDefault(annotation, placeTargetTypes); } - private static boolean isJsr305Default(PsiAnnotation annotation, boolean nullable, PsiAnnotation.TargetType[] placeTargetTypes) { - PsiJavaCodeReferenceElement element = annotation.getNameReferenceElement(); - PsiElement declaration = element == null ? null : element.resolve(); - if (!(declaration instanceof PsiClass)) return false; - - String fqn = nullable ? JAVAX_ANNOTATION_NULLABLE : JAVAX_ANNOTATION_NONNULL; - if (!isAnnotated((PsiClass)declaration, fqn, CHECK_TYPE)) return false; - - PsiAnnotation tqDefault = findAnnotation((PsiClass)declaration, true, "javax.annotation.meta.TypeQualifierDefault"); - if (tqDefault == null) return false; - - Set required = AnnotationTargetUtil.extractRequiredAnnotationTargets(tqDefault.findAttributeValue(null)); - return required != null && (required.isEmpty() || ContainerUtil.intersects(required, Arrays.asList(placeTargetTypes))); - } + @Nullable + protected abstract NullityDefault isJsr305Default(@NotNull PsiAnnotation annotation, @NotNull PsiAnnotation.TargetType[] placeTargetTypes); @NotNull public List getNullables() { diff --git a/java/java-psi-api/src/com/intellij/codeInsight/NullityDefault.java b/java/java-psi-api/src/com/intellij/codeInsight/NullityDefault.java new file mode 100644 index 000000000000..4b9220c9d6d1 --- /dev/null +++ b/java/java-psi-api/src/com/intellij/codeInsight/NullityDefault.java @@ -0,0 +1,18 @@ +// 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.codeInsight; + +import com.intellij.psi.PsiAnnotation; +import org.jetbrains.annotations.NotNull; + +/** + * @author peter + */ +class NullityDefault { + @NotNull final PsiAnnotation annotation; + final boolean isNullableDefault; + + NullityDefault(@NotNull PsiAnnotation annotation, boolean isNullableDefault) { + this.annotation = annotation; + this.isNullableDefault = isNullableDefault; + } +} diff --git a/java/java-tests/testData/inspection/nullableProblems/NullableDefaultOnClassVsNonnullOnPackage.java b/java/java-tests/testData/inspection/nullableProblems/NullableDefaultOnClassVsNonnullOnPackage.java new file mode 100644 index 000000000000..e61e554d06c2 --- /dev/null +++ b/java/java-tests/testData/inspection/nullableProblems/NullableDefaultOnClassVsNonnullOnPackage.java @@ -0,0 +1,29 @@ +package foo; + +import javax.annotation.*; +import javax.annotation.meta.TypeQualifierDefault; +import javax.annotation.meta.When; +import java.lang.annotation.ElementType; + +@Nonnull +@TypeQualifierDefault({ElementType.FIELD, ElementType.METHOD, ElementType.PARAMETER, ElementType.TYPE_USE}) +@interface NonnullByDefault {} + +@Nonnull(when = When.MAYBE) +@TypeQualifierDefault({ElementType.FIELD, ElementType.METHOD, ElementType.PARAMETER, ElementType.TYPE_USE}) +@interface NullableByDefault { } + +@NullableByDefault +class Bug { + + // This is emitting a warning when it shouldn't be. + private String id; + + public String getId() { + return id; + } + + public void setId(String id) { + this.id = id; + } +} \ 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 b7f7c54153da..e030eaa25173 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/NullableStuffInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/NullableStuffInspectionTest.java @@ -226,6 +226,16 @@ public class NullableStuffInspectionTest extends LightCodeInsightFixtureTestCase myFixture.checkHighlighting(true, false, true); } + public void testNullableDefaultOnClassVsNonnullOnPackage() { + DataFlowInspectionTest.addJavaxNullabilityAnnotations(myFixture); + DataFlowInspectionTest.addJavaxDefaultNullabilityAnnotations(myFixture); + myFixture.addFileToProject("foo/package-info.java", "@NonnullByDefault package foo;"); + + myFixture.configureFromExistingVirtualFile(myFixture.copyFileToProject(getTestName(false) + ".java", "foo/Classes.java")); + myFixture.enableInspections(myInspection); + myFixture.checkHighlighting(true, false, true); + } + public void testBeanValidationNotNull() { myFixture.addClass("package javax.annotation.constraints; public @interface NotNull{}"); DataFlowInspection8Test.setCustomAnnotations(getProject(), getTestRootDisposable(), "javax.annotation.constraints.NotNull", "javax.annotation.constraints.Nullable");