Skip to content

False positive SA_LOCAL_SELF_COMPARISON when using instanceof pattern matching#1136

Closed
schosin wants to merge 4 commits into
spotbugs:masterfrom
schosin:instanceof-pattern-matching-false-positive
Closed

False positive SA_LOCAL_SELF_COMPARISON when using instanceof pattern matching#1136
schosin wants to merge 4 commits into
spotbugs:masterfrom
schosin:instanceof-pattern-matching-false-positive

Conversation

@schosin

@schosin schosin commented May 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR adds a failing test when using instanceof pattern matching in Java 14 (preview feature).

In this code, SA_LOCAL_SELF_COMPARISON is incorrectly reported for number:

Number number = 1f;
if (number instanceof Float f) {
    System.out.println("number is a float: " + f);
}

Edit: According to JEP 375 this feature will be going into a second preview with no changes relative to the first preview introduced in JDK 15. This PR should be changed to use JDK 15 as soon as it is released and gradle/other dependencies support JDK 15.

@KengoTODA

KengoTODA commented Jul 13, 2020

Copy link
Copy Markdown
Member

Test is not failing, could you check? @schosin

ThrawnCA
ThrawnCA requested changes Jul 14, 2020
View reviewed changes
Comment thread ...t/java/edu/umd/cs/findbugs/detect/IncorrectSelfComparasionInstanceOfPatternMatchingTest.java Outdated
Comment thread ...t/java/edu/umd/cs/findbugs/detect/IncorrectSelfComparasionInstanceOfPatternMatchingTest.java Outdated
Comment thread spotbugsTestCases/src/java14/IncorrectSelfComparasionInstanceOfPatternMatching.java Outdated
Comment thread spotbugsTestCases/src/java14/IncorrectSelfComparasionInstanceOfPatternMatching.java Outdated

public static void instanceofPatternMatching() {
Number number = 1f;
if (number instanceof Float f) {

@ThrawnCA ThrawnCA Jul 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this correct syntax? Why the f?

@schosin schosin Jul 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is the new syntax introduced in this preview feature. I suppose the false positive is triggered because number and f reference the same instance of the boxed 1f.

@schosin schosin force-pushed the instanceof-pattern-matching-false-positive branch from 259cfed to 083f2b2 Compare July 14, 2020 04:26
@schosin

schosin commented Jul 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

The tests are probably not failing because gradle has to be running on JDK 14. See changes in build.gradle.
Since I never worked with github workflows I haven't made changes to that.

If you prefer I can squash those changes once its done.

@schosin schosin requested a review from ThrawnCA July 14, 2020 04:50
ThrawnCA
ThrawnCA reviewed Jul 14, 2020
View reviewed changes
Comment thread ...t/java/edu/umd/cs/findbugs/detect/IncorrectSelfComparisionInstanceOfPatternMatchingTest.java Outdated
Comment thread spotbugsTestCases/src/java14/IncorrectSelfComparisionInstanceOfPatternMatching.java Outdated
@schosin schosin force-pushed the instanceof-pattern-matching-false-positive branch from 083f2b2 to fdec527 Compare July 15, 2020 04:33
@schosin

schosin commented Jul 15, 2020

Copy link
Copy Markdown
Contributor Author

Fixed remaining typos and rebased on top of master.

@schosin schosin requested a review from ThrawnCA July 19, 2020 05:47
ThrawnCA
ThrawnCA previously approved these changes Jul 20, 2020
View reviewed changes

@ThrawnCA ThrawnCA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks ok, but please try to avoid force-pushes since they break the history.

KengoTODA
KengoTODA requested changes Jul 21, 2020
View reviewed changes

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

Test is still not failing.

Comment thread ...st/java/edu/umd/cs/findbugs/detect/IncorrectSelfComparisonInstanceOfPatternMatchingTest.java Outdated
@schosin

schosin commented Jul 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Test won't be failing since the github workflow is only configured for java 8. For the tests to fail gradle has to be run on JDK 14.
Since I never worked with github workflows I don't know the best way to configure it.
There's also a java 11 test that's probably never run for the same reason.

As a side note, edu.umd.cs.findbugs.DetectorsTest > testAllRegressionFilesJavac FAILED also fails when running on JDK 14, but I'm not sure what's the best way to fix it.

@schosin schosin requested a review from KengoTODA July 21, 2020 18:10
ThrawnCA
ThrawnCA previously approved these changes Jul 22, 2020
View reviewed changes
@KengoTODA KengoTODA mentioned this pull request Aug 4, 2020
Closed
@KengoTODA KengoTODA mentioned this pull request Sep 3, 2020
Closed
@KengoTODA

KengoTODA commented Feb 18, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

I created #1435 to solve conflict and run full pre-merge build. Please use that for coming discussion & review.

@KengoTODA KengoTODA closed this Feb 18, 2021
@KengoTODA KengoTODA mentioned this pull request Feb 22, 2021
Merged
KengoTODA added a commit that referenced this pull request Mar 1, 2021
@KengoTODA @schosin
False positive SA_LOCAL_SELF_COMPARISON when using instanceof pattern…
50cb283 
… matching (#1435)

* add test for false positive when using instanceof pattern matching

* #1136: add link to github issue

* ci: use JDK 14 to run test with Java 14

* docs: add an entry to the CHANGELOG

* build: upgrade JaCoCo to 0.8.6 that supports Java 14 officially

* build: use Java 15 instead, because 14 is outdated already

* Update CHANGELOG.md

The bug is caused by Java 14, that is outdated and not maintained any more.
In the SpotBugs side we need no update, so we should have no entry in this file.

Co-authored-by: Ken Schosinsky <schosins@gmail.com>
@ghost

ghost commented May 6, 2021

Copy link
Copy Markdown

Using 4.2.3 this is still a false positive. See 50cb283#commitcomment-49080418

@KengoTODA

KengoTODA commented May 7, 2021

Copy link
Copy Markdown
Member

@LutzHorn It's javac problem, please use Java 16 instead. Java 14 is already out of support.

@SchubP SchubP mentioned this pull request Mar 22, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ThrawnCA ThrawnCA ThrawnCA left review comments

@KengoTODA KengoTODA Awaiting requested review from KengoTODA

Assignees

No one assigned

Labels

bug Java-14

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@schosin @KengoTODA @ThrawnCA