Repository: checkstyle/checkstyle

Problem statement:
FinalLocalVariable: NPE in records
Check documentation: https://checkstyle.org/config_coding.html#FinalLocalVariable

problem was detected during https://github.com/checkstyle/contribution/commit/529fc836f88e0f88148ab7760a5ec7ba4d100715
`**/resources-noncompilable/**/checks/metrics/cyclomaticcomplexity/InputCyclomaticComplexityRecords.java` is name of file in our repo that cause problem.

```
$ cat MyRecord2.java 
//non-compiled with javac: Compilable with Java14

public record MyRecord2(boolean t, boolean f) {
    public MyRecord2 { // violation
        int a = 1;
        int b = 1;
        int c = 1;
        int d = 1;
        int n = 8;
        if (a == 1) { // 2, if
        } else if (a == b // 3, if
                && a == c) { // 4, && operator
            if (c == 2) { // 5, if
            }
        } else if (a == d) { // 6, if
            try {
            } catch (Exception e) { // 7, catch
            }
        } else {
            switch (n) {
                case 1: // 8, case
                    break;
                case 2: // 9, case
                    break;
                case 3: // 10, case
                    break;
                default:
                    break;
            }
        }
        d = a < 0 ? -1 : 1; // 11, ternary operator
    }
}

$ 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">
    <module name="TreeWalker">
       <module name="FinalLocalVariable"/>
    </module>
</module>

$ java -jar checkstyle-8.36.2-all.jar -c config.xml MyRecord2.java
Starting audit...
com.puppycrawl.tools.checkstyle.api.CheckstyleException: Exception was thrown while processing MyRecord2.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.coding.FinalLocalVariableCheck.findFirstUpperNamedBlock(FinalLocalVariableCheck.java:781)
	at com.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheck.isSameVariables(FinalLocalVariableCheck.java:800)
	at com.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheck.access$700(FinalLocalVariableCheck.java:142)
	at com.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheck$ScopeData.findFinalVariableCandidateForAst(FinalLocalVariableCheck.java:863)
	at com.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheck.getFinalCandidate(FinalLocalVariableCheck.java:444)
	at com.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheck.visitToken(FinalLocalVariableCheck.java:304)
	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
```

after fix we should remove this exclude from contribution repo.


Hints:
@anhminhtran235 , can you help us to resole this issue ?
@romani Absolutely! I am on it.
@romani Is this the 2nd NPE found after the fact because of our ignore list? Maybe we should run regression on everything to see if there is anything else.
Please suggest how to run to detect only exceptions. Right now ignore list is for several files only, that are compiled by design on all jdk.
Ignore unrelated exceptions (Like https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/my_check.xml#L45 ) and either put all check severity to ignore or suppress all non-exception violations from a check.
make sense.
But right now checkstyle validation is better form before, we filter out only files (not that much of them) that are non-compilable.
but I agree that this is good model for activation of validation for openjdk14 codebase. We might miss new syntax (same as we do now), but we will not miss exceptions in Checks. 
https://github.com/checkstyle/checkstyle/issues/8901

@romani @rnveach It's interesting that a null pointer exception occurred. It shouldn't.
When I look at the exception stack trace:

```
Caused by: java.lang.NullPointerException
	at com.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheck.findFirstUpperNamedBlock(FinalLocalVariableCheck.java:781)
	at com.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheck.isSameVariables(FinalLocalVariableCheck.java:800)
	at com.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheck.access$700(FinalLocalVariableCheck.java:142)
	at com.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheck$ScopeData.findFinalVariableCandidateForAst(FinalLocalVariableCheck.java:863)
.
.
.
```
Basically `findFinalVariableCandidateForAst` called `isSameVariables`:
```
        public Optional<FinalVariableCandidate> findFinalVariableCandidateForAst(DetailAST ast) {
            Optional<FinalVariableCandidate> result = Optional.empty();
            DetailAST storedVariable = null;
            final Optional<FinalVariableCandidate> candidate =
                Optional.ofNullable(scope.get(ast.getText()));
            if (candidate.isPresent()) {
                storedVariable = candidate.get().variableIdent;
            }
            if (storedVariable != null && isSameVariables(storedVariable, ast)) { // RIGHT HERE
                result = candidate;
            }
            return result;
        }
```
`storedVariable` is checked != null. So it shouldn't be equal to null.
Then `isSameVariables` called `findFirstUpperNamedBlock`:
```
private static boolean isSameVariables(DetailAST ast1, DetailAST ast2) {
        final DetailAST classOrMethodOfAst1 =
            findFirstUpperNamedBlock(ast1); // RIGHT HERE
        final DetailAST classOrMethodOfAst2 =
            findFirstUpperNamedBlock(ast2);
        return classOrMethodOfAst1 == classOrMethodOfAst2 && ast1.getText().equals(ast2.getText());
    }
```
`ast1` is `storedVariable`. So it shouldn't be null either.
Then it crashed at `findFirstUpperNamedBlock` due to `null pointer exception`:
```
private static DetailAST findFirstUpperNamedBlock(DetailAST ast) {
        DetailAST astTraverse = ast;
        while (astTraverse.getType() != TokenTypes.METHOD_DEF // CRASHED HERE
                && astTraverse.getType() != TokenTypes.CLASS_DEF
                && astTraverse.getType() != TokenTypes.ENUM_DEF
                && astTraverse.getType() != TokenTypes.CTOR_DEF
                && !ScopeUtil.isClassFieldDef(astTraverse)) {
            astTraverse = astTraverse.getParent();
        }
        return astTraverse;
    }
```
But `astTraverse` shouldn't be null. (I haven't run the debugger. I will do that tomorrow morning).
Did I miss something here? The only thing I didn't understand is this:
```
at com.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheck.access$700(FinalLocalVariableCheck.java:142)
```
It stands between `findFinalVariableCandidateForAst` and `isSameVariables` in the exception stack trace. So maybe it does something that I don't know about?
You will have to debug it. Already looking at the code and file, I can see how it will produce an NPE.

> access$700

I forget the exact name for this, but its an intermediator between 2 methods/fields that can't normally be accessed because of the visibility each is in. It requires Java to create this extra to bridge the gap between the 2 visibilities.

`findFinalVariableCandidateForAst` is inside a separate class than `isSameVariables`, one is in a nested static class and the other is in the main class. `isSameVariables` is a private method which means it cannot be normally accessed by another external class. That is the gap that must be bridged for the 2 to talk to each other even though the Java rules say this is allowed.

Just imagine how this would play out, if you had to code this manually for these 2 methods to be in completely separate classes and not nested. That is what the intermediator is doing.

This should not play into the NPE.
@anhminhtran235 , please copy case code to our UT Inputs, write UT method (there are tons of examples on how to do this) and debug it.
Checkstyle use Test Driven Development model for bug fixes.
I ran the unit test and understood how it will produce NPE now. It's because of this
> astTraverse = astTraverse.getParent();

I'm trying to figure out what happens there. I'll tell you guys if there's any questions / updates.
@romani @rnveach I think I figured out what went wrong.

First of all, the error occurs when checking this line:
> d = a < 0 ? -1 : 1; // 11, ternary operator

Conditional expression ` a < 0 ? -1 : 1` is not a problem. In fact, if I change the whole MyRecord2.java to the following:
```
public record MyRecord2(boolean t, boolean f) {
    public MyRecord2 { 
        int a = 1;
        a = 2; // ERROR when checking this line
    }
}
```
The same error occurred.

The reason is simple:

When `isSameVariables` checks if `int a = 1` and `a = 2` are the same variables, it called `findFirstUpperNamedBlock` to see if they belongs to the same block.
```
private static DetailAST findFirstUpperNamedBlock(DetailAST ast) {
        DetailAST astTraverse = ast;
        while (astTraverse.getType() != TokenTypes.METHOD_DEF
                && astTraverse.getType() != TokenTypes.CLASS_DEF
                && astTraverse.getType() != TokenTypes.ENUM_DEF
                && astTraverse.getType() != TokenTypes.CTOR_DEF
                && !ScopeUtil.isClassFieldDef(astTraverse)) {
            astTraverse = astTraverse.getParent();
        }
        return astTraverse;
    }
```
It should stop at `MyRecord2`, but it didn't because `MyRecord2` is a `COMPACT_CTOR_DEF`, which is NOT included in the while loop condition. So, the program keeps executing `astTraverse = astTraverse.getParent();` until `NPE`.

Adding `&& astTraverse.getType() != TokenTypes.COMPACT_CTOR_DEF` solves the problem.

```
$ cat MyRecord2.java 
//non-compiled with javac: Compilable with Java14

public record MyRecord2(boolean t, boolean f) {
    public MyRecord2 { // violation
        int a = 1;
        int b = 1;
        int c = 1;
        int d = 1;
        int n = 8;
        if (a == 1) { // 2, if
        } else if (a == b // 3, if
                && a == c) { // 4, && operator
            if (c == 2) { // 5, if
            }
        } else if (a == d) { // 6, if
            try {
            } catch (Exception e) { // 7, catch
            }
        } else {
            switch (n) {
                case 1: // 8, case
                    break;
                case 2: // 9, case
                    break;
                case 3: // 10, case
                    break;
                default:
                    break;
            }
        }
        d = a < 0 ? -1 : 1; // 11, ternary operator
    }
}

cat FinalLocalVariableCheckTest.java
.
.
.
@Test
    public void testNPE() throws Exception {
        final DefaultConfiguration checkConfig =
            createModuleConfig(FinalLocalVariableCheck.class);
        final String[] expected = {
            "5:13: " + getCheckMessage(MSG_KEY, "a"),
            "6:13: " + getCheckMessage(MSG_KEY, "b"),
            "7:13: " + getCheckMessage(MSG_KEY, "c"),
            "9:13: " + getCheckMessage(MSG_KEY, "n"),
        };
        verify(checkConfig, getPath("MyRecord2.java"), expected);
    }
```
20/20 test cases passed. No null pointer exception.

**Question:** I want to make a PR, but was a bit concern about Java version. Should I add `testNPE` in my PR? Because `MyRecord2.java` only works with Java 14. Also, is there anything else I need to change in this PR? @romani mentioned 
> after fix we should remove this exclude from contribution repo.

One final question: Is there any command I need to run to make sure nothing went wrong?
@anhminhtran235 you need to place your test input to non-compileable resources folder, you can find examples for any check that checks records.

Please name your test method to something like testRecordsInput. Test file can be shortened to cover only things that is needed.

By the way, instead of adding there one more comparison with token, please change whole expression to use util method https://github.com/checkstyle/checkstyle/blob/3c2249b239cbfe002af3649f1f7c1f9ae61df3b1/src/main/java/com/puppycrawl/tools/checkstyle/utils/TokenUtil.java#L308
I messed up and now the PR is closed. What should I do? Should I open a new PR / Keep adding commit to current PR?
Reopen PR, always push with force to GitHub
![img](https://user-images.githubusercontent.com/54651212/96489852-db11c780-11f4-11eb-8027-3a0b2c5ba29e.PNG)

@romani I can't. The button is disabled. I tried to push force but nothing happens. I think the PR has been merged and closed. According to [Stack overflow](https://stackoverflow.com/a/12838195/10566888), it cannot be reopened.

Should I open a new PR? Sorry for the trouble
please create new PR and keep it open, even work in incomplete or CI is red or .... .

Patch:
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheck.java
--- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheck.java
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheck.java
@@ -33,6 +33,7 @@
 import com.puppycrawl.tools.checkstyle.api.TokenTypes;
 import com.puppycrawl.tools.checkstyle.utils.CheckUtil;
 import com.puppycrawl.tools.checkstyle.utils.ScopeUtil;
+import com.puppycrawl.tools.checkstyle.utils.TokenUtil;
 
 /**
  * <p>
@@ -778,10 +779,8 @@ private static boolean isInLambda(DetailAST paramDef) {
      */
     private static DetailAST findFirstUpperNamedBlock(DetailAST ast) {
         DetailAST astTraverse = ast;
-        while (astTraverse.getType() != TokenTypes.METHOD_DEF
-                && astTraverse.getType() != TokenTypes.CLASS_DEF
-                && astTraverse.getType() != TokenTypes.ENUM_DEF
-                && astTraverse.getType() != TokenTypes.CTOR_DEF
+        while (!TokenUtil.isOfType(astTraverse, TokenTypes.METHOD_DEF, TokenTypes.CLASS_DEF,
+                TokenTypes.ENUM_DEF, TokenTypes.CTOR_DEF, TokenTypes.COMPACT_CTOR_DEF)
                 && !ScopeUtil.isClassFieldDef(astTraverse)) {
             astTraverse = astTraverse.getParent();
         }


Test patch:
diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheckTest.java
--- a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheckTest.java
+++ b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheckTest.java
@@ -86,6 +86,17 @@ public void testDefault() throws Exception {
         verify(checkConfig, getPath("InputFinalLocalVariable.java"), expected);
     }
 
+    @Test
+    public void testRecordsInput() throws Exception {
+        final DefaultConfiguration checkConfig =
+            createModuleConfig(FinalLocalVariableCheck.class);
+        final String[] expected = {
+            "15:17: " + getCheckMessage(MSG_KEY, "b"),
+        };
+        verify(checkConfig,
+            getNonCompilablePath("InputFinalLocalVariableCheckRecords.java"), expected);
+    }
+
     @Test
     public void testParameter() throws Exception {
         final DefaultConfiguration checkConfig =
diff --git a/src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/checks/coding/finallocalvariable/InputFinalLocalVariableCheckRecords.java b/src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/checks/coding/finallocalvariable/InputFinalLocalVariableCheckRecords.java
new file mode 100644
--- /dev/null
+++ b/src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/checks/coding/finallocalvariable/InputFinalLocalVariableCheckRecords.java
@@ -0,0 +1,19 @@
+//non-compiled with javac: Compilable with Java14
+package com.puppycrawl.tools.checkstyle.checks.coding.finallocalvariable;
+
+/* Config:
+ * default
+ */
+public record InputFinalLocalVariableCheckRecords(boolean t, boolean f) {
+    public InputFinalLocalVariableCheckRecords {
+        int a = 0; // ok
+        a = 1;
+    }
+
+    record bad(int i) {
+        public bad {
+            int b = 0; // violation
+        }
+    }
+}
+