Skip to content

Commit 9ebcdd2

Browse files
JulianBroudyJulianBroudy
authored
Introduce AssertJOptionalAssertion check (#1547)
1 parent eeed6d5 commit 9ebcdd2

File tree

2 files changed

+207
-0
lines changed

2 files changed

+207
-0
lines changed

‎error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssertJOptionalAssertion.java‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
package tech.picnic.errorprone.bugpatterns;
2+
3+
import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
4+
import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
5+
import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION;
6+
import static com.google.errorprone.matchers.Matchers.allOf;
7+
import static com.google.errorprone.matchers.Matchers.argumentCount;
8+
import static com.google.errorprone.matchers.Matchers.instanceMethod;
9+
import static java.util.Objects.requireNonNull;
10+
import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL;
11+
12+
import com.google.auto.service.AutoService;
13+
import com.google.common.collect.ImmutableMap;
14+
import com.google.common.collect.Iterables;
15+
import com.google.errorprone.BugPattern;
16+
import com.google.errorprone.VisitorState;
17+
import com.google.errorprone.bugpatterns.BugChecker;
18+
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
19+
import com.google.errorprone.fixes.SuggestedFix;
20+
import com.google.errorprone.fixes.SuggestedFixes;
21+
import com.google.errorprone.matchers.Description;
22+
import com.google.errorprone.matchers.Matcher;
23+
import com.google.errorprone.util.ASTHelpers;
24+
import com.sun.source.tree.ExpressionTree;
25+
import com.sun.source.tree.MethodInvocationTree;
26+
import java.util.Optional;
27+
import tech.picnic.errorprone.utils.SourceCode;
28+
29+
/**
30+
* A {@link BugChecker} that flags AssertJ equality and identity checks on unconditionally unwrapped
31+
* {@link Optional} instances for simplification.
32+
*
33+
* <p>This bug checker cannot be replaced with a simple Refaster rule, as the Refaster approach
34+
* would require that all overloads of the mentioned methods (such as {@link
35+
* org.assertj.core.api.AbstractStringAssert#isEqualTo(String)}) are explicitly enumerated. This bug
36+
* checker generically matches all such current and future overloads.
37+
*/
38+
@AutoService(BugChecker.class)
39+
@BugPattern(
40+
summary = "Prefer `.hasValue(value)` and `.containsSame(value)` over more verbose alternatives",
41+
link = BUG_PATTERNS_BASE_URL + "AssertJOptionalAssertion",
42+
linkType = CUSTOM,
43+
severity = SUGGESTION,
44+
tags = SIMPLIFICATION)
45+
public final class AssertJOptionalAssertion extends BugChecker
46+
implements MethodInvocationTreeMatcher {
47+
private static final long serialVersionUID = 1L;
48+
private static final ImmutableMap<String, String> REPLACEMENT_METHODS =
49+
ImmutableMap.of("isEqualTo", "hasValue", "isSameAs", "containsSame");
50+
private static final Matcher<MethodInvocationTree> ASSERTION =
51+
allOf(
52+
instanceMethod()
53+
.onDescendantOf("org.assertj.core.api.Assert")
54+
.namedAnyOf(REPLACEMENT_METHODS.keySet()),
55+
argumentCount(1));
56+
private static final Matcher<ExpressionTree> OPTIONAL_UNWRAP =
57+
instanceMethod()
58+
.onExactClass(Optional.class.getCanonicalName())
59+
.namedAnyOf("get", "orElseThrow");
60+
61+
/** Instantiates a new {@link AssertJOptionalAssertion} instance. */
62+
public AssertJOptionalAssertion() {}
63+
64+
@Override
65+
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
66+
if (!ASSERTION.matches(tree, state)) {
67+
return Description.NO_MATCH;
68+
}
69+
70+
return extractOptionalUnwrap(tree, state)
71+
.map(optionalUnwrap -> describeMatch(tree, suggestFix(tree, optionalUnwrap, state)))
72+
.orElse(Description.NO_MATCH);
73+
}
74+
75+
private static Optional<MethodInvocationTree> extractOptionalUnwrap(
76+
MethodInvocationTree tree, VisitorState state) {
77+
if (!(ASTHelpers.getReceiver(tree) instanceof MethodInvocationTree receiver)
78+
|| receiver.getArguments().size() != 1
79+
|| !ASTHelpers.getSymbol(receiver).isStatic()) {
80+
/* This doesn't look like the start of an assertion statement. */
81+
return Optional.empty();
82+
}
83+
84+
if (!(Iterables.getOnlyElement(receiver.getArguments()) instanceof MethodInvocationTree subject)
85+
|| !OPTIONAL_UNWRAP.matches(subject, state)) {
86+
/* The assertion doesn't involve the unconditional unwrapping of an `Optional`. */
87+
return Optional.empty();
88+
}
89+
90+
return Optional.of(subject);
91+
}
92+
93+
private static SuggestedFix suggestFix(
94+
MethodInvocationTree assertion, MethodInvocationTree optionalUnwrap, VisitorState state) {
95+
ExpressionTree optional =
96+
requireNonNull(
97+
ASTHelpers.getReceiver(optionalUnwrap), "Method invocation must have receiver");
98+
return SuggestedFixes.renameMethodInvocation(assertion, getReplacementMethod(assertion), state)
99+
.toBuilder()
100+
.replace(optionalUnwrap, SourceCode.treeToString(optional, state))
101+
.build();
102+
}
103+
104+
private static String getReplacementMethod(MethodInvocationTree tree) {
105+
return requireNonNull(
106+
REPLACEMENT_METHODS.get(ASTHelpers.getSymbol(tree).getSimpleName().toString()),
107+
"Unexpected method name");
108+
}
109+
}

‎error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AssertJOptionalAssertionTest.java‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
package tech.picnic.errorprone.bugpatterns;
2+
3+
import com.google.errorprone.BugCheckerRefactoringTestHelper;
4+
import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;
5+
import com.google.errorprone.CompilationTestHelper;
6+
import org.junit.jupiter.api.Test;
7+
8+
final class AssertJOptionalAssertionTest {
9+
@Test
10+
void identification() {
11+
CompilationTestHelper.newInstance(AssertJOptionalAssertion.class, getClass())
12+
.addSourceLines(
13+
"A.java",
14+
"import static org.assertj.core.api.Assertions.assertThat;",
15+
"",
16+
"import java.util.Optional;",
17+
"import java.util.stream.Stream;",
18+
"import org.assertj.core.api.AbstractAssert;",
19+
"",
20+
"class A {",
21+
" void m() {",
22+
" assertThat(\"foo\").isEqualTo(\"bar\");",
23+
" assertThat(\"baz\").isSameAs(\"qux\");",
24+
"",
25+
" assertThat(Optional.of(1)).isEqualTo(Optional.of(2));",
26+
" assertThat(Optional.of(3)).isSameAs(Optional.of(4));",
27+
"",
28+
" assertThat(1).isEqualTo(Optional.of(2).orElseThrow()).isEqualTo(3);",
29+
" assertThat(4).isSameAs(Optional.of(5).orElseThrow()).isSameAs(6);",
30+
"",
31+
" nullaryAssertThat().isEqualTo(1);",
32+
" nullaryAssertThat().isSameAs(2);",
33+
"",
34+
" AbstractAssert<?, ?> assertion = assertThat(Optional.of(1).orElseThrow());",
35+
" assertion.isEqualTo(2);",
36+
" assertion.isSameAs(3);",
37+
"",
38+
" assertThat(Optional.of(1).get()).isNotEqualTo(2);",
39+
" // BUG: Diagnostic contains:",
40+
" assertThat(Optional.of(\"foo\").get()).isEqualTo(\"bar\");",
41+
" // BUG: Diagnostic contains:",
42+
" assertThat(Stream.empty().findAny().get()).isSameAs(new Object());",
43+
"",
44+
" assertThat(Optional.<Number>of(1).orElseThrow()).isNotEqualTo(2);",
45+
" // BUG: Diagnostic contains:",
46+
" assertThat(Optional.<Object>of(\"foo\").orElseThrow()).isEqualTo(\"bar\");",
47+
" // BUG: Diagnostic contains:",
48+
" assertThat(Stream.<String>empty().findFirst().orElseThrow()).isSameAs(toString());",
49+
"",
50+
" assertThat(Optional.of(1).orElseThrow(IllegalArgumentException::new)).isNotEqualTo(2);",
51+
" // BUG: Diagnostic contains:",
52+
" assertThat(Optional.of(\"foo\").orElseThrow(IllegalStateException::new)).isEqualTo(\"bar\");",
53+
" // BUG: Diagnostic contains:",
54+
" assertThat(Stream.empty().findAny().orElseThrow(RuntimeException::new)).isSameAs(new Object());",
55+
" }",
56+
"",
57+
" static <T> AbstractAssert<?, ?> nullaryAssertThat() {",
58+
" return null;",
59+
" }",
60+
"}")
61+
.doTest();
62+
}
63+
64+
@Test
65+
void replacement() {
66+
BugCheckerRefactoringTestHelper.newInstance(AssertJOptionalAssertion.class, getClass())
67+
.addInputLines(
68+
"A.java",
69+
"import static org.assertj.core.api.Assertions.assertThat;",
70+
"",
71+
"import java.util.Optional;",
72+
"import java.util.stream.Stream;",
73+
"",
74+
"class A {",
75+
" void m() {",
76+
" assertThat(Optional.of(1).get()).isEqualTo(2);",
77+
" assertThat(Optional.of(\"foo\").orElseThrow()).isSameAs(\"bar\");",
78+
" assertThat(Stream.empty().findAny().orElseThrow(IllegalArgumentException::new))",
79+
" .isEqualTo(new Object());",
80+
" }",
81+
"}")
82+
.addOutputLines(
83+
"A.java",
84+
"import static org.assertj.core.api.Assertions.assertThat;",
85+
"",
86+
"import java.util.Optional;",
87+
"import java.util.stream.Stream;",
88+
"",
89+
"class A {",
90+
" void m() {",
91+
" assertThat(Optional.of(1)).hasValue(2);",
92+
" assertThat(Optional.of(\"foo\")).containsSame(\"bar\");",
93+
" assertThat(Stream.empty().findAny()).hasValue(new Object());",
94+
" }",
95+
"}")
96+
.doTest(TestMode.TEXT_MATCH);
97+
}
98+
}

0 commit comments

Comments
 (0)