Exclude methods throwing UnsupportedOperationException from setter-like methods when considering a class as immutable
#1672
Conversation
…like methods when considering a class as immutable Methods throwing `UnsupportedOperationException` must not be considered as setters, even if their name is setter-like. Partial fix for issue [spotbugs#1601](spotbugs#1601)
| @@ -54,6 +54,10 @@ public int getN() { | |||
| public Immutable setN(int n) { | |||
| return new Immutable(n); | |||
| } | |||
|
|
|||
| public void setNUnsupported(int n) throws UnsupportedOperationException { | |||
There was a problem hiding this comment.
Just to make sure - would it work without declaration of UnsupportedOperationException being thrown?
There was a problem hiding this comment.
No, because we cannot check here the body of the function. IMO UnsupportedOperationException being thrown is better to be declared if the whole function call is not supported in this class.
There was a problem hiding this comment.
Can method declare to throw UnsupportedOperationException and modify state anyway? And even not throw anything at all?
There was a problem hiding this comment.
In the end, if code deliberately tries to mask bad behaviour, it will probably succeed.
There was a problem hiding this comment.
Yes, this is just a heuristics to find immutable classes. If you declare a function as a thrower but modify the state then this is a setter and the class is not immutable. The heuristics cannot be 100% accurate.
There was a problem hiding this comment.
Can it lead to false-negatives?
There was a problem hiding this comment.
Of course. As I mentioned, this is a heuristics. Unfortunately, unlike C++'s const member functions, Java has no means that forbids a method to modify the attributes of the class instance. Instead, we try to find methods which look like setters. If we do not find any then we consider the class for immutable. However, if the methods are named in a foreign language, or the setters are ill named (like eg. foo()), then we consider the class as immutable which leads to false negatives.
There was a problem hiding this comment.
I see, thank you for your explanation.
spotbugs/src/main/java/edu/umd/cs/findbugs/util/MutableClasses.java
Outdated
Show resolved
Hide resolved
spotbugs/src/main/java/edu/umd/cs/findbugs/util/MutableClasses.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@ThrawnCA I think this change isn't so valuable and better to keep our implementation simple. No matter how methods are implemented, just check fields and the class are final or not then it should be enough. I know that the current SpotBugs implementation is not like what I mean, but it's better to stop making it far from the ideal state.
And even if this change works, UnsupportedOperationException is an unchecked exception so ExceptionTable could have no record for the exception. So I think this PR is not reasonable nor practical.
It's just my personal opinion. 🙇♂️
How do you think? Is it still better to merge this PR? Thanks in advance!
|
You are right, Although Guava classes are considered as immutable explicitly, we cannot be sure that there are no similar classes in a user's code. Thus I think that this PR is useful to reduce the number of false positives but not in its current format but after rewriting it to check the body of the function instead of its exception specification. |
|
IIUC, this can be unblocked, i.e. the requested changes are no longer required. |
Methods throwing
UnsupportedOperationExceptionmust not be considered as setters, even if their name is setter-like.Partial fix for issue [#1601](https://github.com/spotbugs/spotbugs/issues/1601\)
Make sure these boxes are checked before submitting your PR -- thank you!
CHANGELOG.mdif you have changed SpotBugs code