google / google/error-prone

Invalid `CheckReturnValue` suggested fix when a variable must be final or effectively final

Open
#2,624 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
7.2k
Forks
820
Avg merge
5h 9m
Merged PRs (30d)
50

Description

`CheckReturnValue` suggests a fix that causes a non-compilable state for lambda expressions since the variables defined in lambda expressions must be final or effectively final.

You can find a failing test here: https://github.com/PicnicSupermarket/error-prone/commit/769049be6ac59d2f8fc7d17f87059d502c4a8743. The test failure:

```java
import com.google.errorprone.annotations.CheckReturnValue;
import java.util.List;

public class TestClass {
public void test() {
var variable = new CustomType();
// BUG: Diagnostic contains: Ignored return value
List.of(1).forEach(unused -> variable.get());
}
static final class CustomType {
@CheckReturnValue
public CustomType get() {
return null;
}
}
}

Actual Source:
=================

import com.google.errorprone.annotations.CheckReturnValue;
import java.util.List;

public class TestClass {
public void test() {
var variable = new CustomType();
// BUG: Diagnostic contains: Ignored return value
List.of(1).forEach(unused -> variable = variable.get());
}
static final class CustomType {
@CheckReturnValue
public CustomType get() {
return null;
}
}
}

Diffs:
======

Found 1 nodes that differed in expected and actual trees.

> Difference in expected tree and actual tree.
Expected node: Line 8 COMPILATION_UNIT->CLASS(TestClass)->METHOD(test)->BLOCK(non-static)->EXPRESSION_STATEMENT->METHOD_INVOCATION->LAMBDA_EXPRESSION->METHOD_INVOCATION->METHOD_INVOCATION
Actual node: Line 8 COMPILATION_UNIT->CLASS(TestClass)->METHOD(test)->BLOCK(non-static)->EXPRESSION_STATEMENT->METHOD_INVOCATION->LAMBDA_EXPRESSION->ASSIGNMENT->ASSIGNMENT
Expected node kind to be but was .
```

I have a naive fix but this would prevent potential fixes for variables defined in lambda expressions.

```diff
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractReturnValueIgnored.java b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractReturnValueIgnored.java
index d5236902b..0739f866a 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractReturnValueIgnored.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractReturnValueIgnored.java
@@ -223,12 +223,12 @@ public abstract class AbstractReturnValueIgnored extends BugChecker

Fix fix = SuggestedFix.emptyFix();
Symbol symbol = getSymbol(identifierExpr);
- if (identifierExpr != null
- && symbol != null
+ if (symbol != null
&& !symbol.name.contentEquals("this")
+ && state.getPath().getParentPath().getLeaf().getKind() != Kind.LAMBDA_EXPRESSION
&& returnType != null
&& state.getTypes().isAssignable(returnType, identifierType)) {
- // Fix by assigning the assigning the result of the call to the root receiver reference.
+ // Fix by assigning the result of the call to the root receiver reference.
fix =
SuggestedFix.prefixWith(
methodInvocationTree, state.getSourceForNode(identifierExpr) + " = ");

```

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.