Fix for the heuristics that recognizes immutable classes #1603

baloghadamsoftware · 2021-07-05T12:27:10Z

Class names containting the substring "immutable" are considered now immutable.
Fixing issue #1601.

Make sure these boxes are checked before submitting your PR -- thank you!

Added an entry into CHANGELOG.md if you have changed SpotBugs code

KengoTODA

Please resolve a conflict in the CHANGELOG.md file.

And I personally feel that it's not good to use class/package name to verify its immutability.
The target classes could be implemented by coder who has less knowledge about immutability, and they may use .*immutable.* as a magic word to remove "noisy" warning made by SpotBugs... :\

spotbugs/src/test/java/edu/umd/cs/findbugs/util/MutableClassesTest.java

baloghadamsoftware · 2021-07-06T14:47:31Z

And I personally feel that it's not good to use class/package name to verify its immutability.
The target classes could be implemented by coder who has less knowledge about immutability, and they may use .*immutable.* as a magic word to remove "noisy" warning made by SpotBugs... :\

I agree with you, coders may misuse this heuristic. I do not know how often that happens, actually I never thought that someone would intentionally silence a true positive just because of laziness. Now I changed it to only look for the word "immutable" in the class name but not in the package name. Is it OK? If not, then I can simply put the signatures of the Guava immutable classes into the list of know immutables. (However, this does not protect against false positives coming from other APIs.)

KengoTODA · 2021-07-07T01:12:14Z

Now I changed it to only look for the word "immutable" in the class name but not in the package name. Is it OK?

Not sure. I personally think it's not enough and better to use the signatures of Guava immutable classes, but let's wait opinions from other community members.

baloghadamsoftware · 2021-07-07T11:27:02Z

Now I changed it to only look for the word "immutable" in the class name but not in the package name. Is it OK?

Not sure. I personally think it's not enough and better to use the signatures of Guava immutable classes, but let's wait opinions from other community members.

OK, I will put the Guava signatures into the list for now. However, it came to my mind that it is much easier for a developer to use a @immutable annotation than change the name of the class or the package. And we must support this annotation to avoid false positives reported by the users. Thus I do not think that many would rename their classes or packages to forcefully silence false positives.

ThrawnCA · 2021-07-08T00:12:45Z

I think there's too much scope for false positives. Just within the past few months, I overrode an ImmutableMultiDict class to enable a single limited operation for a specific reason, and named the result MostlyImmutableMultiDict. Using this heuristic, my subclass would be treated as immutable, when it shouldn't be.

rkraneis · 2021-07-09T10:04:01Z

Should immutable classes from java.time package be added as well? Since 4.3.0 we get EI_EXPOSE_REP for those, too ...
(java.time package description reads “[...] All the classes are immutable and thread-safe. [...]”)

baloghadamsoftware · 2021-07-09T10:25:32Z

Should immutable classes from java.time package be added as well? Since 4.3.0 we get EI_EXPOSE_REP for those, too ...
(java.time package description reads “[...] All the classes are immutable and thread-safe. [...]”)

Sure. I will update this PR to include them.

callum-kilby · 2021-07-15T09:39:51Z

Just checking in, what is the status of this request? There's other reporters with the same issue, with some additional suggestions for checking for immutability

Class names containting the substring "immutable" are considered now immutable. Fixing issue spotbugs#1601.

…ckage name. Also added a check for an annotation ending with "Immutable".

… checked.

baloghadamsoftware · 2021-07-15T11:52:07Z

Should immutable classes from java.time package be added as well? Since 4.3.0 we get EI_EXPOSE_REP for those, too ...
(java.time package description reads “[...] All the classes are immutable and thread-safe. [...]”)

Sure. I will update this PR to include them.

I think there's too much scope for false positives. Just within the past few months, I overrode an ImmutableMultiDict class to enable a single limited operation for a specific reason, and named the result MostlyImmutableMultiDict. Using this heuristic, my subclass would be treated as immutable, when it shouldn't be.

What about Detecting *.Immutable*? I think there is hardly anyone who calls a class ImmutableLikeXXX.

…Test.java Co-authored-by: Kengo TODA <skypencil+github@gmail.com>

…eClassesTest.java" This reverts commit efaed90.

baloghadamsoftware · 2021-07-15T12:47:31Z

@KengoTODA & @ThrawnCA For now I only added the Guava immutable classes, the java.date package and the @immutable annotation.

baloghadamsoftware · 2021-07-15T13:47:53Z

Just checking in, what is the status of this request? There's other reporters with the same issue, with some additional suggestions for checking for immutability

Where are they? I searched in the issues and discussions, but I could not find any. It would be good to read them and include all the classes they claims immutable into the list of known immutable classes.

callum-kilby · 2021-07-15T14:07:56Z

Just checking in, what is the status of this request? There's other reporters with the same issue, with some additional suggestions for checking for immutability

Where are they? I searched in the issues and discussions, but I could not find any. It would be good to read them and include all the classes they claims immutable into the list of known immutable classes.

I'm looking at the other comments in my issue:
#1601

It's mostly java.time classes, but also Java.lang.Class. There's also some discussion on checking for a jdk.internal.ValueBased annotation, or providing the previous behaviour for these checks.

callum-kilby · 2021-07-16T09:28:24Z

Is it possible to include an additional whitelist that is set as a plugin configuration option? It would let users specify immutable classes that they have defined in their project without asking them to be added to the spotbugs whitelist

…upportedOperationException.

baloghadamsoftware · 2021-07-20T14:37:58Z

With the last modification it seems that we finally got rid of all false positives. Now all the findings are true positives. However, there are quite many of them so I would propose changing the priority of all the findings of the FindReturnRef detector to low.

KengoTODA

This PR does not fix existing false positive, it introduces many new false negatives and it won't benefit most of Java developers.

KengoTODA · 2021-07-23T05:31:24Z

spotbugs/src/main/java/edu/umd/cs/findbugs/util/MutableClasses.java


    private static final List<String> SETTER_LIKE_NAMES = Arrays.asList(
            "set", "put", "add", "insert", "delete", "remove", "erase", "clear", "push", "pop",
-            "enqueue", "dequeue", "write", "append", "replace");
+            "enqueue", "dequeue", "append", "replace");


This change is backward compatible and will affect many users.
So personally I don't want to merge this change to SpotBugs.

Do you mean the omission of "write" from the list? Actually, I found that methods called write...() are usually no setters but they write to a stream.

KengoTODA · 2021-07-23T05:33:50Z

spotbugs/src/main/java/edu/umd/cs/findbugs/util/MutableClasses.java

+            return false;
+        }
+
+        // Non-public setters cannot be used from outside the package so disregard them.


I think this change is not so natural. The package affect nothing to the immutability of class architecture.

Yes, this can be disputed. If you whish, I can change this part to only omit methods which are private or protected.

baloghadamsoftware · 2021-07-23T11:29:34Z

Thank you, for your review @KengoTODA!

This PR does not fix existing false positive, it introduces many new false negatives and it won't benefit most of Java developers.

Which part, do you mean? Actually, this PR is quite complex and contains different fixes. Originally, I implemented a heuristics to replace the placeholder (which was just a list of 4 mutable classes and all the rest were considered as immutable) in PR #1533. However, my heuristics was incomplete and contained some bugs. It caused tons of findings, especially in the FindReturnRef detector, and some of them were false positives. Users reported them here: #1601.

In this patch I tried to fix these issues and also made a long and tiring manual research to find all the possible false positives and fix the heuristics to not to report them. The fixes in this PR:

Static methods are surely no setters, because they cannot write instance fields. I fixed this in this PR.
Setter-like methods which throw UnsupportedOperationException are no setters. I fixed this in this PR.
Private methods are no setters. If there are no public setters, then the only usage of private methods is in the constructor or block initializers. This means that the class is still immutable. What could be disputed here is the case of package-public methods. In this PR I decided to omit them, but if you think it is wrong then I can include them.
When checking the findigs I noticed that methods beginning with the word write are usually no setters, but stream output methods. For this reason I removed this tag from the list in this PR.
There are methods with name set...(), add...() etc which create a new object instead of modifying the current one. In the original heuristics I tried to detect them by comparing their result type with the class they are defined in, but for some reason it did not work as I expected. (I forgot to test them explicitly.) Furthermore, I found cases where they return a different class type but do not modify the this object. For this reason I changed this part of the algorithm in this PR to disregard methods which return any object type. This can be disputed, of course, if you do not agree.
For performance reasons (and also because the heuristics was too weak initially) I also had a fixed list of immutable classes. In this PR I extended this list with e.g. Guava immutable classes.
There are packages in the Java API that contain immutable classes only. Instead of putting these classes into the previous list one-by-one, I created a new list for these packages in this PR.

The result is that maybe we have some additional false negatives but all the false positives disappeared.

candrews · 2021-08-20T13:34:35Z

Here's a (simple) PR that fixes some incorrect class names in MutableClasses and adds a missing one (java.net.Inet6Address): #1654

candrews · 2021-08-20T13:37:44Z

This PR fixes #1634

baloghadamsoftware · 2021-09-01T14:55:10Z

This pull request is now split up to #1670, #1671, #1672, #1673, #1674, #1676, #1677 and #1678.

KengoTODA requested changes Jul 6, 2021

View reviewed changes

spotbugs/src/test/java/edu/umd/cs/findbugs/util/MutableClassesTest.java Outdated Show resolved Hide resolved

baloghadamsoftware force-pushed the ImmutableFix branch from 33545e2 to f90d90a Compare July 6, 2021 14:41

baloghadamsoftware requested a review from KengoTODA July 6, 2021 14:58

baloghadamsoftware force-pushed the ImmutableFix branch from f90d90a to e71e551 Compare July 7, 2021 19:36

Ádám Balogh added 3 commits July 15, 2021 13:20

Fix for the heuristics that recognizes immutable classes.

492b24d

Class names containting the substring "immutable" are considered now immutable. Fixing issue spotbugs#1601.

Change to look for "*immutable*" only in class name but not in the pa…

b2fe191

…ckage name. Also added a check for an annotation ending with "Immutable".

Instead of pattern matching on the class name, only Guava classes are…

a9b15af

… checked.

baloghadamsoftware force-pushed the ImmutableFix branch from e71e551 to a199bc8 Compare July 15, 2021 11:45

All classes of java.time are now considered as immutables.

a199bc8

Balogh, Ádám and others added 2 commits July 15, 2021 13:52

Update spotbugs/src/test/java/edu/umd/cs/findbugs/util/MutableClasses…

efaed90

…Test.java Co-authored-by: Kengo TODA <skypencil+github@gmail.com>

Revert "Update spotbugs/src/test/java/edu/umd/cs/findbugs/util/Mutabl…

4e3caad

…eClassesTest.java" This reverts commit efaed90.

Ádám Balogh added 6 commits July 16, 2021 15:23

Heuristics improved & java.math added to the known immutable packages.

46375d7

Support for jdk.internal.ValueBased added

26160ac

Disclose static methods from setters and also methods which throw Uns…

64ea604

…upportedOperationException.

"write" removed from the list of setter-like names

4087bd4

Non-public methods are not setters

61eb881

Typo fix

7a336dd

baloghadamsoftware mentioned this pull request Jul 21, 2021

Add option to user-defined list of immutable classes #1619

Draft

1 task

Ill-named and redundant test case removed, changelog updated

65a4388

KengoTODA requested changes Jul 23, 2021

View reviewed changes

baloghadamsoftware mentioned this pull request Aug 11, 2021

EI_EXPOSE_REP and EI_EXPOSE_REP2 occur if using Guava Immutable objects #1601

Open

baloghadamsoftware closed this Sep 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for the heuristics that recognizes immutable classes #1603

Fix for the heuristics that recognizes immutable classes #1603

baloghadamsoftware commented Jul 5, 2021

KengoTODA left a comment

baloghadamsoftware commented Jul 6, 2021

KengoTODA commented Jul 7, 2021

baloghadamsoftware commented Jul 7, 2021

ThrawnCA commented Jul 8, 2021

rkraneis commented Jul 9, 2021 •

edited

Loading

baloghadamsoftware commented Jul 9, 2021

callum-kilby commented Jul 15, 2021

baloghadamsoftware commented Jul 15, 2021 •

edited

Loading

baloghadamsoftware commented Jul 15, 2021

baloghadamsoftware commented Jul 15, 2021

callum-kilby commented Jul 15, 2021

callum-kilby commented Jul 16, 2021

baloghadamsoftware commented Jul 20, 2021

KengoTODA left a comment

KengoTODA Jul 23, 2021

baloghadamsoftware Jul 23, 2021

KengoTODA Jul 23, 2021

baloghadamsoftware Jul 23, 2021

baloghadamsoftware commented Jul 23, 2021

candrews commented Aug 20, 2021

candrews commented Aug 20, 2021

baloghadamsoftware commented Sep 1, 2021

Fix for the heuristics that recognizes immutable classes #1603

Fix for the heuristics that recognizes immutable classes #1603

Conversation

baloghadamsoftware commented Jul 5, 2021

KengoTODA left a comment

Choose a reason for hiding this comment

baloghadamsoftware commented Jul 6, 2021

KengoTODA commented Jul 7, 2021

baloghadamsoftware commented Jul 7, 2021

ThrawnCA commented Jul 8, 2021

rkraneis commented Jul 9, 2021 • edited Loading

baloghadamsoftware commented Jul 9, 2021

callum-kilby commented Jul 15, 2021

baloghadamsoftware commented Jul 15, 2021 • edited Loading

baloghadamsoftware commented Jul 15, 2021

baloghadamsoftware commented Jul 15, 2021

callum-kilby commented Jul 15, 2021

callum-kilby commented Jul 16, 2021

baloghadamsoftware commented Jul 20, 2021

KengoTODA left a comment

Choose a reason for hiding this comment

KengoTODA Jul 23, 2021

Choose a reason for hiding this comment

baloghadamsoftware Jul 23, 2021

Choose a reason for hiding this comment

KengoTODA Jul 23, 2021

Choose a reason for hiding this comment

baloghadamsoftware Jul 23, 2021

Choose a reason for hiding this comment

baloghadamsoftware commented Jul 23, 2021

candrews commented Aug 20, 2021

candrews commented Aug 20, 2021

baloghadamsoftware commented Sep 1, 2021

rkraneis commented Jul 9, 2021 •

edited

Loading

baloghadamsoftware commented Jul 15, 2021 •

edited

Loading