OuterTypeFileName throws NPE on record definition in method

[nrmancuso]

Check documentation: https://checkstyle.sourceforge.io/config_misc.html#OuterTypeFilename

➜  full-record-grammar /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.
➜  full-record-grammar 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="OuterTypeFilename"/>
  </module>
</module>
➜  full-record-grammar cat TestClass.java 
public class TestClass {
    class LocalRecordHelper {
        Class<?> m(int x) {
            record R76 (int x) { }
            return R76.class;
        }
    }
}
➜  full-record-grammar java $RUN_LOCALE -jar ~/IdeaProjects/checkstyle/target/checkstyle-8.36-SNAPSHOT-all.jar -c config.xml TestClass.java
Starting audit...
com.puppycrawl.tools.checkstyle.api.CheckstyleException: Exception was thrown while processing TestClass.java
	at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:311)
	at com.puppycrawl.tools.checkstyle.Checker.process(Checker.java:221)
	at com.puppycrawl.tools.checkstyle.Main.runCheckstyle(Main.java:408)
	at com.puppycrawl.tools.checkstyle.Main.runCli(Main.java:331)
	at com.puppycrawl.tools.checkstyle.Main.execute(Main.java:190)
	at com.puppycrawl.tools.checkstyle.Main.main(Main.java:125)
Caused by: java.lang.NullPointerException
	at com.puppycrawl.tools.checkstyle.checks.OuterTypeFilenameCheck.visitToken(OuterTypeFilenameCheck.java:142)
	at com.puppycrawl.tools.checkstyle.TreeWalker.notifyVisit(TreeWalker.java:340)
	at com.puppycrawl.tools.checkstyle.TreeWalker.processIter(TreeWalker.java:451)
	at com.puppycrawl.tools.checkstyle.TreeWalker.walk(TreeWalker.java:278)
	at com.puppycrawl.tools.checkstyle.TreeWalker.processFiltered(TreeWalker.java:151)
	at com.puppycrawl.tools.checkstyle.api.AbstractFileSetCheck.process(AbstractFileSetCheck.java:87)
	at com.puppycrawl.tools.checkstyle.Checker.processFile(Checker.java:337)
	at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:298)
	... 5 more
Checkstyle ends with 1 errors.

Relevant grammar from java.g:

checkstyle/src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java.g

Lines 1121 to 1122 in 86e2754

	// record declaration, note that you cannot have modifiers in this case
	| recordDefinition[#null]

Since the modifiers AST is null when a record definition is inside of a method, a NPE is thrown here:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/OuterTypeFilenameCheck.java

Line 142 in 86e2754

if (modifiers.findFirstToken(TokenTypes.LITERAL_PUBLIC) != null

[nrmancuso]

Some ideas to solve this issue:

1. Create an empty modifiers AST to pass to the recordDefinition rule in java.g
   Example:

        // record declaration, note that you cannot have modifiers in this case so
        // we create an empty modifiers AST to avoid NPE
        |   recordDefinition[(AST) getASTFactory().create(MODIFIERS,"MODIFIERS")]

2. Update ScopeUtil.java to check if the modifiers AST is null; if so, the visibility of the record definition is package and ScopeUtil.java will report the correct visibility

[nrmancuso]

AST before proposed grammar change:

    |       |       |--RECORD_DEF -> RECORD_DEF [4:12]
    |       |       |   |--LITERAL_RECORD -> record [4:12]
    |       |       |   |--IDENT -> R76 [4:19]
    |       |       |   |--LPAREN -> ( [4:23]
    |       |       |   |--RECORD_COMPONENTS -> RECORD_COMPONENTS [4:24]
    |       |       |   |   `--RECORD_COMPONENT_DEF -> RECORD_COMPONENT_DEF [4:24]
    |       |       |   |       |--ANNOTATIONS -> ANNOTATIONS [4:24]
    |       |       |   |       |--TYPE -> TYPE [4:24]
    |       |       |   |       |   `--LITERAL_INT -> int [4:24]
    |       |       |   |       `--IDENT -> x [4:28]
    |       |       |   |--RPAREN -> ) [4:29]
    |       |       |   `--OBJBLOCK -> OBJBLOCK [4:31]
    |       |       |       |--LCURLY -> { [4:31]
    |       |       |       `--RCURLY -> } [4:33]

AST after proposed grammar change:

    |       |       |--RECORD_DEF -> RECORD_DEF [4:12]
    |       |       |   |--MODIFIERS -> MODIFIERS [4:12]
    |       |       |   |--LITERAL_RECORD -> record [4:12]
    |       |       |   |--IDENT -> R76 [4:19]
    |       |       |   |--LPAREN -> ( [4:23]
    |       |       |   |--RECORD_COMPONENTS -> RECORD_COMPONENTS [4:24]
    |       |       |   |   `--RECORD_COMPONENT_DEF -> RECORD_COMPONENT_DEF [4:24]
    |       |       |   |       |--ANNOTATIONS -> ANNOTATIONS [4:24]
    |       |       |   |       |--TYPE -> TYPE [4:24]
    |       |       |   |       |   `--LITERAL_INT -> int [4:24]
    |       |       |   |       `--IDENT -> x [4:28]
    |       |       |   |--RPAREN -> ) [4:29]
    |       |       |   `--OBJBLOCK -> OBJBLOCK [4:31]
    |       |       |       |--LCURLY -> { [4:31]
    |       |       |       `--RCURLY -> } [4:33]

Note that this change only affects record definitions within method definitions. Example of record definition within a class:

checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/grammar/java14/InputJava14Records.txt

Lines 1127 to 1142 in 86e2754

	|--RECORD_DEF -> RECORD_DEF [165:4]
	| |--MODIFIERS -> MODIFIERS [165:4]
	| |--LITERAL_RECORD -> record [165:4]
	| |--IDENT -> RI [165:11]
	| |--LPAREN -> ( [165:13]
	| |--RECORD_COMPONENTS -> RECORD_COMPONENTS [165:14]
	| | `--RECORD_COMPONENT_DEF -> RECORD_COMPONENT_DEF [165:14]
	| | |--ANNOTATIONS -> ANNOTATIONS [165:14]
	| | |--TYPE -> TYPE [165:14]
	| | | `--LITERAL_INT -> int [165:14]
	| | |--ELLIPSIS -> ... [165:17]
	| | `--IDENT -> xs [165:21]
	| |--RPAREN -> ) [165:23]
	| `--OBJBLOCK -> OBJBLOCK [165:25]
	| |--LCURLY -> { [165:25]
	| `--RCURLY -> } [165:27]

This is the record definition usage that would be affected:

checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/grammar/java14/InputJava14Records.txt

Lines 1244 to 1257 in 86e2754

	| | |--RECORD_DEF -> RECORD_DEF [177:12]
	| | | |--LITERAL_RECORD -> record [177:12]
	| | | |--IDENT -> R76 [177:19]
	| | | |--LPAREN -> ( [177:23]
	| | | |--RECORD_COMPONENTS -> RECORD_COMPONENTS [177:24]
	| | | | `--RECORD_COMPONENT_DEF -> RECORD_COMPONENT_DEF [177:24]
	| | | | |--ANNOTATIONS -> ANNOTATIONS [177:24]
	| | | | |--TYPE -> TYPE [177:24]
	| | | | | `--LITERAL_INT -> int [177:24]
	| | | | `--IDENT -> x [177:28]
	| | | |--RPAREN -> ) [177:29]
	| | | `--OBJBLOCK -> OBJBLOCK [177:31]
	| | | |--LCURLY -> { [177:31]
	| | | `--RCURLY -> } [177:33]

[romani]

@nmancus1 , please update issue to mention that we probably do empty modifiers for other types, so update of AST is good to keep structure the same among types.

[nrmancuso]

CLI output from current master:

➜  full-record-grammar /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.
➜  full-record-grammar cat TestClass.java
public class TestClass {
        void method(int x) {
            class MyClass {}
            record MyRecord(){}
        }
}


➜  full-record-grammar java $RUN_LOCALE -jar ~/IdeaProjects/checkstyle/target/checkstyle-8.36-SNAPSHOT-all.jar -t TestClass.java
CLASS_DEF -> CLASS_DEF [1:0]
|--MODIFIERS -> MODIFIERS [1:0]
|   `--LITERAL_PUBLIC -> public [1:0]
|--LITERAL_CLASS -> class [1:7]
|--IDENT -> TestClass [1:13]
`--OBJBLOCK -> OBJBLOCK [1:23]
    |--LCURLY -> { [1:23]
    |--METHOD_DEF -> METHOD_DEF [2:8]
    |   |--MODIFIERS -> MODIFIERS [2:8]
    |   |--TYPE -> TYPE [2:8]
    |   |   `--LITERAL_VOID -> void [2:8]
    |   |--IDENT -> method [2:13]
    |   |--LPAREN -> ( [2:19]
    |   |--PARAMETERS -> PARAMETERS [2:20]
    |   |   `--PARAMETER_DEF -> PARAMETER_DEF [2:20]
    |   |       |--MODIFIERS -> MODIFIERS [2:20]
    |   |       |--TYPE -> TYPE [2:20]
    |   |       |   `--LITERAL_INT -> int [2:20]
    |   |       `--IDENT -> x [2:24]
    |   |--RPAREN -> ) [2:25]
    |   `--SLIST -> { [2:27]
    |       |--CLASS_DEF -> CLASS_DEF [3:12]
    |       |   |--MODIFIERS -> MODIFIERS [3:12]
    |       |   |--LITERAL_CLASS -> class [3:12]
    |       |   |--IDENT -> MyClass [3:18]
    |       |   `--OBJBLOCK -> OBJBLOCK [3:26]
    |       |       |--LCURLY -> { [3:26]
    |       |       `--RCURLY -> } [3:27]
    |       |--RECORD_DEF -> RECORD_DEF [4:12]
    |       |   |--LITERAL_RECORD -> record [4:12]
    |       |   |--IDENT -> MyRecord [4:19]
    |       |   |--LPAREN -> ( [4:27]
    |       |   |--RECORD_COMPONENTS -> RECORD_COMPONENTS [4:28]
    |       |   |--RPAREN -> ) [4:28]
    |       |   `--OBJBLOCK -> OBJBLOCK [4:29]
    |       |       |--LCURLY -> { [4:29]
    |       |       `--RCURLY -> } [4:30]
    |       `--RCURLY -> } [5:8]
    `--RCURLY -> } [6:0]

CLI output from proposed changes:

➜  full-record-grammar /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.
➜  full-record-grammar cat TestClass.java       
public class TestClass {
        void method(int x) {
            class MyClass {}
            record MyRecord(){}
        }
}


➜  full-record-grammar java $RUN_LOCALE -jar ~/IdeaProjects/checkstyle/target/checkstyle-8.36-SNAPSHOT-all.jar -t TestClass.java
CLASS_DEF -> CLASS_DEF [1:0]
|--MODIFIERS -> MODIFIERS [1:0]
|   `--LITERAL_PUBLIC -> public [1:0]
|--LITERAL_CLASS -> class [1:7]
|--IDENT -> TestClass [1:13]
`--OBJBLOCK -> OBJBLOCK [1:23]
    |--LCURLY -> { [1:23]
    |--METHOD_DEF -> METHOD_DEF [2:8]
    |   |--MODIFIERS -> MODIFIERS [2:8]
    |   |--TYPE -> TYPE [2:8]
    |   |   `--LITERAL_VOID -> void [2:8]
    |   |--IDENT -> method [2:13]
    |   |--LPAREN -> ( [2:19]
    |   |--PARAMETERS -> PARAMETERS [2:20]
    |   |   `--PARAMETER_DEF -> PARAMETER_DEF [2:20]
    |   |       |--MODIFIERS -> MODIFIERS [2:20]
    |   |       |--TYPE -> TYPE [2:20]
    |   |       |   `--LITERAL_INT -> int [2:20]
    |   |       `--IDENT -> x [2:24]
    |   |--RPAREN -> ) [2:25]
    |   `--SLIST -> { [2:27]
    |       |--CLASS_DEF -> CLASS_DEF [3:12]
    |       |   |--MODIFIERS -> MODIFIERS [3:12]
    |       |   |--LITERAL_CLASS -> class [3:12]
    |       |   |--IDENT -> MyClass [3:18]
    |       |   `--OBJBLOCK -> OBJBLOCK [3:26]
    |       |       |--LCURLY -> { [3:26]
    |       |       `--RCURLY -> } [3:27]
    |       |--RECORD_DEF -> RECORD_DEF [4:12]
    |       |   |--MODIFIERS -> MODIFIERS [4:12]
    |       |   |--LITERAL_RECORD -> record [4:12]
    |       |   |--IDENT -> MyRecord [4:19]
    |       |   |--LPAREN -> ( [4:27]
    |       |   |--RECORD_COMPONENTS -> RECORD_COMPONENTS [4:28]
    |       |   |--RPAREN -> ) [4:28]
    |       |   `--OBJBLOCK -> OBJBLOCK [4:29]
    |       |       |--LCURLY -> { [4:29]
    |       |       `--RCURLY -> } [4:30]
    |       `--RCURLY -> } [5:8]
    `--RCURLY -> } [6:0]

In order to be more consistent with other types, such as classes, we need to employ the proposed changes to our java grammar. Many of our checks depend on a MODIFIERS AST, whether empty or not.

[romani]

fix is merged.