If you move first commit to separate PR, I will merge it by single review.

@romani done #15189 please merge ASAP. because this PR will be blocked until the supplemental gets merged
I will drop the supplemental comment here and rebase after merging the other one.

github, generate site

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/48f0cc6_2024024108/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/48f0cc6_2024024108/checks/coding/unusedlocalvariable.html#Examples

github, generate report

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/48f0cc6_2024034204/reports/diff/index.html

github, generate report

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/48f0cc6_2024141939/reports/diff/index.html

github, generate report

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/48f0cc6_2024161154/reports/diff/index.html

github, generate report

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/48f0cc6_2024174138/reports/diff/index.html

@nrmancuso @rnveach ping

@mahfouz72 In the future, can you label the regressions for us on what they are if there are multiple and hide the ones we shouldn't be looking at if they are old. I believe GH regression action accepts labels as a parameter but I don't use it so I don't know what it is.

I expect to see regression on

• Old behavior versus new behavior with property turned off. There should be no differences.
  • https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/48f0cc6_2024174138/reports/diff/index.html
• Old behavior versus new behavior with property turned on (this is both default modules). This is where differences should be.
  • https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/48f0cc6_2024161154/reports/diff/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/48f0cc6_2024161154/reports/diff/checkstyle/index.html#A28
Am I missing something, why isn't there a removal on line 7? Is our check basically confused because there are 2 variables with the same name?
We ignore try/catch for lines 8-12, correct?

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/48f0cc6_2024161154/reports/diff/openjdk21/index.html#A1
I am reading this right and this file isn't compilable and so not an issue with this check?

Am I missing something, why isn't there a removal on line 7? Is our check basically confused because there are 2 variables with the same name?

There was no violation before check got confused on line 8. and registered _ as used because it is encountered in resource specification in try

PS D:\CS\test> cat src/Test.java                                                                                               
public class Test {
    void m() {
        int _ = 0; // ok
        try (final Lock _ = null) { } // check recognized '_' in resource as usage of the above variable
    }
    void m2() {
        int _ = 0; // violation
    }
    class Lock implements AutoCloseable {
        @Override
        public void close() {}
    }
}
PS D:\CS\test> java  -jar checkstyle-10.17.0-all.jar -c config.xml src/Test.java
Starting audit...
[ERROR] D:\CS\test\src\Test.java:7:9: Unused local variable '_'. [UnusedLocalVariable]
Audit done.
Checkstyle ends with 1 errors.
PS D:\CS\test> 

We ignore try/catch for lines 8-12, correct?

Yes, the check currently ignores try we should add support for try in this check in another issue and the exception parameter is not even considered a local variable so it is ik to get ignored by this check, catch parameters needs a new check

I am reading this right and this file isn't compilable and so not an issue with this check?

Yes

github, generate site

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/749efc5_2024142740/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/749efc5_2024142740/checks/coding/unusedlocalvariable.html#Examples

github, generate site

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/97f1f24_2024131946/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/97f1f24_2024131946/checks/coding/unusedlocalvariable.html#Examples

will be a bug for people who have used _ as a normal identifier before (java 9)

Please make an issue for this. We won't attach it to GSoC.

I am not sure if I understand this but yes it is normal to compile we can have more than one unnamed variable in a file (after Java 21), before java 9 this wouldn't compile because it was a valid identifier

Think this answers me. It won't compile pre-9 because of multiple variables with the same name, but after 21 it is because of the unnamed. I was wrongly thinking this was a pre-9 file.

if they declare some local variable named _ and a resource _ in a try then the resource will cancel the violation of the local variable

@rnveach sorry I missed something. this won't compile before Java 9. You can't have two variables with the same name. accordingly, This is not a bug that affects either Java 9 or Java21+. Do you agree?

I am good, but the following need to be done:

#15187 (comment) #15187 (comment)

Based on this comment, I will let @rnveach merge this PR once he is good.

@mahfouz72 Isn't this a bug before Java 9 that there aren't 2 violations, both on the _ variables and not the resource?

$ cat TestClass.java
public class TestClass {
    void method1() {
int _ = 0;
    }

    void method2() {
int _ = 0;

try (Lock _ = null) {}
    }
}

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>
    <property name="severity" value="warning"/>
    <property name="haltOnException" value="false"/>

    <module name="TreeWalker">
<module name="UnusedLocalVariable"/>
    </module>
</module>

$ java -jar checkstyle-10.17.0-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[WARN] TestClass.java:3:1: Unused local variable '_'. [UnusedLocalVariable]
Audit done.

void method2() {
int _ = 0;

try (Lock _ = null) {}
}

The thing is that this method is not compilable before java 9. You can't have 2 variables with the same name in a single scope

PS D:\CS\test> cat src/Test.java
public class Test {
    void test1() {
        int a = 1;
        try (var a = lock()) {

        }
    }
    AutoCloseable lock() {
        return null;
    }
}
PS D:\CS\test> javac src/Test.java
src\Test.java:4: error: variable a is already defined in method test1()
        try (var a = lock()) {
                 ^
1 error 

resource

validating a resource is another issue #15024

ok, maybe I will look into a bit but it seems a concern that this check thinks a try-with-resource is a usage. My concerns are resolved for now.

maybe I will look into a bit but it seems a concern that this check thinks a try-with-resource is a usage

It is a concern of course but doesn't result in a bug. if it works don't touch it : )

My concerns are resolved for now.

Then please merge when you can this is left for you to merge

if it works don't touch it

You can't prove it "works" if we can't say it will never be a problem. The only way we have seen it is with duplicate variable name which isn't valid. We haven't ruled out all circumstances, which is what I want to try and look into.

BTW, isn't this still a bug on Java 21+ if you don't suppress unnamed variables? Nothing says this has to be enabled if you are on a high enough java version.

isn't this still a bug on Java 21+ if you don't suppress unnamed variables?

yes , but it is very nonsense if I am on java 21+ and want to violate all unnamed variables as unused

If you want I can opem issue to remember this once I am on laptop

Continuing on for bug at #15187 (comment),

It appears our IDENT handling might be too aggressive. If it finds an ident which doesn't match a few exclusions we register it as a possible variable usage.

It picked up Lock _ because _ is an ident and it looks to see if it is a variable and if so, it is being "used".

I am going to run regression with a change and see if anything pops up.

Edit:
Regression provided at https://rveach.no-ip.org/checkstyle/regression/1/

https://rveach.no-ip.org/checkstyle/regression/1/openjdk17/index.html#A3
It looks like we can't simply ignore resources altogether. This is a usage.

Making a change and trying another run.

It looks like we can't simply ignore resources altogether.

yeah, then it should be ignored onlt if it is a new variable declaration not a usage of previous variable

https://rveach.no-ip.org/checkstyle/regression/2/
master...rnveach:checkstyle:pr_15187_test

Second regression showed less cases.

This still seems like a concern that ident is being so far reaching. I am not sure if we could satisfy pitest to merge a PR even if we did make an issue.

I am going to merge this PR for now.