Ensure reentrancy ordering works with spec types

[bmeck]

In particular:

map.upsert(foo, () => {
  map.delete(foo);
  return 'bar';
})

Should no-op instead of updating to 'bar'. Alternatively we could find the new entry with that key, but either way it needs to be specified. I think no-op is sensible.

To some extent as well:

map.insert(foo, undefined, () => {
  map.set(foo, 'intercepted');
  return 'bar';
})

[ljharb]

why would it update to bar?

[bmeck]

@ljharb typo, incomplete is now fixed.

[ljharb]

Why would this no-op? I'd expect the return value of the callback to be unconditionally inserted. Mutations of the map for the foo key should be wiped out by whatever the semantics of the method does.

[bmeck]

@ljharb it depends on how you think of the Map, the spec at least frames it as having a new Entry whenever you add a missing key value pair. The .delete is removing the existing Entry, so it would not update the value in the Map in a visible way. This has some implications for things like @wycats interest in making a direct Entry API. If we don't treat the delete as decoupling the Map and the Entry that means that Entry needs to track at all times if it is coupled with the Map and that obtaining an Entry could keep the Map alive, and even if the value is deleted from a Map you cant GC the Entry associated with it.

Alternatively, replacing any different/missing Entry with the one at the start of the upsert user callback. I'm not entirely sure about that, but it would be a bit harder at least to myself to think about.

I think opinions on an Entry API and how the choice here affects it is likely the most important thing.

[rbuckton]

If upsert were to be implemented in terms of has/get/set, then I would expect map.get(foo) === 'bar'. Especially if the intent is to replace this kind of boilerplate with upsert:

if (map.has(key)) {
  map.set(key, upsertfn(map.get(key)));
}

If my upsertfn in the example above mutated map, I'd still expect a subsequent map.get(key) to be the value returned by upsertfn.

[bmeck]

Some data around some reentrancy:

• A variety of languages don't use lambda's and don't have reentrancy because of it
• Rust has a borrow check error
• Java has a concurrent modification error
• Kotlin has (via getOrElse) the reentrant inserted value rather than rechecking map
• JS doesn't do checks for reentrancy, but does prevent infinite iteration:

  a=[1,2];
  // adding past length
  a[higherOrderFunctionMethod](v => {
  a.push(4);
  if (v === 4) throw Error(); // never throws, only ever sees 1, 2
  return true;
  });
  // modification in place
  a[higherOrderFunctionMethod]((v, i) => {
  a[i] = 4;
  if (v === 4) throw Error(); // never throws, only ever sees 1, 2
  return true;
  });
  // modification in iteration
  a[higherOrderFunctionMethod]((v, i) => {
  a[i + 1] = 4;
  if (v === 4) throw Error(); // throws!
  return true;
  });

So far none of the examples actually use a semantic that double checks after the function is called to ensure it matches what is currently in the collection.

I think unless there is a reason that I'm not seeing both are viable but existing semantics lean towards adding an error to concurrent modification, or keeping the reference to any original Entry rather than re-grabbing an Entry.

[bmeck]

Not going to throw, matching Array method explanation above.

Closed by https://github.com/tc39/proposal-upsert/commit/c8776c68957fdc7226446cd292ce98f826ebb125