Found while auditing for the crash class fixed in the PR that guards ForLoopIncrementInUpdate, WhileInsteadOfFor and DefaultComesLastVisitor against J collections that non-Java parsers leave empty.
CombineSemanticallyEqualCatchBlocks.java:599:
J.Case compareTo = (J.Case) j;
if (_case.getStatements().size() != compareTo.getStatements().size() ||
doesNotContainSameComments(_case.getPrefix(), compareTo.getPrefix())) {
isEqual.set(false);
return _case;
}
this.visit(_case.getCaseLabels().get(0), compareTo.getCaseLabels().get(0));
Two problems:
-
Unguarded .get(0). The size check above covers getStatements(), not getCaseLabels(). The Java parser always puts at least one label on a J.Case (default: becomes a J.Identifier named "default"), but other parsers do not — Go emits a J.Case with no labels at all, which is what made DefaultComesLastVisitor.isDefaultCase throw IndexOutOfBoundsException in production. Not reachable from Go here, since this comparator only descends into catch bodies and Go has no try/catch, but reachable from any J-based language that has both try/catch and a label-less case.
-
Only label 0 is compared. For a multi-label case the remaining labels are ignored, so case 1, 2: compares equal to case 1, 3: and two catch blocks that are not semantically equal can be combined. This one bites plain Java today.
The natural fix closes both: add getCaseLabels().size() to the early-return size check, then loop over all labels rather than indexing 0. Since that changes Java behaviour for multi-label cases it wants its own tests, which is why it was left out of the crash-fix PR.
Found while auditing for the crash class fixed in the PR that guards
ForLoopIncrementInUpdate,WhileInsteadOfForandDefaultComesLastVisitoragainstJcollections that non-Java parsers leave empty.CombineSemanticallyEqualCatchBlocks.java:599:Two problems:
Unguarded
.get(0). The size check above coversgetStatements(), notgetCaseLabels(). The Java parser always puts at least one label on aJ.Case(default:becomes aJ.Identifiernamed"default"), but other parsers do not — Go emits aJ.Casewith no labels at all, which is what madeDefaultComesLastVisitor.isDefaultCasethrowIndexOutOfBoundsExceptionin production. Not reachable from Go here, since this comparator only descends intocatchbodies and Go has no try/catch, but reachable from anyJ-based language that has both try/catch and a label-less case.Only label 0 is compared. For a multi-label case the remaining labels are ignored, so
case 1, 2:compares equal tocase 1, 3:and two catch blocks that are not semantically equal can be combined. This one bites plain Java today.The natural fix closes both: add
getCaseLabels().size()to the early-return size check, then loop over all labels rather than indexing 0. Since that changes Java behaviour for multi-label cases it wants its own tests, which is why it was left out of the crash-fix PR.