checkstyle / checkstyle

Checkstyle is a development tool to help programmers write Java code that adheres to a coding standard. By default it supports the Google Java Style Guide and Sun Code Conventions, but is highly configurable. It can be invoked with an ANT task and a command line program.
https://checkstyle.org
GNU Lesser General Public License v2.1
8.31k stars 3.66k forks source link

New Check UnusedTryResourceShouldBeUnnamed #15484

Open mahfouz72 opened 1 month ago

mahfouz72 commented 1 month ago

child of #14942 Violate the non-use of try resources. They should be unnamed.

Originally proposed to extend UnusedLocalVariableCheck in #15024 but looks like the majority is voting for a new check to achieve this behaviour.


Config

$ cat config.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">
    <module name="TreeWalker">
        <module name="UnusedTryResourceShouldBeUnnamed"/>
    </module>
</module>

Example

public class Test {
     void test() {
         // violation below, 'Unused try resource 'a' should be unnamed'
         try (var a = lock()){

         } catch (Exception e) {

         }

         // ok below, declared as unnamed
         try (var _ = lock()){

         } catch (Exception e) {

         }
     }

     AutoCloseable lock() {
         return null;
     }
}

Expected Output

$ java -jar checkstyle-10.18.0-all.jar -c config.xml Test.java 
Starting audit...
[ERROR] D:\Test.java:4:15:Unused try resource should be unnamed [UnusedTryResourceShouldBeUnnamed]
Audit done.
Checkstyle ends with 1 errors.
romani commented 1 month ago

do we expect such check to be very different from UnusedLambdaParameterShouldBeUnnamed ?

mahfouz72 commented 1 month ago

No, almost the same

romani commented 1 month ago

@nrmancuso, is it reasonable to combine them in one Check and manage target by "tokens" ? There prons and cons ..... for both approaches.

rnveach commented 1 month ago

is it reasonable to combine them

Not by their current check name. A lambda is not a try-with-resource, and vice versa.

rnveach commented 1 month ago

I am good with first post.

nrmancuso commented 1 month ago

@nrmancuso, is it reasonable to combine them in one Check and manage target by "tokens" ? There prons and cons ..... for both approaches.

It is better to not create another JavadocMethod; small, simple, easy to reason about checks make for easy maintenance. Checking for some unused variable will have different nuances for each type (lambda, catch parameter, etc).

romani commented 1 month ago

ok, lets do separate. code sharing is technical issue and can be resolved easily, separate testing and full independent coverage will help us to not loose us functional coverage of them individually.