From 57b85a658132c4325e30af7959ee4714ac0d39d4 Mon Sep 17 00:00:00 2001 From: cpovirk Date: Thu, 2 Jul 2026 08:59:02 -0700 Subject: [PATCH] `EffectivelyPrivate`: Exclude anything declared within an effectively private class that has a visible subclass in the same file. See discussion in https://github.com/google/error-prone/issues/5182. PiperOrigin-RevId: 941723947 --- .../bugpatterns/EffectivelyPrivate.java | 101 ++++++++++++------ .../bugpatterns/EffectivelyPrivateTest.java | 54 ++++++++++ 2 files changed, 121 insertions(+), 34 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/EffectivelyPrivate.java b/core/src/main/java/com/google/errorprone/bugpatterns/EffectivelyPrivate.java index 40a6788e7b0..fd31960fa0c 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/EffectivelyPrivate.java +++ b/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,21 +28,21 @@ 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; @@ -49,9 +50,7 @@ @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 hasVisibleSubclass = findClassesWithVisibleSubclasses(tree, state); + new SuppressibleTreePathScanner(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 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 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_TO_REMOVE = ImmutableSet.of(Modifier.PUBLIC, Modifier.PROTECTED); + + private static ImmutableSet findClassesWithVisibleSubclasses( + CompilationUnitTree compilationUnit, VisitorState state) { + ImmutableSet.Builder hasVisibleSubclass = ImmutableSet.builder(); + new TreePathScanner() { + @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(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/EffectivelyPrivateTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/EffectivelyPrivateTest.java index 0e38ae6332c..c4e5671367e 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/EffectivelyPrivateTest.java +++ b/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(); + } }