Issue #11973: Fix NPE on empty switch statements in VariableDeclarati…

[stoyanK7]

Resolves #11973

---

Diff Regression config: https://gist.githubusercontent.com/stoyanK7/c67715129a58ae4891e4311252de8ea2/raw/01e5f6b1844ceef6fab5c7f14030ecfa12281f48/config.xml

[nrmancuso]

Please see failures in pitest and generate check regression report.

Item:

[stoyanK7]

@nick-mancuso

While attempting to kill the mutation I discovered that the following code does not raise any violations. Shall I make an issue for it? I believe the newfound bug first needs to be fixed before I can proceed with the current issue.

int issue11973(int a) {
    int sw = 5; // violation 'Distance between variable ''sw'' declaration and its first usage is 2, but allowed 1.'
    return switch (a) {
        case 0 -> {
            a++;
            yield sw + a;
        }
    };
}

[nrmancuso]

@nick-mancuso

While attempting to kill the mutation I discovered that the following code does not raise any violations. Shall I make an issue for it? I believe the newfound bug first needs to be fixed before I can proceed with the current issue.

Please make an issue for this, make sure to explain why we need to fix new issue before we complete this PR.

[romani]

yes, better to separate issue to be addressed in separate issues/PRs.
you can combine fixing few problems in one PR, if code is fix in code is close to each other and "same" problem/context.

[romani]

@stoyanK7 , NPE is blocker to use Check at all, can we continue this PR to at least fix NPE and even fine with false positived.

Exception - force users to disable Check at all in config.
false positive - force users to suppress certain file of user code or specific violation.
false negative - does not bother users at all.

So lets fix exception as primary goal.

[stoyanK7]

@romani Alright, will focus my attention to fix this NPE :)

[romani]

Items

[stoyanK7]

So pitest-coding-1 is going to fail with the following mutation here:

Unnecessary suppressed mutation(s) found and should be removed:

Source File: "VariableDeclarationUsageDistanceCheck.java"
Class: "com.puppycrawl.tools.checkstyle.checks.coding.VariableDeclarationUsageDistanceCheck"
Method: "lambda$getFirstCaseGroupOrSwitchRule$2"
Line Contents: ".orElseGet(() -> block.findFirstToken(TokenTypes.SWITCH_RULE));"
Mutator: "org.pitest.mutationtest.engine.gregor.mutators.experimental.NakedReceiverMutator"
Description: "replaced call to com/puppycrawl/tools/checkstyle/api/DetailAST::findFirstToken with receiver"

Can anybody help me and explain what it does(how the code looks after mutation) since documentation is scarce(practically nonexistent).
This is what I could find

• NakedReceiverMutator.java
• NakedReceiverMutatorTest.java
• Official docs on Experimental Naked Receiver (EXPERIMENTAL_NAKED_RECEIVER)

[rnveach]

"replaced call to com/puppycrawl/tools/checkstyle/api/DetailAST::findFirstToken with receiver"

I believe the mutation is replacing block.findFirstToken(TokenTypes.SWITCH_RULE) with block.
This is clearly shown in the javadoc, https://github.com/hcoles/pitest/blob/master/pitest/src/main/java/org/pitest/mutationtest/engine/gregor/mutators/experimental/NakedReceiverMutator.java#L31-L46 .

[stoyanK7]

@rnveach Already tried that and testVariableDeclarationUsageDistanceSwitchExpressions2() fails when block.findFirstToken(TokenTypes.SWITCH_RULE) is changed to just block:

[rnveach]

@stoyanK7 Please read the top of the pitest report from CI.

Unnecessary suppressed mutation(s) found and should be removed:

It means you are now killing this mutation and can remove it from the suppressions.

[stoyanK7]

@rnveach Oh my god, you are right, thanks! I should have actually read whats up. I assumed it is a surviving one since that was always the case previously, but it turns out pitest also fails when there are unnecessarily suppressed mutations. Removing it right now.

[nrmancuso]

Please update documentation for getFirstCaseGroupOrSwitchRule to mention that we will return null if switch block is empty.

[romani]

please review drone CI falure:

src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/checks/coding/variabledeclarationusagedistance/InputVariableDeclarationUsageDistanceCheckSwitchExpressions2.java:28: error: yield outside of switch expression
yield 2;
^

item:

[romani]

items

[romani]

Items

[romani]

Items

[romani]

Items

[romani]

I might was no clear. I know it is very unusual and sattle.
We allow tests only by Inputs with embedded config.
All other forms of testing are well proven "way to hell" in our repo .

Please no updates in TokenUtilTest.

Let's come back to "map.or" solution , sorry that we confused you and lost valuable time.

Let me know if you interested in more details why we follow unusual philosophy

[stoyanK7]

Inputs with embedded config.

This is not possible here since it is not a check/filter itself.

---

Please no updates in TokenUtilTest
Let's come back to "map.or" solution

Reverted back. Done.

---

It's alright. I would not call it wasted time. Dug around the codebase and learned a few things about the rules of this repository and how pull requests should be.

All other forms of testing are well proven "way to hell" in our repo .
Let me know if you interested in more details why we follow unusual philosophy

Yes, I'd like to know why. In my eyes, such a test does not seem harder to maintain than any other check's tests and input files.

[romani]

We try our best to use tests only like verifyWithInlineConfigParser(getNonCompilablePath(filename).

It makes us confident that we always use compilable input files. And all code in Check can handle any possible-and-compilable combinations of java code and all code in Check are there for a reason (pitest) no extra code and no dead code. Removal/mutation of any part of it immediately detected by Junit tests.

As you might notice in above statement we do not mention about util methods at all , as potentially until method can be copied to class and be part of it. It is our details of implementation that we use util methods, we do not want to let anyone know it, and it subject to change at any time.

If we allow testing of utility methods separately, all logic of all Checks can be generalized and moved to utilities so whole test pressure will also move to utilities. And we will struggle with auto measure of quality of Check.
In result maintainers will win in short term, user will loose in long term, usage will drop, maintainers will not found way to invite new contributions, project is dead eventually.

Right now as you might noticed, new contributor does not need to know all ast and all utilities and all rules. All you need is to put a java file to test and place debug in Check visitToken.

[romani]

We did not born smart, it took us 20 years to do all types of testing to end up where we are. So are of code is still old style

[stoyanK7]

I get it now - all in the name of maintainability and keeping the project going and easy to contribute to. Which I have noticed btw.

What does that mean for the util tests though? Do you want them to move over to such testing verifyWithInlineConfigParser? Is it even possible? Or do you expect tests from checks that use util methods to cover them?

[romani]

Ok to merge

[stoyanK7]

Github, generate report

[github-actions]

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4dab5af_2022060049/reports/diff/index.html

[romani]

OMG, we had exception on project that we in regression list. We might need to have CI script that runs updated in commit check against all such projects to expect no exceptions.

CIs are restarted

[nrmancuso]

Item:

[rnveach]

we had exception on project that we in regression list.

Regression config is missing turning on validateBetweenScopes as seen in regression report
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4dab5af_2022060049/reports/diff/openjdk17/index.html
https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/checks-nonjavadoc-error.xml#L349

Also want to remind we are missing checkstyle/contribution#548 from regression config as well

[romani]

@Vyom-Yadav , please glance at failure
Is it capture#xxx problem ?
#12210

[Vyom-Yadav]

@romani For nullness checker it is capture#, for methods-resource-fenum it is
Message: "@MustCall method close may not have been invoked on temp-var-20910 or any of its aliases."
temp-var-20910, this will also vary with runs.

[romani]

@nrmancuso , lets merge this without Checker green state, in worse case we will add one suppression to our files.
While we are learning this Checker tool, we ok to grow tech debt

[nrmancuso]

Checker is already unstable, I am fine to merge on red.