Skip to content

Commit fdb5d94

Browse files
AbhishekHerbertSamueltimtebeekgithub-actions[bot]jevanlingen
authored
Fix ClassCastException when handling nested exception class names in CombineSemanticallyEqualCatchBlocks (#598)
* Fix: support nested catch types, cleanup test, and align ControlFlowIndentation * Fix: support nested catch types, cleanup test, and align ControlFlowIndentation * Delete .vscode/settings.json * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Polish * Polish * Polish --------- Co-authored-by: Tim te Beek <timtebeek@gmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: lingenj <jacob.van.lingen@moderne.io>
1 parent 7ad747b commit fdb5d94

File tree

2 files changed

+96
-75
lines changed

2 files changed

+96
-75
lines changed

‎src/main/java/org/openrewrite/staticanalysis/CombineSemanticallyEqualCatchBlocks.java

Lines changed: 57 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@
2727
import java.util.*;
2828
import java.util.concurrent.atomic.AtomicBoolean;
2929

30-
import static java.util.Collections.emptyMap;
30+
import static java.util.Collections.*;
31+
import static org.openrewrite.java.tree.Space.EMPTY;
3132

3233
@Incubating(since = "7.25.0")
3334
public class CombineSemanticallyEqualCatchBlocks extends Recipe {
@@ -86,13 +87,16 @@ public J visitTry(J.Try tryable, ExecutionContext ctx) {
8687
for (int j = i + 1; j < catches.size(); j++) {
8788
J.Try.Catch to = catches.get(j);
8889
// Both 'from' and 'to' may be multi-catches.
89-
for (J.Identifier fromIdentifier : getCaughtExceptions(from)) {
90-
for (J.Identifier toIdentifier : getCaughtExceptions(to)) {
91-
if (fromIdentifier.getType() != null && toIdentifier.getType() != null &&
92-
TypeUtils.isAssignableTo(toIdentifier.getType(), fromIdentifier.getType())) {
90+
for (NameTree fromException : getCaughtExceptions(from)) {
91+
for (NameTree toException : getCaughtExceptions(to)) {
92+
JavaType fromType = TypeUtils.asFullyQualified(fromException.getType());
93+
JavaType toType = TypeUtils.asFullyQualified(toException.getType());
94+
if (fromType != null && toType != null && TypeUtils.isAssignableTo(toType, fromType)) {
9395
Map<J.Try.Catch, Set<J.Identifier>> subTypesMap = parentChildClassRelationship.computeIfAbsent(from, key -> new HashMap<>());
9496
Set<J.Identifier> childClassIdentifiers = subTypesMap.computeIfAbsent(to, key -> new HashSet<>());
95-
childClassIdentifiers.add(fromIdentifier);
97+
if (fromException instanceof J.Identifier) {
98+
childClassIdentifiers.add((J.Identifier) fromException);
99+
}
96100
}
97101
}
98102
}
@@ -200,12 +204,12 @@ public J visitMultiCatch(J.MultiCatch multiCatch, ExecutionContext ctx) {
200204
@Override
201205
public J visitCatch(J.Try.Catch _catch, ExecutionContext ctx) {
202206
J.Try.Catch c = (J.Try.Catch) super.visitCatch(_catch, ctx);
203-
if (c == scope && !isMultiCatch(c)) {
207+
if (c == scope && !(c.getParameter().getTree().getTypeExpression() instanceof J.MultiCatch)) {
204208
if (c.getParameter().getTree().getTypeExpression() != null) {
205209
List<JRightPadded<NameTree>> combinedCatches = combineEquivalentCatches();
206210
c = maybeAutoFormat(c, c.withParameter(c.getParameter()
207211
.withTree(c.getParameter().getTree()
208-
.withTypeExpression(new J.MultiCatch(Tree.randomId(), Space.EMPTY, Markers.EMPTY, combinedCatches)))),
212+
.withTypeExpression(new J.MultiCatch(Tree.randomId(), EMPTY, Markers.EMPTY, combinedCatches)))),
209213
ctx);
210214
}
211215
}
@@ -214,62 +218,59 @@ public J visitCatch(J.Try.Catch _catch, ExecutionContext ctx) {
214218

215219
private List<JRightPadded<NameTree>> combineEquivalentCatches() {
216220
Set<J.Identifier> removeIdentifiers = new HashSet<>();
217-
218221
List<JRightPadded<NameTree>> combinedCatches = new ArrayList<>();
222+
219223
for (J.Try.Catch equivalentCatch : equivalentCatches) {
220224
Set<J.Identifier> childClasses = childClassesToExclude.get(equivalentCatch);
221225
if (childClasses != null) {
222226
// Remove child classes that will be unnecessary since the parent exists in the new multi-catch.
223227
removeIdentifiers.addAll(childClasses);
224228
}
225229

226-
// Whitespace works slightly differently between single catches and multi-catches.
227-
// The prefix of each `J.Identifier` is set to `Space.EMPTY` so that auto-format will make all the appropriate changes.
228-
if (isMultiCatch(equivalentCatch)) {
229-
if (equivalentCatch.getParameter().getTree().getTypeExpression() != null) {
230-
J.MultiCatch newMultiCatch = (J.MultiCatch) equivalentCatch.getParameter().getTree().getTypeExpression();
231-
List<JRightPadded<NameTree>> rightPaddedAlternatives = newMultiCatch.getPadding().getAlternatives();
232-
for (JRightPadded<NameTree> alternative : rightPaddedAlternatives) {
233-
J.Identifier identifier = (J.Identifier) alternative.getElement();
234-
identifier = identifier.withPrefix(Space.EMPTY);
235-
alternative = alternative.withElement(identifier);
236-
combinedCatches.add(alternative);
230+
TypeTree typeExpr = equivalentCatch.getParameter().getTree().getTypeExpression();
231+
if (typeExpr instanceof J.MultiCatch) {
232+
for (JRightPadded<NameTree> alternative : ((J.MultiCatch) typeExpr).getPadding().getAlternatives()) {
233+
NameTree name = alternative.getElement();
234+
if (name instanceof J.Identifier) {
235+
if (removeIdentifiers.contains((J.Identifier) name)) {
236+
continue; // Skip redundant subtype
237+
}
238+
combinedCatches.add(alternative.withElement(((J.Identifier) name).withPrefix(EMPTY)));
239+
} else if (name instanceof J.FieldAccess) {
240+
combinedCatches.add(alternative.withElement(((J.FieldAccess) name).withPrefix(EMPTY)));
241+
} else {
242+
combinedCatches.add(alternative); // fallback
237243
}
238244
}
239-
} else {
240-
if (equivalentCatch.getParameter().getTree().getTypeExpression() != null) {
241-
J.Identifier identifier = ((J.Identifier) equivalentCatch.getParameter().getTree().getTypeExpression());
242-
identifier = identifier.withPrefix(Space.EMPTY);
243-
JRightPadded<NameTree> rightPadded = JRightPadded.build(identifier);
244-
combinedCatches.add(rightPadded);
245+
} else if (typeExpr instanceof J.Identifier) {
246+
J.Identifier identifier = (J.Identifier) typeExpr;
247+
if (!removeIdentifiers.contains(identifier)) {
248+
combinedCatches.add(JRightPadded.build(identifier.withPrefix(EMPTY)));
245249
}
250+
} else if (typeExpr instanceof J.FieldAccess) {
251+
combinedCatches.add(JRightPadded.build(((J.FieldAccess) typeExpr).withPrefix(EMPTY)));
246252
}
247253
}
248254

249-
// Add exceptions in `scope` last to filter out exceptions that are children of parent classes
250-
// that were added into the new catch.
251-
if (isMultiCatch(scope)) {
252-
J.MultiCatch multiCatch = (J.MultiCatch) scope.getParameter().getTree().getTypeExpression();
253-
if (multiCatch != null) {
254-
List<JRightPadded<NameTree>> alternatives = multiCatch.getPadding().getAlternatives();
255-
for (int i = alternatives.size() - 1; i >= 0; i--) {
256-
if (!removeIdentifiers.contains((J.Identifier) alternatives.get(i).getElement())) {
257-
JRightPadded<NameTree> alternative = alternatives.get(i);
258-
alternative = alternative.withElement(alternative.getElement().withPrefix(Space.EMPTY));
259-
// Preserve the order of the original catches.
260-
combinedCatches.add(0, alternative);
261-
}
255+
TypeTree scopeExpr = scope.getParameter().getTree().getTypeExpression();
256+
if (scopeExpr instanceof J.MultiCatch) {
257+
List<JRightPadded<NameTree>> alternatives = ((J.MultiCatch) scopeExpr).getPadding().getAlternatives();
258+
for (int i = alternatives.size() - 1; i >= 0; i--) {
259+
NameTree name = alternatives.get(i).getElement();
260+
if (name instanceof J.Identifier && !removeIdentifiers.contains(name)) {
261+
combinedCatches.add(0, alternatives.get(i).withElement(((J.Identifier) name).withPrefix(EMPTY)));
262+
} else if (!(name instanceof J.Identifier)) {
263+
combinedCatches.add(0, alternatives.get(i));
262264
}
263265
}
264266
} else {
265-
J.Identifier identifier = (J.Identifier) scope.getParameter().getTree().getTypeExpression();
266-
if (identifier != null && !removeIdentifiers.contains(identifier)) {
267-
identifier = identifier.withPrefix(Space.EMPTY);
268-
JRightPadded<NameTree> newCatch = JRightPadded.build(identifier);
269-
// Preserve the order of the original catches.
270-
combinedCatches.add(0, newCatch);
267+
if (scopeExpr instanceof J.Identifier && !removeIdentifiers.contains(scopeExpr)) {
268+
combinedCatches.add(0, JRightPadded.build(((J.Identifier) scopeExpr).withPrefix(EMPTY)));
269+
} else if (scopeExpr instanceof J.FieldAccess) {
270+
combinedCatches.add(0, JRightPadded.build(((J.FieldAccess) scopeExpr).withPrefix(EMPTY)));
271271
}
272272
}
273+
273274
return combinedCatches;
274275
}
275276
}
@@ -285,7 +286,7 @@ private static boolean containSameComments(J.Block body1, J.Block body2) {
285286
* This visitor is a slight variation of {@link SemanticallyEqual} that accounts for differences
286287
* in comments between two trees. The visitor was separated, because comments are not considered
287288
* a part of semantic equivalence.
288-
*
289+
* <p>
289290
* Bug fixes related to semantic equality that are found by {@link CombineSemanticallyEqualCatchBlocks}
290291
* should be applied to {@link SemanticallyEqual} too.
291292
*/
@@ -436,8 +437,8 @@ public J.ArrayType visitArrayType(J.ArrayType arrayType, J j) {
436437
J.ArrayType compareTo = (J.ArrayType) j;
437438
if (!TypeUtils.isOfType(arrayType.getType(), compareTo.getType()) ||
438439
doesNotContainSameComments(arrayType.getPrefix(), compareTo.getPrefix()) ||
439-
nullMissMatch(arrayType.getAnnotations(), compareTo.getAnnotations()) ||
440-
arrayType.getAnnotations().size() != compareTo.getAnnotations().size()) {
440+
nullMissMatch(arrayType.getAnnotations(), compareTo.getAnnotations()) ||
441+
arrayType.getAnnotations().size() != compareTo.getAnnotations().size()) {
441442
isEqual.set(false);
442443
return arrayType;
443444
}
@@ -1795,32 +1796,14 @@ public <N extends NameTree> N visitTypeName(N firstTypeName, J j) {
17951796
/**
17961797
* Collection the caught exceptions from a {@link J.Try.Catch}.
17971798
*/
1798-
private static Set<J.Identifier> getCaughtExceptions(J.Try.Catch aCatch) {
1799-
Set<J.Identifier> caughtExceptions = new HashSet<>();
1800-
if (isMultiCatch(aCatch)) {
1801-
J.MultiCatch multiCatch = (J.MultiCatch) aCatch.getParameter().getTree().getTypeExpression();
1802-
if (multiCatch != null) {
1803-
for (NameTree alternative : multiCatch.getAlternatives()) {
1804-
J.Identifier identifier = (J.Identifier) alternative;
1805-
caughtExceptions.add(identifier);
1806-
}
1807-
}
1808-
} else {
1809-
J.Identifier identifier = (J.Identifier) aCatch.getParameter().getTree().getTypeExpression();
1810-
if (identifier != null) {
1811-
caughtExceptions.add(identifier);
1812-
}
1813-
}
1814-
return caughtExceptions;
1815-
}
1816-
1817-
/**
1818-
* Returns true of a {@link J.Try.Catch} is a {@link J.MultiCatch}.
1819-
* Note: A null type expression will produce a false negative, but the recipe will not
1820-
* change catches with a null type.
1821-
*/
1822-
private static boolean isMultiCatch(J.Try.Catch aCatch) {
1823-
return aCatch.getParameter().getTree().getTypeExpression() instanceof J.MultiCatch;
1799+
private static List<NameTree> getCaughtExceptions(J.Try.Catch aCatch) {
1800+
TypeTree typeExpr = aCatch.getParameter().getTree().getTypeExpression();
1801+
if (typeExpr instanceof J.MultiCatch) {
1802+
return ((J.MultiCatch) typeExpr).getAlternatives();
1803+
} else if (typeExpr != null) { // Can be J.Identifier or J.FieldAccess
1804+
return singletonList(typeExpr);
1805+
}
1806+
return emptyList();
18241807
}
18251808
}
18261809
}

‎src/test/java/org/openrewrite/staticanalysis/CombineSemanticallyEqualCatchBlocksTest.java

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,6 @@ void log(String msg) {}
453453
);
454454
}
455455

456-
457456
@Test
458457
void combineSameCatchBlocksWithVariableDeclaration() {
459458
rewriteRun(
@@ -488,4 +487,43 @@ void method() {
488487
)
489488
);
490489
}
490+
491+
@Test
492+
void catchingNestedClassName() {
493+
//language=java
494+
rewriteRun(
495+
java(
496+
"""
497+
class A {
498+
public static class Exception extends RuntimeException {}
499+
}
500+
501+
class B {
502+
public static class Exception extends RuntimeException {}
503+
}
504+
"""
505+
),
506+
java(
507+
"""
508+
class Test {
509+
void method() {
510+
try {
511+
} catch (A.Exception ex) {
512+
} catch (B.Exception ex) {
513+
}
514+
}
515+
}
516+
""",
517+
"""
518+
class Test {
519+
void method() {
520+
try {
521+
} catch (A.Exception | B.Exception ex) {
522+
}
523+
}
524+
}
525+
"""
526+
)
527+
);
528+
}
491529
}

0 commit comments

Comments
 (0)