From 2bcc14fd237a36e521ce7181b982026513ce37b9 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Thu, 14 Mar 2024 18:29:39 -0700 Subject: [PATCH] ClassInitializationDeadlock: improve handling of classes that can't be initialized outside the current compilation unit Consider all classes in the super type hierarchy between the subclass and containing superclass, to catch deadlocks where an intermediate supertype can be instantiated. This also fixes an NPE with an existing check for the direct super type. https://github.com/google/error-prone/issues/2062 PiperOrigin-RevId: 615968917 --- .../ClassInitializationDeadlock.java | 118 ++++++++++++------ .../ClassInitializationDeadlockTest.java | 42 +++++++ 2 files changed, 121 insertions(+), 39 deletions(-) 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(); + } }