diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ClassInitializationDeadlock.java b/core/src/main/java/com/google/errorprone/bugpatterns/ClassInitializationDeadlock.java index bc61acd3db7f..7210e2a5f2e0 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ClassInitializationDeadlock.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ClassInitializationDeadlock.java @@ -16,12 +16,20 @@ package com.google.errorprone.bugpatterns; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.fixes.SuggestedFixes.prettyType; import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.util.ASTHelpers.getSymbol; import static com.google.errorprone.util.ASTHelpers.isEffectivelyPrivate; import static com.google.errorprone.util.ASTHelpers.isStatic; +import static java.util.stream.Collectors.joining; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Streams; +import com.google.common.graph.GraphBuilder; +import com.google.common.graph.MutableGraph; +import com.google.common.graph.Traverser; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.matchers.Description; @@ -38,6 +46,8 @@ import com.sun.source.util.TreePathScanner; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol.ClassSymbol; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.code.Types; import javax.lang.model.element.ElementKind; /** See the summary. */ @@ -131,56 +141,86 @@ private void handle(ExpressionTree tree) { if (use.equals(classSymbol)) { return; } - if (isEffectivelyPrivate(use) - && ((ClassSymbol) use).getSuperclass().asElement().equals(classSymbol)) { - // Cycles involving `private` member classes are usually benign, because the private - // class cannot be directly accessed from outside the current file, so typically accessing - // it requires first accessing the enclosing class. - // - // See also discussion of `private` classes in - // https://errorprone.info/bugpattern/ClassInitializationDeadlock - return; - } - if (use.getSimpleName().toString().startsWith("AutoValue_")) { - // Cycles involving AutoValue-generated code are usually benign, because the generated - // code should only be accessed within the declaration of the corresponding base class, - // so the base class will always be initialized first. - // - // See also discussion of AutoValue in - // https://errorprone.info/bugpattern/ClassInitializationDeadlock - return; - } - if (!ASTHelpers.scope(use.members()) - .anyMatch(ClassInitializationDeadlock::nonPrivateConstructorOrFactory)) { - // If the class can't be instantiated outside the current compilation unit, because it has - // no non-private - // constructors or static methods (which could be factory methods), it can't be directly - // instantiated outside - // the current file. Similar to private classes, assume that initializations of it will - // first initialize the - // enclosing class. + if (!use.isSubClass(classSymbol, state.getTypes())) { return; } if (!isStatic(use)) { // Nested inner classes implicitly take the enclosing instance as a constructor parameter, - // and can't be - // initialized without first initializing their containing class. + // and can't be initialized without first initializing their containing class. + return; + } + ImmutableSet nonPrivateInstantiators = + nonPrivateInstantiators((ClassSymbol) use, classSymbol, state.getTypes()); + if (nonPrivateInstantiators.isEmpty()) { return; } - if (use.isSubClass(classSymbol, state.getTypes())) { - state.reportMatch( - buildDescription(tree) - .setMessage( - String.format( - "Possible class initialization deadlock: %s is a subclass of the" - + " containing class %s", - use, classSymbol)) - .build()); + StringBuilder message = new StringBuilder(); + message.append( + String.format( + "Possible class initialization deadlock: %s is a subclass of the" + + " containing class %s", + prettyType(use.asType(), state), prettyType(classSymbol.asType(), state))); + if (!nonPrivateInstantiators.contains(use)) { + message.append( + String.format( + " (via %s, which can be initialized from outside the current file)", + nonPrivateInstantiators.stream() + .map(s -> prettyType(s.asType(), state)) + .collect(joining(", ")))); } + state.reportMatch(buildDescription(tree).setMessage(message.toString()).build()); } }.scan(path, null); } + // Cycles involving classes that can't be instantiated outside the current file are usually + // benign, since accessing them typically requires first accessing the enclosing class. + // See also discussion of `private` classes in + // https://errorprone.info/bugpattern/ClassInitializationDeadlock + private ImmutableSet nonPrivateInstantiators( + ClassSymbol from, ClassSymbol to, Types types) { + MutableGraph superTypes = GraphBuilder.directed().build(); + superTypeClosure(from, to, types, superTypes); + return Streams.stream(Traverser.forGraph(superTypes::predecessors).depthFirstPreOrder(to)) + .filter(current -> current != to && nonPrivateInstantiator(current)) + .collect(toImmutableSet()); + } + + private static void superTypeClosure( + ClassSymbol current, ClassSymbol stop, Types types, MutableGraph superTypes) { + if (current == stop) { + return; + } + for (Type t : types.directSupertypes(current.type)) { + ClassSymbol s = (ClassSymbol) t.tsym; + superTypes.putEdge(current, s); + superTypeClosure(s, stop, types, superTypes); + } + } + + // Returns true if the class can't be instantiated outside the current compilation unit. + boolean nonPrivateInstantiator(ClassSymbol use) { + if (isEffectivelyPrivate(use)) { + // If the class (or a containing class) is private, it can't be instantiated outside the + // current unit. + return false; + } + if (!ASTHelpers.scope(use.members()) + .anyMatch(ClassInitializationDeadlock::nonPrivateConstructorOrFactory)) { + // If the class has no non-private constructors or static methods (which could be factory + // methods), it can't be directly instantiated outside the current file. + return false; + } + if (use.getSimpleName().toString().startsWith("AutoValue_")) { + // AutoValue generated code is necessarily package-private, but should only be accessed + // within the declaration of the corresponding base class. See also the discussion of + // AutoValue in + // https://errorprone.info/bugpattern/ClassInitializationDeadlock + return false; + } + return true; + } + private static boolean nonPrivateConstructorOrFactory(Symbol symbol) { if (symbol.isPrivate()) { return false; diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ClassInitializationDeadlockTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ClassInitializationDeadlockTest.java index 6a998ccfa626..8cade0cb24b4 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ClassInitializationDeadlockTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ClassInitializationDeadlockTest.java @@ -224,4 +224,46 @@ public void negativeSelf() { "}") .doTest(); } + + @Test + public void negativePrivateInterface() { + testHelper + .addSourceLines( + "A.java", // + "public class A {", + " private interface I {}", + " static final I i = new I() {};", + "}") + .doTest(); + } + + @Test + public void intermediateNonPrivate() { + testHelper + .addSourceLines( + "A.java", // + "public class A {", + " // BUG: Diagnostic contains: C is a subclass of the containing class A (via B, which" + + " can be initialized from outside the current file)", + " public static final C i = new C();", + " public static class B extends A {}", + " private static class C extends B {}", + "}") + .doTest(); + } + + @Test + public void negativeNonPrivateUnrelatedSuper() { + testHelper + .addSourceLines( + "A.java", // + "public class A {", + " public static final C i = new C();", + " public interface B {", + " default void f() {}", + " }", + " private static class C extends A implements B {}", + "}") + .doTest(); + } }