openrewrite / openrewrite/rewrite-static-analysis

CombineSemanticallyEqualCatchBlocks indexes J.Case#getCaseLabels() unguarded, and only ever compares the first label

Open Beginner friendly
#985 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Java
Stars
62
Forks
112
Avg merge
1d 19h
Merged PRs (30d)
40

Description

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:

  1. 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.

  2. 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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at CombineSemanticallyEqualCatchBlocks.java:599 and inspect the surrounding comparison logic and existing tests for catch-block combination. Done means label-less cases no longer cause an unguarded access, every case label participates in comparison, and tests cover differing multi-label cases; run the relevant Java test suite.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
devtools, testing-qa
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.