Skip to content

[java] NullAssignment false positive#1033

Merged
jsotuyod merged 2 commits into
pmd:masterfrom
adangel:issue-629
Apr 25, 2018
Merged

[java] NullAssignment false positive#1033
jsotuyod merged 2 commits into
pmd:masterfrom
adangel:issue-629

Conversation

@adangel

@adangel adangel commented Apr 10, 2018

Copy link
Copy Markdown
Member

Fixes #629

Note: This is a partial fix only.
This FP is not detected:

String key;
if (a) {
	key = "a";
} else if (b) {
	key = "b";
} else {
	key = null;
}

@adangel adangel added the a:false-positive PMD flags a piece of code that is not problematic label Apr 10, 2018
@adangel adangel added this to the 6.3.0 milestone Apr 10, 2018
@adangel adangel mentioned this pull request Apr 10, 2018
Closed
jsotuyod
jsotuyod reviewed Apr 10, 2018
View reviewed changes
Comment thread ...java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/NullAssignment.xml Outdated

@jsotuyod jsotuyod Apr 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure this is actually right... the lambda is a find boundary. From its point of view, it's a return test ? truthy : null which would be ok (it's not an assignment). For the assignmet you would have to understand the semantics of computeIfAbsent

@adangel adangel Apr 11, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You're right - in this specific example (for computeIfAbsent) the result is a null assignment. But it won't be always the "computeIfAbsent" usage case....

I'll change the test to not expect a violation here, otherwise we'll probably get many false positives.

@adangel adangel Apr 23, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@jsotuyod I've adjusted the test+implementation to ignore lambdas.
I've also rebase this PR against master, it should be ready now.

@adangel adangel added the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Apr 11, 2018
@adangel adangel removed the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Apr 23, 2018
jsotuyod
jsotuyod approved these changes Apr 25, 2018
View reviewed changes

@jsotuyod jsotuyod left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

awesome! I'll merge it soon. Shall we open a new issue to track the one scenario we are still not handling?

@jsotuyod jsotuyod merged commit fd71799 into pmd:master Apr 25, 2018
@adangel

adangel commented Apr 25, 2018

Copy link
Copy Markdown
Member Author

I've created a new issue: #1050

@adangel adangel deleted the issue-629 branch April 26, 2018 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsotuyod jsotuyod jsotuyod approved these changes

Assignees

No one assigned

Labels

a:false-positive PMD flags a piece of code that is not problematic is:waiting-for-review

Projects

None yet

Milestone

6.3.0

Development

Successfully merging this pull request may close these issues.

[java] NullAssignment false positive

2 participants

@adangel @jsotuyod