Skip to content

Commit

Permalink
Updated NamedParameters to improve error message when comment doesn't…
Browse files Browse the repository at this point in the history 
… conform

to style and also to detect bad after-block comments. Change the suggestion to include spaces around the equals sign (the check is agnostic to spaces).

This change also removes @RequiresNamedParameters. We probably shouldn't be providing a new language feature which legitimizes an API design which should instead be refactored to improve usability.

RELNOTES: Change suggestion for NamedParameters to include spaces around the equals sign, improved errormessage and removed @RequiresNamedParameters

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=158570586
  • Loading branch information
andrewrice authored and Eddie Aftandilian committed Jun 28, 2017
1 parent 4691d02 commit 96f1ed6
Show file tree
Hide file tree
Showing 9 changed files with 286 additions and 196 deletions.
38 changes: 0 additions & 38 deletions annotations/src/main/java/com/google/errorprone/annotations/RequiresNamedParameters.java
View file

This file was deleted.

3 changes: 2 additions & 1 deletion ...va/com/google/errorprone/bugpatterns/argumentselectiondefects/NameInCommentHeuristic.java
View file
Expand Up @@ -52,7 +52,8 @@ public boolean isAcceptableChange(
.noneMatch(
p -> {
MatchType match =
NamedParameterComment.match(comments.get(p.formal().index()), p.formal().name());
NamedParameterComment.match(comments.get(p.formal().index()), p.formal().name())
.matchType();
return match == MatchType.EXACT_MATCH || match == MatchType.APPROXIMATE_MATCH;
});
}
Expand Down
168 changes: 60 additions & 108 deletions ...ava/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterChecker.java
View file
Expand Up @@ -19,21 +19,18 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.errorprone.BugPattern.Category.JDK;
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.matchers.Matchers.symbolHasAnnotation;

import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Streams;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.annotations.RequiresNamedParameters;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher;
import com.google.errorprone.bugpatterns.argumentselectiondefects.NamedParameterComment.MatchType;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.util.ASTHelpers;
import com.google.errorprone.util.Commented;
import com.google.errorprone.util.Comments;
Expand All @@ -44,7 +41,6 @@
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Symbol.VarSymbol;
import com.sun.tools.javac.parser.Tokens.Comment;
import com.sun.tools.javac.parser.Tokens.Comment.CommentStyle;
import com.sun.tools.javac.util.List;
import java.util.Optional;
import java.util.stream.Collectors;
Expand All @@ -58,13 +54,6 @@
@BugPattern(
name = "NamedParameters",
summary = "Parameter name in argument comment is missing or incorrect",
explanation =
"For clarity, and to avoid potentially incorrectly swapping arguments, arguments may be "
+ "explicitly matched to their parameter by preceding them with a block comment "
+ "containing the parameter name followed by an equals sign (\"=\"). Mismatches between "
+ "the name in the comment and the actual name will then cause a compilation error. If "
+ "the called method is annotated with RequiresNamedParameters then an error will occur "
+ "if any names are omitted.",
category = JDK,
severity = WARNING
)
Expand All @@ -74,139 +63,127 @@ public class NamedParameterChecker extends BugChecker
@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
return matchNewClassOrMethodInvocation(
ASTHelpers.getSymbol(tree), Comments.findCommentsForArguments(tree, state), tree, state);
ASTHelpers.getSymbol(tree), Comments.findCommentsForArguments(tree, state), tree);
}

@Override
public Description matchNewClass(NewClassTree tree, VisitorState state) {
return matchNewClassOrMethodInvocation(
ASTHelpers.getSymbol(tree), Comments.findCommentsForArguments(tree, state), tree, state);
ASTHelpers.getSymbol(tree), Comments.findCommentsForArguments(tree, state), tree);
}

private Description matchNewClassOrMethodInvocation(
MethodSymbol symbol,
ImmutableList<Commented<ExpressionTree>> arguments,
Tree tree,
VisitorState state) {
MethodSymbol symbol, ImmutableList<Commented<ExpressionTree>> arguments, Tree tree) {

if (symbol == null) {
return Description.NO_MATCH;
}

boolean commentsRequired = hasRequiresNamedParametersAnnotation().matches(tree, state);

// if we don't have parameter names available then raise an error if comments required, else
// silently ignore
// if we don't have parameter names available then give up
List<VarSymbol> parameters = symbol.getParameters();
if (containsSyntheticParameterName(parameters)) {
return commentsRequired
? buildDescription(tree)
.setMessage(
"Method requires parameter name comments but parameter names are not available.")
.build()
: Description.NO_MATCH;
return Description.NO_MATCH;
}

ImmutableList<LabelledArgument> labelledArguments =
LabelledArgument.createFromParametersList(parameters, arguments);

ImmutableList<LabelledArgument> incorrectlyLabelledArguments =
labelledArguments
.stream()
.filter(labelledArgument -> !labelledArgument.isCorrectlyAnnotated(commentsRequired))
.collect(toImmutableList());

if (incorrectlyLabelledArguments.isEmpty()) {
return Description.NO_MATCH;
}

// Build fix
// In general: if a comment is missing and it should be there then we suggest adding it
// If a comment is wrong but matches the parameter name of a different argument then we suggest
// swapping the arguments. If a comment is wrong and matches nothing then we suggest changing it

SuggestedFix.Builder fixBuilder = SuggestedFix.builder();
for (LabelledArgument argumentWithBadLabel : incorrectlyLabelledArguments) {
switch (argumentWithBadLabel.match()) {
ImmutableList.Builder<String> incorrectParameterDescriptions = ImmutableList.builder();
for (LabelledArgument labelledArgument : labelledArguments) {
switch (labelledArgument.matchedComment().matchType()) {
case NOT_ANNOTATED:
fixBuilder.prefixWith(
argumentWithBadLabel.actualParameter().tree(),
NamedParameterComment.toCommentText(argumentWithBadLabel.parameterName()));
case EXACT_MATCH:
break;
case BAD_MATCH:
case APPROXIMATE_MATCH:
// we know that this has a comment because it was a bad match
Comment badLabel = argumentWithBadLabel.label().get();
removeComment(labelledArgument.matchedComment().comment(), fixBuilder);
addComment(labelledArgument, fixBuilder);
incorrectParameterDescriptions.add(
String.format(
"%s (comment does not conform to required style)",
labelledArgument.parameterName()));
break;
case BAD_MATCH:
Comment badLabel = labelledArgument.matchedComment().comment();
Optional<LabelledArgument> maybeGoodTarget =
findGoodSwap(argumentWithBadLabel, labelledArguments);
findGoodSwap(labelledArgument, labelledArguments);

if (maybeGoodTarget.isPresent()) {
LabelledArgument argumentWithCorrectLabel = maybeGoodTarget.get();
fixBuilder.swap(
argumentWithBadLabel.actualParameter().tree(),
labelledArgument.actualParameter().tree(),
argumentWithCorrectLabel.actualParameter().tree());

Optional<Comment> correctLabel = argumentWithCorrectLabel.label();
if (correctLabel.isPresent()) {
replaceComment(badLabel, correctLabel.get().getText(), fixBuilder);
if (argumentWithCorrectLabel.matchedComment().matchType() == MatchType.NOT_ANNOTATED) {
removeComment(badLabel, fixBuilder);
addComment(argumentWithCorrectLabel, fixBuilder);
} else {
replaceComment(badLabel, "", fixBuilder);
fixBuilder.prefixWith(
argumentWithCorrectLabel.actualParameter().tree(),
NamedParameterComment.toCommentText(argumentWithCorrectLabel.parameterName()));
replaceComment(
badLabel,
argumentWithCorrectLabel.matchedComment().comment().getText(),
fixBuilder);
}
} else {
// there were no matches so maybe the comment is wrong - suggest a fix to change it
replaceComment(
badLabel,
NamedParameterComment.toCommentText(argumentWithBadLabel.parameterName()),
NamedParameterComment.toCommentText(labelledArgument.parameterName()),
fixBuilder);
}
incorrectParameterDescriptions.add(
String.format(
"%s (comment does not match formal parameter name)",
labelledArgument.parameterName()));
break;
case EXACT_MATCH:
throw new IllegalArgumentException(
"There should be no good matches in the list of bad matches");
}
}

if (fixBuilder.isEmpty()) {
return Description.NO_MATCH;
}

return buildDescription(tree)
.setMessage(
"Parameters with incorrectly labelled arguments: "
+ incorrectlyLabelledArguments
.stream()
.map(NamedParameterChecker::describeIncorrectlyLabelledArgument)
.collect(Collectors.joining(", ")))
+ incorrectParameterDescriptions.build().stream().collect(Collectors.joining(", ")))
.addFix(fixBuilder.build())
.build();
}

private static String describeIncorrectlyLabelledArgument(LabelledArgument p) {
switch (p.match()) {
case NOT_ANNOTATED:
case APPROXIMATE_MATCH:
return String.format("%s (missing name label)", p.parameterName());
case BAD_MATCH:
return String.format("%s (label doesn't match parameter name)", p.parameterName());
case EXACT_MATCH:
// fall through
}
throw new IllegalArgumentException("Impossible match type in list of bad matches");
}

private static boolean containsSyntheticParameterName(List<VarSymbol> parameters) {
return parameters
.stream()
.map(p -> p.getSimpleName().toString())
.anyMatch(p -> p.matches("arg[0-9]"));
private static void addComment(
LabelledArgument labelledArgument, SuggestedFix.Builder fixBuilder) {
fixBuilder.prefixWith(
labelledArgument.actualParameter().tree(),
NamedParameterComment.toCommentText(labelledArgument.parameterName()));
}

/**
* Replace the given comment with the replacementText. The replacement text is used verbatim and
* so should begin and end with block comment delimiters
*/
private static void replaceComment(
Comment comment, String replacementText, SuggestedFix.Builder fixBuilder) {
int commentStart = comment.getSourcePos(0);
int commentEnd = commentStart + comment.getText().length();
fixBuilder.replace(commentStart, commentEnd, replacementText);
}

private static void removeComment(Comment comment, SuggestedFix.Builder fixBuilder) {
replaceComment(comment, "", fixBuilder);
}

private static boolean containsSyntheticParameterName(List<VarSymbol> parameters) {
return parameters
.stream()
.map(p -> p.getSimpleName().toString())
.anyMatch(p -> p.matches("arg[0-9]"));
}

/**
* Search all arguments for a target argument which would make a good swap with the source
* argument. A good swap would result in the label for the source argument (exactly) matching the
Expand All @@ -222,11 +199,11 @@ private static Optional<LabelledArgument> findGoodSwap(
}

boolean sourceLabelMatchesTarget =
NamedParameterComment.match(source.actualParameter(), target.parameterName())
NamedParameterComment.match(source.actualParameter(), target.parameterName()).matchType()
== MatchType.EXACT_MATCH;

MatchType targetCommentMatch =
NamedParameterComment.match(target.actualParameter(), source.parameterName());
NamedParameterComment.match(target.actualParameter(), source.parameterName()).matchType();

boolean targetLabelMatchesSource =
targetCommentMatch == MatchType.EXACT_MATCH
Expand All @@ -239,10 +216,6 @@ private static Optional<LabelledArgument> findGoodSwap(
return Optional.empty();
}

private static Matcher<Tree> hasRequiresNamedParametersAnnotation() {
return symbolHasAnnotation(RequiresNamedParameters.class.getCanonicalName());
}

/** Information about an argument, the name attached to it with a comment */
@AutoValue
abstract static class LabelledArgument {
Expand All @@ -251,28 +224,7 @@ abstract static class LabelledArgument {

abstract Commented<ExpressionTree> actualParameter();

abstract MatchType match();

boolean isCorrectlyAnnotated(boolean commentRequired) {
switch (match()) {
case EXACT_MATCH:
return true;
case BAD_MATCH:
case APPROXIMATE_MATCH:
return false;
case NOT_ANNOTATED:
return !commentRequired;
}
return false;
}

Optional<Comment> label() {
return Streams.findLast(
actualParameter()
.beforeComments()
.stream()
.filter(c -> c.getStyle() == CommentStyle.BLOCK));
}
abstract NamedParameterComment.MatchedComment matchedComment();

static ImmutableList<LabelledArgument> createFromParametersList(
List<VarSymbol> parameters, ImmutableList<Commented<ExpressionTree>> actualParameters) {
Expand Down

0 comments on commit 96f1ed6

Please sign in to comment.