Core: Unimplement Map from CharSequenceMap to obey contract #11704
Conversation
This file contains 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
findepi
force-pushed
the
findepi/core-unimplement-map-from-charsequencemap-to-obey-contract-491472
branch
from
2c5fe4c to
02cb792
Compare
|
BTW Guava has a concept of |
nastra
reviewed
Dec 5, 2024
nastra
reviewed
Dec 9, 2024
findepi
force-pushed
the
findepi/core-unimplement-map-from-charsequencemap-to-obey-contract-491472
branch
from
53b446f to
1c03a14
Compare
findepi
force-pushed
the
findepi/core-unimplement-map-from-charsequencemap-to-obey-contract-491472
branch
from
1c03a14 to
fca920d
Compare
`CharSequenceMap` did not follow `Map` contract fully - `Map.keySet` is supposed to be a view, but `CharSequenceMap` returned a copy. Fixing this one can be hard given how `CharSequenceMap` is implemented internally. - `Map.equals` should compare maps by value. I.e. different map implementations with same key/value pairs are supposed to be equal. `CharSequenceMap` could certainly implement this correctly, but it does not today. The benefit of doing so is unclear and time necessary to do so correctly is non-negligible (we would need to define semantics what to when `CharSequenceMap` is compared to a map with more keys, but such that each key compared as char sequences is equal to some key in the `CharSequenceMap`). Also, having a Map implementation that does not compare the keys by equality is error-prone: - it's likely that new default Map interface methods assume equality-based comparison semantics. JDK maintains non-equality based Map implementation (Comparator-based and Identity-based), and it surely doesn't go without maintenance burden (and not without bugs either, e.g. https://bugs.openjdk.org/browse/JDK-8178355) - it's not unlikely someone tries to make a defensive copy of a map with `ImmutableMap.copyOf` or `Map.copyOf`, resulting in a map with different semantics. Instead of fixing the `CharSequenceMap` to obey the `Map` contract, it seems more pragmatic to just stop implementing the `Map` interface, which this commit does, resulting in a simple to understand class contract and behavior. This commit provides a copy of `CharSequenceMap`, `PathMap`, with `Map` interface removed. Additional positive side-effects of the change - less code and easier to understand contracts - strongly typed `get` and `containsKey` methods (the need to accept `Object` in these was forced by `Map` interface). `CharSequenceSet` has similar issues, to be addressed in a follow-up. Rename CharSequenceMap to PathMap Restore original CharSequenceMap Deprecate CharSequenceMap
findepi
force-pushed
the
findepi/core-unimplement-map-from-charsequencemap-to-obey-contract-491472
branch
from
fca920d to
6e99c84
Compare
|
@nastra PTAL. after this lands i'd like to fix the |
Sorry for the delay. I'm a bit overloaded currently with reviews, so this might me take a while to get back to. Hopefully someone else can find some time to review this |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
CharSequenceMapdid not followMapcontract fullyMap.keySetis supposed to be a view, butCharSequenceMapreturned a copy. Fixing this one can be hard given howCharSequenceMapis implemented internally.Map.equalsshould compare maps by value. I.e. different map implementations with same key/value pairs are supposed to be equal.CharSequenceMapcould certainly implement this correctly, but it does not today. The benefit of doing so is unclear and time necessary to do so correctly is non-negligible (we would need to define semantics what to whenCharSequenceMapis compared to a map with more keys, but such that each key compared as char sequences is equal to some key in theCharSequenceMap).Also, having a Map implementation that does not compare the keys by equality is error-prone:
ImmutableMap.copyOforMap.copyOf, resulting in a map with different semantics.Instead of fixing the
CharSequenceMapto obey theMapcontract, it seems more pragmatic to just stop implementing theMapinterface, which this commit does, resulting in a simple to understand class contract and behavior.This PR provides a copy of
CharSequenceMap,PathMap, withMapinterface removed.Additional positive side-effects of the change
getandcontainsKeymethods (the need to acceptObjectin these was forced byMapinterface).CharSequenceSethas similar issues, to be addressed in a follow-up.cc @anuragmantri @aokolnychyi @amogh-jahagirdar @nastra