Skip to content

[java] CloseResource - Fix #2764: False-negative when re-assigning variable#2811

Merged
adangel merged 1 commit into
pmd:masterfrom
andipabst:master
Oct 22, 2020
Merged

[java] CloseResource - Fix #2764: False-negative when re-assigning variable#2811
adangel merged 1 commit into
pmd:masterfrom
andipabst:master

Conversation

@andipabst

@andipabst andipabst commented Oct 1, 2020

Copy link
Copy Markdown
Contributor

Describe the PR

This PR fixes a false-negative when re-assigning a variable without closing it before.

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by travis)
  • Added (in-code) documentation (if needed)

@ghost

ghost commented Oct 1, 2020 •
edited by ghost
Loading

Copy link
Copy Markdown
1 Message
📖 This changeset introduces 10 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 2 configuration errors.
Full report
✅ This changeset introduces 10 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 2 configuration errors.
Full report
✅ This changeset introduces 9 new violations, 5 new errors and 0 new configuration errors,
removes 7 violations, 0 errors and 2 configuration errors.
Full report
✅ This changeset introduces 9 new violations, 7 new errors and 0 new configuration errors,
removes 7 violations, 0 errors and 2 configuration errors.
Full report

Generated by 🚫 Danger

adangel
adangel approved these changes Oct 3, 2020
View reviewed changes

@adangel adangel 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.

Thanks, looks good!

I've retriggered the travis build to get the pmd-regression-tester results. They should arrive soon at #2811 ...

@adangel adangel self-assigned this Oct 3, 2020
@adangel adangel added this to the 6.29.0 milestone Oct 3, 2020
adangel
adangel requested changes Oct 3, 2020
View reviewed changes

@adangel adangel 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.

@andipabst
Now that the regression tester ran, there are some problems, see https://chunk.io/pmd/29b0228064074bb390f40398e6069462/diff/spring-framework/index.html

  • There are two NPEs:
java.lang.NullPointerException
 	at net.sourceforge.pmd.lang.java.rule.errorprone.CloseResourceRule.isAssignmentForVariable(CloseResourceRule.java:733)
 	at net.sourceforge.pmd.lang.java.rule.errorprone.CloseResourceRule.isReassignedBeforeBeingClosed(CloseResourceRule.java:690)
 	at net.sourceforge.pmd.lang.java.rule.errorprone.CloseResourceRule.checkForResources(CloseResourceRule.java:172)
 	at net.sourceforge.pmd.lang.java.rule.errorprone.CloseResourceRule.visit(CloseResourceRule.java:156)

and

java.lang.NullPointerException
	at net.sourceforge.pmd.lang.java.rule.errorprone.CloseResourceRule.isAssignmentForVariable(CloseResourceRule.java:735)
	at net.sourceforge.pmd.lang.java.rule.errorprone.CloseResourceRule.isReassignedBeforeBeingClosed(CloseResourceRule.java:690)
	at net.sourceforge.pmd.lang.java.rule.errorprone.CloseResourceRule.checkForResources(CloseResourceRule.java:172)
	at net.sourceforge.pmd.lang.java.rule.errorprone.CloseResourceRule.visit(CloseResourceRule.java:156)

Could you please have another look? Thanks!

@adangel adangel changed the title Fix #2764: False-negative when re-assigning variable [java] CloseResource - Fix #2764: False-negative when re-assigning variable Oct 3, 2020
@andipabst

andipabst commented Oct 10, 2020

Copy link
Copy Markdown
Contributor Author

@adangel Thanks for showing me this. I added a check for null, but I am not sure if this fixes the regression, since I can not reproduce it in a unit test. Can you please tell me how I can run the regression test locally for myself?

@adangel

adangel commented Oct 10, 2020

Copy link
Copy Markdown
Member

@andipabst The regression tester runs on every pull request. If you want to run it locally, have a look at https://github.com/pmd/pmd-regression-tester/blob/master/README.rdoc

To run it like in the PR, you could execute the following (in the PMD source directory, on your PR-branch):

bundle install
bundle exec pmdtester --local-git-repo ./ --base-branch master --patch-branch HEAD --mode online --auto-gen-config

After a while, you should have a diff report under target/diff/index.html.

But I don't think, running the tool locally is needed.

Looking at the new report generated by travis (https://chunk.io/pmd/26b5c9e13f1348e7a69413236d1f8ac5/diff/index.html), there are still some null pointers in the following files:

I'd suggest starting with one file. Put it simply as is into a new test case in closeresource.xml . That way, you should easily be able to reproduce the NPE. Then try to minimize the code sample to reproduce it, e.g. by removing one method after another. The NPE occurs while visiting a method, so find the method that is problematic.

@adangel adangel added the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Oct 10, 2020
@andipabst

andipabst commented Oct 14, 2020

Copy link
Copy Markdown
Contributor Author

@adangel Thank you, I did not notice that the top comment of the pmd-test bot was updated with the new result, my bad!

I had missed another null check, but could find it, now the regression test is producing no more errors. I also changed the location of the violation. Now the error gets flagged on the reassignment instead of the initial declaration.

@adangel adangel removed the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Oct 17, 2020
adangel
adangel approved these changes Oct 17, 2020
View reviewed changes

@adangel adangel 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.

Thanks, looks good!

adangel added a commit that referenced this pull request Oct 22, 2020
@adangel
[doc] Update release notes, refs #2811, fixes #2764
76e8333
adangel added a commit that referenced this pull request Oct 22, 2020
@adangel
Merge branch 'pr-2811' into master
873564e 
[java] CloseResource - Fix #2764: False-negative when re-assigning variable #2811
@adangel adangel merged commit 77b1837 into pmd:master Oct 22, 2020
@djh82 djh82 mentioned this pull request Dec 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adangel adangel adangel approved these changes

Assignees

@adangel adangel

Labels

None yet

Projects

None yet

Milestone

6.29.0

Development

Successfully merging this pull request may close these issues.

[java] CloseResourceRule does not recognize multiple assignment done to resource

2 participants

@andipabst @adangel