Skip to content

Commit

Permalink
Extract error messages for CheckReturnValue out into a separate class.
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 493408029
  • Loading branch information
chaoren authored and Error Prone Team committed Dec 6, 2022
1 parent 4e0a00c commit fb1d0b9
Show file tree
Hide file tree
Showing 2 changed files with 138 additions and 57 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}

Expand All @@ -295,12 +295,6 @@ private static ImmutableList<String> presentCrvRelevantAnnotations(Symbol symbol
.collect(toImmutableList());
}

private static String conflictingAnnotations(List<String> 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).
Expand All @@ -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))
Expand All @@ -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
Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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<String> annotations, String elementType) {
return annotations.stream().map(a -> "@" + a).collect(joining(" and "))
+ " cannot be applied to the same "
+ elementType;
}

/**
* Error message for when
*
* <ol>
* <li>the result of a method or constructor invocation is ignored, and
* <li>the {@link ResultUsePolicy} of the invoked method or constructor evaluates to {@link
* ResultUsePolicy#EXPECTED}.
* </ol>
*/
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
*
* <ol>
* <li>a method or constructor is referenced in such a way that its return value would be
* ignored if invoked through the reference, and
* <li>the {@link ResultUsePolicy} of the referenced method or constructor evaluates to {@link
* ResultUsePolicy#EXPECTED}.
* </ol>
*/
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;
}
}
}

0 comments on commit fb1d0b9

Please sign in to comment.