diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java index 6fbb5d0ebfb9..7f2dab5c5664 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java @@ -124,7 +124,7 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState if (info.isPresent()) { state.reportMatch(buildDescription(tree).setMessage(info.message()).build()); } - if (!hasImmutableAnnotation(lambdaType, state)) { + if (!typeOrSuperHasImmutableAnnotation(lambdaType, state)) { return NO_MATCH; } checkClosedTypes(tree, state, lambdaType, analysis); @@ -132,11 +132,6 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState return NO_MATCH; } - private boolean hasImmutableAnnotation(TypeSymbol tsym, VisitorState state) { - return immutableAnnotations.stream() - .anyMatch(annotation -> hasAnnotation(tsym, annotation, state)); - } - @Override public Description matchMemberReference(MemberReferenceTree tree, VisitorState state) { // check instantiations of `@ImmutableTypeParameter`s in method references @@ -150,7 +145,7 @@ public Description matchMemberReference(MemberReferenceTree tree, VisitorState s if (info.isPresent()) { state.reportMatch(buildDescription(tree).setMessage(info.message()).build()); } - if (!hasImmutableAnnotation(memberReferenceType, state)) { + if (!typeOrSuperHasImmutableAnnotation(memberReferenceType, state)) { return NO_MATCH; } if (getSymbol(getReceiver(tree)) instanceof ClassSymbol) { @@ -480,7 +475,7 @@ private void handleIdentifier(Symbol symbol) { for (var entry : typesClosed.asMap().entrySet()) { var classSymbol = entry.getKey(); var methods = entry.getValue(); - if (!hasImmutableAnnotation(classSymbol.type.tsym, state)) { + if (!typeOrSuperHasImmutableAnnotation(classSymbol.type.tsym, state)) { String message = format( "%s, but accesses instance method(s) '%s' on '%s' which is not @Immutable.", @@ -587,6 +582,15 @@ private Type immutableSupertype(Symbol sym, VisitorState state) { return null; } + private boolean hasImmutableAnnotation(TypeSymbol tsym, VisitorState state) { + return immutableAnnotations.stream() + .anyMatch(annotation -> hasAnnotation(tsym, annotation, state)); + } + + private boolean typeOrSuperHasImmutableAnnotation(TypeSymbol tsym, VisitorState state) { + return hasImmutableAnnotation(tsym, state) || immutableSupertype(tsym, state) != null; + } + /** * Gets the set of in-scope immutable type parameters from the containerOf specs on * {@code @Immutable} annotations. diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafety.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafety.java index 2c80a2d9b401..b3888007e7fd 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafety.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ThreadSafety.java @@ -714,31 +714,36 @@ private AnnotationInfo getAnnotation( if (!(sym instanceof ClassSymbol)) { return null; } - Type superClass = ((ClassSymbol) sym).getSuperclass(); - AnnotationInfo superAnnotation = getInheritedAnnotation(superClass.asElement(), state); - if (superAnnotation == null) { - return null; - } - // If an annotated super-type was found, look for any type arguments to the super-type that - // are in the super-type's containerOf spec, and where the arguments are type parameters - // of the current class. - // E.g. for `Foo extends Super` if `Super` is annotated - // `@ThreadSafeContainerAnnotation Y` - // then `Foo` is has X implicitly annotated `@ThreadSafeContainerAnnotation X` - ImmutableList.Builder containerOf = ImmutableList.builder(); - for (int i = 0; i < superClass.getTypeArguments().size(); i++) { - Type arg = superClass.getTypeArguments().get(i); - TypeVariableSymbol formal = superClass.asElement().getTypeParameters().get(i); - if (!arg.hasTag(TypeTag.TYPEVAR)) { + for (Type superClass : state.getTypes().closure(sym.type)) { + if (superClass.tsym.equals(sym)) { + continue; + } + AnnotationInfo superAnnotation = getInheritedAnnotation(superClass.asElement(), state); + if (superAnnotation == null) { continue; } - TypeSymbol argSym = arg.asElement(); - if (argSym.owner == sym - && superAnnotation.containerOf().contains(formal.getSimpleName().toString())) { - containerOf.add(argSym.getSimpleName().toString()); + // If an annotated super-type was found, look for any type arguments to the super-type that + // are in the super-type's containerOf spec, and where the arguments are type parameters + // of the current class. + // E.g. for `Foo extends Super` if `Super` is annotated + // `@ThreadSafeContainerAnnotation Y` + // then `Foo` is has X implicitly annotated `@ThreadSafeContainerAnnotation X` + ImmutableList.Builder containerOf = ImmutableList.builder(); + for (int i = 0; i < superClass.getTypeArguments().size(); i++) { + Type arg = superClass.getTypeArguments().get(i); + TypeVariableSymbol formal = superClass.asElement().getTypeParameters().get(i); + if (!arg.hasTag(TypeTag.TYPEVAR)) { + continue; + } + TypeSymbol argSym = arg.asElement(); + if (argSym.owner == sym + && superAnnotation.containerOf().contains(formal.getSimpleName().toString())) { + containerOf.add(argSym.getSimpleName().toString()); + } } + return AnnotationInfo.create(superAnnotation.typeName(), containerOf.build()); } - return AnnotationInfo.create(superAnnotation.typeName(), containerOf.build()); + return null; } /** diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableCheckerTest.java index 82dd6e1b109a..adb481b68954 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableCheckerTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableCheckerTest.java @@ -2006,7 +2006,8 @@ public void immutableUpperBoundAndContainerOfInconsistency() { "@Immutable class Test {", " final WithContainerOf a = null;", " final WithoutContainerOf b = null;", - " // BUG: Diagnostic contains: field 'c' of type 'WithContainerOf'", + // Even though MutableImpl is mutable, it extends an @Immutable class, so we believe it + // is. (The violation is suppressed at the MutableImpl declaration.) " final WithContainerOf c = null;", " final WithoutContainerOf d = null;", "}") @@ -2774,6 +2775,25 @@ public void methodReference_onMutableType() { .doTest(); } + @Test + public void methodReference_onSubtypeOfImmutableType() { + compilationHelper + .addSourceLines( + "Test.java", + "import com.google.errorprone.annotations.Immutable;", + "import java.util.HashMap;", + "import java.util.Map;", + "abstract class Test {", + " @Immutable interface ImmutableFunction { String apply(String b); }", + " interface SubFunction extends ImmutableFunction {}", + " void test(ImmutableFunction f) {", + " SubFunction sf = null;", + " test(sf::apply);", + " }", + "}") + .doTest(); + } + @Test public void methodReference_onExpressionWithMutableType() { compilationHelper