Issue #12542: new TreeWalker property to skip exceptions #14779

Lmh-java · 2024-04-10T00:34:51Z

Diff Regression config: https://gist.githubusercontent.com/Lmh-java/3a5a8c875c81c4d71a5d19da3541fe2e/raw/8c46739f01519889daf23af48af3ce119c16a155/treewalker-base-config.xml
Diff Regression patch config: https://gist.githubusercontent.com/Lmh-java/6a86d78094428f7bf6aa01580cb5d0b7/raw/0e89015ce2c11c91698db7aaf0bdd05491992f1f/treewalker-patch-config.xml
Diff Regression projects: https://gist.githubusercontent.com/Lmh-java/53136c15cf1a376e3addeb00be0685a5/raw/40d234556e0371dc514df3b34e502de4f55993a6/treewalker-projects.properties

src/main/resources/com/puppycrawl/tools/checkstyle/messages.properties

Lmh-java · 2024-04-10T07:30:45Z

I think the 2 failing CIs are related to 2 new suppressions of non-compilable files. These new suppressions do not exist on the master branch, so these two non-compilable files will still fail the CI.

romani

item:

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java

romani · 2024-04-11T14:25:22Z

ci command https://app.circleci.com/pipelines/github/checkstyle/checkstyle/25089/workflows/538c9779-440e-4a6e-a9eb-1361989b4ead/jobs/570860?invite=true#step-103-2 , you can try to run it on your local.

recheck what is missing at https://github.com/checkstyle/contribution/blob/a2f8b1575c42b9156fdcedabbb528296eb32e09a/checkstyle-tester/projects-to-test-on.properties#L7
to skip such files

please send PR to that repo.

Lmh-java · 2024-04-11T19:20:16Z

@romani PR is created here checkstyle/contribution#852.

romani · 2024-04-11T19:32:16Z

Merged, CIs are restarted

romani

Last:

src/test/java/com/puppycrawl/tools/checkstyle/CheckerTest.java

romani

last:

src/xdocs/config.xml

romani

Ok to merge.
Thanks a lot!

Lmh-java · 2024-04-13T19:22:03Z

Sorry for didn't catch that

rnveach

I don't see any regression provided to ensure this is working.

We need 2 reports. One with default treewalker (expecting no changes), another with new property turned on (show exceptions removed).

rnveach · 2024-04-14T00:01:12Z

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java

- getFilteredViolations(file.getAbsolutePath(), contents, rootAST);
- addViolations(filteredViolations);
+ catch (CheckstyleException ex) {
+ if (!skipFileOnJavaParseException) {


This catch spans almost the entire method and has the possibility to catch more than just parsing exceptions. There is no check that we are catching parse exceptions. We could be catching much more.

This should be trimmed back to ensure it only catches parse exceptions.

There is issue #12803 to allow us to have more methods that throw checkstyle exceptions.

@rnveach I agree, this is definitely not the best way to suppress expections. However, I was trying to only enclose final DetailAST rootAST = JavaParser.parse(contents); with try-catch. But, in that case, I need to use a return to block the following procedure, which will cause our CI to complain, since it is not allowed to have a return in a void method. Do you agree to suppress it?

I do not agree to suppress yet. There should ways around this that follow our style rules.

I can also add a local variable flag. When an exception occurs set the flag to true, and we guard the following procedure with this flag. If this flag is true, then we do not proceed the following procedure. What about this? I felt like this is somehow over-engineering and it is not very elegant, so that's why I didn't use this at the first try.

Good catch, we should catch only parse exceptions, not CheckstyleException.

We need to target with try catch only

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/JavaParser.java

Line 103 in 86195e0

catch (IllegalStateException ex) {

So try catch should wrap only final DetailAST rootAST = JavaParser.parse(contents); and we can catch CheckstyleException in this case

done, work around by using a skip flag.

@romani @rnveach This workaround using a skip flag seems bring us more problems and potential suppressions with nullness.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java

Lines 169 to 198 in e84dc9c

try {

rootAST = JavaParser.parse(contents);

}

catch (CheckstyleException ex) {

if (skipFileOnJavaParseException) {

skip = true;

}

else {

throw ex;

}

}

if (!skip) {

if (!ordinaryChecks.isEmpty()) {

walk(rootAST, contents, AstState.ORDINARY);

}

if (!commentChecks.isEmpty()) {

final DetailAST astWithComments = JavaParser.appendHiddenCommentNodes(rootAST);

walk(astWithComments, contents, AstState.WITH_COMMENTS);

}

if (filters.isEmpty()) {

addViolations(violations);

}

else {

final SortedSet<Violation> filteredViolations =

getFilteredViolations(file.getAbsolutePath(), contents, rootAST);

addViolations(filteredViolations);

}

violations.clear();

}

New surviving error(s) found: <checkerFrameworkError unstable="false"> <fileName>src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java</fileName> <specifier>argument</specifier> <message>incompatible argument for parameter ast of TreeWalker.walk.</message> <lineContent>walk(rootAST, contents, AstState.ORDINARY);</lineContent> <details> found : @Initialized @Nullable DetailAST required: @Initialized @NonNull DetailAST </details> </checkerFrameworkError> <checkerFrameworkError unstable="false"> <fileName>src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java</fileName> <specifier>argument</specifier> <message>incompatible argument for parameter root of JavaParser.appendHiddenCommentNodes.</message> <lineContent>final DetailAST astWithComments = JavaParser.appendHiddenCommentNodes(rootAST);</lineContent> <details> found : @Initialized @Nullable DetailAST required: @Initialized @NonNull DetailAST </details> </checkerFrameworkError> <checkerFrameworkError unstable="false"> <fileName>src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java</fileName> <specifier>argument</specifier> <message>incompatible argument for parameter rootAST of TreeWalker.getFilteredViolations.</message> <lineContent>getFilteredViolations(file.getAbsolutePath(), contents, rootAST);</lineContent> <details> found : @Initialized @Nullable DetailAST required: @Initialized @NonNull DetailAST </details> </checkerFrameworkError>

I felt like this structure of code is too complicated. This is the core code of the project, it would be hard for later maintainers if we do more workarounds In this case, do you prefer to still work on this workaround (add a dummy initial value for rooAST instead of null), or prefer to suppress a single return statement, or there are any better solutions. Please share your thoughts. I am out of ideas.

it would be hard for later maintainers

If we want to have better code, please do this actively, right now this issue stuck in not done state, and users still having unfortunate decisions to disable checkstyle because it doesn't support modern language features.
You will not avoid catch of exception, so Checker will be not happy, and you will do just playing with creation of method that hide such exceptions handling, but conceptually all will stay the same.

If we start new method creation, you will stick in review more. My suggestion is pass review quicker, get feature out to users, and work in separate PR in beautiful code as much as we need, no rush. You can create new PR, right now and collaborate on ideal form of code.
There is no problem with mutation of variable, we do this in a lot of places, one day we will improve, but existence of this doesn't block users to receive bug fixes and new features.

My suggestion is pass review quicker, get feature out to users, and work in separate PR in beautiful code as much as we need, no rush.

Thanks for your advice. I will aim to finish this PR right now by suppressing the checker shown above. :)

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java

rnveach · 2024-04-14T00:08:18Z

@romani In addition, is this something to enable for google/sun styles?

What about our default regression configurations?

Lmh-java · 2024-04-14T01:27:49Z

@romani In addition, is this something to enable for google/sun styles?

What about our default regression configurations?

@rnveach I was also thinking about this the other day. I think we can copy the original 2 check config files an make another 2 with this property on. I.e., we have 4 built-in config: 1. Sun 2. Google 3. Sun No Exception 4. Google No Exception.

My reason is: there are users who want to use the original check with exceptions, and for other group of user, in their cases, they might want to ignore the exceptions.

About our default regressions:
I would like to use it in the new performance CI (#14599), since this property costs less than a long list of suppressions and will make the benchmark more accurate. I think that's probably same in other regression tests.

rnveach · 2024-04-14T02:01:22Z

I think we can copy the original 2 check config files

I am against duplicating the configs. There is more work that goes into ensuring how they are built and we don't need the headache of maintaining 2 copies.

romani · 2024-04-14T02:32:32Z

is this something to enable for google/sun styles?

No, we are not going to promote this property, user should use it only if they really want it, and they understand side effects.

What about our default regression configurations?

I do not think we need this flag in our configs. We should clearly see exception in report.

Lmh-java · 2024-04-14T05:26:58Z

GitHub, generate report

github-actions · 2024-04-14T05:48:27Z

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

Lmh-java · 2024-04-14T05:55:06Z

GitHub, generate report

github-actions · 2024-04-14T06:12:26Z

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

Lmh-java · 2024-04-14T06:45:35Z

GitHub, generate report

github-actions · 2024-05-02T13:34:28Z

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

Lmh-java · 2024-05-03T03:05:33Z

GitHub, generate report

Lmh-java · 2024-05-03T03:14:14Z

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

I would expect no TreeWalker checks to run on files that we are skipping. I see that we are now passing a null AST to checks; I am not sure about what side effects this could possibly have. If we want to explore a bit, we could use the same config that we use in our no-exception openjdk executions (without the file filters) to see what other checks are effected. We will need to add a test reproducing the issue seen in the regression report.

@Lmh-java please help us to understand what we need to do to avoid passing a null AST to TreeWalker checks.

This is a mistake. I introduced skip to skip the checks when exception happened. However, somehow it did not skip checks. I should not pass null to checks. Otherwise there will be lots of changes with some specific checks. They are hard to detect. Let me fix this.

Passing null AST to major checks will not cause any problem. However, for some special checks (such as NoCodeInFile shown in the regression report), there will be a different. In this case of NoCodeInFile, there is a guard for null.

@Override
public void finishTree(DetailAST ast) {
    if (ast == null) {
        log(DEFAULT_LINE_NUMBER, MSG_KEY_NO_CODE);
    }
}

so it will be logged.

Edit: based on observation locally, I think NoCodeInFile is the only check that expect null AST to input. If AST is null, it means there is no code in file.

github-actions · 2024-05-03T03:22:14Z

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

romani

Items

src/xdocs/config.xml

...le/com/puppycrawl/tools/checkstyle/treewalker/InputTreeWalkerSkipParsingExceptionConfig.java

nrmancuso

Item:

src/test/java/com/puppycrawl/tools/checkstyle/TreeWalkerTest.java

romani

ok to merge

romani · 2024-05-11T14:26:22Z

GitHub, generate report

github-actions · 2024-05-11T14:43:54Z

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

romani · 2024-05-11T15:32:15Z

@Lmh-java , please update config to make severity as IGNORE and generate one more report

romani · 2024-05-11T21:22:35Z

@Lmh-java , please extend tests to kill mutation https://github.com/checkstyle/checkstyle/actions/runs/9040829453/job/24845471896?pr=14779#step:7:24

You can reproduce it on your local by ./.ci/pitest.sh "pitest-tree-walker" command in terminal.

Lmh-java · 2024-05-12T16:44:31Z

GitHub, generate report

Lmh-java · 2024-05-12T16:51:10Z

@Lmh-java , please update config to make severity as IGNORE and generate one more report

Done, waiting for the report

Lmh-java · 2024-05-12T16:51:38Z

@Lmh-java , please extend tests to kill mutation https://github.com/checkstyle/checkstyle/actions/runs/9040829453/job/24845471896?pr=14779#step:7:24

You can reproduce it on your local by ./.ci/pitest.sh "pitest-tree-walker" command in terminal.

Done

github-actions · 2024-05-12T17:12:09Z

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

romani · 2024-05-12T18:21:22Z

Last update, please add this Checker stuff to suppression https://github.com/checkstyle/checkstyle/actions/runs/9052825433/job/24870931959?pr=14779#step:6:1707

nrmancuso

We could stand to improve this code in the future, but let's get this feature into user's hands first.

rnveach

Still reviewing the code.

Add an example to https://checkstyle.org/config.html#TreeWalker_Examples and explain when they would use such a feature and why. Be sure to explain we will only skip checks under TreeWalker and not under Checker.

rnveach · 2024-05-15T14:31:00Z

src/xdocs/config.xml

@@ -539,6 +539,20 @@
 <td><code>.java</code></td>
 <td>3.0</td>
 </tr>
+ <tr>
+ <td>javaParseExceptionSeverity</td>
+ <td>Severity level to log Java parsing exceptions when they are skipped.</td>


=> Specify the severity level...

rnveach · 2024-05-15T14:33:18Z

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java

+ }
+
+ /**
+ * Sets severity level to log Java parsing exceptions when they are skipped.


=> Setter to specify the severity level...

rnveach · 2024-05-15T14:46:37Z

@romani

What about our default regression configurations?
I do not think we need this flag in our configs. We should clearly see exception in report.

Our default regression configuration ignores these exceptions. So it doesn't make sense we wouldn't add this.
https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/my_check.xml#L45

rnveach · 2024-05-15T14:49:06Z

@Lmh-java Can you organize your regression reports? They seem a bit scattered. Also, the ones I am seeing only run against 1 project. We normally request for them to be run on all projects. This can impact more than just JDK 17 projects. If older projects show no differences, then it shows there is no harm.

rnveach · 2024-05-15T14:50:38Z

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java

 }
- if (filters.isEmpty()) {
- addViolations(violations);
+ catch (CheckstyleException ex) {


https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/c6a72a2_2024054348/reports/diff/openjdk17/index.html#A1

This report shows we can get parse errors from more than just CheckstyleExceptions. It seems to me we need to expand it to all throwables for this to work properly.

Edit: We will also need a test case to show this. Let me know what the new test name will be.

@Lmh-java , please add this file to our Inputs https://github.com/openjdk/jdk/blob/master/test/langtools/tools/javac/diags/examples/VarargsAndReceiver.java without header comment.

romani · 2024-05-15T15:38:33Z

@Lmh-java, please do us a favor

Please send PR to remove this block
https://github.com/checkstyle/contribution/blob/fb4a65b361fd6c6887444eb0e89a9191ec89d789/checkstyle-tester/my_check.xml#L43-L47
And use your new properties.

Lmh-java force-pushed the minghao/treewalker-property branch from 6b3557a to 800073f Compare April 10, 2024 06:39

Lmh-java commented Apr 10, 2024

View reviewed changes

src/main/resources/com/puppycrawl/tools/checkstyle/messages.properties Outdated Show resolved Hide resolved

Lmh-java marked this pull request as ready for review April 10, 2024 07:35

romani requested changes Apr 11, 2024

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java Outdated Show resolved Hide resolved

Lmh-java force-pushed the minghao/treewalker-property branch from 800073f to 55f4771 Compare April 11, 2024 20:37

romani requested changes Apr 12, 2024

View reviewed changes

src/test/java/com/puppycrawl/tools/checkstyle/CheckerTest.java Outdated Show resolved Hide resolved

Lmh-java force-pushed the minghao/treewalker-property branch from 55f4771 to be6cfce Compare April 12, 2024 05:45

Lmh-java requested a review from romani April 12, 2024 06:43

Lmh-java force-pushed the minghao/treewalker-property branch from be6cfce to b95374d Compare April 12, 2024 20:14

romani requested changes Apr 13, 2024

View reviewed changes

src/xdocs/config.xml Outdated Show resolved Hide resolved

Lmh-java force-pushed the minghao/treewalker-property branch from b95374d to c6a72a2 Compare April 13, 2024 18:52

rnveach self-requested a review April 13, 2024 19:03

romani approved these changes Apr 13, 2024

View reviewed changes

romani assigned rnveach Apr 13, 2024

rnveach reviewed Apr 14, 2024

View reviewed changes

rnveach requested changes Apr 14, 2024

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java Show resolved Hide resolved

Lmh-java force-pushed the minghao/treewalker-property branch 2 times, most recently from 99701c6 to 07b5542 Compare May 3, 2024 21:40

romani requested changes May 3, 2024

View reviewed changes

src/xdocs/config.xml Outdated Show resolved Hide resolved

...le/com/puppycrawl/tools/checkstyle/treewalker/InputTreeWalkerSkipParsingExceptionConfig.java Show resolved Hide resolved

nrmancuso requested changes May 7, 2024

View reviewed changes

src/test/java/com/puppycrawl/tools/checkstyle/TreeWalkerTest.java Outdated Show resolved Hide resolved

Lmh-java force-pushed the minghao/treewalker-property branch from 07b5542 to f15fbed Compare May 11, 2024 03:32

romani approved these changes May 11, 2024

View reviewed changes

Lmh-java force-pushed the minghao/treewalker-property branch from f15fbed to 5885b6d Compare May 12, 2024 16:40

Issue checkstyle#12542: new TreeWalker property to skip exceptions

ac0129b

Lmh-java force-pushed the minghao/treewalker-property branch from 5885b6d to ac0129b Compare May 13, 2024 01:34

nrmancuso approved these changes May 15, 2024

View reviewed changes

nrmancuso assigned rnveach and unassigned nrmancuso May 15, 2024

rnveach requested changes May 15, 2024

View reviewed changes

	try {
	rootAST = JavaParser.parse(contents);
	}
	catch (CheckstyleException ex) {
	if (skipFileOnJavaParseException) {
	skip = true;
	}
	else {
	throw ex;
	}
	}

	if (!skip) {
	if (!ordinaryChecks.isEmpty()) {
	walk(rootAST, contents, AstState.ORDINARY);
	}
	if (!commentChecks.isEmpty()) {
	final DetailAST astWithComments = JavaParser.appendHiddenCommentNodes(rootAST);
	walk(astWithComments, contents, AstState.WITH_COMMENTS);
	}
	if (filters.isEmpty()) {
	addViolations(violations);
	}
	else {
	final SortedSet<Violation> filteredViolations =
	getFilteredViolations(file.getAbsolutePath(), contents, rootAST);
	addViolations(filteredViolations);
	}
	violations.clear();
	}

Issue #12542: new TreeWalker property to skip exceptions #14779

Are you sure you want to change the base?

Issue #12542: new TreeWalker property to skip exceptions #14779

Conversation

Lmh-java commented Apr 10, 2024 • edited

Lmh-java commented Apr 10, 2024

romani left a comment

Choose a reason for hiding this comment

romani commented Apr 11, 2024 • edited

Lmh-java commented Apr 11, 2024

romani commented Apr 11, 2024

romani left a comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

Lmh-java commented Apr 13, 2024

rnveach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lmh-java Apr 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani Apr 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lmh-java Apr 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lmh-java Apr 18, 2024 • edited

Choose a reason for hiding this comment

rnveach commented Apr 14, 2024 • edited

Lmh-java commented Apr 14, 2024

rnveach commented Apr 14, 2024

romani commented Apr 14, 2024 • edited

Lmh-java commented Apr 14, 2024

github-actions bot commented Apr 14, 2024

Lmh-java commented Apr 14, 2024

github-actions bot commented Apr 14, 2024

Lmh-java commented Apr 14, 2024

github-actions bot commented May 2, 2024

Lmh-java commented May 3, 2024

Lmh-java commented May 3, 2024 • edited

github-actions bot commented May 3, 2024

romani left a comment

Choose a reason for hiding this comment

nrmancuso left a comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

romani commented May 11, 2024

github-actions bot commented May 11, 2024

romani commented May 11, 2024

romani commented May 11, 2024 • edited

Lmh-java commented May 12, 2024

Lmh-java commented May 12, 2024

Lmh-java commented May 12, 2024

github-actions bot commented May 12, 2024

romani commented May 12, 2024

nrmancuso left a comment

Choose a reason for hiding this comment

rnveach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach commented May 15, 2024

rnveach commented May 15, 2024

rnveach May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani commented May 15, 2024 • edited

Lmh-java commented Apr 10, 2024 •

edited

romani commented Apr 11, 2024 •

edited

Lmh-java Apr 14, 2024 •

edited

romani Apr 14, 2024 •

edited

Lmh-java Apr 14, 2024 •

edited

Lmh-java Apr 18, 2024 •

edited

rnveach commented Apr 14, 2024 •

edited

romani commented Apr 14, 2024 •

edited

Lmh-java commented May 3, 2024 •

edited

romani commented May 11, 2024 •

edited

rnveach May 15, 2024 •

edited

romani commented May 15, 2024 •

edited