EffectivelyPrivate: Exclude anything declared within an effectively private class that has a visible subclass in the same file.

core/src/main/java/com/google/errorprone/bugpatterns/EffectivelyPrivate.java

@@ -19,6 +19,7 @@
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
 import static com.google.errorprone.fixes.SuggestedFixes.removeModifiers;
 import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.util.ASTHelpers.enclosingClass;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
 import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
 import static com.google.errorprone.util.ASTHelpers.isEffectivelyPrivate;
@@ -27,31 +28,29 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
-import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
-import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
-import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher;
-import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
 import com.google.errorprone.matchers.Description;
 import com.sun.source.tree.ClassTree;
+import com.sun.source.tree.CompilationUnitTree;
 import com.sun.source.tree.MethodTree;
 import com.sun.source.tree.ModifiersTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.VariableTree;
+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.Symbol.MethodSymbol;
 import com.sun.tools.javac.code.Symbol.VarSymbol;
+import com.sun.tools.javac.code.Type;
 import java.util.Collections;
-import java.util.Optional;
 import javax.inject.Inject;
 import javax.lang.model.element.Modifier;
 
 /** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
 @BugPattern(
     summary = "This declaration has public or protected modifiers, but is effectively private.",
     severity = WARNING)
-public final class EffectivelyPrivate extends BugChecker
-    implements MethodTreeMatcher, VariableTreeMatcher, ClassTreeMatcher {
-
+public final class EffectivelyPrivate extends BugChecker implements CompilationUnitTreeMatcher {
   private final WellKnownKeep wellKnownKeep;
 
   @Inject
@@ -60,55 +59,89 @@ public final class EffectivelyPrivate extends BugChecker
   }
 
   @Override
-  public Description matchVariable(VariableTree tree, VisitorState state) {
-    VarSymbol sym = getSymbol(tree);
-    if (!sym.getKind().isField()) {
-      return NO_MATCH;
-    }
-    return match(tree, tree.getModifiers(), state);
-  }
+  public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) {
+    ImmutableSet<ClassSymbol> hasVisibleSubclass = findClassesWithVisibleSubclasses(tree, state);
+    new SuppressibleTreePathScanner<Void, Void>(state) {
+      @Override
+      public Void visitVariable(VariableTree tree, Void unused) {
+        VarSymbol sym = getSymbol(tree);
+        if (sym.getKind().isField()) {
+          match(tree, tree.getModifiers(), hasVisibleSubclass, state);
+        }
+        return super.visitVariable(tree, null);
+      }
 
-  @Override
-  public Description matchClass(ClassTree tree, VisitorState state) {
-    return match(tree, tree.getModifiers(), state);
-  }
+      @Override
+      public Void visitClass(ClassTree tree, Void unused) {
+        match(tree, tree.getModifiers(), hasVisibleSubclass, state);
+        return super.visitClass(tree, null);
+      }
 
-  @Override
-  public Description matchMethod(MethodTree tree, VisitorState state) {
-    return match(tree, tree.getModifiers(), state);
+      @Override
+      public Void visitMethod(MethodTree tree, Void unused) {
+        match(tree, tree.getModifiers(), hasVisibleSubclass, state);
+        return super.visitMethod(tree, null);
+      }
+    }.scan(tree, null);
+    return NO_MATCH;
   }
 
-  private Description match(Tree tree, ModifiersTree modifiers, VisitorState state) {
+  private void match(
+      Tree tree,
+      ModifiersTree modifiers,
+      ImmutableSet<ClassSymbol> hasVisibleSubclass,
+      VisitorState state) {
     Symbol sym = getSymbol(tree);
     if (!isEffectivelyPrivate(sym)) {
-      return NO_MATCH;
+      return;
     }
     if (wellKnownKeep.shouldKeep(tree)) {
-      return NO_MATCH;
+      return;
     }
     if (sym instanceof MethodSymbol methodSymbol) {
       if (hasAnnotation(methodSymbol, "java.lang.Override", state)) {
-        return NO_MATCH;
+        return;
       }
       // TODO: cushon - technically this should only match final classes, otherwise it could break
       // a subclass that relies on inheriting a method of a particular visibility to fulfil and
       // interface contract. Skip that for now, since many classes don't rely on that and also
       // aren't explicitly final.
       if (streamSuperMethods(methodSymbol, state.getTypes()).findAny().isPresent()) {
-        return NO_MATCH;
+        return;
       }
     }
-    if (Collections.disjoint(modifiers.getFlags(), MODIFIER_TO_REMOVE)) {
-      return NO_MATCH;
+    var enclosingClass = enclosingClass(sym);
+    if (hasVisibleSubclass.contains(enclosingClass)) {
+      return;
     }
-    Optional<SuggestedFix> fix = removeModifiers(modifiers, state, MODIFIER_TO_REMOVE);
-    if (fix.isEmpty()) {
-      // The fix may be empty for implicit modifiers, e.g. on enum constant fields
-      return NO_MATCH;
+    if (Collections.disjoint(modifiers.getFlags(), MODIFIER_TO_REMOVE)) {
+      return;
     }
-    return describeMatch(tree, fix.get());
+    removeModifiers(modifiers, state, MODIFIER_TO_REMOVE)
+        // The fix may be empty for implicit modifiers, e.g. on enum constant fields
+        .ifPresent(fix -> state.reportMatch(describeMatch(tree, fix)));
   }
 
   private static final ImmutableSet<Modifier> MODIFIER_TO_REMOVE =
       ImmutableSet.of(Modifier.PUBLIC, Modifier.PROTECTED);
+
+  private static ImmutableSet<ClassSymbol> findClassesWithVisibleSubclasses(
+      CompilationUnitTree compilationUnit, VisitorState state) {
+    ImmutableSet.Builder<ClassSymbol> hasVisibleSubclass = ImmutableSet.builder();
+    new TreePathScanner<Void, Void>() {
+      @Override
+      public Void visitClass(ClassTree tree, Void unused) {
+        ClassSymbol sym = getSymbol(tree);
+        if (!isEffectivelyPrivate(sym)) {
+          for (Type superType : state.getTypes().closure(sym.type)) {
+            if (superType.tsym instanceof ClassSymbol classSymbol) {
+              hasVisibleSubclass.add(classSymbol);
+            }
+          }
+        }
+        return super.visitClass(tree, null);
+      }
+    }.scan(compilationUnit, null);
+    return hasVisibleSubclass.build();
+  }
 }

core/src/test/java/com/google/errorprone/bugpatterns/EffectivelyPrivateTest.java

@@ -73,4 +73,58 @@ public void run() {}
             """)
         .doTest();
   }
+
+  @Test
+  public void negative_madeVisibleBySubclass() {
+    testHelper
+        .addSourceLines(
+            "Test.java",
+            """
+            class T {
+              private class A {
+                public void foo() {}
+              }
+
+              public class B extends A {}
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void positive_subclassAlsoPrivate() {
+    testHelper
+        .addSourceLines(
+            "Test.java",
+            """
+            class T {
+              private class A {
+                // BUG: Diagnostic contains:
+                public void foo() {}
+              }
+
+              private class B extends A {}
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void negative_chainOfPrivateClasses() {
+    testHelper
+        .addSourceLines(
+            "Test.java",
+            """
+            class T {
+              private class A {
+                public void foo() {}
+              }
+
+              private class B extends A {}
+
+              public class C extends B {}
+            }
+            """)
+        .doTest();
+  }
 }