Skip to content

Commit

Permalink
Fix minor error-message problems:
Browse files Browse the repository at this point in the history
- Fix a bug with the "Full API" line for local classes.
- Convert an em dash + line break into a double dash, and remove the accidental preceding period: The line break was kind of weird, and an em dash won't necessarily render well in all environments.

While I was in the file, IntelliJ was getting on me to inline a couple fields into the constructor and to remove an unnecessary cast to `MethodType`, so I did those things, too.

PiperOrigin-RevId: 479350008
  • Loading branch information
cpovirk authored and Error Prone Team committed Oct 6, 2022
1 parent e06811f commit bf5d25f
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,13 @@
import static com.google.errorprone.bugpatterns.checkreturnvalue.Rules.mapAnnotationSimpleName;
import static com.google.errorprone.fixes.SuggestedFix.emptyFix;
import static com.google.errorprone.fixes.SuggestedFixes.qualifyType;
import static com.google.errorprone.util.ASTHelpers.enclosingClass;
import static com.google.errorprone.util.ASTHelpers.getAnnotationsWithSimpleName;
import static com.google.errorprone.util.ASTHelpers.getSymbol;
import static com.google.errorprone.util.ASTHelpers.getType;
import static com.google.errorprone.util.ASTHelpers.hasDirectAnnotationWithSimpleName;
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 com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -67,7 +69,6 @@
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Type.MethodType;
import com.sun.tools.javac.processing.JavacProcessingEnvironment;
import java.util.Optional;
import javax.lang.model.element.ElementKind;
Expand All @@ -92,8 +93,6 @@ public class CheckReturnValue extends AbstractReturnValueIgnored
static final String CRV_PACKAGES = "CheckReturnValue:Packages";

private final MessageTrailerStyle messageTrailerStyle;
private final Optional<ResultUsePolicy> constructorPolicy;
private final Optional<ResultUsePolicy> methodPolicy;
private final ResultUsePolicyEvaluator evaluator;

public CheckReturnValue(ErrorProneFlags flags) {
Expand All @@ -102,8 +101,6 @@ public CheckReturnValue(ErrorProneFlags flags) {
flags
.getEnum("CheckReturnValue:MessageTrailerStyle", MessageTrailerStyle.class)
.orElse(NONE);
this.constructorPolicy = defaultPolicy(flags, CHECK_ALL_CONSTRUCTORS);
this.methodPolicy = defaultPolicy(flags, CHECK_ALL_METHODS);

ResultUsePolicyEvaluator.Builder builder =
ResultUsePolicyEvaluator.builder()
Expand All @@ -127,7 +124,13 @@ public CheckReturnValue(ErrorProneFlags flags) {
flags
.getList(CRV_PACKAGES)
.ifPresent(packagePatterns -> builder.addRule(PackagesRule.fromPatterns(packagePatterns)));
this.evaluator = builder.addRule(globalDefault(methodPolicy, constructorPolicy)).build();
this.evaluator =
builder
.addRule(
globalDefault(
defaultPolicy(flags, CHECK_ALL_METHODS),
defaultPolicy(flags, CHECK_ALL_CONSTRUCTORS)))
.build();
}

private static Optional<ResultUsePolicy> defaultPolicy(ErrorProneFlags flags, String flag) {
Expand Down Expand Up @@ -305,11 +308,7 @@ protected Description describeReturnValueIgnored(NewClassTree tree, VisitorState
protected Description describeReturnValueIgnored(MemberReferenceTree tree, VisitorState state) {
MethodSymbol symbol = getSymbol(tree);
Type type = state.getTypes().memberType(getType(tree.getQualifierExpression()), symbol);
// TODO(cgdecker): There are probably other types than MethodType that we could resolve here
String parensAndMaybeEllipsis =
type instanceof MethodType && ((MethodType) type).getParameterTypes().isEmpty()
? "()"
: "(...)";
String parensAndMaybeEllipsis = type.getParameterTypes().isEmpty() ? "()" : "(...)";

String shortCallWithoutNew;
String shortCall;
Expand All @@ -330,8 +329,8 @@ protected Description describeReturnValueIgnored(MemberReferenceTree tree, Visit
String message =
String.format(
"The result of `%s` must be used\n"
+ "`%s` acts as an implementation of `%s`.\n"
+ " which is a `void` method, so it doesn't use the result of `%s`.\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"
Expand All @@ -355,8 +354,12 @@ protected Description describeReturnValueIgnored(MemberReferenceTree tree, Visit
}

private String apiTrailer(MethodSymbol symbol, VisitorState state) {
if (symbol.enclClass().isAnonymous()) {
// I don't think we have a defined format for members of anonymous classes.
// (isLocal returns true for both local classes and anonymous classes. That's good for us.)
if (isLocal(enclosingClass(symbol))) {
/*
* We don't have a defined format for members of local and anonymous classes. After all, their
* generated class names can change easily as other such classes are introduced.
*/
return "";
}
switch (messageTrailerStyle) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ public void testResolvedToVoidLambda() {
public void testResolvedToVoidMethodReference(boolean predicate) {
// BUG: Diagnostic contains: The result of `increment()` must be used
//
// `this.intValue::increment` acts as an implementation of `Runnable.run`.
// which is a `void` method, so it doesn't use the result of `increment()`.
// `this.intValue::increment` acts as an implementation of `Runnable.run`
// -- which is a `void` method, so it doesn't use the result of `increment()`.
//
// To use the result, you may need to restructure your code.
//
Expand All @@ -82,8 +82,8 @@ public void testResolvedToVoidMethodReference(boolean predicate) {
public void testConstructorResolvedToVoidMethodReference() {
// BUG: Diagnostic contains: The result of `new MyObject()` must be used
//
// `MyObject::new` acts as an implementation of `Runnable.run`.
// which is a `void` method, so it doesn't use the result of `new MyObject()`.
// `MyObject::new` acts as an implementation of `Runnable.run`
// -- which is a `void` method, so it doesn't use the result of `new MyObject()`.
//
// To use the result, you may need to restructure your code.
//
Expand Down

0 comments on commit bf5d25f

Please sign in to comment.