fix #1025 Error Prone StrictCollectionIncompatibleType to guard collection usage
#1027
Conversation
…lection usage This check is an improvement over `CollectionIncompatibleType` because it validates that values exist in the same type hierarchy, where it could theoretically be possible the input implements the collection type, but the type system doesn't have enough information to be confident. This check allows both subtypes and supertypes, but does not check for shared supertypes, which are not common.
Generate changelog in
|
|
Just tested locally. This does not catch the bug that inspired me to file the ticket, because this does not catch method references. However, it seems to catch the simpler case. which is not caught, but this is: @carterkozak is aware and prefers to stick with this at first, then support method references if this doesn't cause problems. |
...r-prone/src/main/java/com/palantir/baseline/errorprone/StrictCollectionIncompatibleType.java
Outdated
Show resolved
Hide resolved
|
Verified on a large internal project. Three failures, all of which appear to be legitimate. |
| altNames = {"SuspiciousMethodCalls", "CollectionIncompatibleType"}, | ||
| link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", | ||
| linkType = BugPattern.LinkType.CUSTOM, | ||
| severity = BugPattern.SeverityLevel.WARNING, |
There was a problem hiding this comment.
These are pretty serious, and our large internal project had no false positives. We could bump this to ERROR.
There was a problem hiding this comment.
Let's leave it WARNING for now, and bump it in a week or so.
There was a problem hiding this comment.
What sort of additional testing for primitives are you looking for, @dansanduleac?
| " }", | ||
| " boolean f4(List<Integer> in) {", | ||
| " // BUG: Diagnostic contains: incompatible types", | ||
| " return in.remove(5L);", |
There was a problem hiding this comment.
This tests boxing the wrong primitive results in a failure
There was a problem hiding this comment.
oh somehow glossed over these, I was scanning for a function with a type of int or something
| " return in.contains(null);", | ||
| " }", | ||
| " boolean f1(Collection<Integer> in) {", | ||
| " return in.contains(3);", |
There was a problem hiding this comment.
Tests boxing the expected primitive works as expected.
|
dozer? |
|
Released 2.29.0 |
This check is an improvement over
CollectionIncompatibleTypebecause itvalidates that values exist in the same type hierarchy, where it could
theoretically be possible the input implements the collection type,
but the type system doesn't have enough information to be confident.
This check allows both subtypes and supertypes, but does not check
for shared supertypes, which are not common.
After this PR
==COMMIT_MSG==
Error Prone
StrictCollectionIncompatibleTypeto guard collection usage==COMMIT_MSG==