Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue # 11187: OperatorWrapCheck throws NPE on guarded patterns #11188

Merged
merged 1 commit into from
Jan 28, 2022
Merged

Issue # 11187: OperatorWrapCheck throws NPE on guarded patterns #11188

merged 1 commit into from
Jan 28, 2022

Conversation

nrmancuso
Copy link
Member

@nrmancuso nrmancuso commented Jan 12, 2022 •
edited

@nrmancuso nrmancuso marked this pull request as draft January 12, 2022 14:59
@nrmancuso nrmancuso force-pushed the issue-11187 branch 4 times, most recently from 5e9d5ab to acb73d8 Compare January 13, 2022 15:06
@nrmancuso nrmancuso self-assigned this Jan 16, 2022
@nrmancuso
Copy link
Member Author

Github, generate report

@github-actions
Copy link
Contributor

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

@nrmancuso nrmancuso marked this pull request as ready for review January 20, 2022 19:35
@nrmancuso
Copy link
Member Author

CI failure is not related: Connect to www.w3.org:80 [www.w3.org/128.30.52.100] failed: Connection timed out

@nrmancuso nrmancuso removed their assignment Jan 20, 2022
rnveach
rnveach reviewed Jan 22, 2022
View reviewed changes
src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/OperatorWrapCheck.java
private static boolean isInPatternDefinition(DetailAST node) {
DetailAST parent = node.getParent();
while (parent != null && parent.getType() != TokenTypes.PATTERN_DEF) {
parent = parent.getParent();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not?

DetailAST parent = node;
do {
    parent = parent.getParent();
} while (parent != null && parent.getType() != TokenTypes.PATTERN_DEF);

Also we are passing through parents uncontrolled. It doesn't matter if we pass any other kind of def before hitting the pattern def?

Copy link
Member Author

@nrmancuso nrmancuso Jan 22, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not...

Please explain the advantage of this; to me the name of variable declared (parent) and initialization (node.getParent();) makes sense. Since we are checking operands, there is absolutely no chance that node is null (it will always have an operator as a parent).

It doesn't matter if we pass any other kind of def before hitting the pattern def?

To the check, no.And a local variable (pattern definition) will always have an enclosing block of some sort, so we certainly could just check for a method definition, etc., and stop there. But, I generally avoid this sort of "optimization" since pitest normally fails on this sort of thing, unless we also remove the null check.

On second thought, I might be able to create a case to make this fail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know why, but my brain not always work quick on do-while structure. I prefer while loop in this case.

Copy link
Member

@rnveach rnveach Jan 23, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal in this case, but I see the duplication of getParent and possibly an extra unnecessary condition check at the start of the loop and I think how to optimize it more. Hence the do-while version. If getParent was more complex or more method calls on the line, then it would be a better sell.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I might be able to create a case to make this fail.

I am assuming this item is still TODO until told otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I saw the update, but I am still not really seeing a conclusion to this item:

It doesn't matter if we pass any other kind of def before hitting the pattern def?

For example, it doesn't matter if we pass through an anonymous class to get to the pattern def or patterns are such that we are either in it or not for what we stop on.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anonymous class to get to the pattern def

It is not possible to declare an anonymous class within a pattern definition.

patterns are such that we are either in it or not for what we stop on.

Please restate this, I am not sure that I understand what you are asking.

Copy link
Member

@romani romani Jan 26, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder. This is fix for exception, it should merged sooner. If we think there is some false positive on code that we can not imagine easily, it is better to address in separate issue.

Copy link
Member

@rnveach rnveach Jan 26, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will approve the PR, but we have seen cases before where we either go too deep in the tree or go past too many key nodes when looking for "something" and we are accidentally pulling in nodes wrongly or passing things that should have stopped us. branchContains is one good example. #5124 (comment) details the specifics of how it was going to far and wasn't be used carefully enough.

Whenever I see a loop where we are scanning up or down uncontrolled, I get concerned. I don't know the new Java syntax well, so I can't explain a possible situation to prompt a more meaningful conversation. I am just making sure this won't be an issue later and we are truly good with the code.

Copy link
Member Author

@nrmancuso nrmancuso Jan 27, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Condition is updated to exit loop ASAP.

romani
romani approved these changes Jan 22, 2022
View reviewed changes
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last commit is ok to merge

@nrmancuso nrmancuso force-pushed the issue-11187 branch 5 times, most recently from 95a9a25 to 586d278 Compare January 24, 2022 01:32
@nrmancuso nrmancuso removed the blocked label Jan 24, 2022
@nrmancuso
Copy link
Member Author

Github, generate report

@github-actions
Copy link
Contributor

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

@nrmancuso
Copy link
Member Author

Github, generate report

@github-actions
Copy link
Contributor

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

@nrmancuso nrmancuso mentioned this pull request Jan 26, 2022
Closed
@nrmancuso
Copy link
Member Author

Github, generate report

@nrmancuso
Copy link
Member Author

Github, rebase

@github-actions
Copy link
Contributor

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

@romani
Copy link
Member

romani commented Jan 27, 2022

Regression , NPE is detected by CI.

@nrmancuso
Copy link
Member Author

Github, generate report

@github-actions
Copy link
Contributor

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

@nrmancuso
Copy link
Member Author

Github, generate report

@github-actions
Copy link
Contributor

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

@rnveach rnveach closed this Jan 28, 2022
@rnveach rnveach reopened this Jan 28, 2022
@rnveach rnveach merged commit 7124ab9 into checkstyle:master Jan 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romani romani romani approved these changes

@pbludov pbludov pbludov approved these changes

@rnveach rnveach rnveach approved these changes

Assignees

@rnveach rnveach

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OperatorWrapCheck throws NPE on guarded patterns
4 participants
@nrmancuso @romani @pbludov @rnveach