Issue #80: Resolve MultiVariableDeclaration Bug #104

Anmol202005 · 2025-08-29T04:33:03Z

Fixes: #80.

src/main/java/org/checkstyle/autofix/recipe/FinalLocalVariable.java

Anmol202005 · 2025-08-31T17:42:20Z

timurt · 2025-08-31T23:38:30Z

src/main/java/org/checkstyle/autofix/recipe/FinalLocalVariable.java

    @Override
    public TreeVisitor<?, ExecutionContext> getVisitor() {
-        return new LocalVariableVisitor();
+        return new JavaIsoVisitor<>() {


we create this anon impl JavaIsoVisitor, after that LocalVariableVisitor and MarkViolationVisitor

It looks like all of these objects have the same generic extension, and I am curious, instead of having three visitor objects, can we have only one?

is it true, that you have to visit each variable declaration twice?

The first time to mark everything, the second time to add modifiers. If so, maybe we can have two sequential visitors

Yes, we visit each variable twice.

This is the correct approach IMO.
kindly see: https://docs.openrewrite.org/authoring-recipes/multiple-visitors

timurt · 2025-08-31T23:43:38Z

src/main/java/org/checkstyle/autofix/recipe/FinalLocalVariable.java

+    }
+
+    public static class FinalLocalVariableMarker implements Marker {
+        private final UUID id;


why do we need id? couldn't find usages of this field

Marker interface contains abstract method like getId() which we need to implement.
Since Id is not used i have updated the marker class as :

public static class FinalLocalVariableMarker implements Marker { public FinalLocalVariableMarker() {} @Override public UUID getId() { return null; } @Override public <M extends Marker> M withId(UUID id) { return null; } }

timurt · 2025-08-31T23:53:45Z

src/main/java/org/checkstyle/autofix/recipe/FinalLocalVariable.java

+        }
+
+        @Override
+        public J.Block visitBlock(J.Block block, ExecutionContext executionContext) {


I don't quite understand why we should specifically handle visiting blocks. My assumption was that any variable declaration node would be visited in visitVariableDeclarations?

Could you please provide an example of a variable declaration that won't be handled by visitVariableDeclarations but will be handled by visitBlock

kindly see: #80

example:

int a = 1, b = 2, c = 3; // 1 violation ... a = 7; b = 8;

multiple variable declaration can not be handled by visitVariableDeclarations.

timurt · 2025-08-31T23:56:08Z

src/main/java/org/checkstyle/autofix/recipe/FinalLocalVariable.java

    }

-    private final class LocalVariableVisitor extends JavaIsoVisitor<ExecutionContext> {
+    private final class MarkViolationVisitor extends JavaIsoVisitor<ExecutionContext> {


We definitely need more documentation around these two visitors. The logic looks cumbersome, and I think we should provide some details on how it works

github-actions

Some suggestions could not be made:

src/main/java/org/checkstyle/autofix/recipe/FinalLocalVariable.java
- lines 28-27
- lines 243-244

github-actions

Some suggestions could not be made:

src/main/java/org/checkstyle/autofix/recipe/FinalLocalVariable.java
- lines 249-249

rdiachenko

@Anmol202005 could you go to Checkstyle, take all test inputs from finallocalvariable subpackage in tests, and put them into this PR? Lots of cases are already covered by those test inputs, let's reuse them and check the recipe works expected.

Anmol202005 · 2025-09-03T12:40:25Z

@rdiachenko done. Kindly review

src/main/java/org/checkstyle/autofix/recipe/FinalLocalVariable.java

rdiachenko

lgtm

romani

ok to merge.

@Anmol202005 , do we test compilation of Input and Output files by maven compilation ?

I see we did not, we definetely should before this project goes to massive development.
example of how we did this in main project: https://github.com/checkstyle/checkstyle/blob/1f71110827d1b2b71f4a2434ab20253d344fcbb0/pom.xml#L1462-L1473 please add in separate issue.

We shold be confident that after auto update is done, code is still compilable.

Anmol202005 · 2025-09-06T19:37:51Z

ok to merge.

@Anmol202005 , do we test compilation of Input and Output files by maven compilation ?

I see we did not, we definetely should before this project goes to massive development. example of how we did this in main project: https://github.com/checkstyle/checkstyle/blob/1f71110827d1b2b71f4a2434ab20253d344fcbb0/pom.xml#L1462-L1473 please add in separate issue.

We shold be confident that after auto update is done, code is still compilable.

@rdiachenko @romani
created issue: #109

github-actions bot reviewed Aug 29, 2025

View reviewed changes

src/main/java/org/checkstyle/autofix/recipe/FinalLocalVariable.java Outdated Show resolved Hide resolved

Anmol202005 marked this pull request as draft August 29, 2025 04:36

Anmol202005 force-pushed the tryTrial branch from 92617e9 to 60ce3bd Compare August 31, 2025 17:39

github-actions bot reviewed Aug 31, 2025

View reviewed changes

src/main/java/org/checkstyle/autofix/recipe/FinalLocalVariable.java Outdated Show resolved Hide resolved

Anmol202005 marked this pull request as ready for review August 31, 2025 17:41

Anmol202005 force-pushed the tryTrial branch from 60ce3bd to 60eda3c Compare August 31, 2025 17:51

rdiachenko self-assigned this Aug 31, 2025

rdiachenko requested a review from timurt August 31, 2025 22:53

timurt reviewed Aug 31, 2025

View reviewed changes

Anmol202005 force-pushed the tryTrial branch from 60eda3c to 03a10eb Compare September 1, 2025 04:21

github-actions bot reviewed Sep 1, 2025

View reviewed changes

Anmol202005 force-pushed the tryTrial branch 2 times, most recently from 9374d70 to d85995b Compare September 1, 2025 04:34

github-actions bot reviewed Sep 1, 2025

View reviewed changes

Anmol202005 force-pushed the tryTrial branch from d85995b to 6938b2f Compare September 1, 2025 04:42

rdiachenko reviewed Sep 1, 2025

View reviewed changes

Anmol202005 force-pushed the tryTrial branch from 6938b2f to 8d8bd4c Compare September 3, 2025 12:39

rdiachenko reviewed Sep 3, 2025

View reviewed changes

src/main/java/org/checkstyle/autofix/recipe/FinalLocalVariable.java Show resolved Hide resolved

src/main/java/org/checkstyle/autofix/recipe/FinalLocalVariable.java Outdated Show resolved Hide resolved

Issue checkstyle#80: Resolve MultiVariableDeclaration Bug

b0371e0

Anmol202005 force-pushed the tryTrial branch from 8d8bd4c to b0371e0 Compare September 4, 2025 05:17

rdiachenko approved these changes Sep 4, 2025

View reviewed changes

rdiachenko assigned timurt and unassigned rdiachenko Sep 4, 2025

romani approved these changes Sep 5, 2025

View reviewed changes

Anmol202005 mentioned this pull request Sep 6, 2025

Add Maven compilation testing for Input and Output files #109

Closed

romani merged commit fe50883 into checkstyle:main Sep 6, 2025
3 checks passed

rdiachenko mentioned this pull request Sep 7, 2025

Fix FinalLocalVariable Recipe for Multiple Variable Declarations #80

Closed

Issue #80: Resolve MultiVariableDeclaration Bug #104

Issue #80: Resolve MultiVariableDeclaration Bug #104

Uh oh!

Conversation

Anmol202005 commented Aug 29, 2025

Uh oh!

Uh oh!

Uh oh!

Anmol202005 commented Aug 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anmol202005 Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

rdiachenko left a comment

Choose a reason for hiding this comment

Uh oh!

Anmol202005 commented Sep 3, 2025

Uh oh!

Uh oh!

Uh oh!

rdiachenko left a comment

Choose a reason for hiding this comment

Uh oh!

romani left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anmol202005 commented Sep 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Anmol202005 Sep 1, 2025 •

edited

Loading

romani left a comment •

edited

Loading

Anmol202005 commented Sep 6, 2025 •

edited

Loading