Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DangerousIdentityKey identifies unsafe map and set keys #1731

Merged
merged 7 commits into from
Apr 17, 2021

Conversation

schlosna
Copy link
Contributor

@schlosna schlosna commented Apr 15, 2021

Before this PR

Using a java.util.regex.Pattern as a Map or Set key could lead to unexpected behavior as Pattern does not override equals & 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.

@changelog-app
Copy link

changelog-app bot commented Apr 15, 2021

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

PatternAsKeyOfSetOrMap identifies regex Pattern used as map or set key

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from ferozco April 15, 2021 18:52
Copy link
Contributor

@carterkozak carterkozak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question about whether we want to detect the general problem, no worries if we'd rather punt that for another time.


@Override
protected boolean isBadType(Type type, VisitorState state) {
return ASTHelpers.isSubtype(type, state.getTypeFromString("java.util.regex.Pattern"), state);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, solving the general problem would be interesting and we could test against some larger projects to assess signal/noise

schlosna and others added 2 commits April 15, 2021 15:30
…s/hashCode (#1732)

Co-authored-by: Carter Kozak <ckozak@palantir.com>
@carterkozak carterkozak changed the title PatternAsKeyOfSetOrMap identifies regex Pattern used as map or set key DangerousIdentityKey identifies unsafe map and set keys Apr 15, 2021
+ " consider using a List instead. Otherwise, use IdentityHashMap/Set,"
+ " or an Iterable/List of pairs.",
severity = SeverityLevel.WARNING)
public final class DangerousIdentityKey extends AbstractAsKeyOfSetOrMap {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😢 .

https://github.com/google/error-prone/blob/c601758e81723a8efc4671726b8363be7a306dce/core/src/main/java/com/google/errorprone/bugpatterns/AbstractAsKeyOfSetOrMap.java#L43-L91

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 217 to 219
" // BUG: Diagnostic contains: does not override",
" return Stream.of(\".\").collect(",
" Collectors.toMap(Pattern::compile, s -> s));",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right now we don't actually catch this case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1733 will handle this

@schlosna
Copy link
Contributor Author

👍

@bulldozer-bot bulldozer-bot bot merged commit 7667a35 into develop Apr 17, 2021
@bulldozer-bot bulldozer-bot bot deleted the ds/Pattern-key branch April 17, 2021 01:35
@svc-autorelease
Copy link
Collaborator

Released 3.77.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants