Bug 210317 – The rhs in `ReadModifyResolveNode` should be evaluated before throwing an exception if the lhs is read-only

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Devin Rousso]

```
(function() {
    let count = 0;

    const x = 42;
    try {
        x += (() => ++count)();
    } catch { }

    if (count !== 1)
        throw new Error(`expected 1 but got ${count}`);
})();
```

See NOTE 2 on <https://tc39.es/proposal-logical-assignment/#sec-assignment-operators-runtime-semantics-evaluation>.

[Devin Rousso]

Created attachment 396038 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 396038 [details]
Patch

Does this cause a test262 test to change, or maybe not one we have imported?

[Ross Kirsling]

Comment on attachment 396038 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396038&action=review

> JSTests/stress/const-read-modify-assignment-eval-rhs-before-exception.js:24
> +    try {
> +        testConstReadModifyAssignmentRHSEvalBeforeException += (() => ++count)();
> +    } catch { }

I'm a bit worried about the fact that this test doesn't fail before your patch.
I spent a good while playing and I'm honestly not sure how to hit the `!var.local() && var.isReadOnly()` path in ReadModifyResolveNode (or AssignResolveNode).

(Tried top-level const, non-configurable non-writable global var, freezing the global object and then trying to set a global var in strict mode...these all lack `var.local()`, but none satisfy `var.isReadOnly()`.)

[Ross Kirsling]

Alright, so here is a test that hits the non-local read-only path, fails before your patch, and succeeds with it:

(function () {
  let count = 0;

  const x = 42;
  function captureX() { return x; }

  try {
    x -= (() => count++)();
  } catch {
    if (count === 1)
      return;
  }
  throw new Error('failed!');
})();


I...actually don't understand why. I just adapted it from here:
https://github.com/WebKit/webkit/blob/master/JSTests/stress/const-semantics.js#L125-L129

[Devin Rousso]

(In reply to Ross Kirsling from comment #4)
> Alright, so here is a test that hits the non-local read-only path, fails before your patch, and succeeds with it:
> 
> (function () {
>   let count = 0;
> 
>   const x = 42;
>   function captureX() { return x; }
> 
>   try {
>     x -= (() => count++)();
>   } catch {
>     if (count === 1)
>       return;
>   }
>   throw new Error('failed!');
> })();
> 
> 
> I...actually don't understand why. I just adapted it from here: https://github.com/WebKit/webkit/blob/master/JSTests/stress/const-semantics.js#L125-L129

(o.0) funky stuff :P

Thanks for the help! :)

[Devin Rousso]

Created attachment 396098 [details]
Patch

[Ross Kirsling]

Comment on attachment 396098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396098&action=review

r=me

> JSTests/stress/const-read-modify-assignment-eval-rhs-before-exception.js:27
> +Object.defineProperty(globalThis, "testConstReadModifyAssignmentRHSEvalBeforeException", {

Hmm, if you don't remove this test you should probably at least change the property name, since it's a `!var.local() && !var.isReadOnly()` red herring.

[Devin Rousso]

Comment on attachment 396098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396098&action=review

>> JSTests/stress/const-read-modify-assignment-eval-rhs-before-exception.js:27
>> +Object.defineProperty(globalThis, "testConstReadModifyAssignmentRHSEvalBeforeException", {
> 
> Hmm, if you don't remove this test you should probably at least change the property name, since it's a `!var.local() && !var.isReadOnly()` red herring.

I'll just remove it :)

[Devin Rousso]

Created attachment 396121 [details]
Patch

[EWS]

Committed r259905: <https://trac.webkit.org/changeset/259905>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396121 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61605729>

[Saam Barati]

(In reply to Ross Kirsling from comment #4)
> Alright, so here is a test that hits the non-local read-only path, fails
> before your patch, and succeeds with it:
> 
> (function () {
>   let count = 0;
> 
>   const x = 42;
>   function captureX() { return x; }
> 
>   try {
>     x -= (() => count++)();
>   } catch {
>     if (count === 1)
>       return;
>   }
>   throw new Error('failed!');
> })();
> 
> 
> I...actually don't understand why. I just adapted it from here:
> https://github.com/WebKit/webkit/blob/master/JSTests/stress/const-semantics.
> js#L125-L129

This makes sense. !!.local() means it wasn’t captured. So it lives in a bytecode local for this function that’s being compiled. If .local() is false, it’s captured

[Ross Kirsling]

(In reply to Saam Barati from comment #12)
> This makes sense. !!.local() means it wasn’t captured. So it lives in a
> bytecode local for this function that’s being compiled. If .local() is
> false, it’s captured

Ahh, that's clear then, thanks! My brain was stuck on thinking about "local vs. global"...