diff --git a/mug-errorprone/src/main/java/com/google/mu/errorprone/AbstractBugChecker.java b/mug-errorprone/src/main/java/com/google/mu/errorprone/AbstractBugChecker.java index 3d0ce3af7b..8d3ed14b0c 100644 --- a/mug-errorprone/src/main/java/com/google/mu/errorprone/AbstractBugChecker.java +++ b/mug-errorprone/src/main/java/com/google/mu/errorprone/AbstractBugChecker.java @@ -1,14 +1,125 @@ package com.google.mu.errorprone; -import com.google.common.collect.ImmutableMap; + +import static com.google.common.base.Preconditions.checkNotNull; + +import com.google.common.base.Strings; +import com.google.errorprone.VisitorState; +import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.bugpatterns.BugChecker; -import com.google.mu.util.Substring; -import com.google.mu.util.stream.BiStream; -import static com.google.mu.util.stream.GuavaCollectors.toImmutableMap; +import com.google.errorprone.matchers.Description; +import com.sun.source.tree.MemberReferenceTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.Tree; -/** Abstract class providing convenience to BugChecker implementations. */ +/** + * A convenience base class allowing subclasses to use precondition-style checking such as {@code + * checkingOn(tree).require(formatString != null, "message...")}. + * + *

The subclass should implement one or multiple of the "mixin" interfaces ({@link + * ConstructorCallCheck}, {@link MethodInvocationCheck}, {@link MemberReferenceCheck}). They adapt + * from the {@link ErrorReport} exception thrown by the `check` abstract methods to the {@link + * com.google.errorprone.matchers.Description} return value expected by Error-Prone. + */ abstract class AbstractBugChecker extends BugChecker { - private static final Substring.Pattern PLACEHOLDER = Substring.between('{', '}'); - private static final ImmutableMap MAP = BiStream.empty().collect(toImmutableMap()); + /** + * Mixin interface for checkers that check constructor calls. Subclasses can implement the {@link + * #checkConstructorCall} and throw {@code ErrorReport} to indicate failures. + */ + interface ConstructorCallCheck extends NewClassTreeMatcher { + /** DO NOT override this method. Implement {@link #checkConstructorCall} instead. */ + @Override + public default Description matchNewClass(NewClassTree tree, VisitorState state) { + return ErrorReport.checkAndReportError(tree, state, this::checkConstructorCall); + } + + void checkConstructorCall(NewClassTree tree, VisitorState state) throws ErrorReport; + } + + /** + * Mixin interface for checkers that check member references. Subclasses can implement the {@link + * #checkMemberReference} and throw {@code ErrorReport} to indicate failures. + */ + interface MemberReferenceCheck extends MemberReferenceTreeMatcher { + /** DO NOT override this method. Implement {@link #checkMemberReference} instead. */ + @Override + public default Description matchMemberReference(MemberReferenceTree tree, VisitorState state) { + return ErrorReport.checkAndReportError(tree, state, this::checkMemberReference); + } + + void checkMemberReference(MemberReferenceTree tree, VisitorState state) throws ErrorReport; + } + + /** + * Mixin interface for checkers that check method invocations. Subclasses can implement the {@link + * #checkMethodInvocation} and throw {@code ErrorReport} to indicate failures. + */ + interface MethodInvocationCheck extends MethodInvocationTreeMatcher { + /** DO NOT override this method. Implement {@link #checkMethodInvocation} instead. */ + @Override + public default Description matchMethodInvocation( + MethodInvocationTree tree, VisitorState state) { + return ErrorReport.checkAndReportError(tree, state, this::checkMethodInvocation); + } + + void checkMethodInvocation(MethodInvocationTree tree, VisitorState state) throws ErrorReport; + } + + /** Starts checking on {@code node}. Errors will be reported as pertaining to this node. */ + final NodeCheck checkingOn(Tree node) { + checkNotNull(node); + return (condition, message, args) -> { + if (condition) { + return checkingOn(node); + } + throw new ErrorReport(buildDescription(node), message, args); + }; + } + + /** Fluently checking on a tree node. */ + interface NodeCheck { + /** + * Checks that {code condition} holds or else reports error using {@code message} with {@code + * args}. If an arg is an instance of {@link Tree}, the source code of that tree node will be + * reported in the error message. + */ + @CanIgnoreReturnValue + NodeCheck require(boolean condition, String message, Object... args) throws ErrorReport; + } + + /** An error report of a violation. */ + static final class ErrorReport extends Exception { + private final Description.Builder description; + private final Object[] args; + + private ErrorReport(Description.Builder description, String message, Object... args) { + super(message, null, false, false); + this.description = description; + this.args = args.clone(); + } + + private static Description checkAndReportError( + T tree, VisitorState state, Checker impl) { + try { + impl.check(tree, state); + } catch (ErrorReport report) { + return report.buildDescription(state); + } + return Description.NO_MATCH; + } + + private interface Checker { + void check(T tree, VisitorState state) throws ErrorReport; + } + private Description buildDescription(VisitorState state) { + for (int i = 0; i < args.length; i++) { + if (args[i] instanceof Tree) { + args[i] = state.getSourceForNode((Tree) args[i]); + } + } + return description.setMessage(Strings.lenientFormat(getMessage(), args)).build(); + } + } } diff --git a/mug-errorprone/src/main/java/com/google/mu/errorprone/FormatStringUtils.java b/mug-errorprone/src/main/java/com/google/mu/errorprone/FormatStringUtils.java new file mode 100644 index 0000000000..86c764cb6a --- /dev/null +++ b/mug-errorprone/src/main/java/com/google/mu/errorprone/FormatStringUtils.java @@ -0,0 +1,79 @@ +package com.google.mu.errorprone; + + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.mu.util.Optionals.optionally; +import static com.google.mu.util.Substring.consecutive; +import static com.google.mu.util.Substring.first; +import static com.google.mu.util.Substring.BoundStyle.INCLUSIVE; + +import java.util.List; +import java.util.Optional; + +import com.google.common.base.CharMatcher; +import com.google.common.collect.ImmutableList; +import com.google.errorprone.VisitorState; +import com.google.errorprone.util.ASTHelpers; +import com.google.mu.util.Substring; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; +import com.sun.tools.javac.api.JavacTrees; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.VarSymbol; + +/** Some common utils for format and unformat checks. */ +final class FormatStringUtils { + static final Substring.Pattern PLACEHOLDER_PATTERN = + consecutive(CharMatcher.noneOf("{}")::matches).immediatelyBetween("{", INCLUSIVE, "}", INCLUSIVE); + static final Substring.RepeatingPattern PLACEHOLDER_NAMES_PATTERN = + consecutive(CharMatcher.noneOf("{}")::matches).immediatelyBetween("{", "}").repeatedly(); + + static ImmutableList placeholderVariableNames(String formatString) { + return PLACEHOLDER_NAMES_PATTERN + .from(formatString) + .map(first('=').toEnd()::removeFrom) // for Cloud resource name syntax + .collect(toImmutableList()); + } + + static Optional getInlineStringArg(Tree expression, VisitorState state) { + ImmutableList args = + invocationArgs(expression).stream() + .map(ASTHelpers::stripParentheses) + .filter(arg -> isStringType(arg, state)) + .collect(toImmutableList()); + return optionally(args.size() == 1, () -> args.get(0)); + } + + static Optional findFormatString(Tree unformatter, VisitorState state) { + if (unformatter instanceof IdentifierTree) { + Symbol symbol = ASTHelpers.getSymbol(unformatter); + if (symbol instanceof VarSymbol) { + Tree def = JavacTrees.instance(state.context).getTree(symbol); + if (def instanceof VariableTree) { + return findFormatString(((VariableTree) def).getInitializer(), state); + } + } + return Optional.empty(); + } + return getInlineStringArg(unformatter, state) + .map(tree -> ASTHelpers.constValue(tree, String.class)); + } + + private static List invocationArgs(Tree tree) { + if (tree instanceof NewClassTree) { + return ((NewClassTree) tree).getArguments(); + } + if (tree instanceof MethodInvocationTree) { + return ((MethodInvocationTree) tree).getArguments(); + } + return ImmutableList.of(); + } + + private static boolean isStringType(ExpressionTree arg, VisitorState state) { + return ASTHelpers.isSameType(ASTHelpers.getType(arg), state.getSymtab().stringType, state); + } +} diff --git a/mug-errorprone/src/main/java/com/google/mu/errorprone/StringUnformatArgsCheck.java b/mug-errorprone/src/main/java/com/google/mu/errorprone/StringUnformatArgsCheck.java new file mode 100644 index 0000000000..6a631fab7b --- /dev/null +++ b/mug-errorprone/src/main/java/com/google/mu/errorprone/StringUnformatArgsCheck.java @@ -0,0 +1,302 @@ +package com.google.mu.errorprone; + +import static com.google.mu.util.stream.GuavaCollectors.toImmutableMap; +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.mu.util.stream.BiStream.toAdjacentPairs; +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static com.google.errorprone.matchers.Matchers.instanceMethod; + +import java.util.List; +import java.util.function.BiConsumer; +import java.util.function.BiFunction; +import java.util.function.BinaryOperator; +import java.util.function.Consumer; +import java.util.function.Function; +import java.util.stream.Collector; + +import com.google.auto.service.AutoService; +import com.google.common.base.CaseFormat; +import com.google.common.base.CharMatcher; +import com.google.common.base.Strings; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.LambdaExpressionTree; +import com.sun.source.tree.MemberReferenceTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.MethodTree; +import com.sun.tools.javac.api.JavacTrees; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Type; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.LinkType; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.matchers.method.MethodMatchers.MethodClassMatcher; +import com.google.errorprone.util.ASTHelpers; +import com.google.mu.errorprone.AbstractBugChecker.ErrorReport; +import com.google.mu.function.Quarternary; +import com.google.mu.function.Quinary; +import com.google.mu.function.Senary; +import com.google.mu.function.Ternary; +import com.google.mu.util.CaseBreaker; +import com.google.mu.util.stream.BiCollector; +import com.google.mu.util.stream.BiStream; + +/** + * Checks that the "unformat" methods (methods that parse an input string according to a predefined + * format string with curly braced pattern placeholders) are invoked with the correct lambda + * according to the string format. + */ +@BugPattern( + summary = + "Checks that StringFormat and ResourceNamePattern callers pass in lambda" + + " that accept the same number of placeholders as defined in the string format.", + link = "go/java-tips/024#compile-time-check", + linkType = LinkType.CUSTOM, + severity = ERROR) +@AutoService(BugChecker.class) +public final class StringUnformatArgsCheck extends AbstractBugChecker + implements AbstractBugChecker.MethodInvocationCheck { + private static final MethodClassMatcher MATCHER = + instanceMethod().onDescendantOf("com.google.mu.util.StringFormat"); + private static final CharMatcher ALPHA_NUM = + CharMatcher.inRange('a', 'z') + .or(CharMatcher.inRange('A', 'Z')) + .or(CharMatcher.inRange('0', '9')); + private static final ImmutableMap UNFORMAT_MAPPER_TYPES = + BiStream.of( + Consumer.class, 1, + BiConsumer.class, 2, + Function.class, 1, + BiFunction.class, 2, + BinaryOperator.class, 2) + .append(Ternary.class, 3) + .append(Quarternary.class, 4) + .append(Quinary.class, 5) + .append(Senary.class, 6) + .append(Collector.class, 1) + .append(BiCollector.class, 2) + .mapKeys(TypeName::of) + .collect(toImmutableMap()); + private static final String NONCAPTURING_PLACEHOLDER = "..."; + + @Override + public void checkMethodInvocation(MethodInvocationTree tree, VisitorState state) + throws ErrorReport { + if (!MATCHER.matches(tree, state)) { + return; + } + MethodSymbol symbol = ASTHelpers.getSymbol(tree); + if (symbol.isVarArgs()) { + return; + } + ExpressionTree stringArg = + BiStream.zip(symbol.getParameters(), tree.getArguments()) + .filterKeys(param -> isStringType(param.type, state)) + .values() + .findFirst() + .orElse(null); + if (stringArg == null) { + return; // Not an unformat or parse method. + } + ExpressionTree unformatMapperArg = + BiStream.zip(symbol.getParameters(), tree.getArguments()) + .filterKeys(param -> isUnformatMapperType(param.type, state)) + .values() + .findFirst() + .orElse(null); + int expectedPlaceholders = + symbol.getParameters().stream() + .map(param -> expectedNumPlaceholders(param.type, state)) + .filter(num -> num > 0) + .findFirst() + .orElse(0); + ExpressionTree unformatter = ASTHelpers.getReceiver(tree); + if (unformatter == null) { + return; // Must be a private instance method. + } + String formatString = FormatStringUtils.findFormatString(unformatter, state).orElse(null); + checkingOn(tree) + .require( + formatString == null + || FormatStringUtils + .PLACEHOLDER_NAMES_PATTERN + .match(formatString) + .collect(toAdjacentPairs()) + // In "{foo}{bar}", bar.index() - 2 is at the end of foo. + .noneMatch((p1, p2) -> p2.index() - 2 <= p1.index() + p1.length()), + "Format string defined by %s with two placeholders immediately next to each other is" + + " inherently ambiguous to parse.", + unformatter); + if (unformatMapperArg == null || expectedPlaceholders == 0) { + return; // No unformat mapper function parameter to check + } + checkingOn(unformatter) + .require( + formatString != null, + "Compile-time format string required for validating the lambda parameter [%s];" + + " definition not found. As a result, the lambda parameters cannot be" + + " validated at compile-time.\nIf your format string is dynamically loaded or" + + " dynamically computed, and you opt to use the API despite the risk of" + + " not having comile-time guarantee, consider suppressing the error with" + + " @SuppressWarnings(\"StringUnformatArgsCheck\").", + unformatMapperArg); + ImmutableList placeholderVariableNames = + FormatStringUtils.placeholderVariableNames(formatString).stream() + .filter(n -> !n.equals(NONCAPTURING_PLACEHOLDER)) + .collect(toImmutableList()); + checkingOn(tree) + .require( + placeholderVariableNames.size() == expectedPlaceholders, + "%s capturing placeholders defined by: %s; %s expected by %s", + placeholderVariableNames.size(), + unformatter, + expectedPlaceholders, + unformatMapperArg); + if (unformatMapperArg instanceof LambdaExpressionTree) { + checkLambdaParameters( + tree, (LambdaExpressionTree) unformatMapperArg, placeholderVariableNames); + } else if (unformatMapperArg instanceof MemberReferenceTree) { + checkMethodReference( + tree, (MemberReferenceTree) unformatMapperArg, placeholderVariableNames, state); + } + } + + private void checkLambdaParameters( + ExpressionTree unformatInvocation, + LambdaExpressionTree lambda, + List placeholderVariableNames) + throws ErrorReport { + ImmutableList lambdaParamNames = + lambda.getParameters().stream() + .map(param -> param.getName().toString()) + .collect(toImmutableList()); + ImmutableList normalizedLambdaParamNames = + normalizeNamesForComparison(lambdaParamNames); + ImmutableList normalizedPlaceholderNames = + normalizeNamesForComparison(placeholderVariableNames); + checkingOn(unformatInvocation) + .require( + !outOfOrder(normalizedLambdaParamNames, normalizedPlaceholderNames), + "lambda variables %s appear to be in inconsistent order with the placeholder" + + " variables as defined by: %s", + lambdaParamNames, + ASTHelpers.getReceiver(unformatInvocation)); + for (int i = 0; i < placeholderVariableNames.size(); i++) { + checkingOn(unformatInvocation) + .require( + mightBeForSameThing( + normalizedLambdaParamNames.get(i), normalizedPlaceholderNames.get(i)), + "Lambda variable `%s` doesn't look to be for placeholder {%s} as defined by: %s\n" + + "Consider using %s as the lambda variable name or renaming the {%s}" + + " placeholder. A prefix or suffix will work too.", + lambdaParamNames.get(i), + placeholderVariableNames.get(i), + ASTHelpers.getReceiver(unformatInvocation), + placeholderVariableNames.get(i), + placeholderVariableNames.get(i)); + } + } + + private void checkMethodReference( + ExpressionTree unformatInvocation, + MemberReferenceTree methodRef, + List placeholderVariableNames, + VisitorState state) + throws ErrorReport { + MethodTree method = JavacTrees.instance(state.context).getTree(ASTHelpers.getSymbol(methodRef)); + if (method == null) { + return; // This shouldn't happen. But if it did, we don't want to fail compilation. + } + ImmutableList paramNames = + method.getParameters().stream() + .map(param -> param.getName().toString()) + .collect(toImmutableList()); + ImmutableList normalizedParamNames = normalizeNamesForComparison(paramNames); + ImmutableList normalizedPlaceholderNames = + normalizeNamesForComparison(placeholderVariableNames); + if (normalizedParamNames.size() != placeholderVariableNames.size()) { + // Can happen if the method ref is like SomeType::someInstanceMethod. + // Because we only have string placeholders, SomeType can only be String or its super + // types. It's an unusual case and there is no way users can ensure name match without + // resorting to explicit lambda. Just trust the programmer. + return; + } + checkingOn(unformatInvocation) + .require( + !outOfOrder(normalizedParamNames, normalizedPlaceholderNames), + "Parameters of referenced method %s(%s) appear to be in inconsistent order with the" + + " placeholder variables as defined by: %s", + methodRef, + String.join(", ", paramNames), + ASTHelpers.getReceiver(unformatInvocation)); + if (normalizedParamNames.size() < 3 + && !ASTHelpers.inSamePackage(ASTHelpers.getSymbol(method), state)) { + // For 1 or 2-arg public API, don't fail on parameter names, because it may make it harder + // to evolve common APIs. + // We still validate out-of-order error which should unlikely happen with API evolution. + // + // It should be rare for idiomatic common public API to have 3+ String args + // because such methods would be easy to mess up. + return; + } + for (int i = 0; i < placeholderVariableNames.size(); i++) { + checkingOn(unformatInvocation) + .require( + mightBeForSameThing(normalizedParamNames.get(i), normalizedPlaceholderNames.get(i)), + "Method parameter `%s` of referenced method `%s` doesn't look to be for" + + " placeholder {%s} as defined by: %s\n" + + "Consider using `%s` as the method parameter name, renaming the {%s}" + + " placeholder, or using a lambda expression where you can use the" + + " placeholder name as the parameter name.", + paramNames.get(i), + methodRef, + placeholderVariableNames.get(i), + ASTHelpers.getReceiver(unformatInvocation), + placeholderVariableNames.get(i), + placeholderVariableNames.get(i)); + } + } + + private static ImmutableList normalizeNamesForComparison(List names) { + return names.stream() + .map(name -> CaseBreaker.toCase(CaseFormat.UPPER_CAMEL, name)) // id and jobId should match + .map(ALPHA_NUM.negate()::removeFrom) + .collect(toImmutableList()); + } + + private static boolean isStringType(Type type, VisitorState state) { + return ASTHelpers.isSameType(type, state.getSymtab().stringType, state); + } + + private static boolean isUnformatMapperType(Type type, VisitorState state) { + return expectedNumPlaceholders(type, state) > 0; + } + + private static int expectedNumPlaceholders(Type type, VisitorState state) { + return BiStream.from(UNFORMAT_MAPPER_TYPES) + .filterKeys(mapperType -> mapperType.isSameType(type, state)) + .values() + .findFirst() + .orElse(0); + } + + private static boolean outOfOrder(List names1, List names2) { + ImmutableSet nameSet = ImmutableSet.copyOf(names2); + return names1.size() > 1 + && names1.stream().allMatch(nameSet::contains) + && !names1.equals(names2); + } + + private static boolean mightBeForSameThing(String name1, String name2) { + return name1.startsWith(name2) + || name2.startsWith(name1) + || name1.endsWith(name2) + || name2.endsWith(name1) + || Strings.commonPrefix(name1, name2).length() > 3; + } +} diff --git a/mug-errorprone/src/main/java/com/google/mu/errorprone/TypeName.java b/mug-errorprone/src/main/java/com/google/mu/errorprone/TypeName.java new file mode 100644 index 0000000000..314adf0d39 --- /dev/null +++ b/mug-errorprone/src/main/java/com/google/mu/errorprone/TypeName.java @@ -0,0 +1,45 @@ +package com.google.mu.errorprone; + +import java.util.Objects; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.suppliers.Supplier; +import com.google.errorprone.util.ASTHelpers; +import com.sun.tools.javac.code.Type; + +/** Represents a (lazily memoized) type name with convenient methods to operate on it. */ +final class TypeName { + private final String name; + private final Supplier memoized; + + TypeName(String name) { + this.name = Objects.requireNonNull(name); + this.memoized = VisitorState.memoize(state -> state.getTypeFromString(name)); + } + + static TypeName of(Class cls) { + return new TypeName(cls.getTypeName()); + } + + boolean isSameType(Type type, VisitorState state) { + return ASTHelpers.isSameType(memoized.get(state), type, state); + } + + @Override + public boolean equals(Object obj) { + if (obj instanceof TypeName) { + return name.equals(((TypeName) obj).name); + } + return false; + } + + @Override + public int hashCode() { + return name.hashCode(); + } + + @Override + public String toString() { + return name; + } +}