openrewrite / rewrite-static-analysis

OpenRewrite recipes for identifying and fixing static analysis issues.
Apache License 2.0
32 stars 54 forks source link

org.openrewrite.staticanalysis.FinalizeLocalVariables should not finalized try/catch #359

Open blipper opened 1 month ago

blipper commented 1 month ago

What is the smallest, simplest way to reproduce the problem?

From https://github.com/checkstyle/checkstyle/issues/3323

public class TestClass {
    public void somePublicMethod() {
        try (FileOutputStream out = new FileOutputStream("......")) {
            //...
        }
    }
}

What did you expect to see?

public class TestClass {
    public void somePublicMethod() {
        try (FileOutputStream out = new FileOutputStream("......")) {
            //...
        }
    }
}

What did you see instead?

public class TestClass {
    public void somePublicMethod() {
        try (final FileOutputStream out = new FileOutputStream("......")) {
            //...
        }
    }
}

This is an interesting one as Checkstyle will flag this as a redundant modifier via https://github.com/checkstyle/checkstyle/issues/3323 RedundantModifier. The JLS spec has try/with resources variables implicitly final as per https://docs.oracle.com/javase/specs/jls/se7/html/jls-14.html#jls-14.20.3

You can see the implementation here https://github.com/checkstyle/checkstyle/blob/888a78f904ea173a4d5e71825aa63c3f35130376/src/main/java/com/puppycrawl/tools/checkstyle/checks/modifier/RedundantModifierCheck.java#L211

Are you interested in contributing a fix to OpenRewrite?

timtebeek commented 1 month ago

Thanks for the report! Looks like we could do something very similar to what we already do for for-loops: https://github.com/openrewrite/rewrite-static-analysis/blob/6646f06a29dd7dee48674dc3389959036fcfe47d/src/main/java/org/openrewrite/staticanalysis/FinalizeLocalVariables.java#L61-L63