Skip to content

Commit 2cb3b54

Browse files
graememorganError Prone Team
authored andcommitted
ImmutableChecker: fix the heuristic around MemberSelects.
This was previously complaining about something like: x -> { Foo f = new Foo(); f.someList.get(0); // Complains that someList is mutable. } Obviously this doesn't matter, because `f` is owned by the lambda. This would have made the check a bit _over_-zealous. I don't think I saw any false positives in the flumes previously, but then it's probably quite rare for people to access fields directly rather than via a getter. PiperOrigin-RevId: 449938995
1 parent 97a7dbc commit 2cb3b54

File tree

2 files changed

+53
-9
lines changed

2 files changed

+53
-9
lines changed

core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -154,17 +154,17 @@ public Void visitMethodInvocation(MethodInvocationTree tree, Void unused) {
154154

155155
@Override
156156
public Void visitMemberSelect(MemberSelectTree tree, Void unused) {
157-
// Special case the access of fields to allow accessing fields which would pass an immutable
158-
// check.
157+
// Note: member selects are not intrinsically problematic; the issue is what might be on the
158+
// LHS of them, which is going to be handled by another visit* method.
159+
160+
// If we're only seeing a field access, don't complain about the fact we closed around
161+
// `this`. This is special-case as it would otherwise be vexing to complain about accessing
162+
// a field of type ImmutableList.
159163
if (tree.getExpression() instanceof IdentifierTree
160-
&& getSymbol(tree) instanceof VarSymbol) {
164+
&& getSymbol(tree) instanceof VarSymbol
165+
&& ((IdentifierTree) tree.getExpression()).getName().contentEquals("this")) {
161166
handleIdentifier(getSymbol(tree));
162-
// If we're only seeing a field access, don't complain about the fact we closed around
163-
// `this`.
164-
if (tree.getExpression() instanceof IdentifierTree
165-
&& ((IdentifierTree) tree.getExpression()).getName().contentEquals("this")) {
166-
return null;
167-
}
167+
return null;
168168
}
169169
return super.visitMemberSelect(tree, null);
170170
}

core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableCheckerTest.java

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2525,6 +2525,25 @@ public void lambda_canCloseAroundImmutableField() {
25252525
.doTest();
25262526
}
25272527

2528+
@Test
2529+
public void lambda_cannotCloseAroundMutableFieldQualifiedWithThis() {
2530+
compilationHelper
2531+
.addSourceLines(
2532+
"Test.java",
2533+
"import com.google.errorprone.annotations.Immutable;",
2534+
"import java.util.ArrayList;",
2535+
"import java.util.List;",
2536+
"class Test {",
2537+
" @Immutable interface ImmutableFunction<A, B> { A apply(B b); }",
2538+
" private int b = 1;",
2539+
" void test(ImmutableFunction<Integer, Integer> f) {",
2540+
" // BUG: Diagnostic contains:",
2541+
" test(x -> this.b);",
2542+
" }",
2543+
"}")
2544+
.doTest();
2545+
}
2546+
25282547
@Test
25292548
public void lambda_cannotCloseAroundMutableLocal() {
25302549
compilationHelper
@@ -2809,4 +2828,29 @@ public void lambda_immutableTypeParam() {
28092828
"}")
28102829
.doTest();
28112830
}
2831+
2832+
@Test
2833+
public void chainedGettersAreAcceptable() {
2834+
compilationHelper
2835+
.addSourceLines(
2836+
"Test.java",
2837+
"import com.google.errorprone.annotations.Immutable;",
2838+
"import java.util.ArrayList;",
2839+
"import java.util.List;",
2840+
"class Test {",
2841+
" final Test t = null;",
2842+
" final List<String> xs = new ArrayList<>();",
2843+
" final List<String> getXs() {",
2844+
" return xs;",
2845+
" }",
2846+
" @Immutable interface ImmutableFunction { String apply(String b); }",
2847+
" void test(ImmutableFunction f) {",
2848+
" test(x -> {",
2849+
" Test t = new Test();",
2850+
" return t.xs.get(0) + t.getXs().get(0) + t.t.t.xs.get(0);",
2851+
" });",
2852+
" }",
2853+
"}")
2854+
.doTest();
2855+
}
28122856
}

0 commit comments

Comments
 (0)