Bug 210317

Created attachment 396038 [details] Patch

Comment on attachment 396038 [details] Patch Does this cause a test262 test to change, or maybe not one we have imported?

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()`.)

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

(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! :)

Created attachment 396098 [details] Patch

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.

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 :)

Created attachment 396121 [details] Patch

Committed r259905: <https://trac.webkit.org/changeset/259905> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396121 [details].

<rdar://problem/61605729>

(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

(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"...