From 08e222c543214942594fae201f6746685aaf3c12 Mon Sep 17 00:00:00 2001 From: Error Prone Team Date: Tue, 13 Oct 2026 12:20:12 -0700 Subject: [PATCH] Resolve canonical owner of static imports to the most general accessible supertype. `StaticImports.tryCreate` previously resolved the canonical owner of an imported static member directly to `member.owner` without verifying whether that declaring type is accessible from the package of the importing compilation unit. When a public type inherits constants from a package-private supertype or super-interface (such as protobuf `large_enum = true` shard interfaces), `NonCanonicalStaticMemberImport` suggested importing from the inaccessible package-private type outside its package. Walk `types.closure(baseType.type)` in `canonicalOwner` to select the most general supertype that is a subtype of `member.owner` and whose type (and any enclosing types) is accessible from the importing package, and only treat a single imported `ClassSymbol` as a canonical type import when it is similarly accessible from the importing package. PiperOrigin-RevId: 998810316 --- .../errorprone/bugpatterns/StaticImports.java | 62 +++- .../NonCanonicalStaticMemberImportTest.java | 324 ++++++++++++++++++ 2 files changed, 367 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StaticImports.java b/core/src/main/java/com/google/errorprone/bugpatterns/StaticImports.java index c9af955069a..c57acfc7c17 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/StaticImports.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/StaticImports.java @@ -33,6 +33,7 @@ import com.sun.tools.javac.code.Types; import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.util.Name; +import java.util.Objects; import org.jspecify.annotations.Nullable; /** @@ -133,18 +134,16 @@ private static StaticImportInfo create( // TODO(gak): we should instead find the most specific subtype with a member that is _used_ Type canonicalOwner = null; for (Symbol member : members) { - Type owner = types.erasure(member.owner.type); + Type owner = canonicalOwner(member, baseType, types, pkgSym); if (canonicalOwner == null || types.isSubtype(owner, canonicalOwner)) { canonicalOwner = owner; } } - if (canonicalOwner == null) { - return null; - } - - if (members.size() == 1 && getOnlyElement(members) instanceof ClassSymbol) { - return StaticImportInfo.create(access.toString(), getOnlyElement(members).toString()); + if (members.size() == 1 + && getOnlyElement(members) instanceof ClassSymbol classSymbol + && isTypeAccessibleFromPackage(classSymbol, pkgSym)) { + return StaticImportInfo.create(access.toString(), classSymbol.toString()); } return StaticImportInfo.create( access.getExpression().toString(), @@ -153,6 +152,24 @@ private static StaticImportInfo create( members); } + /** + * Finds the most canonical type that owns or inherits {@code member} from {@code baseType} and is + * accessible from {@code pkg}. + */ + private static Type canonicalOwner( + Symbol member, Symbol.TypeSymbol baseType, Types types, Symbol.PackageSymbol pkg) { + Type memberOwner = types.erasure(member.owner.type); + Type owner = types.erasure(baseType.type); + for (Type superType : types.closure(baseType.type)) { + Type erasedSuperType = types.erasure(superType); + if (types.isSubtype(erasedSuperType, memberOwner) + && isTypeAccessibleFromPackage(erasedSuperType.tsym, pkg)) { + owner = erasedSuperType; + } + } + return owner; + } + /** * Looks for a field or method with the given {@code identifier}, in {@code typeSym} or one of its * super-types or super-interfaces, and that is visible from the {@code start} symbol. @@ -183,18 +200,8 @@ private static ImmutableSet lookup( if (!isStatic(member)) { continue; } - switch ((int) (member.flags() & Flags.AccessFlags)) { - case Flags.PRIVATE: - continue; - case 0: - case Flags.PROTECTED: - if (enclosingPackage(member).orElse(null) != pkg) { - continue; - } - break; - case Flags.PUBLIC: - default: - break; + if (!isAccessibleFromPackage(member, pkg)) { + continue; } if (member.isMemberOf(start, types)) { members.add(member); @@ -204,5 +211,22 @@ private static ImmutableSet lookup( return members.build(); } + private static boolean isTypeAccessibleFromPackage(Symbol type, Symbol.PackageSymbol pkg) { + for (Symbol curr = type; !(curr instanceof Symbol.PackageSymbol); curr = curr.owner) { + if (!isAccessibleFromPackage(curr, pkg)) { + return false; + } + } + return true; + } + + private static boolean isAccessibleFromPackage(Symbol sym, Symbol.PackageSymbol pkg) { + return switch ((int) (sym.flags() & Flags.AccessFlags)) { + case Flags.PRIVATE -> false; + case 0, Flags.PROTECTED -> Objects.equals(enclosingPackage(sym).orElse(null), pkg); + default -> true; + }; + } + private StaticImports() {} } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/NonCanonicalStaticMemberImportTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/NonCanonicalStaticMemberImportTest.java index e63d48c8a88..720d8a4e3d4 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/NonCanonicalStaticMemberImportTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/NonCanonicalStaticMemberImportTest.java @@ -322,4 +322,328 @@ class Test {} .expectUnchanged() .doTest(); } + + @Test + public void packagePrivateSuperInterface_differentPackage_unchanged() { + refactoringHelper + .addInputLines( + "a/Super.java", + """ + package a; + + interface Super { + int CONST = 42; + } + """) + .expectUnchanged() + .addInputLines( + "a/Sub.java", + """ + package a; + + public interface Sub extends Super {} + """) + .expectUnchanged() + .addInputLines( + "b/Test.java", + """ + package b; + + import static a.Sub.CONST; + + class Test { + int x = CONST; + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void packagePrivateSuperInterface_samePackage_importsFromSuperInterface() { + refactoringHelper + .addInputLines( + "a/Super.java", + """ + package a; + + interface Super { + int CONST = 42; + } + """) + .expectUnchanged() + .addInputLines( + "a/Sub.java", + """ + package a; + + public interface Sub extends Super {} + """) + .expectUnchanged() + .addInputLines( + "a/Test.java", + """ + package a; + + import static a.Sub.CONST; + + class Test { + int x = CONST; + } + """) + .addOutputLines( + "a/Test.java", + """ + package a; + + import static a.Super.CONST; + + class Test { + int x = CONST; + } + """) + .doTest(); + } + + @Test + public void packagePrivateSuperClass_publicIntermediateClass_importsFromIntermediateClass() { + refactoringHelper + .addInputLines( + "a/HiddenSuper.java", + """ + package a; + + class HiddenSuper { + public static final int CONST = 42; + } + """) + .expectUnchanged() + .addInputLines( + "a/Middle.java", + """ + package a; + + public class Middle extends HiddenSuper {} + """) + .expectUnchanged() + .addInputLines( + "b/Sub.java", + """ + package b; + + import a.Middle; + + public class Sub extends Middle {} + """) + .expectUnchanged() + .addInputLines( + "b/Test.java", + """ + package b; + + import static b.Sub.CONST; + + class Test { + int x = CONST; + } + """) + .addOutputLines( + "b/Test.java", + """ + package b; + + import static a.Middle.CONST; + + class Test { + int x = CONST; + } + """) + .doTest(); + } + + @Test + public void packagePrivateEnclosingClass_differentPackage_unchanged() { + refactoringHelper + .addInputLines( + "a/Outer.java", + """ + package a; + + class Outer { + public static class Inner { + public static final int CONST = 42; + } + } + """) + .expectUnchanged() + .addInputLines( + "a/Sub.java", + """ + package a; + + public class Sub extends Outer.Inner {} + """) + .expectUnchanged() + .addInputLines( + "b/Test.java", + """ + package b; + + import static a.Sub.CONST; + + class Test { + int x = CONST; + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void inaccessibleSubtypeMethodOverloads_differentPackage_importsFromSuperClass() { + refactoringHelper + .addInputLines( + "a/Super.java", + """ + package a; + + public class Super { + public static int foo() { + return 42; + } + } + """) + .expectUnchanged() + .addInputLines( + "a/Sub.java", + """ + package a; + + public class Sub extends Super { + static int foo(int x) { + return x; + } + + private static int foo(String s) { + return 0; + } + } + """) + .expectUnchanged() + .addInputLines( + "b/Test.java", + """ + package b; + + import static a.Sub.foo; + + class Test { + int x = foo(); + } + """) + .addOutputLines( + "b/Test.java", + """ + package b; + + import static a.Super.foo; + + class Test { + int x = foo(); + } + """) + .doTest(); + } + + @Test + public void packagePrivateSuperClass_publicStaticMemberClass_unchanged() { + BugCheckerRefactoringTestHelper.newInstance(NonCanonicalStaticImport.class, getClass()) + .addInputLines( + "a/Super.java", + """ + package a; + + class Super { + public static class Inner {} + } + """) + .expectUnchanged() + .addInputLines( + "a/Sub.java", + """ + package a; + + public class Sub extends Super {} + """) + .expectUnchanged() + .addInputLines( + "b/Test.java", + """ + package b; + + import static a.Sub.Inner; + + class Test { + Inner x; + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void packagePrivateSuperClass_publicIntermediateClass_staticMemberClass() { + refactoringHelper + .addInputLines( + "a/HiddenSuper.java", + """ + package a; + + class HiddenSuper { + public static class Inner {} + } + """) + .expectUnchanged() + .addInputLines( + "a/Middle.java", + """ + package a; + + public class Middle extends HiddenSuper {} + """) + .expectUnchanged() + .addInputLines( + "b/Sub.java", + """ + package b; + + import a.Middle; + + public class Sub extends Middle {} + """) + .expectUnchanged() + .addInputLines( + "b/Test.java", + """ + package b; + + import static b.Sub.Inner; + + class Test { + Inner x; + } + """) + .addOutputLines( + "b/Test.java", + """ + package b; + + import static a.Middle.Inner; + + class Test { + Inner x; + } + """) + .doTest(); + } }