diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/NotJavadoc.java b/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/NotJavadoc.java index 83dbc4a57a3..5cfccb15c9d 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/NotJavadoc.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/NotJavadoc.java @@ -22,11 +22,12 @@ import static com.google.errorprone.fixes.SuggestedFix.replace; import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.util.ASTHelpers.getStartPosition; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.isRecord; import static com.google.errorprone.util.ErrorProneTokens.getTokens; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableRangeSet; -import com.google.common.collect.ImmutableSet; import com.google.common.collect.Range; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; @@ -39,7 +40,7 @@ import com.sun.source.tree.ClassTree; import com.sun.source.tree.CompilationUnitTree; import com.sun.source.tree.MethodTree; -import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; import com.sun.source.util.TreePath; import com.sun.source.util.TreeScanner; @@ -52,14 +53,23 @@ public final class NotJavadoc extends BugChecker implements CompilationUnitTreeM @Override public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { ImmutableMap javadocableTrees = getJavadoccableTrees(tree); - ImmutableSet seeminglyJavadocableTrees = getSeeminglyJavadocableTrees(tree); + ImmutableMap seeminglyJavadocableTrees = + getSeeminglyJavadocableTrees(tree); ImmutableRangeSet suppressedRegions = suppressedRegions(state); for (ErrorProneToken token : getTokens(state.getSourceCode().toString(), state.context)) { for (ErrorProneComment comment : token.comments()) { - if (!comment.getStyle().equals(ErrorProneCommentStyle.JAVADOC_BLOCK) - || comment.getText().equals("/**/")) { - continue; + switch (comment.getStyle()) { + case JAVADOC_BLOCK -> { + if (comment.getText().equals("/**/")) { + continue; + } + } + case JAVADOC_LINE -> {} + default -> { + continue; + } } + // if this is a valid location for Javadoc, skip over this comment if (javadocableTrees.containsKey(token.pos())) { continue; } @@ -70,38 +80,71 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s continue; } - int endPos = 2; - while (comment.getText().charAt(endPos) == '*') { - endPos++; - } - var message = - seeminglyJavadocableTrees.contains(token.pos()) - ? "Local classes (or methods within local classes) cannot be documented with" - + " Javadoc." - : message(); - state.reportMatch( + String message = + switch (seeminglyJavadocableTrees.get(token.pos())) { + case CLASS -> + "This comment is attached to a local class, but local classes cannot be" + + " documented with Javadoc. Please convert this to a regular comment."; + case METHOD -> + "This comment is attached to a method inside a local class, but methods inside" + + " local classes cannot be documented with Javadoc. Please convert this to" + + " a regular comment."; + case RECORD_COMPONENT -> + "This comment seems to be attached to a record component," + + " but record components cannot be documented with Javadoc. Please move this" + + " to an @param tag on the record class."; + case null -> message(); + }; + var description = buildDescription(getDiagnosticPosition(comment.getSourcePos(0), tree)) - .setMessage(message) - .addFix(replace(comment.getSourcePos(1), comment.getSourcePos(endPos - 1), "")) - .build()); + .setMessage(message); + // we only emit a fix for Classic Javadoc (not Markdown Javadoc) + if (comment.getStyle().equals(ErrorProneCommentStyle.JAVADOC_BLOCK)) { + int endPos = 2; + while (comment.getText().charAt(endPos) == '*') { + endPos++; + } + description.addFix( + replace(comment.getSourcePos(1), comment.getSourcePos(endPos - 1), "")); + } + state.reportMatch(description.build()); } } return NO_MATCH; } - private static ImmutableSet getSeeminglyJavadocableTrees(CompilationUnitTree tree) { - ImmutableSet.Builder builder = ImmutableSet.builder(); + private enum JavadocableTreeKind { + CLASS, + METHOD, + RECORD_COMPONENT + } + + private static ImmutableMap getSeeminglyJavadocableTrees( + CompilationUnitTree tree) { + ImmutableMap.Builder builder = ImmutableMap.builder(); tree.accept( new TreeScanner() { @Override - public Void scan(Tree tree, Void unused) { - if (tree instanceof MethodTree || tree instanceof ClassTree) { - builder.add(getStartPosition(tree)); + public Void visitClass(ClassTree tree, Void unused) { + builder.put(getStartPosition(tree), JavadocableTreeKind.CLASS); + return super.visitClass(tree, null); + } + + @Override + public Void visitMethod(MethodTree tree, Void unused) { + builder.put(getStartPosition(tree), JavadocableTreeKind.METHOD); + return super.visitMethod(tree, null); + } + + @Override + public Void visitVariable(VariableTree tree, Void unused) { + if (isRecord(getSymbol(tree))) { + builder.put(getStartPosition(tree), JavadocableTreeKind.RECORD_COMPONENT); } - return super.scan(tree, null); + return super.visitVariable(tree, null); } }, null); - return builder.build(); + return builder.buildKeepingLast(); } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/Utils.java b/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/Utils.java index c654296a013..402d049467d 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/Utils.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/javadoc/Utils.java @@ -176,7 +176,7 @@ public Void visitMethod(MethodTree methodTree, Void unused) { @Override public Void visitVariable(VariableTree variableTree, Void unused) { ElementKind kind = getSymbol(variableTree).getKind(); - if (kind == ElementKind.FIELD) { + if (kind == ElementKind.FIELD && !ASTHelpers.isRecord(getSymbol(variableTree))) { javadoccablePositions.put(ASTHelpers.getStartPosition(variableTree), getCurrentPath()); } // For enum constants, skip past the desugared class declaration. diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/javadoc/NotJavadocTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/javadoc/NotJavadocTest.java index 1136f64ede5..8ba42ef8e28 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/javadoc/NotJavadocTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/javadoc/NotJavadocTest.java @@ -63,7 +63,7 @@ public void nestedClass() { """ class Test { void test() { - // BUG: Diagnostic contains: Local classes + // BUG: Diagnostic contains: local class /** Not Javadoc. */ class A {} } @@ -251,7 +251,13 @@ public record Test( /** age (must be positive) */ int age) {} """) - .expectUnchanged() + .addOutputLines( + "Test.java", + """ + public record Test( + /* age (must be positive) */ + int age) {} + """) .doTest(TEXT_MATCH); } @@ -272,7 +278,21 @@ public record Test( */ int age) {} """) - .expectUnchanged() + // TODO(kak): it would be nice to hoist the @param up to the record's Javadocs + .addOutputLines( + "Test.java", +""" +public record Test( + /* + * @param age Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor + * incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud + * exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure + * dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. + * Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit + * anim id est laborum. + */ + int age) {} +""") .doTest(TEXT_MATCH); } @@ -286,6 +306,7 @@ public record Test( /// age (must be positive) int age) {} """) + // TODO(b/494275366): Add a fix for this. .expectUnchanged() .doTest(TEXT_MATCH); } @@ -306,6 +327,7 @@ public record Test( /// mollit anim id est laborum. int age) {} """) + // TODO(b/494275366): Add a fix for this. .expectUnchanged() .doTest(TEXT_MATCH); }