Make Key record defensively copy its array#154
Open
adamw7 wants to merge 1 commit into
Open
Conversation
record Key(String[] values) exposed its internal array both on the way in (caller keeps a reference) and on the way out (the synthesized accessor returns the internal reference). Mutating the array after construction changes Arrays.hashCode(values) and Arrays.equals(values, other.values) — which silently corrupts any Set<Key> or Map<Key, ?> the record was stored in. Add a compact constructor and accessor override that clone the array. Also tighten equals to use instanceof pattern matching (equivalent to the previous class-check path).
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.
Summary
record Key(String[] values)let the caller keep a reference to the array passed into the constructor, and the synthesized accessor handed back the internal reference. Becauseequals/hashCodeare computed from that array viaArrays.equals/Arrays.hashCode, any later mutation of the caller's or returned array silently corruptsKeyinstances stored in aSetor used as aMapkey.Add a compact constructor and an accessor override that clone the array so the record is effectively immutable.
Also switch
equalstoinstanceofpattern-matching — same behaviour, less boilerplate.Test plan
mvn -pl data testpasses (UniquenessCheckTestexercisesKeyindirectly)Keyin aHashSet, mutate the original array, verifyset.contains(key)still returns true