checkstyle icon indicating copy to clipboard operation
checkstyle copied to clipboard

IllegalIdentifierName should be updated to support new Java 22 keywords

Open rnveach opened this issue 1 year ago • 2 comments

Identified at https://github.com/checkstyle/checkstyle/pull/15084#discussion_r1649748634 ,

https://checkstyle.org/checks/naming/illegalidentifiername.html#IllegalIdentifierName

Checks identifiers with a pattern for a set of illegal names, such as those that are restricted or contextual keywords.

JLS Keywords as of Java 22: https://docs.oracle.com/javase/specs/jls/se22/html/jls-3.html#jls-3.9

$ cat TestClass.java
public class Test {
    public void test() {
int exports = 0;
int opens = 0;
int requires = 0;
int uses = 0;
int yield = 0;         // violation
int module = 0;
int permits = 0;       // violation
int sealed = 0;        // violation
int var = 0;           // violation
// int non-sealed = 0; // doesn't compile
int provides = 0;
int to = 0;
int when = 0;
int open = 0;
int record = 0;        // violation
int transitive = 0;
int with = 0;
    }
}

PS> C:\Programs\jdk-22.0.1\bin\javac.exe .\TestClass.java
PS>

$ 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="IllegalIdentifierName"/>
    </module>
</module>

$ java -jar checkstyle-10.17.0-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[WARN] TestClass.java:7:5: Name 'yield' must match pattern '(?i)^(?!(record|yield|var|permits|sealed|_)$).+$'. [IllegalIdentifierName]
[WARN] TestClass.java:9:5: Name 'permits' must match pattern '(?i)^(?!(record|yield|var|permits|sealed|_)$).+$'. [IllegalIdentifierName]
[WARN] TestClass.java:10:5: Name 'sealed' must match pattern '(?i)^(?!(record|yield|var|permits|sealed|_)$).+$'. [IllegalIdentifierName]
[WARN] TestClass.java:11:5: Name 'var' must match pattern '(?i)^(?!(record|yield|var|permits|sealed|_)$).+$'. [IllegalIdentifierName]
[WARN] TestClass.java:17:5: Name 'record' must match pattern '(?i)^(?!(record|yield|var|permits|sealed|_)$).+$'. [IllegalIdentifierName]
Audit done.

I am expecting violations on the other local variables not commented out. The JLS defines them as Contextual Keywords and our documentation clearly says these are targeted by this check.

Documentation should be updated. We will only support the latest keywords and users can apply an override to the property to revert to older Java version keywords (example at https://github.com/checkstyle/checkstyle/pull/15117#discussion_r1654849384 ).

We should also extend our check update docs to always consider this check when new keywords are added to the language.

rnveach avatar Jun 22 '24 17:06 rnveach

We should also extend our check update docs to always consider this check when new keywords are added to the language.

nrmancuso avatar Jun 22 '24 18:06 nrmancuso

Identified at https://github.com/checkstyle/checkstyle/pull/15117#discussion_r1654849384 , we should also document that we will only support the latest keywords and users can apply an override to the property to revert to older Java version keywords. I will also update first post.

rnveach avatar Jun 26 '24 17:06 rnveach

I am expecting violations on the other local variables not commented out. The JLS defines them as Contextual Keywords and our documentation clearly says these are targeted by this check.

Do we need to add all of them in a single PR?

mahfouz72 avatar Aug 11 '24 10:08 mahfouz72

I am removing "approval" label until we agree on it. we can update examples to show user how to violate names is context where it is confusing - https://github.com/checkstyle/checkstyle/pull/15486#discussion_r1713058911

romani avatar Aug 11 '24 21:08 romani

We need to rethink this Check, something wrong in trend to forbid general words.

record is good word and used a lot, especially in DB related applications

romani avatar Aug 12 '24 02:08 romani

@romani Please open this issue you talk about removing this check and having something else in it place.

rnveach avatar Aug 12 '24 03:08 rnveach

https://github.com/checkstyle/checkstyle/issues/15499

romani avatar Aug 13 '24 12:08 romani