Have `InvalidParam` support records #35

rickie · 2022-07-18T08:00:05Z

rickie

Some context.

rickie · 2022-07-18T08:02:14Z

core/src/main/java/com/google/errorprone/bugpatterns/javadoc/InvalidParam.java

@@ -101,6 +110,15 @@ public Description matchMethod(MethodTree methodTree, VisitorState state) {
    return Description.NO_MATCH;
  }

+  private static MethodTree getGeneratedConstructor(ClassTree classTree) {
+    return classTree.getMembers().stream()


For records in some cases the generated constructor could not be found by using ASTHelpers#isGeneratedConstructor. That's why it's implemented with the contentEquals("<init>") check.

Perhaps that is a bigger issue we should solve. However, I think that is out of scope for this PR.

rickie · 2022-07-18T08:02:40Z

core/src/main/java/com/google/errorprone/bugpatterns/javadoc/InvalidParam.java

+        .map(MethodTree.class::cast)
+        .findFirst()
+        .filter(tree -> tree.getName().contentEquals("<init>"))
+        .orElseThrow(() -> new IllegalStateException("Records must have a generated ctor"));


This error message could be better.

rickie · 2022-07-18T08:07:33Z

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

@@ -798,6 +798,13 @@ private static boolean isFinal(Symbol symbol) {
    return (symbol.flags() & Flags.FINAL) == Flags.FINAL;
  }

+  private static final long RECORD_FLAG = 1L << 61;


We can't use Flags.RECORD yet, because that would fail with JDK 11...

dhoepelman

Any timeline on merging this? 😇 We're peppering our code with @SupressWarning("InvalidParam") at the moment.

dhoepelman · 2022-08-15T07:37:29Z

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

@@ -798,6 +798,13 @@ private static boolean isFinal(Symbol symbol) {
    return (symbol.flags() & Flags.FINAL) == Flags.FINAL;
  }

+  private static final long RECORD_FLAG = 1L << 61;


Suggested change

private static final long RECORD_FLAG = 1L << 61;

/**

* Flag for a record class, its canonical constructor and components. Can be replaced by

* com.sun.tools.javac.code.Flags.RECORD once support minimum JDK version is 14.

*/

private static final long RECORD_FLAG = 1L << 61;

Although I really like the suggestion, I used this one-liner because it's used in the Error Prone project twice like that and wanted to be consistent.

So I'd lean towards leaving it as-is. (When we use this in error-prone-support we should definitely use this as Javadoc.

Oh I misread, this is in the ASTHelpers of course... In that case, it is a good idea to add this. Will commit that.

Stephan202 · 2022-09-06T17:13:11Z

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

@@ -807,6 +807,17 @@ private static boolean isFinal(Symbol symbol) {
    return (symbol.flags() & Flags.FINAL) == Flags.FINAL;
  }

+  /**
+   * Flag for a record class, its canonical constructor and components. Can be replaced by {@code


W.r.t. components: not sure what is meant by that, but based on some tests with the debugger this does not include members. (The original documentation talks about the "state vector"; see below.) Maybe we should leave this out?

Upon further debugging, I found that a subset of record members are flagged. I now better understand what is meant, so will update the documentation.

Stephan202 · 2022-09-06T17:16:54Z

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

+  /** Returns true if the given {@link Symbol} is a record. */
+  public static boolean isRecord(Symbol symbol) {
+    return (symbol.flags() & RECORD_FLAG) == RECORD_FLAG;
+  }


The JDK 17 Javadoc for com.sun.tools.javac.code.Flags.RECORD says:

/** * Flag to indicate that a class is a record. The flag is also used to mark fields that are * part of the state vector of a record and to mark the canonical constructor */

Not sure what the state vector is, but tests indicate that this method also returns true for the canonical constructor. We should either fix that or update the documentation.

^ Okay, IIUC the state vector comprises the fields that are populated by the canonical constructor.

Stephan202 · 2022-09-07T12:17:30Z

core/src/main/java/com/google/errorprone/bugpatterns/javadoc/InvalidParam.java

@@ -74,11 +75,19 @@ public final class InvalidParam extends BugChecker implements ClassTreeMatcher,
  public Description matchClass(ClassTree classTree, VisitorState state) {
    DocTreePath path = getDocTreePath(state);
    if (path != null) {
-      ImmutableSet<String> parameters = ImmutableSet.of();
+      // XXX: `isRecord`... should it also be able to handle `Tree`s?


I think we should defer it for now. We might want to improve this in a follow-up PR where we look at record handling in general.

Stephan202

Rebased and added a commit.

Suggested commit message:

Have `InvalidParam` support records 

Fixes #2321.

Stephan202 · 2022-09-07T14:27:09Z

core/src/main/java/com/google/errorprone/bugpatterns/javadoc/InvalidParam.java

@@ -101,6 +110,15 @@ public Description matchMethod(MethodTree methodTree, VisitorState state) {
    return Description.NO_MATCH;
  }

+  private static MethodTree getGeneratedConstructor(ClassTree classTree) {


Given the current implementation:

Suggested change

private static MethodTree getGeneratedConstructor(ClassTree classTree) {

private static MethodTree getFirstConstructor(ClassTree classTree) {

This is not necessarily a generated constructor; see e.g. the normalConstructor_record test case.

That said, what we what, IIUC, is:

Suggested change

private static MethodTree getGeneratedConstructor(ClassTree classTree) {

private static MethodTree getCanonicalRecordConstructor(ClassTree classTree) {

But this does raise the question: can we reliable assume that the first constructor is always the canonical constructor? It might be better to explicitly test against #isRecord; will push a proposal.

Stephan202 · 2022-09-07T14:47:52Z

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

@@ -807,6 +807,17 @@ private static boolean isFinal(Symbol symbol) {
    return (symbol.flags() & Flags.FINAL) == Flags.FINAL;
  }

+  /**
+   * Flag for a record class, its canonical constructor and components. Can be replaced by {@code


Upon further debugging, I found that a subset of record members are flagged. I now better understand what is meant, so will update the documentation.

Stephan202 · 2022-09-07T14:49:01Z

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

+  /** Returns true if the given {@link Symbol} is a record. */
+  public static boolean isRecord(Symbol symbol) {
+    return (symbol.flags() & RECORD_FLAG) == RECORD_FLAG;
+  }


^ Okay, IIUC the state vector comprises the fields that are populated by the canonical constructor.

Stephan202 · 2022-09-07T14:53:56Z

core/src/main/java/com/google/errorprone/bugpatterns/javadoc/InvalidParam.java

+      ImmutableSet<String> parameters =
+          ASTHelpers.isRecord(ASTHelpers.getSymbol(classTree))
+              ? getGeneratedConstructor(classTree).getParameters().stream()
+                  .map(v -> v.getName().toString())


Suggested change

.map(v -> v.getName().toString())

.map(p -> p.getName().toString())

rickie · 2022-09-08T09:01:35Z

Filed google#3437.

Fixes google#2321.

rickie · 2022-12-29T14:37:54Z

Fixed in google#2321.

rickie requested a review from Stephan202 July 18, 2022 08:00

rickie commented Jul 18, 2022

View reviewed changes

rickie force-pushed the rossendrijver/invalidparam_records branch from 625a631 to b565ccc Compare July 18, 2022 08:06

rickie commented Jul 18, 2022

View reviewed changes

rickie force-pushed the rossendrijver/invalidparam_records branch from b565ccc to 2455ece Compare July 18, 2022 08:49

dhoepelman approved these changes Aug 15, 2022

View reviewed changes

rickie force-pushed the rossendrijver/invalidparam_records branch from 2455ece to 341454f Compare August 18, 2022 09:56

Stephan202 reviewed Sep 7, 2022

View reviewed changes

rickie requested a review from Stephan202 September 7, 2022 13:46

Stephan202 force-pushed the rossendrijver/invalidparam_records branch from 1e941a9 to 4953d01 Compare September 7, 2022 15:05

Stephan202 approved these changes Sep 7, 2022

View reviewed changes

rickie force-pushed the rossendrijver/invalidparam_records branch from 4953d01 to 16759a4 Compare September 8, 2022 08:08

rickie changed the title ~~Support records in InvalidParam~~ Have InvalidParam support records Sep 8, 2022

rickie force-pushed the rossendrijver/invalidparam_records branch from 16759a4 to 2ccb778 Compare September 26, 2022 10:04

rickie force-pushed the rossendrijver/invalidparam_records branch from 2ccb778 to bc2b5da Compare November 25, 2022 13:31

Have InvalidParam support records

128ca4d

Fixes google#2321.

rickie force-pushed the rossendrijver/invalidparam_records branch from bc2b5da to 128ca4d Compare December 13, 2022 14:36

rickie closed this Dec 29, 2022

Stephan202 deleted the rossendrijver/invalidparam_records branch December 30, 2022 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have `InvalidParam` support records #35

Have `InvalidParam` support records #35

rickie commented Jul 18, 2022 •

edited

rickie left a comment

rickie Jul 18, 2022

rickie Jul 18, 2022

rickie Jul 18, 2022 •

edited

dhoepelman left a comment

dhoepelman Aug 15, 2022

rickie Aug 18, 2022

rickie Aug 18, 2022 •

edited

Stephan202 Sep 6, 2022

Stephan202 Sep 7, 2022

Stephan202 Sep 6, 2022

Stephan202 Sep 7, 2022

Stephan202 Sep 7, 2022

rickie Sep 7, 2022

Stephan202 left a comment

Stephan202 Sep 7, 2022

Stephan202 Sep 7, 2022

Stephan202 Sep 7, 2022

Stephan202 Sep 7, 2022

rickie commented Sep 8, 2022

rickie commented Dec 29, 2022

	private static MethodTree getGeneratedConstructor(ClassTree classTree) {
	private static MethodTree getFirstConstructor(ClassTree classTree) {

	private static MethodTree getGeneratedConstructor(ClassTree classTree) {
	private static MethodTree getCanonicalRecordConstructor(ClassTree classTree) {

	.map(v -> v.getName().toString())
	.map(p -> p.getName().toString())

Have InvalidParam support records #35

Have InvalidParam support records #35

Conversation

rickie commented Jul 18, 2022 • edited

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie Jul 18, 2022 • edited

Choose a reason for hiding this comment

dhoepelman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie Aug 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Sep 8, 2022

rickie commented Dec 29, 2022

Have `InvalidParam` support records #35

Have `InvalidParam` support records #35

rickie commented Jul 18, 2022 •

edited

rickie Jul 18, 2022 •

edited

rickie Aug 18, 2022 •

edited