From fb1d0b9d849e5aec3959627be761989b13ceec17 Mon Sep 17 00:00:00 2001 From: Chaoren Lin Date: Tue, 6 Dec 2022 14:09:51 -0800 Subject: [PATCH] Extract error messages for `CheckReturnValue` out into a separate class. PiperOrigin-RevId: 493408029 --- .../bugpatterns/CheckReturnValue.java | 78 ++++-------- .../checkreturnvalue/ErrorMessages.java | 117 ++++++++++++++++++ 2 files changed, 138 insertions(+), 57 deletions(-) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ErrorMessages.java diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java b/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java index 35e2073b0b9..9d794c11b81 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java @@ -22,6 +22,10 @@ import static com.google.errorprone.bugpatterns.checkreturnvalue.AutoValueRules.autoBuilders; import static com.google.errorprone.bugpatterns.checkreturnvalue.AutoValueRules.autoValueBuilders; import static com.google.errorprone.bugpatterns.checkreturnvalue.AutoValueRules.autoValues; +import static com.google.errorprone.bugpatterns.checkreturnvalue.ErrorMessages.annotationOnVoid; +import static com.google.errorprone.bugpatterns.checkreturnvalue.ErrorMessages.conflictingAnnotations; +import static com.google.errorprone.bugpatterns.checkreturnvalue.ErrorMessages.invocationResultIgnored; +import static com.google.errorprone.bugpatterns.checkreturnvalue.ErrorMessages.methodReferenceIgnoresResult; import static com.google.errorprone.bugpatterns.checkreturnvalue.ExternalCanIgnoreReturnValue.externalIgnoreList; import static com.google.errorprone.bugpatterns.checkreturnvalue.ExternalCanIgnoreReturnValue.methodNameAndParams; import static com.google.errorprone.bugpatterns.checkreturnvalue.ExternalCanIgnoreReturnValue.surroundingClass; @@ -43,7 +47,6 @@ import static com.google.errorprone.util.ASTHelpers.isGeneratedConstructor; import static com.google.errorprone.util.ASTHelpers.isLocal; import static com.sun.source.tree.Tree.Kind.METHOD; -import static java.util.stream.Collectors.joining; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -79,7 +82,6 @@ import com.sun.tools.javac.code.Symbol.PackageSymbol; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.processing.JavacProcessingEnvironment; -import java.util.List; import java.util.Optional; import java.util.stream.Stream; import javax.lang.model.element.ElementKind; @@ -283,9 +285,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) { if (!ASTHelpers.isVoidType(method.getReturnType(), state)) { return Description.NO_MATCH; } - String message = - String.format( - "@%s may not be applied to void-returning methods", presentAnnotations.get(0)); + String message = annotationOnVoid(presentAnnotations.get(0)); return buildDescription(tree).setMessage(message).build(); } @@ -295,12 +295,6 @@ private static ImmutableList presentCrvRelevantAnnotations(Symbol symbol .collect(toImmutableList()); } - private static String conflictingAnnotations(List annotations, String targetType) { - return annotations.stream().map(a -> "@" + a).collect(joining(" and ")) - + " cannot be applied to the same " - + targetType; - } - /** * Validate that at most one of {@code CheckReturnValue} and {@code CanIgnoreReturnValue} are * applied to a class (or interface or enum). @@ -318,21 +312,10 @@ public Description matchClass(ClassTree tree, VisitorState state) { } private Description describeInvocationResultIgnored( - ExpressionTree invocationTree, - String shortCall, - String shortCallWithoutNew, - MethodSymbol symbol, - VisitorState state) { + ExpressionTree invocationTree, String shortCall, MethodSymbol symbol, VisitorState state) { + String assignmentToUnused = "var unused = ..."; String message = - String.format( - "The result of `%s` must be used\n" - + "If you really don't want to use the result, then assign it to a variable:" - + " `var unused = ...`.\n" - + "\n" - + "If callers of `%s` shouldn't be required to use its result," - + " then annotate it with `@CanIgnoreReturnValue`.\n" - + "%s", - shortCall, shortCallWithoutNew, apiTrailer(symbol, state)); + invocationResultIgnored(shortCall, assignmentToUnused, apiTrailer(symbol, state)); return buildDescription(invocationTree) .addFix(fixAtDeclarationSite(symbol, state)) .addAllFixes(fixesAtCallSite(invocationTree, state)) @@ -344,18 +327,17 @@ private Description describeInvocationResultIgnored( protected Description describeReturnValueIgnored(MethodInvocationTree tree, VisitorState state) { MethodSymbol symbol = getSymbol(tree); String shortCall = symbol.name + (tree.getArguments().isEmpty() ? "()" : "(...)"); - String shortCallWithoutNew = shortCall; - return describeInvocationResultIgnored(tree, shortCall, shortCallWithoutNew, symbol, state); + return describeInvocationResultIgnored(tree, shortCall, symbol, state); } @Override protected Description describeReturnValueIgnored(NewClassTree tree, VisitorState state) { MethodSymbol symbol = getSymbol(tree); - String shortCallWithoutNew = - state.getSourceForNode(tree.getIdentifier()) + String shortCall = + "new " + + state.getSourceForNode(tree.getIdentifier()) + (tree.getArguments().isEmpty() ? "()" : "(...)"); - String shortCall = "new " + shortCallWithoutNew; - return describeInvocationResultIgnored(tree, shortCall, shortCallWithoutNew, symbol, state); + return describeInvocationResultIgnored(tree, shortCall, symbol, state); } @Override @@ -364,42 +346,24 @@ protected Description describeReturnValueIgnored(MemberReferenceTree tree, Visit Type type = state.getTypes().memberType(getType(tree.getQualifierExpression()), symbol); String parensAndMaybeEllipsis = type.getParameterTypes().isEmpty() ? "()" : "(...)"; - String shortCallWithoutNew; - String shortCall; - if (tree.getMode() == ReferenceMode.NEW) { - shortCallWithoutNew = - state.getSourceForNode(tree.getQualifierExpression()) + parensAndMaybeEllipsis; - shortCall = "new " + shortCallWithoutNew; - } else { - shortCallWithoutNew = tree.getName() + parensAndMaybeEllipsis; - shortCall = shortCallWithoutNew; - } + String shortCall = + (tree.getMode() == ReferenceMode.NEW + ? "new " + state.getSourceForNode(tree.getQualifierExpression()) + : tree.getName()) + + parensAndMaybeEllipsis; String implementedMethod = getType(tree).asElement().getSimpleName() + "." + state.getTypes().findDescriptorSymbol(getType(tree).asElement()).getSimpleName(); String methodReference = state.getSourceForNode(tree); + String assignmentLambda = parensAndMaybeEllipsis + " -> { var unused = ...; }"; String message = - String.format( - "The result of `%s` must be used\n" - + "`%s` acts as an implementation of `%s`" - + " -- which is a `void` method, so it doesn't use the result of `%s`.\n" - + "\n" - + "To use the result, you may need to restructure your code.\n" - + "\n" - + "If you really don't want to use the result, then switch to a lambda that assigns" - + " it to a variable: `%s -> { var unused = ...; }`.\n" - + "\n" - + "If callers of `%s` shouldn't be required to use its result," - + " then annotate it with `@CanIgnoreReturnValue`.\n" - + "%s", + methodReferenceIgnoresResult( shortCall, methodReference, implementedMethod, - shortCall, - parensAndMaybeEllipsis, - shortCallWithoutNew, + assignmentLambda, apiTrailer(symbol, state)); return buildDescription(tree) .setMessage(message) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ErrorMessages.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ErrorMessages.java new file mode 100644 index 00000000000..c497101e654 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ErrorMessages.java @@ -0,0 +1,117 @@ +/* + * Copyright 2022 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.checkreturnvalue; + +import static java.util.stream.Collectors.joining; + +import java.util.List; + +/** Error messages used by {@link com.google.errorprone.bugpatterns.CheckReturnValue}. */ +public class ErrorMessages { + private ErrorMessages() {} + + /** + * Error message for when an annotation used by {@link Rules#mapAnnotationSimpleName} is applied + * to a void-returning method. + */ + public static String annotationOnVoid(String annotation) { + return String.format("@%s may not be applied to void-returning methods", annotation); + } + + /** + * Error message for when annotations mapped to conflicting {@link ResultUsePolicy}s are applied + * to the same element. + * + * @param elementType the {@link java.lang.annotation.ElementType} of the annotated element (e.g., + * "method" or "class"). + */ + public static String conflictingAnnotations(List annotations, String elementType) { + return annotations.stream().map(a -> "@" + a).collect(joining(" and ")) + + " cannot be applied to the same " + + elementType; + } + + /** + * Error message for when + * + *
    + *
  1. the result of a method or constructor invocation is ignored, and + *
  2. the {@link ResultUsePolicy} of the invoked method or constructor evaluates to {@link + * ResultUsePolicy#EXPECTED}. + *
+ */ + public static String invocationResultIgnored( + String shortCall, String assignmentToUnused, String apiTrailer) { + String shortCallWithoutNew = removeNewPrefix(shortCall); + return String.format( + "The result of `%s` must be used\n" + + "If you really don't want to use the result, then assign it to a variable:" + + " `%s`.\n" + + "\n" + + "If callers of `%s` shouldn't be required to use its result," + + " then annotate it with `@CanIgnoreReturnValue`.\n" + + "%s", + shortCall, assignmentToUnused, shortCallWithoutNew, apiTrailer); + } + + /** + * Error message for when + * + *
    + *
  1. a method or constructor is referenced in such a way that its return value would be + * ignored if invoked through the reference, and + *
  2. the {@link ResultUsePolicy} of the referenced method or constructor evaluates to {@link + * ResultUsePolicy#EXPECTED}. + *
+ */ + public static String methodReferenceIgnoresResult( + String shortCall, + String methodReference, + String implementedMethod, + String assignmentLambda, + String apiTrailer) { + String shortCallWithoutNew = removeNewPrefix(shortCall); + return String.format( + "The result of `%s` must be used\n" + + "`%s` acts as an implementation of `%s`" + + " -- which is a `void` method, so it doesn't use the result of `%s`.\n" + + "\n" + + "To use the result, you may need to restructure your code.\n" + + "\n" + + "If you really don't want to use the result, then switch to a lambda that assigns" + + " it to a variable: `%s`.\n" + + "\n" + + "If callers of `%s` shouldn't be required to use its result," + + " then annotate it with `@CanIgnoreReturnValue`.\n" + + "%s", + shortCall, + methodReference, + implementedMethod, + shortCall, + assignmentLambda, + shortCallWithoutNew, + apiTrailer); + } + + private static String removeNewPrefix(String shortCall) { + if (shortCall.startsWith("new ")) { + return shortCall.substring("new ".length()); + } else { + return shortCall; + } + } +}