Create new check IllegalIdentifierName

[nrmancuso]

From #8308

Adding new keywords to a language is very difficult, since they are probably used as identifiers in the existing code. Our parser should accept such keyword as an identifier, but we need a new check. Something like AvoidUsingKeywordsAsIdentifiers.

Originally posted by @pbludov in #8267 (comment)

From https://jaxenter.com/java-14-records-deep-dive-169879.html:

"However, you should refrain from using record as an identifier because it could be included as a keyword in a future Java version."

Java already issues the following errors:

$ cat TestClass.java 
public class record {
}
public class var {
}

$ /usr/lib/jvm/java-14-openjdk-amd64/bin/javac --enable-preview --source 14 TestClass.java 
TestClass.java:1: error: 'record' not allowed here
public class record {
             ^
  as of release 13, 'record' is a restricted type name and cannot be used for type declarations
TestClass.java:5: error: 'var' not allowed here
public class var {
             ^
  as of release 10, 'var' is a restricted type name and cannot be used for type declarations
2 errors

So, the addition of this new check would help remind developers that it is bad practice to use context-sensitive/ restricted keywords as identifiers; this will help to make code more future-proof and easy to read. I would expect the following:

➜  src /usr/lib/jvm/java-14-openjdk/bin/javac --enable-preview --source 14 TestClass.java
Note: TestClass.java uses preview language features.
Note: Recompile with -Xlint:preview for details.
➜  src cat TestClass.java
public class TestClass {
    public static void main(String... args) {
        var var = 4; // violation, "var" should not be used as an identifier.

        int record = 15; // violation, "record" should not be used as an identifier.

        String yield = "yield"; // violation, "yield" should not be used as an identifier.

        record Record // violation, "Record" should not be used as an idenitifier.
                (Record record) { // violation, "record" should not be used as an identifier.
        }
    }
}

[romani]

Can itbe done by https://checkstyle.org/config_naming.html#LocalVariableName by more advanced regexp ?

[nrmancuso]

➜  src /usr/lib/jvm/java-14-openjdk/bin/javac --enable-preview --source 14 TestClass.java                               
Note: TestClass.java uses preview language features.
Note: Recompile with -Xlint:preview for details.
➜  src cat config.xml                                                                                                   
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
        "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
<module name="Checker">
    <property name="charset" value="UTF-8"/>
    <module name="TreeWalker">
        <module name="LocalVariableName">
            <property name="format" value="(?i)^(?!(record|yield|var)$).+$"/>
        </module>
        <module name="TypeName">
            <property name="format" value="(?i)^(?!(record|yield|var)$).+$"/>
        </module>
    </module>
</module>%                                                                                                                     ➜  src cat TestClass.java                                                                                               
public class TestClass {
    public static void main(String... args) {
        var var = 4; // violation, "var" should not be used as an identifier.

        int record = 15; // violation, "record" should not be used as an identifier.

        String yield = "yield"; // violation, "yield" should not be used as an identifier.

        record Record // violation, "Record" should not be used as an idenitifier.
                (Record record) { // should be violation, "record" should not be used as an identifier.
        }

        String yieldString = "yieldString"; // ok, part of another word
        record MyRecord(){} // ok, part of another word
        var variable = 2; // ok, part of another word
    }
}
➜  src java -jar /home/nick/IdeaProjects/checkstyle/target/checkstyle-8.36-SNAPSHOT-all.jar -c config.xml TestClass.java\

Starting audit...
[ERROR] /home/nick/Desktop/full-record-grammar/check-updates/src/TestClass.java:3:13: Name 'var' must match pattern '(?i)^(?!(record|yield|var)$).+$'. [LocalVariableName]
[ERROR] /home/nick/Desktop/full-record-grammar/check-updates/src/TestClass.java:5:13: Name 'record' must match pattern '(?i)^(?!(record|yield|var)$).+$'. [LocalVariableName]
[ERROR] /home/nick/Desktop/full-record-grammar/check-updates/src/TestClass.java:7:16: Name 'yield' must match pattern '(?i)^(?!(record|yield|var)$).+$'. [LocalVariableName]
[ERROR] /home/nick/Desktop/full-record-grammar/check-updates/src/TestClass.java:9:16: Name 'Record' must match pattern '(?i)^(?!(record|yield|var)$).+$'. [TypeName]
Audit done.
Checkstyle ends with 4 errors.

[nrmancuso]

(Record record) { // should be violation, "record" should not be used as an identifier.

Do we need another new check, RecordComponentName?

We have LambdaParameterName, MemberName, and ParameterName. Since record components are basically "members", we should make it possible for users to enforce naming conventions on them.

[pbludov]

The list of keywords may expand when a new version of Java is released.
How should this check change when a new version is released? We can add new keywords to the default list, but this will affect existing users of this check (this is normal for the verbosity level WARNING). Or we can add the java-release property (latest by default). With an additional user-provided stop list in a separate property.

[romani]

Do we need another new check, RecordComponentName?

Yes, existing
https://checkstyle.org/config_naming.html#MemberName
Does not match , there are a lot of unrelated properties.
Please create issue on it.

[romani]

How should this check change when a new version is released?

We update default.
If new keyword is introduced, it do mean that one day it will be at LtS release and stay forever. So user should be prepared for this in advance. If he doesn't not want it, he just override default value. I do not think that extra property will contribute good to this. User just need to adjust regexp.
Regexp is more flexible and users can go crazy like forbidding "slave|master" to be in names :) .

[pbludov]

Fix is merged.