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. :)

rnveach · 2024-04-14T00:04:29Z

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

+            catch (CheckstyleException ex) {
+                if (!skipFileOnJavaParseException) {
+                    throw ex;
+                }


@romani We are not going to display anything when exception occurs? We are going to completely hide that this happened? Even with haltOnException, we display a message to the user to notify this happened.
https://github.com/checkstyle/checkstyle/blob/86195e06404df68905c90232f71a1c88491f9ad8/src/main/java/com/puppycrawl/tools/checkstyle/Checker.java#L349C61-L365

This seems very risk to completely ignore it.

I am for displaying a violation which can be suppressed, similar to halt on exception.

We are not going to display anything when exception occurs? We are going to completely hide that this happened?

If user explicitly configure to skip, we do skip swallow and no noise warning or any other messages. User should use it with caution.

I am for displaying a violation which can be suppressed, similar to halt on exception.

The whole idea of this property is to no extra violations and suppressions.

I am for displaying a violation which can be suppressed, similar to halt on exception.

Yep, same here. Otherwise, how can we easily determine what is being skipped? This is poor design from both a user and testability standpoint.

Imagine you use this property. You expect some violations in a file with newer syntax, but none appear. You want to know if the file is actually being checked or if the check is broken. How can you do this?

If user use this option, he is completely ok to skip checkstyle on some classes. Some Users value new syntax features more than checkstyle validation, it is normal, we should allow this.

It is same as all plugins have skip options, to allow keep plugin plugin still in build but let users decide when to activate it to fail a build or ignore it when they want this.

If user use this option, he is completely ok to skip checkstyle on some classes.

I’m not debating this; it is the essence of this feature. My question is how can a user know what is being skipped?

@Lmh-java we want to emulate the haltOnException property. It is not clear to me why checker should care. Parsing exception happens, we catch it and log the message. Is this behavior not possible?

According to #14779 (comment), do we move these two new properties to Checker instead of TreeWalker?

This is my mistake, these should live in TreeWalker for sure, thanks for calling this out.

Is this behavior not possible?

I think this is possible. let me try. :)

javaParseExceptionSeverity to match what we catch/skip, as there is javadoc parsing.

<property name="skipFileOnJavaParseException" value="true"/> <property name="javaParseExceptionSeverity" value="warning"/>

and I am ok to move parsing to and wrapping with catch to separate methods to return Optional<DetailsAST> to make if (root.ifPresent()) {. Whatever modern code style to unblock this PR.

That's a great idea. Thanks for your advice. I didn't think about Optional before. I will use that instead.

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

nrmancuso · 2024-05-02T12:17:20Z

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.

romani · 2024-05-02T13:16:13Z

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:

nrmancuso · 2024-05-07T12:45:47Z

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

+        try {
+            execute(config, files);
+            assertWithMessage("Exception is expected").fail();
+        }
+        catch (CheckstyleException exception) {
+            assertWithMessage("Error message is unexpected")
+                    .that(exception.getMessage())
+                    .contains("Exception was thrown while processing ");
+        }


Please use

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/internal/utils/TestUtil.java

Line 393 in bfb3e5f

public static <T extends Throwable> T getExpectedThrowable(Class<T> expectedType,

in all cases like this.

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

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

romani self-requested a review April 28, 2024 13:57

Lmh-java force-pushed the minghao/treewalker-property branch 5 times, most recently from 5beaa32 to 2166145 Compare May 1, 2024 14:59

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

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

	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 Apr 14, 2024 • edited

Choose a reason for hiding this comment

romani Apr 14, 2024 • edited

Choose a reason for hiding this comment

nrmancuso Apr 18, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani May 2, 2024 • edited

Choose a reason for hiding this comment

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

nrmancuso commented May 2, 2024

romani commented May 2, 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

Choose a reason for hiding this 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

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 Apr 14, 2024 •

edited

romani Apr 14, 2024 •

edited

nrmancuso Apr 18, 2024 •

edited

romani May 2, 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