Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have InvalidParam support records #35

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 23 additions & 0 deletions check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java
Original file line number Diff line number Diff line change
Expand Up @@ -807,6 +807,29 @@ private static boolean isFinal(Symbol symbol) {
return (symbol.flags() & Flags.FINAL) == Flags.FINAL;
}

/**
* Flag for record types, canonical record constructors and type members that are part of a
* record's state vector. Can be replaced by {@code com.sun.tools.javac.code.Flags.RECORD} once
* the minimum JDK version is 14.
*/
private static final long RECORD_FLAG = 1L << 61;
Copy link
Member Author

@rickie rickie Jul 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@rickie rickie Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Returns true if the given {@link Tree} is a record, a record's canonical constructor or a
* member that is part of a record's state vector.
*/
public static boolean isRecord(Tree tree) {
return isRecord(getSymbol(tree));
}

/**
* Returns true if the given {@link Symbol} is a record, a record's canonical constructor or a
* member that is part of a record's state vector.
*/
public static boolean isRecord(Symbol symbol) {
return symbol != null && (symbol.flags() & RECORD_FLAG) == RECORD_FLAG;
}

/**
* Determines whether a symbol has an annotation of the given type. This includes annotations
* inherited from superclasses due to {@code @Inherited}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.errorprone.bugpatterns.javadoc;

import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.collect.MoreCollectors.onlyElement;
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.bugpatterns.javadoc.Utils.diagnosticPosition;
import static com.google.errorprone.bugpatterns.javadoc.Utils.getBestMatch;
Expand All @@ -26,13 +27,15 @@
import static com.google.errorprone.names.LevenshteinEditDistance.getEditDistance;

import com.google.common.collect.ImmutableSet;
import com.google.common.collect.MoreCollectors;
import com.google.common.collect.Sets;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.doctree.DocTree;
import com.sun.source.doctree.DocTree.Kind;
import com.sun.source.doctree.LiteralTree;
Expand Down Expand Up @@ -74,11 +77,18 @@ 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();
ImmutableSet<String> parameters =
ASTHelpers.isRecord(classTree)
? getCanonicalRecordConstructor(classTree).getParameters().stream()
.map(p -> p.getName().toString())
.collect(toImmutableSet())
: ImmutableSet.of();

ImmutableSet<String> typeParameters =
classTree.getTypeParameters().stream()
.map(t -> t.getName().toString())
.collect(toImmutableSet());

new ParamsChecker(state, classTree, parameters, typeParameters).scan(path, null);
}
return Description.NO_MATCH;
Expand All @@ -101,6 +111,14 @@ public Description matchMethod(MethodTree methodTree, VisitorState state) {
return Description.NO_MATCH;
}

private static MethodTree getCanonicalRecordConstructor(ClassTree classTree) {
return classTree.getMembers().stream()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.filter(MethodTree.class::isInstance)
.map(MethodTree.class::cast)
.filter(ASTHelpers::isRecord)
.collect(onlyElement());
}

/** Checks that documented parameters match the method's parameter list. */
private final class ParamsChecker extends DocTreePathScanner<Void, Void> {
private final VisitorState state;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@

package com.google.errorprone.bugpatterns.javadoc;

import static org.junit.Assume.assumeTrue;

import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;
import com.google.errorprone.CompilationTestHelper;
import com.google.errorprone.util.RuntimeVersion;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand Down Expand Up @@ -164,4 +167,117 @@ public void excludedName_noMatchDespiteSimilarParam() {
"}")
.doTest();
}

@Test
public void negative_record() {
assumeTrue(RuntimeVersion.isAtLeast16());
helper
.addSourceLines(
"Test.java", //
"/**",
" * @param name Name.",
" */",
"public record Test(String name) {}")
.doTest();
}

@Test
public void badParameterName_record() {
assumeTrue(RuntimeVersion.isAtLeast16());
helper
.addSourceLines(
"Test.java",
"/**",
" // BUG: Diagnostic contains: Parameter name `bar` is unknown",
" * @param bar Foo.",
" */",
"public record Test(String foo) {}")
.doTest();
}

@Test
public void multipleConstructors_record() {
assumeTrue(RuntimeVersion.isAtLeast16());
helper
.addSourceLines(
"Test.java",
"/**",
" * @param foo Foo.",
" * @param bar Bar.",
" */",
"public record Test(String foo, Integer bar) {",
" public Test(Integer bar) {",
" this(null, bar);",
" }",
"",
" /**",
" // BUG: Diagnostic contains: Parameter name `bar` is unknown",
" * @param bar Foo.",
" */",
" public Test(String foo) {",
" this(foo, null);",
" }",
"}")
.doTest();
}

@Test
public void typeParameter_record() {
assumeTrue(RuntimeVersion.isAtLeast16());
helper
.addSourceLines(
"Negative.java",
"/**",
" * @param <T> The type parameter.",
" * @param contents Contents.",
" * @param bar Bar.",
" */",
"public record Negative<T>(T contents, String bar) {}")
.addSourceLines(
"Positive.java",
"/**",
" // BUG: Diagnostic contains: Parameter name `E` is unknown",
" * @param <E> The type parameter.",
" * @param contents Contents.",
" * @param bar Bar.",
" */",
"public record Positive<T>(T contents, String bar) {}")
.doTest();
}

@Test
public void compactConstructor_record() {
assumeTrue(RuntimeVersion.isAtLeast16());
helper
.addSourceLines(
"Test.java",
"/**",
" * @param name Name.",
" */",
"public record Test(String name) {",
" public Test {}",
"}")
.doTest();
}

@Test
public void normalConstructor_record() {
assumeTrue(RuntimeVersion.isAtLeast16());
helper
.addSourceLines(
"Test.java",
"/**",
" * @param name Name.",
" */",
"public record Test(String name) {",
" /**",
" // BUG: Diagnostic contains: Parameter name `foo` is unknown",
" * @param foo Name.",
" */",
" public Test(String name) {",
" this.name = name;",
" }",
" }")
.doTest();
}
}