Related to Issue 4249.

I’d like to work on this, since I had written a solution to this problem as part of my destructuring refactoring in 2017.

Since all the browsers have deviate from the spec the same way, I'd rather have the spec be changed (even though it's not quite what I'd expect); there's just less chance of any web compat problems.

I’ve said this before and been wrong about it, but “I’d be surprised if anyone depended on the exception happening after the RHS was evaluated instead of before it”

From the discussion at https://github.com/tc39/ecma262/issues/467 it sounds like tc39 have previously discussed this (or at least a very closely related issue) and decided they do not intend to change the spec.

See https://bugs.chromium.org/p/v8/issues/detail?id=4249 as well

Chaninging the spec here will result in a fundamntal discontinuity between class evaluation order and member assignment; i think it’s worth trying to change in browsers if we can.

caitp@, Well, if no one should be depending on the spec then clearly the best solution here is to change the spec to reflect web reality?

comment#6, This is the behavior in all the major engines, and has been so for a while now. I agree that the spec'd behavior is slightly better (but again this hasn't really ever come up before), but I'd encourage you to be pragmatic about this and be realistic of the options we have. As someone who has recently spent a lot of time fighting losing battles on making spec compat, but potentially non web compat changes, I'm not convinced this is worth it.

We can’t possibly know if it’s worth it until it’s tried - even if only in a nightly/canary. Can we try?

Maybe Chakra folks know if there were compat reasons for needing to change to v8/JSC's behaviours in the first place, or if they made the change for some other reason?

This is a dupe of issue 4249: for "receiver[key] = value()" assignments, we call "value()" before coercing "receiver" to object and "key" to string, which is only observable when one of the coercions fails or has other side effects.

We've thought about fixing this before, because we like being spec compliant.
The issue is that it requires either massive changes to the way the IC system works, or we'd have to add a check before every single property assignment, i.e. we'd be punishing the performance of correct code just for making sure that incorrect code throws its TypeError a little sooner.

Given that all implementations (1) are in agreement, and (2) have behaved this way for years (since forever  maybe?), I don't think it's worth the effort or the perf hit.

+1 to updating the spec to reflect reality.

It wouldn’t take big changes to the IC system.

We have a flag on the IC slot that says “do RequireObjectCoercible in the slow case after the load”, which is set for the final baseValue load of a MemberExpression chain. I think the cost of this would be pretty minimal.

In the example "receiver[key] = value()", the "value()" call is performed before the IC ever gets to see "receiver".

If I'm overlooking some simple solution here, please go ahead and show me one :-)

jkummerow: sorry, that was the “mitigate the cost” aspect, and the fact that changes needed to the IC system would be pretty minor. The rest of it is just “load the base value before the rhs is evaluated”.

yeah, so, I guess it's not as small of a change as I thought it might be :( Only doing RequireObjectCoercible in the IC `miss` case won't cover all cases :(

There are some other options, maybe: alternative version of LdaNamedProperty / LdaKeyedProperty which perform RequireObjectCoercible at the end (and don't cache the result), without touching the IC infrastructure? But that's not too different from just inlining the check in bytecode (just a little more compact).

caitp: unless I'm missing something, the issue is that for code like:

receiver()[key()] = value();

the function "value()" is called before checking whether "receiver" is null/undefined. No change to the IC system or StaKeyedProperty can possibly address that, because the generated code has the following structure:

temp_receiver = Call(receiver);  // Does "baseReference" and "baseValue" steps.
temp_key = Call(key);
temp_val = Call(value);  // This is what should not happen, per spec.
Store(temp_receiver, temp_key, temp_val);  // Does "RequireObjectCoercible" step.

So I can see two ways to fix this:

(1) Change the structure of the code we generate. The IC would have to do the "Call(receiver)" part, rather than having the result value of that call be prepared by the surrounding code before calling the IC. This sounds like a massive change to the IC system to me.

(2) Introduce some new thingie that gets inserted into the code before the RHS is evaluated:

temp_receiver = Call(receiver);
RequireObjectCoercibleIC(temp_receiver);
temp_val = ...

This would mean that *all* stores have to go through an extra check, which I consider an unacceptable performance hit for well-behaved code (after all, the difference is only observable for invalid, failing attempts to store properties).

Given the drawbacks of both of these approaches, I am strongly in favor of fixing the spec to reflect reality instead.

jkummerow, I think we're in agreement about all of this:

1) the thing I mentioned (check for result=null/undefined at end of LoadIC_Miss (and similar cases) doesn't do enough because the result of the load doesn't affect the cache (We'd have to change it to affect the cache entry for it to do anything useful there), or otherwise put the null/undefined check at the end of each handler type, which is a lot of effort, and does essentially the same thing as....

2) put the explicit RequireObjectCoercible in bytecode, not affected by caching. This is easy to do and would be much easier to maintain than some of those alternatives, but has some disadvantages: the (very cheap) null/undefined checks might be done redundantly, and the checks don't get eliminated by the the IC state (or, they're always done instead of "only on the slow path" idea described above). It's also less concise, requiring a few bytes more bytecode than the alternative

so between those options, you either make IC a bit crazier, or you make property storage a bit slower in general.

Basically, I'm with you, the downsides of this change probably outweigh the benefits by a lot, so I'm fine to not keep pursuing it until we change our minds or come up with a performant caching solution that doesn't regress other cases too badly.

See the previous discussion notes from TC39: https://github.com/tc39/tc39-notes/blob/master/es7/2016-03/march-31.md#reference-type-and-implementation-reality-double-evaluation-of-computed-property-expression

One key point here was that SpiderMonkey claimed they implemented the specification. However, it doesn't seem like SpiderMonkey's implementation matches in the test case earlier on this thread. Maybe we should revisit the decision in TC39 given this data.