DangerousIdentityKey identifies unsafe map and set keys #1731
Conversation
Generate changelog in
|
...e-error-prone/src/test/java/com/palantir/baseline/errorprone/PatternAsKeyOfSetOrMapTest.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| protected boolean isBadType(Type type, VisitorState state) { | ||
| return ASTHelpers.isSubtype(type, state.getTypeFromString("java.util.regex.Pattern"), state); |
There was a problem hiding this comment.
I wonder if we should introspect type to detect any final class which doesn't override equals/hashcode from Object, solving the more general problem?
There was a problem hiding this comment.
yeah, solving the general problem would be interesting and we could test against some larger projects to assess signal/noise
...eline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java
Outdated
Show resolved
Hide resolved
…s/hashCode (#1732) Co-authored-by: Carter Kozak <ckozak@palantir.com>
carterkozak
changed the title
| + " consider using a List instead. Otherwise, use IdentityHashMap/Set," | ||
| + " or an Iterable/List of pairs.", | ||
| severity = SeverityLevel.WARNING) | ||
| public final class DangerousIdentityKey extends AbstractAsKeyOfSetOrMap { |
There was a problem hiding this comment.
Should we try to push this upstream to error-prone proper? I'm curious if there is a reason they haven't added something of this shape (though this is the type of thing drilled fairly well by Effective Java, I'd like to have 🤖 compiler assistance).
I can add a test case to confirm that this catches the construction in places like stream collectors, as that's where I noticed one example.
Also worth noting that since we're building on AbstractAsKeyOfSetOrMap it is specific to the actual constructed map/set type (not the apparent type of say Map<Pattern, String>) and analyzes the typical JDK & Guava (minus WeakHash(Map|Set)) hash based maps & sets where hashCode and equals are relevant. Not sure if we need/want a similar check for Caffeine caches & sorted/navigable map & set types. Reminds me of some places I've seen java.net.URL as keys 😢 .
There was a problem hiding this comment.
That's a good point, this should be a strong candidate for an upstream contribution, although I've never had much luck with error-prone contributions, I can reach out again.
We could extend AbstractAsKeyOfSetOrMap to include caches (guava + caffeine) as well as collectors. I can PR something here shortly, and make a similar contribution to the abstract class upstream.
| " // BUG: Diagnostic contains: does not override", | ||
| " return Stream.of(\".\").collect(", | ||
| " Collectors.toMap(Pattern::compile, s -> s));", |
There was a problem hiding this comment.
right now we don't actually catch this case
There was a problem hiding this comment.
#1733 will handle this
|
👍 |
|
Released 3.77.0 |
Before this PR
Using a
java.util.regex.Patternas aMaporSetkey could lead to unexpected behavior asPatterndoes not overrideequals&hashCode.After this PR
==COMMIT_MSG==
DangerousIdentityKey warns where a collection key type does not override equals and hashCode, so comparisons will be done on reference equality only and is likely not what one expects.
==COMMIT_MSG==
Similar to https://errorprone.info/bugpattern/ArrayAsKeyOfSetOrMap & https://errorprone.info/bugpattern/ProtosAsKeyOfSetOrMap
Possible downsides?
This is not fully exhaustive of all potential cases, but identifies most of the common JDK, Guava, Caffeine types of hash-based Map, Set, Multimap, Multiset, and cache collections.