Fix ProtectedMembersInFinalClass to avoid suggesting removing protected from canonical constructors of protected record classes.

check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java

@@ -22,7 +22,6 @@
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.MoreCollectors.onlyElement;
 import static com.google.common.collect.Streams.stream;
-import static com.google.common.collect.Streams.zip;
 import static com.google.errorprone.VisitorState.memoize;
 import static com.google.errorprone.matchers.JUnitMatchers.JUNIT4_RUN_WITH_ANNOTATION;
 import static com.google.errorprone.matchers.Matchers.isSubtypeOf;
@@ -96,6 +95,7 @@
 import com.sun.tools.javac.code.Symbol.ClassSymbol;
 import com.sun.tools.javac.code.Symbol.MethodSymbol;
 import com.sun.tools.javac.code.Symbol.PackageSymbol;
+import com.sun.tools.javac.code.Symbol.RecordComponent;
 import com.sun.tools.javac.code.Symbol.TypeSymbol;
 import com.sun.tools.javac.code.Symbol.VarSymbol;
 import com.sun.tools.javac.code.Symtab;
@@ -829,21 +829,41 @@ public static boolean isRecord(Symbol symbol) {
 
   /** Finds the canonical constructor on a record. */
   public static MethodSymbol canonicalConstructor(ClassSymbol record, VisitorState state) {
-    var fieldTypes =
-        record.getRecordComponents().stream().map(rc -> rc.type).collect(toImmutableList());
+    var recordComponents = record.getRecordComponents();
     return stream(record.members().getSymbols(s -> s.isConstructor()))
         .map(c -> (MethodSymbol) c)
-        .filter(
-            c ->
-                c.getParameters().size() == fieldTypes.size()
-                    && zip(
-                            c.getParameters().stream(),
-                            fieldTypes.stream(),
-                            (a, b) -> isSameType(a.type, b, state))
-                        .allMatch(x -> x))
+        .filter(c -> parametersMatchRecordComponents(c, recordComponents, state))
         .collect(onlyElement());
   }
 
+  /** Returns whether the given method is a record's canonical constructor. */
+  public static boolean isCanonicalRecordConstructor(MethodSymbol symbol, VisitorState state) {
+    if (!symbol.isConstructor()) {
+      return false;
+    }
+    ClassSymbol enclosingClass = symbol.enclClass();
+    if (enclosingClass == null || enclosingClass.getKind() != ElementKind.RECORD) {
+      return false;
+    }
+    return parametersMatchRecordComponents(symbol, enclosingClass.getRecordComponents(), state);
+  }
+
+  private static boolean parametersMatchRecordComponents(
+      MethodSymbol constructor,
+      List<? extends RecordComponent> recordComponents,
+      VisitorState state) {
+    if (constructor.getParameters().size() != recordComponents.size()) {
+      return false;
+    }
+    for (int i = 0; i < recordComponents.size(); i++) {
+      if (!isSameType(
+          constructor.getParameters().get(i).type, recordComponents.get(i).type, state)) {
+        return false;
+      }
+    }
+    return true;
+  }
+
   /**
    * Determines whether a symbol has an annotation of the given type. This includes annotations
    * inherited from superclasses due to {@code @Inherited}.

check_api/src/test/java/com/google/errorprone/util/ASTHelpersTest.java

@@ -26,6 +26,7 @@
 import static com.google.errorprone.util.ASTHelpers.getStartPosition;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
 import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
+import static com.google.errorprone.util.ASTHelpers.isCanonicalRecordConstructor;
 import static java.lang.annotation.ElementType.FIELD;
 import static java.lang.annotation.ElementType.LOCAL_VARIABLE;
 import static java.lang.annotation.ElementType.METHOD;
@@ -81,6 +82,7 @@
 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.PackageSymbol;
 import com.sun.tools.javac.code.Type;
 import com.sun.tools.javac.code.Type.TypeVar;
@@ -2065,9 +2067,12 @@ public static final class CanonicalConstructorFinder extends BugChecker
       implements MethodTreeMatcher {
     @Override
     public Description matchMethod(MethodTree tree, VisitorState state) {
-      return canonicalConstructor((ClassSymbol) getSymbol(tree).owner, state) == getSymbol(tree)
-          ? describeMatch(tree)
-          : Description.NO_MATCH;
+      MethodSymbol symbol = getSymbol(tree);
+      boolean isCanonical = isCanonicalRecordConstructor(symbol, state);
+      boolean isReturnedByCanonicalConstructorMethod =
+          symbol.equals(canonicalConstructor((ClassSymbol) symbol.owner, state));
+      assertThat(isCanonical).isEqualTo(isReturnedByCanonicalConstructorMethod);
+      return isCanonical ? describeMatch(tree) : Description.NO_MATCH;
     }
   }
 

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

@@ -19,12 +19,11 @@
 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.canonicalConstructor;
 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.isCanonicalRecordConstructor;
 import static com.google.errorprone.util.ASTHelpers.isEffectivelyPrivate;
-import static com.google.errorprone.util.ASTHelpers.isRecord;
 import static com.google.errorprone.util.ASTHelpers.streamSuperMethods;
 import static javax.lang.model.element.Modifier.PROTECTED;
 import static javax.lang.model.element.Modifier.PUBLIC;
@@ -108,12 +107,7 @@ private void match(
       }
       if (methodSymbol.isConstructor()) {
         ClassSymbol enclosingClass = enclosingClass(methodSymbol);
-        /*
-         * TODO(cpovirk): Introduce an ASTHelpers.isCanonicalConstructor to avoid scanning all
-         * members again in canonicalConstructor?
-         */
-        if (isRecord(enclosingClass)
-            && methodSymbol.equals(canonicalConstructor(enclosingClass, state))) {
+        if (isCanonicalRecordConstructor(methodSymbol, state)) {
           if (enclosingClass.getModifiers().contains(PUBLIC)
               || enclosingClass.getModifiers().contains(PROTECTED)) {
             // Canonical constructors are required to be at least as visible as the record itself.

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

@@ -20,6 +20,8 @@
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
 import static com.google.errorprone.matchers.Description.NO_MATCH;
 import static com.google.errorprone.matchers.Matchers.hasModifier;
+import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.isCanonicalRecordConstructor;
 import static java.util.stream.Collectors.joining;
 import static javax.lang.model.element.Modifier.FINAL;
 import static javax.lang.model.element.Modifier.PROTECTED;
@@ -38,6 +40,7 @@
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.VariableTree;
 import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
 
 /**
  * Flags protected members in final classes.
@@ -57,6 +60,15 @@ private static boolean methodHasNoParentMethod(MethodTree methodTree, VisitorSta
         .isEmpty();
   }
 
+  private static boolean isConstructorOfProtectedRecord(Tree tree, VisitorState state) {
+    if (!(tree instanceof MethodTree methodTree)) {
+      return false;
+    }
+    MethodSymbol symbol = getSymbol(methodTree);
+    return symbol.enclClass().getModifiers().contains(PROTECTED)
+        && isCanonicalRecordConstructor(symbol, state);
+  }
+
   @Override
   public Description matchClass(ClassTree tree, VisitorState state) {
     if (!HAS_FINAL.matches(tree, state)) {
@@ -71,6 +83,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
                 m ->
                     !(m instanceof MethodTree methodTree)
                         || methodHasNoParentMethod(methodTree, state))
+            .filter(m -> !isConstructorOfProtectedRecord(m, state))
             .filter(m -> !isSuppressed(m, state))
             .collect(toImmutableList());
     if (relevantMembers.isEmpty()) {

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

@@ -151,4 +151,55 @@ protected Test() {}
             """)
         .doTest();
   }
+
+  @Test
+  public void protectedRecordClassCanonicalConstructor() {
+    compilationHelper
+        .addSourceLines(
+            "in/Outer.java",
+            """
+            public class Outer {
+              protected record NestedRecord(int x) {
+                protected NestedRecord {}
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void packagePrivateRecordClassCanonicalConstructor() {
+    compilationHelper
+        .addSourceLines(
+            "in/Outer.java",
+            """
+            public class Outer {
+              record NestedRecord(int x) {
+                // BUG: Diagnostic contains: NestedRecord
+                protected NestedRecord {}
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void protectedRecordClassAuxiliaryConstructor() {
+    compilationHelper
+        .addSourceLines(
+            "in/Outer.java",
+            """
+            public class Outer {
+              protected record NestedRecord(int x) {
+                protected NestedRecord {}
+
+                // BUG: Diagnostic contains: NestedRecord
+                protected NestedRecord(int x, int y) {
+                  this(x + y);
+                }
+              }
+            }
+            """)
+        .doTest();
+  }
 }