Skip to content

Commit 0164cbc

Browse files
wesalvaroError Prone Team
authored and
Error Prone Team
committed
Adds tests and refactors existing tests for clarity.
As currently implemented, this bugpattern checker only handles an extremely narrow issue set and likely gives developers much more confidence in Optional safety than is actually provided. If we paste the same two files into the Checker Framework's live demo for the Optional checker, it passes on all test cases except that it still has an (understandably) false-positive for the OptionalNotPresentNegativeCases.getWhenTestedSafe_equals. I would recommend we highlight the shortcomings of this checker very clearly. PiperOrigin-RevId: 477896538
1 parent fca0c24 commit 0164cbc

File tree

2 files changed

+91
-35
lines changed

2 files changed

+91
-35
lines changed

core/src/test/java/com/google/errorprone/bugpatterns/testdata/OptionalNotPresentNegativeCases.java

Lines changed: 25 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -18,49 +18,53 @@
1818
import java.util.Optional;
1919
import java.util.function.Predicate;
2020

21+
/** Includes true-negative cases and false-positive cases. */
2122
public class OptionalNotPresentNegativeCases {
2223

2324
// Test this doesn't trigger NullPointerException
2425
private final Predicate<Optional<?>> asField = o -> !o.isPresent();
2526

26-
public void testBasic(Optional<String> optional) {
27-
if (optional.get() != null) {
28-
String str = optional.get();
27+
// False-positive
28+
public String getWhenTestedSafe_referenceEquality(Optional<String> optional) {
29+
if (!optional.isPresent()) {
30+
if (optional == Optional.of("OK")) { // always false
31+
// BUG: Diagnostic contains: Optional
32+
return optional.get();
33+
}
2934
}
35+
return "";
3036
}
3137

32-
public void testNested(Optional<String> optional) {
33-
if (7 == 7) {
34-
String str = optional.get();
35-
if (!optional.isPresent()) {
36-
System.out.println("test");
38+
// False-positive
39+
public String getWhenTestedSafe_equals(Optional<String> optional) {
40+
if (!optional.isPresent()) {
41+
if (optional.equals(Optional.of("OK"))) { // always false
42+
// BUG: Diagnostic contains: Optional
43+
return optional.get();
3744
}
3845
}
46+
return "";
3947
}
4048

41-
public void testOptional(Optional<String> optional) {
49+
public String getWhenPresent_blockReassigned(Optional<String> optional) {
4250
if (!optional.isPresent()) {
4351
optional = Optional.of("value");
44-
String str = optional.get();
52+
return optional.get();
4553
}
54+
return "";
4655
}
4756

48-
public void afterIf(Optional<String> optional) {
57+
public String getWhenPresent_localReassigned(Optional<String> optional) {
4958
if (!optional.isPresent()) {
5059
optional = Optional.of("value");
5160
}
52-
String str = optional.get();
53-
}
54-
55-
public void doubleChecked(Optional<String> optional) {
56-
if (!optional.isPresent() || 7 == 7) {
57-
String foo = optional.isPresent() ? optional.get() : "";
58-
}
61+
return optional.get();
5962
}
6063

61-
public void checkMultipleInIf(Optional<String> optional) {
62-
if (!optional.isPresent() || 7 == 7) {
63-
String str = !optional.isPresent() ? optional.get() : "";
64+
public String getWhenPresent_nestedCheck(Optional<String> optional) {
65+
if (!optional.isPresent() || true) {
66+
return optional.isPresent() ? optional.get() : "";
6467
}
68+
return "";
6569
}
6670
}

core/src/test/java/com/google/errorprone/bugpatterns/testdata/OptionalNotPresentPositiveCases.java

Lines changed: 66 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -17,45 +17,97 @@
1717

1818
import java.util.Optional;
1919

20+
/** Includes true-positive and false-negative cases. */
2021
public class OptionalNotPresentPositiveCases {
2122

22-
public void test(Optional<String> testStr) {
23+
// False-negative
24+
public String getWhenUnknown(Optional<String> optional) {
25+
return optional.get();
26+
}
27+
28+
// False-negative
29+
public String getWhenUnknown_testNull(Optional<String> optional) {
30+
if (optional.get() != null) {
31+
return optional.get();
32+
}
33+
return "";
34+
}
35+
36+
// False-negative
37+
public String getWhenAbsent_testAndNestUnrelated(Optional<String> optional) {
38+
if (true) {
39+
String str = optional.get();
40+
if (!optional.isPresent()) {
41+
return "";
42+
}
43+
return str;
44+
}
45+
return "";
46+
}
47+
48+
public String getWhenAbsent(Optional<String> testStr) {
2349
if (!testStr.isPresent()) {
2450
// BUG: Diagnostic contains: Optional
25-
String str = testStr.get();
51+
return testStr.get();
2652
}
53+
return "";
2754
}
2855

29-
public void testMultipleStatements(Optional<String> optional) {
56+
public String getWhenAbsent_multipleStatements(Optional<String> optional) {
3057
if (!optional.isPresent()) {
3158
String test = "test";
3259
// BUG: Diagnostic contains: Optional
33-
String str = optional.get();
60+
return test + optional.get();
3461
}
62+
return "";
3563
}
3664

37-
public void testNestedIf(Optional<String> optional) {
38-
if (!optional.isPresent()) {
39-
if (optional == Optional.of("")) {
40-
// BUG: Diagnostic contains: Optional
41-
String str = optional.get();
42-
}
65+
// False-negative
66+
public String getWhenAbsent_nestedCheck(Optional<String> optional) {
67+
if (!optional.isPresent() || true) {
68+
return !optional.isPresent() ? optional.get() : "";
4369
}
70+
return "";
4471
}
4572

46-
public void testAnd(Optional<String> optional) {
47-
if (!optional.isPresent() && 7 == 7) {
73+
public String getWhenAbsent_compoundIf_false(Optional<String> optional) {
74+
if (!optional.isPresent() && true) {
4875
// BUG: Diagnostic contains: Optional
49-
String str = optional.get();
76+
return optional.get();
5077
}
78+
return "";
5179
}
5280

53-
public String checkedInElse(Optional<String> optional) {
81+
// False-negative
82+
public String getWhenAbsent_compoundIf_true(Optional<String> optional) {
83+
if (!optional.isPresent() || true) {
84+
return optional.get();
85+
}
86+
return "";
87+
}
88+
89+
public String getWhenAbsent_elseClause(Optional<String> optional) {
5490
if (optional.isPresent()) {
5591
return optional.get();
5692
} else {
5793
// BUG: Diagnostic contains: Optional
5894
return optional.get();
5995
}
6096
}
97+
98+
// False-negative
99+
public String getWhenAbsent_localReassigned(Optional<String> optional) {
100+
if (!optional.isPresent()) {
101+
optional = Optional.empty();
102+
}
103+
return optional.get();
104+
}
105+
106+
// False-negative
107+
public String getWhenAbsent_methodScoped(Optional<String> optional) {
108+
if (optional.isPresent()) {
109+
return "";
110+
}
111+
return optional.get();
112+
}
61113
}

0 commit comments

Comments
 (0)