[java] Update rule UnnecessaryCast, make it flag any unnecessary cast#3204
Merged
Conversation
Previously UnnecessaryBoxing, but also handles conversions between primitives
Generated by 🚫 Danger |
... and not being propagated to the enclosing site.
Inference happening in the added test:
```
Phase INVOC_STRICT valueOf(java.lang.Class<T>, java.lang.String) -> T
New bound (ctx 5): 'b <: java.lang.Enum<'b>
Context 5 valueOf(java.lang.Class<'b>, java.lang.String) -> 'b
RETURN
ARGUMENTS
Checking arg 0 against java.lang.Class<'b>
At: /*unknown*/:16 :33..16:46
Expr: this.enumType
New bound (ctx 5): 'b = T
Checking arg 1 against java.lang.String
At: /*unknown*/:16 :48..16:61
Expr: source.trim()
Skipping instantiation of java.lang.String.trim() -> java.lang.String, it's already complete
Ivar instantiated (ctx 5): 'b := T
Success: valueOf(java.lang.Class<T>, java.lang.String) -> java.lang.Enum
```
On line `new bound... 'b = T` what happens is the context will
check that the previous bound `'b <: java.lang.Enum<'b>` against
`'b = T`. Since T has upper bound `Enum`, not `Enum<T>`, an unchecked
conversion is necessary (`Enum ~> Enum<T>`). This needs to be notified
to the inference context so that invocation properly erases the return
type to `Enum`.
This was causing a FP with UnnecessaryCast, as in the previous, bugged
version, the return type of `Enum.valueOf(...)` was inferred to `T`,
which doesn't need to be cast to `T`.
It's a pain otherwise
adangel
reviewed
Apr 23, 2021
adangel
left a comment
Member
There was a problem hiding this comment.
Awesome, great work! 🎉
There is a small issue with lambdas:
https://github.com/checkstyle/checkstyle/blob/checkstyle-8.10/src/test/resources/com/puppycrawl/tools/checkstyle/grammars/InputRegressionJava8Class1.java#L61
This is reported, I think, that's correct - the cast is unnecessary here. But the violation message is Unnecessary cast ({0}). I think, it should be "Unnecessary cast (Comparator)".
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the PR
The rule has been generalized to just flag any unnecessary cast, not just those which use collections. This adds some utilities to query the context type of an expression, which I think we may use further to implement #2973. In fact this branch contains the commits for such a new rule, but I removed them and will submit a revert later.
There is one thing that is not implemented here, which is casts that help disambiguate two method overloads. This is a useful feature but would need some more work. For now all casts in a method argument are ignored.
I think this illustrates nicely how reliable the type resolution code is. To fix FPs in the later commits of the branch, the rule definition didn't have to change, only the typeres implementation. And this added precision will benefit all rules.
Related issues
Ready?
./mvnw clean verifypasses (checked automatically by travis)