Skip to content

Commit

Permalink
Split valid tag check into inline vs block tags in InvalidTag.
Browse files Browse the repository at this point in the history 
This'll also prevent us suggesting renaming invalid inline tags to block ones, which would be sad.

RELNOTES: InvalidTag: point out invalid block tags.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=228312112
  • Loading branch information
@graememorgan @ronshapiro
graememorgan authored and ronshapiro committed Jan 15, 2019
1 parent 10a7694 commit e3b7c73
Showing 1 changed file with 113 additions and 46 deletions.
159 changes: 113 additions & 46 deletions core/src/main/java/com/google/errorprone/bugpatterns/javadoc/InvalidTag.java
View file
Expand Up @@ -19,7 +19,9 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; 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.diagnosticPosition;
import static com.google.errorprone.bugpatterns.javadoc.Utils.replace;


import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSet;
import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern;
import com.google.errorprone.BugPattern.ProvidesFix; import com.google.errorprone.BugPattern.ProvidesFix;
Expand All @@ -29,6 +31,7 @@
import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher;
import com.google.errorprone.bugpatterns.javadoc.InvalidTag.JavadocTag.TagType;
import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.FindIdentifiers; import com.google.errorprone.util.FindIdentifiers;
Expand All @@ -50,6 +53,7 @@
import com.sun.tools.javac.tree.DCTree.DCDocComment; import com.sun.tools.javac.tree.DCTree.DCDocComment;
import com.sun.tools.javac.tree.DCTree.DCErroneous; import com.sun.tools.javac.tree.DCTree.DCErroneous;
import com.sun.tools.javac.tree.DCTree.DCInlineTag; import com.sun.tools.javac.tree.DCTree.DCInlineTag;
import com.sun.tools.javac.util.JCDiagnostic;
import java.util.HashSet; import java.util.HashSet;
import java.util.Optional; import java.util.Optional;
import java.util.Set; import java.util.Set;
Expand All @@ -75,55 +79,70 @@ public final class InvalidTag extends BugChecker
+ "Use {@code %1%s} to refer to parameter names inline."; + "Use {@code %1%s} to refer to parameter names inline.";


/** Non-standard commonly-used tags which we should allow. */ /** Non-standard commonly-used tags which we should allow. */
private static final ImmutableSet<String> KNOWN_OTHER_TAGS = private static final ImmutableSet<JavadocTag> KNOWN_OTHER_TAGS =
ImmutableSet.of( ImmutableSet.of(
"apiNote", blockTag("apiNote"),
"attr", // commonly used by Android blockTag("attr"), // commonly used by Android
"contact", blockTag("contact"),
"hide", blockTag("hide"),
"implNote", blockTag("implNote"),
"implSpec", blockTag("implSpec"),
"required", blockTag("required"),
"team"); blockTag("team"));


/** /**
* HTML tags which imply we're showing code, and should therefore probably escape unknown block * HTML tags which imply we're showing code, and should therefore probably escape unknown block
* tags. * tags.
*/ */
private static final ImmutableSet<String> CODE_TAGS = ImmutableSet.of("code", "pre"); private static final ImmutableSet<String> CODE_TAGS = ImmutableSet.of("code", "pre");


// TODO(b/72685460): Split these into unknown block vs inline tags. private static final ImmutableSet<JavadocTag> COMMON_TAGS =
private static final ImmutableSet<String> COMMON_TAGS =
ImmutableSet.of( ImmutableSet.of(
"code", "deprecated", "docRoot", "link", "linkplain", "literal", "see", "since"); inlineTag("code"),
blockTag("deprecated"),
inlineTag("docRoot"),
inlineTag("link"),
inlineTag("linkplain"),
inlineTag("literal"),
blockTag("see"),
blockTag("since"));


private static final ImmutableSet<String> VALID_METHOD_TAGS = private static final ImmutableSet<JavadocTag> VALID_METHOD_TAGS =
ImmutableSet.<String>builder() ImmutableSet.<JavadocTag>builder()
.addAll(COMMON_TAGS) .addAll(COMMON_TAGS)
.add( .add(
"author", blockTag("author"),
"exception", blockTag("exception"),
"inheritDoc", inlineTag("inheritDoc"),
"param", blockTag("param"),
"return", blockTag("return"),
"serial", blockTag("serial"),
"throws", blockTag("throws"),
"serialData", blockTag("serialData"),
"serialField", blockTag("serialField"),
"value", inlineTag("value"),
"version") blockTag("version"))
.build(); .build();


private static final ImmutableSet<String> VALID_VARIABLE_TAGS = private static final ImmutableSet<JavadocTag> VALID_VARIABLE_TAGS =
ImmutableSet.<String>builder() ImmutableSet.<JavadocTag>builder()
.addAll(COMMON_TAGS) .addAll(COMMON_TAGS)
.add("serial", "serialData", "serialField", "value") .add(
blockTag("serial"),
blockTag("serialData"),
blockTag("serialField"),
inlineTag("value"))
.build(); .build();


private static final ImmutableSet<String> VALID_CLASS_TAGS = private static final ImmutableSet<JavadocTag> VALID_CLASS_TAGS =
ImmutableSet.<String>builder() ImmutableSet.<JavadocTag>builder()
.addAll(COMMON_TAGS) .addAll(COMMON_TAGS)
.add("author", "inheritDoc", "param", "value", "version") .add(
blockTag("author"),
inlineTag("inheritDoc"),
blockTag("param"),
inlineTag("value"),
blockTag("version"))
.build(); .build();


private static final Pattern PARAM_MATCHER = Pattern.compile("\\{@param (.*)}"); private static final Pattern PARAM_MATCHER = Pattern.compile("\\{@param (.*)}");
Expand Down Expand Up @@ -164,14 +183,14 @@ public Description matchVariable(VariableTree variableTree, VisitorState state)
private final class InvalidTagChecker extends DocTreePathScanner<Void, Void> { private final class InvalidTagChecker extends DocTreePathScanner<Void, Void> {
private final VisitorState state; private final VisitorState state;


private final ImmutableSet<String> validTags; private final ImmutableSet<JavadocTag> validTags;
private final ImmutableSet<String> parameters; private final ImmutableSet<String> parameters;


private final Set<DocTree> fixedTags = new HashSet<>(); private final Set<DocTree> fixedTags = new HashSet<>();
private int codeTagNestedDepth = 0; private int codeTagNestedDepth = 0;


private InvalidTagChecker( private InvalidTagChecker(
VisitorState state, ImmutableSet<String> validTags, ImmutableSet<String> parameters) { VisitorState state, ImmutableSet<JavadocTag> validTags, ImmutableSet<String> parameters) {
this.state = state; this.state = state;
this.validTags = validTags; this.validTags = validTags;
this.parameters = parameters; this.parameters = parameters;
Expand All @@ -195,6 +214,23 @@ public Void visitEndElement(EndElementTree endElementTree, Void unused) {


@Override @Override
public Void visitErroneous(ErroneousTree erroneousTree, Void unused) { public Void visitErroneous(ErroneousTree erroneousTree, Void unused) {
JCDiagnostic diagnostic = ((DCErroneous) erroneousTree).diag;
if (diagnostic.getCode().equals("compiler.err.dc.bad.inline.tag")) {
JavadocTag tag = inlineTag(erroneousTree.toString().replace("@", ""));
SuggestedFix fix =
validTags.contains(tag)
? replace(erroneousTree, String.format("{%s}", erroneousTree), state)
: SuggestedFix.builder().build();
String message =
String.format(
"%s is not a valid block tag. Should it be an inline tag instead?", erroneousTree);
state.reportMatch(
buildDescription(diagnosticPosition(getCurrentPath(), state))
.setMessage(message)
.addFix(fix)
.build());
return null;
}
Matcher matcher = PARAM_MATCHER.matcher(erroneousTree.getBody()); Matcher matcher = PARAM_MATCHER.matcher(erroneousTree.getBody());
if (matcher.find()) { if (matcher.find()) {
String parameterName = matcher.group(1); String parameterName = matcher.group(1);
Expand All @@ -218,7 +254,8 @@ public Void visitErroneous(ErroneousTree erroneousTree, Void unused) {
@Override @Override
public Void visitUnknownBlockTag(UnknownBlockTagTree unknownBlockTagTree, Void unused) { public Void visitUnknownBlockTag(UnknownBlockTagTree unknownBlockTagTree, Void unused) {
String tagName = unknownBlockTagTree.getTagName(); String tagName = unknownBlockTagTree.getTagName();
if (KNOWN_OTHER_TAGS.contains(tagName)) { JavadocTag tag = blockTag(tagName);
if (KNOWN_OTHER_TAGS.contains(tag)) {
return super.visitUnknownBlockTag(unknownBlockTagTree, null); return super.visitUnknownBlockTag(unknownBlockTagTree, null);
} }
if (codeTagNestedDepth > 0) { if (codeTagNestedDepth > 0) {
Expand Down Expand Up @@ -263,7 +300,7 @@ public Void visitUnknownBlockTag(UnknownBlockTagTree unknownBlockTagTree, Void u
fixedTags.add(unknownBlockTagTree); fixedTags.add(unknownBlockTagTree);
return super.visitUnknownBlockTag(unknownBlockTagTree, null); return super.visitUnknownBlockTag(unknownBlockTagTree, null);
} }
reportUnknownTag(unknownBlockTagTree, tagName); reportUnknownTag(unknownBlockTagTree, tag);
return super.visitUnknownBlockTag(unknownBlockTagTree, null); return super.visitUnknownBlockTag(unknownBlockTagTree, null);
} }


Expand Down Expand Up @@ -314,7 +351,7 @@ public Void visitUnknownInlineTag(UnknownInlineTagTree unknownInlineTagTree, Voi
fixedTags.add(unknownInlineTagTree); fixedTags.add(unknownInlineTagTree);
return super.visitUnknownInlineTag(unknownInlineTagTree, null); return super.visitUnknownInlineTag(unknownInlineTagTree, null);
} }
reportUnknownTag(unknownInlineTagTree, unknownInlineTagTree.getTagName()); reportUnknownTag(unknownInlineTagTree, inlineTag(name));
return super.visitUnknownInlineTag(unknownInlineTagTree, null); return super.visitUnknownInlineTag(unknownInlineTagTree, null);
} }


Expand All @@ -325,17 +362,23 @@ private boolean isProbablyType(String name) {
|| name.contains("#"); || name.contains("#");
} }


private void reportUnknownTag(DocTree docTree, String tagName) { private void reportUnknownTag(DocTree docTree, JavadocTag tag) {
Optional<String> bestMatch = Utils.getBestMatch(tagName, validTags); Optional<String> bestMatch =
int pos = Utils.getStartPosition(docTree, state) + docTree.toString().indexOf(tagName); Utils.getBestMatch(
String message = String.format("Tag name `%s` is unknown.", tagName); tag.name(),
validTags.stream()
.filter(t -> t.type().equals(tag.type()))
.map(JavadocTag::name)
.collect(toImmutableSet()));
int pos = Utils.getStartPosition(docTree, state) + docTree.toString().indexOf(tag.name());
String message = String.format("Tag name `%s` is unknown.", tag.name());
state.reportMatch( state.reportMatch(
bestMatch bestMatch
.map( .map(
bm -> bm ->
buildDescription(diagnosticPosition(getCurrentPath(), state)) buildDescription(diagnosticPosition(getCurrentPath(), state))
.setMessage(message + String.format(" Did you mean tag `%s`?", bm)) .setMessage(message + String.format(" Did you mean tag `%s`?", bm))
.addFix(SuggestedFix.replace(pos, pos + tagName.length(), bm)) .addFix(SuggestedFix.replace(pos, pos + tag.name().length(), bm))
.build()) .build())
.orElse( .orElse(
buildDescription(diagnosticPosition(getCurrentPath(), state)) buildDescription(diagnosticPosition(getCurrentPath(), state))
Expand All @@ -354,16 +397,16 @@ public Void scan(DocTree docTree, Void unused) {
if (fixedTags.contains(docTree)) { if (fixedTags.contains(docTree)) {
return null; return null;
} }
String tagName = null; JavadocTag tag = null;
if (docTree instanceof DCBlockTag) { if (docTree instanceof DCBlockTag) {
tagName = ((DCBlockTag) docTree).getTagName(); tag = blockTag(((DCBlockTag) docTree).getTagName());
} }
if (docTree instanceof DCInlineTag) { if (docTree instanceof DCInlineTag) {
tagName = ((DCInlineTag) docTree).getTagName(); tag = inlineTag(((DCInlineTag) docTree).getTagName());
} }
if (tagName != null && !validTags.contains(tagName) && !KNOWN_OTHER_TAGS.contains(tagName)) { if (tag != null && !validTags.contains(tag) && !KNOWN_OTHER_TAGS.contains(tag)) {
String message = String message =
String.format("The block tag @%s is not allowed on this type of element.", tagName); String.format("The tag @%s is not allowed on this type of element.", tag.name());
state.reportMatch( state.reportMatch(
buildDescription(diagnosticPosition(getCurrentPath(), state)) buildDescription(diagnosticPosition(getCurrentPath(), state))
.setMessage(message) .setMessage(message)
Expand All @@ -373,4 +416,28 @@ public Void scan(DocTree docTree, Void unused) {
return null; return null;
} }
} }

@AutoValue
abstract static class JavadocTag {
abstract String name();

abstract TagType type();

enum TagType {
BLOCK,
INLINE
}

private static JavadocTag of(String name, TagType type) {
return new AutoValue_InvalidTag_JavadocTag(name, type);
}
}

private static JavadocTag blockTag(String name) {
return JavadocTag.of(name, JavadocTag.TagType.BLOCK);
}

private static JavadocTag inlineTag(String name) {
return JavadocTag.of(name, TagType.INLINE);
}
} }

0 comments on commit e3b7c73

Please sign in to comment.