new Treewalker property to skip all nested Modules if there is parsing exception

[romani]

It is our biggest limitation to not work on non compilable files
https://checkstyle.org/writingchecks.html#Limitations

Code has to be compilable by javac to get valid violations. If it is not, you can get hard to understand parse errors.

All of this is blocker for users:
https://github.com/checkstyle/checkstyle/issues?q=is%3Aopen+is%3Aissue+label%3Aantlr
but in most cases they mostlikely to ignore all Checkstyle activity on such files and let them use fancy new jdk syntax for business reasons.

We rely on plugin to cover this problem from us
But we still have problems with new syntax of Java

We need new property like skipFileOnJavaParseException or some thing similar.
If user wants new features of Java and ok to skip checkstyle in such files, it should be easy to do
We should not limit our users and let them decide : choose one checkstyle of new jdk features
By default it is disabled so exception crash all as before
But we can hint users after they create issue on use, about it, and they will be unblocked

We will introduce a new property called skipFileOnJavaParseException:

<module name="TreeWalker">
  <property name="skipFileOnJavaParseException" value="false"/>

[romani]

@rnveach , @nrmancuso , @pbludov , @strkkk , please share your thoughts

[rnveach]

Why not switch it to Javadoc's method of handling parsing failures and instead print a violation? They can then ignore the failures however they want (by suppression), completely or by file name.

[romani]

It still very complicated concept to suppress. Most users do not know that we have filters and how to manage them.
As they switched to new jdk, and what to proceed with jdk, better to not force them to update suppression each time someone what to use new syntax.

[nrmancuso]

From the description, it seems to me that this exact behavior can be achieved by the haltOnException property and a SuppressionSingleFilter module:

➜  src 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="AbbreviationAsWordInName"/>
  </module>
</module>

➜  src cat Test.java 
public clazz Test {}

➜  src java -jar checkstyle-10.4-all.jar -c config.xml Test.java
Starting audit...
com.puppycrawl.tools.checkstyle.api.CheckstyleException: Exception was thrown while processing Test.java
        at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:307)
        at com.puppycrawl.tools.checkstyle.Checker.process(Checker.java:224)
        at com.puppycrawl.tools.checkstyle.Main.runCheckstyle(Main.java:415)
        at com.puppycrawl.tools.checkstyle.Main.runCli(Main.java:338)
        at com.puppycrawl.tools.checkstyle.Main.execute(Main.java:195)
        at com.puppycrawl.tools.checkstyle.Main.main(Main.java:130)
Caused by: com.puppycrawl.tools.checkstyle.api.CheckstyleException: IllegalStateException occurred while parsing file /home/nick/IdeaProjects/tester/src/Test.java.
        at com.puppycrawl.tools.checkstyle.JavaParser.parse(JavaParser.java:105)
...
org.antlr.v4.runtime.atn.ParserATNSimulator.adaptivePredict(ParserATNSimulator.java:396)
        at com.puppycrawl.tools.checkstyle.grammar.java.JavaLanguageParser.typeDeclaration(JavaLanguageParser.java:662)
        ... 11 more
Checkstyle ends with 1 errors.

➜  src 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">
  <property name="haltOnException" value="false"/>
  <module name="SuppressionSingleFilter">
    <property name="message" value=".*IllegalStateException occurred while parsing file.*"/>
  </module>
  <module name="TreeWalker">
    <module name="AbbreviationAsWordInName"/>
  </module>
</module>

➜  src cat Test.java                                            
public clazz Test {}

➜  src java -jar checkstyle-10.4-all.jar -c config.xml Test.java
Starting audit...
Audit done.

I think that this is fine, since a user finding either property (haltOnException or new property) will fall into one of two cases:

1. User reports error on new syntax, and we point them to this approach, or
2. User comes to report syntax, sees an issue already exists, and sees this config shared by a maintainer in existing issue.

We could certainly expand examples under Checker to include the above config and example, since this approach could also be extended to include other exceptions as well.

[romani]

so workaround is

<module name="Checker">
  <property name="haltOnException" value="false"/>
  <module name="SuppressionSingleFilter">
    <property name="message" value=".*IllegalStateException occurred while parsing file.*"/>
  </module>
....

Lets ask users to test it on real code.

[nrmancuso]

Ok, we have a nuance with workaround at #12542 (comment): #12507 (comment)

there is a nuance, modules are executed sequentially:

➜  src 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">

  <property name="haltOnException" value="false"/>
  <module name="SuppressionSingleFilter">
    <property name="message" value=".*IllegalStateException occurred while parsing file.*"/>
  </module>
  <module name="TreeWalker">
    <module name="IllegalIdentifierName"/>
  </module>
    <module name="RegexpSingleline">
    <property name="format" value="Object param"/>
    <property name="minimum" value="0"/>
    <property name="maximum" value="0"/>
  </module>
</module>
➜  src java -jar checkstyle-10.4-all.jar -c config.xml Test.java
Starting audit...
Audit done.

So, for this workaround to still allow other checks (non TreeWalker) to execute, they must appear in config BEFORE TreeWalker module.

If we make new property, we can overcome this limitation, and should include testing of module order in config.

[romani]

and we will make effect of exception to be scoped to Treewalker.

We have options to make (same a in Checker, but we did this option mostly for our testing tool, not as user request)

<module name="TreeWalker">
  <property name="haltOnException" value="false"/>

or smth like:

<module name="TreeWalker">
  <property name="skipFileOnParseExcepion" value="false"/>

[rnveach]

Is there any reason we can't synch behavior between javadoc and java in this regards?

[romani]

I see no problems to have similar/additional property for javadoc parser.

BUT I do not think we should mix them, as javadoc parsing problem is too frequent, nobody knows javadoc syntax, no much tools can enforce it, no much interest from users to deal to with javadoc.

[romani]

@rnveach , @nrmancuso , @pbludov , @strkkk please share your opinion/vote on this issue.

[nrmancuso]

Ok, let me try to sum up our discussion:

1. We will introduce a new property called skipFileOnJavaParseException:

<module name="TreeWalker">
  <property name="skipFileOnJavaParseException" value="false"/>

2. We will introduce a new property called skipFileOnJavadocParseException:

<module name="TreeWalker">
  <property name="skipFileOnJavadocParseException" value="false"/>

3. Update documentation accordingly

I agree with all above points, due to nondeterminism of module execution blocking solution at #12507 (comment).

I am good to introduce new properties, however I feel from a user standpoint that I would rather create a BeforeExecutionExclusionFileFilter for each offending file. With the new properties, you really don't know what files are being skipped. Perhaps we should log (as WARN level) all skipped files.

@romani if we are all in agreement, please edit issue description with my proposed task list and property names.

[romani]

We will introduce a new property

Yes.

BeforeExecutionExclusionFileFilter is not very convenient in real project you have to love checkstyle to always update its config.
Most team members don't care and want to spend less time on configuring checkstyle. Especially with new syntax, as all team members try to use new feature to feel how it works.

Checkstyle is tool on a side, it is not main tool. It should not block users to do what they want. If user ok to skip validation on whole file (many files) in favor of new jdk syntax, checkstyle should not block people.

I would do each properties in separate issues, I still have doubts that users need javadoc, as nobody complained on this.

For logging, we do not do logging, I am not sure how it will work in ant/maven/gradle , we can improve on logging separately.

[cowwoc]

Any update on this issue?

[romani]

@cowwoc , please try #12542 (comment)

@rnveach , please approve issue if you agree. Looks like I and @nrmancuso is in agreement.

We can skip name="skipFileOnJavadocParseException now and create separate issue on it to make it to happen sooner. It is more complicated and parser is in each Check and AST is cached for a file, so each file have numerous javadoc trees. We do not need to skip whole file if only one javadoc in it has problems.

[romani]

@cowwoc , what jdk version you use and we do not support? Do we already have issue in our issue tracker for your problem?

[cowwoc]

@romani JDK 20, and yes... this is the issue: #13279

[romani]

@cowwoc , Does workaround work for you ?

[rnveach]

@romani Let me know when first post contains all final details I am approving.

[romani]

@rnveach , description is already in final state.

skipFileOnJavadocParseException

better to not make it now, as we create parser in each javadoc Check, so it is not treewalker and it is more complicated. We will think on this some time later on, if demand on this grow.

[cowwoc]

@romani I believe it did.

[Lmh-java]

I am on this