Skip to content

Commit

Permalink
ClassInitializationDeadlock: improve handling of classes that can't b…
Browse files Browse the repository at this point in the history 
…e 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.

#2062

PiperOrigin-RevId: 617314919
  • Loading branch information
@cushon
cushon authored and Error Prone Team committed Mar 19, 2024
1 parent 449618c commit e5f0814
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 39 deletions.
118 changes: 79 additions & 39 deletions core/src/main/java/com/google/errorprone/bugpatterns/ClassInitializationDeadlock.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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. */
Expand Down Expand Up @@ -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<ClassSymbol> 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<ClassSymbol> nonPrivateInstantiators(
ClassSymbol from, ClassSymbol to, Types types) {
MutableGraph<ClassSymbol> 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<ClassSymbol> 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;
Expand Down
42 changes: 42 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/ClassInitializationDeadlockTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}

0 comments on commit e5f0814

Please sign in to comment.