search

tc39 / proposal-upsert

ECMAScript Proposal, specs, and reference implementation for Map.prototype.upsert
https://tc39.es/proposal-upsert/
MIT License
202 stars 14 forks source link

fixes #40: account for mutation of map in getOrInsertComputed callbacks #71

Open michaelficarra opened 6 days ago

michaelficarra commented 6 days ago

Fixes #40. Just checks for presence of the key again after calling the callback. @dead-claudia suggested we could have a note about a potential optimisation here, but I'll leave that to the editor group to decide upon integration.

Closes #73.

bakkot commented 6 days ago

I would strongly prefer that this throw rather than returning the value which was inserted while the callback was running. If that's not acceptable, it should return the value produced by the callback. In no circumstances should it call the callback and then silently not insert that value.

michaelficarra commented 6 days ago

@bakkot Read it again, it sets the value and returns that value, regardless of what it was set to in the interim.

bakkot commented 6 days ago

Ah, good good. Well, I'd still prefer an error.

Incidentally, Java says:

Non-concurrent implementations should override this method and, on a best-effort basis, throw a ConcurrentModificationException if it is detected that the mapping function modifies this map during computation.

michaelficarra commented 6 days ago

I'm also fine with locking and throwing. This design question should probably be posed to committee @dminor.

bakkot commented 6 days ago

Locking requires a lot more implementation work. Maybe it's easy in practice and doesn't slow down other uses, ~in which case that would be ideal~, but I was envisioning just replacing the "if the value is already present after calling the callback, then update its entry" parts of this PR with "[...], then throw".

Actually, on further thought, I am against locking even if it is easy to do because right now map.set(foo, bar) never throws, and I don't want people to have to think about the fact that it might. The place the error should originate is at the getOrInsertComputed call.

michaelficarra commented 6 days ago

I'll just open a second PR right now to make it easier for people to compare.

edit: See #73.

dminor commented 4 days ago

I've requested a larger timebox, and I'll add this issue to the slides for discussion in committee.