fix: set.map produces valid underlying map #1606
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
src/Set.js
Outdated
| @@ -200,7 +206,7 @@ function updateSet(set, newMap) { | |||
| set._map = newMap; | |||
| return set; | |||
| } | |||
| return newMap === set._map | |||
| return newMap.equals(set._map) | |||
There was a problem hiding this comment.
For performance reasons, it's important that this is a cheap reference equality check. Specifically, we expect newMap to be a new reference if it has been altered
There was a problem hiding this comment.
Gotcha. I'll see what I can do to maintain the same reference in the map. I did notice that map and mapEntries have different code paths. Is there a recommendation on how I should approach this?
There was a problem hiding this comment.
I tested two approaches:
- doing
mapEntriesand keeping track if there was any alterations and onlyupdateSetif changed - using
withMutationsandremove/addwhen there is a change
The latter was faster in some micro-benchmarks so I've opted for that solution.
src/Set.js
Outdated
| this.forEach(value => { | ||
| const mapped = mapper.call(context, value, value, set); | ||
| if (mapped !== value) { | ||
| set.remove(value); |
There was a problem hiding this comment.
I think this is buggy. For example, if given the set {1, 2, 3} and applying .map(v => v + 1) (your test case), it looks like it results in the set {4} instead of the set {2, 3, 4}. That's because it does these operations: remove 1, add 2, remove 2, add 3, remove 3, add 4.
Maybe one alternate approach is to collect remove and add operations (accounting for no-op if the mapper didn't do anything) and apply all removes before all adds.
There was a problem hiding this comment.
Great point! Adjusted the tests to capture this point and updated the code per your recommendation.
src/Set.js
Outdated
| const removes = []; | ||
| const adds = []; | ||
| this.forEach(value => { | ||
| const mapped = mapper.call(context, value, value, set); |
There was a problem hiding this comment.
The last value here should be this - the original immutable map instead of the transiently mutable one.
Fixes #1604
As explained in issue,
set.mapwould produce a set whose underlying_maphad mismatched key-value pairs (expected to always be equal for sets). The original code only mapped the value and kept the original set's keys which causedhaschecks (which uses the_mapkeys) to be incorrect.mapEntrieson the underlying_mapupdateSetusesequalsinstead of===sincemapEntriesproduces a different map reference