Skip to content

Commit 4939448

Browse files
klueverError Prone Team
authored andcommitted
Only hoist the last statement into the assertThrows() lambda.
PiperOrigin-RevId: 927391038
1 parent 30fd05a commit 4939448

3 files changed

Lines changed: 22 additions & 38 deletions

File tree

core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsUtils.java

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818

1919
import static com.google.common.collect.ImmutableList.toImmutableList;
2020
import static com.google.common.collect.Iterables.getLast;
21-
import static com.google.common.collect.Iterables.getOnlyElement;
2221
import static com.google.errorprone.fixes.SuggestedFixes.renameVariableUsages;
2322
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
2423
import static java.util.stream.Collectors.joining;
@@ -112,6 +111,10 @@ public static Optional<Fix> tryFailToAssertThrows(
112111
resources.stream().map(state::getSourceForNode).collect(joining("\n", "try (", ") {\n")));
113112
fixSuffix = "\n}";
114113
}
114+
// Hoist all but the last statement out of the lambda to narrow the exception scope.
115+
for (StatementTree statement : throwingStatements.subList(0, throwingStatements.size() - 1)) {
116+
fixPrefix.append(state.getSourceForNode(statement)).append("\n");
117+
}
115118
if (!catchStatements.isEmpty()) {
116119
String name = catchTree.getParameter().getName().toString();
117120
String newName = namer.avoidShadowing(name);
@@ -133,30 +136,22 @@ public static Optional<Fix> tryFailToAssertThrows(
133136
.map(t -> state.getSourceForNode(t) + ", ")
134137
.orElse(""),
135138
state.getSourceForNode(catchTree.getParameter().getType())));
136-
boolean useExpressionLambda =
137-
throwingStatements.size() == 1
138-
&& getOnlyElement(throwingStatements) instanceof ExpressionStatementTree;
139+
StatementTree lastStatement = getLast(throwingStatements);
140+
boolean useExpressionLambda = lastStatement instanceof ExpressionStatementTree;
139141
if (!useExpressionLambda) {
140142
fixPrefix.append("{");
141143
}
142-
fix.replace(
143-
getStartPosition(tryTree),
144-
getStartPosition(throwingStatements.iterator().next()),
145-
fixPrefix.toString());
144+
fix.replace(getStartPosition(tryTree), getStartPosition(lastStatement), fixPrefix.toString());
146145
if (useExpressionLambda) {
147-
fix.postfixWith(
148-
((ExpressionStatementTree) throwingStatements.iterator().next()).getExpression(), ")");
146+
fix.postfixWith(((ExpressionStatementTree) lastStatement).getExpression(), ")");
149147
} else {
150-
fix.postfixWith(getLast(throwingStatements), "});");
148+
fix.postfixWith(lastStatement, "});");
151149
}
152150
if (catchStatements.isEmpty()) {
153-
fix.replace(
154-
state.getEndPosition(getLast(throwingStatements)),
155-
state.getEndPosition(tryTree),
156-
fixSuffix);
151+
fix.replace(state.getEndPosition(lastStatement), state.getEndPosition(tryTree), fixSuffix);
157152
} else {
158153
fix.replace(
159-
/* startPos= */ state.getEndPosition(getLast(throwingStatements)),
154+
/* startPos= */ state.getEndPosition(lastStatement),
160155
/* endPos= */ getStartPosition(catchStatements.getFirst()),
161156
"\n");
162157
fix.replace(

core/src/test/java/com/google/errorprone/bugpatterns/MissingFailTest.java

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -948,13 +948,8 @@ class ExceptionTest {
948948
@Test
949949
public void f() throws Exception {
950950
Path p = Paths.get("NOSUCH");
951-
IOException e =
952-
assertThrows(
953-
IOException.class,
954-
() -> {
955-
Files.readAllBytes(p);
956-
Files.readAllBytes(p);
957-
});
951+
Files.readAllBytes(p);
952+
IOException e = assertThrows(IOException.class, () -> Files.readAllBytes(p));
958953
assertThat(e).hasMessageThat().contains("NOSUCH");
959954
}
960955

core/src/test/java/com/google/errorprone/bugpatterns/TryFailRefactoringTest.java

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -45,12 +45,12 @@ public void catchBlock() {
4545
4646
class ExceptionTest {
4747
@Test
48-
public void f(String message) throws Exception {
48+
public void f(String msg) throws Exception {
4949
Path p = Paths.get("NOSUCH");
5050
try {
5151
Files.readAllBytes(p);
5252
Files.readAllBytes(p);
53-
fail(message);
53+
fail(msg);
5454
} catch (IOException e) {
5555
assertThat(e).hasMessageThat().contains("NOSUCH");
5656
}
@@ -81,16 +81,10 @@ public void g() throws Exception {
8181
8282
class ExceptionTest {
8383
@Test
84-
public void f(String message) throws Exception {
84+
public void f(String msg) throws Exception {
8585
Path p = Paths.get("NOSUCH");
86-
IOException e =
87-
assertThrows(
88-
message,
89-
IOException.class,
90-
() -> {
91-
Files.readAllBytes(p);
92-
Files.readAllBytes(p);
93-
});
86+
Files.readAllBytes(p);
87+
IOException e = assertThrows(msg, IOException.class, () -> Files.readAllBytes(p));
9488
assertThat(e).hasMessageThat().contains("NOSUCH");
9589
}
9690
@@ -169,11 +163,11 @@ public void tryWithResources() {
169163
170164
class ExceptionTest {
171165
@Test
172-
public void f(String message, CharSource cs) throws IOException {
166+
public void f(String msg, CharSource cs) throws IOException {
173167
try (BufferedReader buf = cs.openBufferedStream();
174168
PushbackReader pbr = new PushbackReader(buf)) {
175169
pbr.read();
176-
fail(message);
170+
fail(msg);
177171
} catch (IOException e) {
178172
assertThat(e).hasMessageThat().contains("NOSUCH");
179173
}
@@ -195,10 +189,10 @@ public void f(String message, CharSource cs) throws IOException {
195189
196190
class ExceptionTest {
197191
@Test
198-
public void f(String message, CharSource cs) throws IOException {
192+
public void f(String msg, CharSource cs) throws IOException {
199193
try (BufferedReader buf = cs.openBufferedStream();
200194
PushbackReader pbr = new PushbackReader(buf)) {
201-
IOException e = assertThrows(message, IOException.class, () -> pbr.read());
195+
IOException e = assertThrows(msg, IOException.class, () -> pbr.read());
202196
assertThat(e).hasMessageThat().contains("NOSUCH");
203197
}
204198
}

0 commit comments

Comments
 (0)