Bug 209716

Created attachment 394858 [details] [Patch] WIP Needs tests, but otherwise works! :)

Created attachment 394873 [details] Patch I'm not sure if this is the "right" way of writing a JSC test, but I think I gave it a good try :)

Comment on attachment 394873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394873&action=review > JSTests/stress/logical-assignment-operator-and.js:21 > +shouldBe(`var x; x &&= 42`, undefined); A few comments on testing for now: - I personally don't write shouldBe with a string to eval; I think it's fine for the test to stop if we can't pass the basic tests. - You shouldn't need to test aspects of the language which don't interact with the feature -- namely, var/let/const should have no bearing on these operators, so you can just use literals directly (except when you're actually testing getters and setters later on).

(In reply to Ross Kirsling from comment #3) > - You shouldn't need to test aspects of the language which don't interact > with the feature -- namely, var/let/const should have no bearing on these > operators, so you can just use literals directly (except when you're > actually testing getters and setters later on). On the other hand, some important interactions that you do want to test are when using these operators as part of a larger subexpression like `x &&= y &&= z`, `x &&= y ||= z`, `x &&= y = z`, `x &&= y && z`, ... :D

(In reply to Ross Kirsling from comment #4) > (In reply to Ross Kirsling from comment #3) > > - You shouldn't need to test aspects of the language which don't interact with the feature -- namely, var/let/const should have no bearing on these operators, so you can just use literals directly (except when you're actually testing getters and setters later on). I agree with you with respect to `var` vs `let`, as I don't think they act any differently in any of the cases that would matter for this feature, but I do think there should be tests for `const`, given that there is logic in `ShortCircuitAssignResolveNode` specifically checking for whether the variable being assigned is read-only. There are other parts that I'm not sure how to test either, like `emitTDZCheckIfNecessary` or `leftHandSideNeedsCopy`, which I plan on finding out about this week. > On the other hand, some important interactions that you do want to test are when using these operators as part of a larger subexpression like `x &&= y &&= z`, `x &&= y ||= z`, `x &&= y = z`, `x &&= y && z`, ... :D Doh! I completely forgot about chaining assignment operators 🤦‍♂️

Comment on attachment 394873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394873&action=review > JSTests/stress/logical-assignment-operator-or.js:54 > +shouldThrow(`const x = true; x ||= 42`, TypeError); This actually shouldn't throw. Since these operators short circuit, we should only throw if when we attempt to assign to something that is readonly.

Comment on attachment 394873 [details] Patch I also think we should add tests for things like: ``` (function() { this ??= 42; return this; }).call(null); ``` ``` let x = {} Object.defineProperty(x, "a", { value: null, writable: false, }); x.a ??= 42 ``` ``` x ??= 42; let x = null; ``` as well as `"use strict";` variants for many of the existing tests :)

Comment on attachment 394873 [details] Patch oh and: ``` let x = null; (function() { x ??= 42; })(); ``` with a `const` version too

Created attachment 395021 [details] Patch Ensure that an error is thrown only when actually assigning to a read-only value. Remove some unnecessary code. Add additional tests.

Created attachment 395181 [details] Patch

Comment on attachment 395181 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395181&action=review > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2859 > +// ------------------------------ ShortCircuitAssignResolveNode ----------------------------------- I think these should be renamed ShortCircuitReadModify*Node, if they're based on ReadModify*Node and not Assign*Node. I got really confused for a bit. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2896 > + emitShortCircuitAssignment(generator, local.get(), m_operator, shortCircuitLabel.get()); nit: may as well say result.get() instead of local.get() > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2942 > + generator.emitReadOnlyExceptionIfNeeded(var); Doesn't this need a var.isReadOnly guard? > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2943 > + result = generator.emitNodeInTailPosition(result.get(), m_right); Seems like emitReadModifyAssignment does a emitExpressionInfo after emitting the node just in this one case. Specifically: generator.emitExpressionInfo(divot(), divotStart(), divotEnd()); > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2944 > + result = generator.emitPutToScope(scope.get(), var, result.get(), ThrowIfNotFound, InitializationMode::NotInitialization); Doesn't this need a !var.isReadOnly guard? > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3089 > + RefPtr<RegisterID> thisValue = generator.ensureThis(); We can't use the thisValue from above?

Comment on attachment 395181 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395181&action=review >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2859 >> +// ------------------------------ ShortCircuitAssignResolveNode ----------------------------------- > > I think these should be renamed ShortCircuitReadModify*Node, if they're based on ReadModify*Node and not Assign*Node. I got really confused for a bit. That's a good point. I'll change :) >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2942 >> + generator.emitReadOnlyExceptionIfNeeded(var); > > Doesn't this need a var.isReadOnly guard? I was thinking that this be "skipped" by the jump to `shortCircuitLabel` that's provided to `emitShortCircuitAssignment`. Is that not the case? >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2943 >> + result = generator.emitNodeInTailPosition(result.get(), m_right); > > Seems like emitReadModifyAssignment does a emitExpressionInfo after emitting the node just in this one case. Specifically: > generator.emitExpressionInfo(divot(), divotStart(), divotEnd()); Good catch! >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3089 >> + RefPtr<RegisterID> thisValue = generator.ensureThis(); > > We can't use the thisValue from above? I thought so as well, but I saw that `ReadModifyDotNode::emitBytecode` did this as well, so I did too. I wasn't sure if there was some way that the rhs could have some sort of side effect that could muck with this path. I don't think so, but I figured it'd be safer to match existing functionality than possibly be slightly different.

(In reply to Devin Rousso from comment #12) > >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2942 > >> + generator.emitReadOnlyExceptionIfNeeded(var); > > > > Doesn't this need a var.isReadOnly guard? > > I was thinking that this be "skipped" by the jump to `shortCircuitLabel` > that's provided to `emitShortCircuitAssignment`. Is that not the case? This is the whole non-local variable path, so we know that the value wasn't something to short circuit on, but not whether the variable is read-only. I believe the current code will throw when setting any global variable in strict mode? > >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3089 > >> + RefPtr<RegisterID> thisValue = generator.ensureThis(); > > > > We can't use the thisValue from above? > > I thought so as well, but I saw that `ReadModifyDotNode::emitBytecode` did > this as well, so I did too. I wasn't sure if there was some way that the > rhs could have some sort of side effect that could muck with this path. I > don't think so, but I figured it'd be safer to match existing functionality > than possibly be slightly different. Hmm, I now see that AssignBracketNode does what you're saying, but ReadModifyDotNode / ReadModifyBracketNode don't.

(In reply to Ross Kirsling from comment #13) > (In reply to Devin Rousso from comment #12) > > >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2942 > > >> + generator.emitReadOnlyExceptionIfNeeded(var); > > > > > > Doesn't this need a var.isReadOnly guard? > > > > I was thinking that this be "skipped" by the jump to `shortCircuitLabel` that's provided to `emitShortCircuitAssignment`. Is that not the case? > > This is the whole non-local variable path, so we know that the value wasn't something to short circuit on, but not whether the variable is read-only. I believe the current code will throw when setting any global variable in strict mode? That's not necessarily true though, right? The short circuiting depends on which operator is used. Something like ``` Object.defineProperty(globalThis, "foo", { value: false, writable: false, configurable: false, }); function bar() { foo &&= 42; } bar(); ``` should short circuit and _not_ assign/throw, right? Shouldn't the `ReadonlyPropertyWriteError` only be emitted/reachable if we _don't_ short circuit? > > >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3089 > > >> + RefPtr<RegisterID> thisValue = generator.ensureThis(); > > > > > > We can't use the thisValue from above? > > > > I thought so as well, but I saw that `ReadModifyDotNode::emitBytecode` did this as well, so I did too. I wasn't sure if there was some way that the rhs could have some sort of side effect that could muck with this path. I don't think so, but I figured it'd be safer to match existing functionality than possibly be slightly different. > > Hmm, I now see that AssignBracketNode does what you're saying, but ReadModifyDotNode / ReadModifyBracketNode don't. Oh! You're totally right 🤣 That'll teach me to only look at one thing 😛

Comment on attachment 395181 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395181&action=review >>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2942 >>> + generator.emitReadOnlyExceptionIfNeeded(var); >> >> Doesn't this need a var.isReadOnly guard? > > I was thinking that this be "skipped" by the jump to `shortCircuitLabel` that's provided to `emitShortCircuitAssignment`. Is that not the case? After talking with you in Slack, I get what you mean now. This will always throw in strict mode, even if the `var` is NOT read-only. We should wrap this in an `isReadOnly` guard. I was under the assumption/misunderstanding that `BytecodeGenerator::emitReadOnlyExceptionIfNeeded` checked that for me 😅

Created attachment 396009 [details] Patch

Created attachment 396017 [details] Patch

Created attachment 396018 [details] Patch

Created attachment 396025 [details] Patch we nede to throw an exception if the lhs is read-only _after_ emitting the rhs

Comment on attachment 396025 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396025&action=review > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2861 > +// FIXME: should this be moved to be a method on BytecodeGenerator? This seems fine in here > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2895 > + Ref<Label> shortCircuitLabel = generator.newLabel(); Nit: I’d name this something like “skipAssignment”. Also, I tend to name things in without including the type name in variable name.

(In reply to Saam Barati from comment #20) > Nit: I’d name this something like “skipAssignment”. Also, I tend to name > things in without including the type name in variable name. Or maybe `afterAssignment` so that it names a location instead of an action?

Created attachment 396458 [details] Patch rebase and confirm that existing tests cover the non-local variable resolve assignment path

Created attachment 396466 [details] Patch - only `emitExpressionInfo` in `ShortCircuitReadModifyResolveNode` if the non-local is not readonly (or we are not in strict mode) - use `emitNode` instead of `emitNodeInTailPosition` as technically the entire assignment is in tail position, not just the rhs (it's equivalent to `a && (a = b)`, NOT `a && (a = a && b)`)

Comment on attachment 396466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396466&action=review > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2961 > + if (!isReadOnly) { > + result = generator.emitPutToScope(scope.get(), var, result.get(), ThrowIfNotFound, InitializationMode::NotInitialization); > + generator.emitProfileType(result.get(), var, divotStart(), divotEnd()); > + } > + > + generator.emitLabel(afterAssignment.get()); > + return generator.move(dst, result.get()); Hmm, but now we don't emitProfileType in the short-circuit case, which is different from the local() paths. 🤔

Comment on attachment 396466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396466&action=review >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2961 >> + return generator.move(dst, result.get()); > > Hmm, but now we don't emitProfileType in the short-circuit case, which is different from the local() paths. 🤔 Gah, i forgot 🤦‍♂️

Created attachment 396486 [details] Patch After talking with @Ross Kirsling a bit more offline, it looks like the logic we have is correct. A simple test is to compare the dumped bytecodes for `a ??= b` and `a ?? (a = b)`. >>> x ?? (x = 42) [ 0] enter [ 1] get_scope loc4 [ 3] mov loc5, loc4 [ 6] check_traps [ 7] mov loc6, Undefined(const0) [ 10] mov loc6, Undefined(const0) [ 13] resolve_scope loc6, loc4, 0, GlobalProperty, 0 [ 20] get_from_scope loc7, loc6, 0, 2048<ThrowIfNotFound|GlobalProperty|NotInitialization|NotStrictMode>, 0, 0 [ 28] mov loc6, loc7 [ 31] profile_type loc6, 0, ProfileTypeBytecodeClosureVar, 0, GlobalProperty [ 38] jnundefined_or_null loc6, 28(->66) [ 41] resolve_scope loc7, loc4, 0, GlobalProperty, 0 [ 48] mov loc6, Int32: 42(const1) [ 51] put_to_scope loc7, 0, loc6, 1050624<DoNotThrowIfNotFound|GlobalProperty|NotInitialization|NotStrictMode>, 0, 0 [ 59] profile_type loc6, 0, ProfileTypeBytecodeClosureVar, 0, GlobalProperty [ 66] end loc6 >>> x ??= 42 [ 0] enter [ 1] get_scope loc4 [ 3] mov loc5, loc4 [ 6] check_traps [ 7] mov loc6, Undefined(const0) [ 10] mov loc6, Undefined(const0) [ 13] resolve_scope loc7, loc4, 0, GlobalProperty, 0 [ 20] get_from_scope loc6, loc7, 0, 2048<ThrowIfNotFound|GlobalProperty|NotInitialization|NotStrictMode>, 0, 0 [ 28] jnundefined_or_null loc6, 21(->49) [ 31] mov loc6, Int32: 42(const1) [ 34] put_to_scope loc7, 0, loc6, 2048<ThrowIfNotFound|GlobalProperty|NotInitialization|NotStrictMode>, 0, 0 [ 42] profile_type loc6, 0, ProfileTypeBytecodeClosureVar, 0, GlobalProperty [ 49] end loc6 As we can see, we emit the same things other than - the `mov` (28) and `profile_type` (31) from the lhs `x` in `x ?? (x = 42)` - the second `resolve_scope` (41) from the rhs `x` in `x ?? (x = 42)` both of which are expected and a benefit of the short circuiting.

I just did a bit more testing to verify things are as we expect: $ (function() { const x = null; function capture() { return x; } x ??= 42 })() [ 0] enter [ 1] get_scope loc4 [ 3] mov loc5, loc4 [ 6] check_traps [ 7] create_lexical_environment loc7, loc4, Cell: 0x10cfbda08 (0x10d1f8460:[0x4fa9, SymbolTable, {}, NonArray, Leaf]), StructureID: 20393(const1), <JSValue()>(const2) [ 12] mov loc4, loc7 [ 15] new_func loc8, loc4, 0 [ 19] mov loc6, loc8 [ 22] put_to_scope loc7, 0, Null(const3), 1049604<DoNotThrowIfNotFound|LocalClosureVar|ConstInitialization|NotStrictMode>, 1, 0 [ 30] profile_type Null(const3), 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 37] get_from_scope loc8, loc7, 0, 2052<ThrowIfNotFound|LocalClosureVar|NotInitialization|NotStrictMode>, 1, 0 [ 45] jnundefined_or_null loc8, 9(->54) [ 48] mov loc8, Int32: 42(const4) [ 51] throw_static_error String (atomic),8Bit:(1),length:(41): Attempted to assign to readonly property., StructureID: 16209(const5), TypeError [ 54] profile_type Undefined(const6), 0, ProfileTypeBytecodeFunctionReturnStatement, 0, GlobalProperty [ 61] ret Undefined(const6) vs $ (function() { const x = null; function capture() { return x; } x ?? (x = 42) })() [ 0] enter [ 1] get_scope loc4 [ 3] mov loc5, loc4 [ 6] check_traps [ 7] create_lexical_environment loc7, loc4, Cell: 0x1056bda08 (0x1058f8460:[0xdbc6, SymbolTable, {}, NonArray, Leaf]), StructureID: 56262(const1), <JSValue()>(const2) [ 12] mov loc4, loc7 [ 15] new_func loc8, loc4, 0 [ 19] mov loc6, loc8 [ 22] put_to_scope loc7, 0, Null(const3), 1049604<DoNotThrowIfNotFound|LocalClosureVar|ConstInitialization|NotStrictMode>, 1, 0 [ 30] profile_type Null(const3), 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 37] get_from_scope loc9, loc7, 0, 2052<ThrowIfNotFound|LocalClosureVar|NotInitialization|NotStrictMode>, 1, 0 [ 45] mov loc8, loc9 [ 48] profile_type loc8, 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 55] jnundefined_or_null loc8, 9(->64) [ 58] mov loc8, Int32: 42(const4) [ 61] throw_static_error String (atomic),8Bit:(1),length:(41): Attempted to assign to readonly property., StructureID: 52815(const5), TypeError [ 64] profile_type Undefined(const6), 0, ProfileTypeBytecodeFunctionReturnStatement, 0, GlobalProperty [ 71] ret Undefined(const6) ---------- $ (function() { const x = null; x ??= 42 })() [ 0] enter [ 1] get_scope loc4 [ 3] mov loc5, loc4 [ 6] check_traps [ 7] mov loc6, <JSValue()>(const2) [ 10] mov loc6, Null(const3) [ 13] profile_type loc6, 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 20] jnundefined_or_null loc6, 9(->29) [ 23] mov loc6, Int32: 42(const4) [ 26] throw_static_error String (atomic),8Bit:(1),length:(41): Attempted to assign to readonly property., StructureID: 3307(const5), TypeError [ 29] profile_type loc6, 0, ProfileTypeBytecodeDoesNotHaveGlobalID, 0, GlobalProperty [ 36] profile_type Undefined(const6), 0, ProfileTypeBytecodeFunctionReturnStatement, 0, GlobalProperty [ 43] ret Undefined(const6) vs $ (function() { const x = null; x ?? (x = 42) })() [ 0] enter [ 1] get_scope loc4 [ 3] mov loc5, loc4 [ 6] check_traps [ 7] mov loc6, <JSValue()>(const2) [ 10] mov loc6, Null(const3) [ 13] profile_type loc6, 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 20] profile_type loc6, 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 27] mov loc7, loc6 [ 30] jnundefined_or_null loc7, 16(->46) [ 33] mov loc7, Int32: 42(const4) [ 36] throw_static_error String (atomic),8Bit:(1),length:(41): Attempted to assign to readonly property., StructureID: 10823(const5), TypeError [ 39] profile_type loc7, 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 46] profile_type Undefined(const6), 0, ProfileTypeBytecodeFunctionReturnStatement, 0, GlobalProperty [ 53] ret Undefined(const6) so it looks like things are being emitted as we want :)

Here's some examples with `let` instead of `const` to hit the non-`isReadOnly` path $ (function() { let x = null; function capture() { return x; } x ??= 42 })() [ 0] enter [ 1] get_scope loc4 [ 3] mov loc5, loc4 [ 6] check_traps [ 7] create_lexical_environment loc7, loc4, Cell: 0x10dcbda08 (0x10def8460:[0x9b9e, SymbolTable, {}, NonArray, Leaf]), StructureID: 39838(const1), <JSValue()>(const2) [ 12] mov loc4, loc7 [ 15] new_func loc8, loc4, 0 [ 19] mov loc6, loc8 [ 22] put_to_scope loc7, 0, Null(const3), 1048580<DoNotThrowIfNotFound|LocalClosureVar|Initialization|NotStrictMode>, 1, 0 [ 30] profile_type Null(const3), 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 37] get_from_scope loc8, loc7, 0, 2052<ThrowIfNotFound|LocalClosureVar|NotInitialization|NotStrictMode>, 1, 0 [ 45] jnundefined_or_null loc8, 21(->66) [ 48] mov loc8, Int32: 42(const4) [ 51] put_to_scope loc7, 0, loc8, 2052<ThrowIfNotFound|LocalClosureVar|NotInitialization|NotStrictMode>, 1, 0 [ 59] profile_type loc8, 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 66] profile_type Undefined(const5), 0, ProfileTypeBytecodeFunctionReturnStatement, 0, GlobalProperty [ 73] ret Undefined(const5) vs $ (function() { let x = null; function capture() { return x; } x ?? (x = 42) })() [ 0] enter [ 1] get_scope loc4 [ 3] mov loc5, loc4 [ 6] check_traps [ 7] create_lexical_environment loc7, loc4, Cell: 0x1128bda08 (0x112af8460:[0xe4a, SymbolTable, {}, NonArray, Leaf]), StructureID: 3658(const1), <JSValue()>(const2) [ 12] mov loc4, loc7 [ 15] new_func loc8, loc4, 0 [ 19] mov loc6, loc8 [ 22] put_to_scope loc7, 0, Null(const3), 1048580<DoNotThrowIfNotFound|LocalClosureVar|Initialization|NotStrictMode>, 1, 0 [ 30] profile_type Null(const3), 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 37] get_from_scope loc9, loc7, 0, 2052<ThrowIfNotFound|LocalClosureVar|NotInitialization|NotStrictMode>, 1, 0 [ 45] mov loc8, loc9 [ 48] profile_type loc8, 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 55] jnundefined_or_null loc8, 21(->76) [ 58] mov loc8, Int32: 42(const4) [ 61] put_to_scope loc7, 0, loc8, 1050628<DoNotThrowIfNotFound|LocalClosureVar|NotInitialization|NotStrictMode>, 1, 0 [ 69] profile_type loc8, 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 76] profile_type Undefined(const5), 0, ProfileTypeBytecodeFunctionReturnStatement, 0, GlobalProperty [ 83] ret Undefined(const5) ---------- $ (function() { let x = null; x ??= 42 })() [ 0] enter [ 1] get_scope loc4 [ 3] mov loc5, loc4 [ 6] check_traps [ 7] mov loc6, <JSValue()>(const2) [ 10] mov loc6, Null(const3) [ 13] profile_type loc6, 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 20] jnundefined_or_null loc6, 13(->33) [ 23] mov loc6, Int32: 42(const4) [ 26] profile_type loc6, 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 33] profile_type Undefined(const5), 0, ProfileTypeBytecodeFunctionReturnStatement, 0, GlobalProperty [ 40] ret Undefined(const5) vs $ (function() { let x = null; x ?? (x = 42) })() [ 0] enter [ 1] get_scope loc4 [ 3] mov loc5, loc4 [ 6] check_traps [ 7] mov loc6, <JSValue()>(const2) [ 10] mov loc6, Null(const3) [ 13] profile_type loc6, 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 20] profile_type loc6, 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 27] mov loc7, loc6 [ 30] jnundefined_or_null loc7, 16(->46) [ 33] mov loc6, Int32: 42(const4) [ 36] profile_type loc6, 1, ProfileTypeBytecodeLocallyResolved, 0, GlobalProperty [ 43] mov loc7, loc6 [ 46] profile_type Undefined(const5), 0, ProfileTypeBytecodeFunctionReturnStatement, 0, GlobalProperty [ 53] ret Undefined(const5) so we're also good here too :)

Oh i should've mentioned, I did change the code ever so slightly to move the `emitProfileType` to not be emitted if a local variable is short circuited.

(In reply to Devin Rousso from comment #27) > $ (function() { const x = null; x ??= 42 })() > [ 0] enter > [ 1] get_scope loc4 > [ 3] mov loc5, loc4 > [ 6] check_traps > [ 7] mov loc6, <JSValue()>(const2) > [ 10] mov loc6, Null(const3) > [ 13] profile_type loc6, 1, ProfileTypeBytecodeLocallyResolved, 0, > GlobalProperty > [ 20] jnundefined_or_null loc6, 9(->29) > [ 23] mov loc6, Int32: 42(const4) > [ 26] throw_static_error String (atomic),8Bit:(1),length:(41): Attempted to > assign to readonly property., StructureID: 3307(const5), TypeError > [ 29] profile_type loc6, 0, ProfileTypeBytecodeDoesNotHaveGlobalID, > 0, GlobalProperty > [ 36] profile_type Undefined(const6), 0, > ProfileTypeBytecodeFunctionReturnStatement, 0, GlobalProperty > [ 43] ret Undefined(const6) oops, looks like I forgot one additional spot

Created attachment 396492 [details] Patch

Created attachment 396493 [details] Patch fix dot/bracket too

Comment on attachment 396493 [details] Patch Woohoo! Let's land this once EWS is green.

Comment on attachment 396493 [details] Patch Oh whoa, a whole results/ directory ended up in the latest patch, it seems. Marking cq- just to be safe.

Created attachment 396500 [details] Patch (In reply to Ross Kirsling from comment #34) > Comment on attachment 396493 [details] > Patch > > Oh whoa, a whole results/ directory ended up in the latest patch, it seems. Marking cq- just to be safe. Oops. Probably some accidental addition from `run-jsc-stress-tests` :(

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

<rdar://problem/61811886>

Comment on attachment 396500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396500&action=review > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2870 > + case Operator::NullishEq: Wait, I missed that this got updated to NullishEq/NULLISHEQUAL somewhere along the line—can you change it back to Coalesce in a follow-up? ?? and ?. together are the nullish operators, "coalesce" is really the word that's unique to ??.

Comment on attachment 396500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396500&action=review >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2870 >> + case Operator::NullishEq: > > Wait, I missed that this got updated to NullishEq/NULLISHEQUAL somewhere along the line—can you change it back to Coalesce in a follow-up? > ?? and ?. together are the nullish operators, "coalesce" is really the word that's unique to ??. I changed it to "nullish" to match the filename of the tests in test262. Personally, I find "nullish" to be much clearer than "coalesce" (which I don't think is a good word for any of this), and as you said they're all related to "nullish", so this would be a new "nullish equal" among the already existing "nullish chain" and "nullish or".

(In reply to Devin Rousso from comment #39) > Comment on attachment 396500 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396500&action=review > > >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2870 > >> + case Operator::NullishEq: > > > > Wait, I missed that this got updated to NullishEq/NULLISHEQUAL somewhere along the line—can you change it back to Coalesce in a follow-up? > > ?? and ?. together are the nullish operators, "coalesce" is really the word that's unique to ??. > > I changed it to "nullish" to match the filename of the tests in test262. > Personally, I find "nullish" to be much clearer than "coalesce" (which I > don't think is a good word for any of this), and as you said they're all > related to "nullish", so this would be a new "nullish equal" among the > already existing "nullish chain" and "nullish or". Many test262 filenames are quite odd, so I wouldn't worry about that part... ?? forms a CoalesceExpression in the spec (https://tc39.es/ecma262/#prod-CoalesceExpression) and this is the equals version of that, so it's quite important that the two match. Since "logical" means "truthy", "logical and operator" and "nullish coalesce operator" are in direct alignment in their naming. We don't refer to just one of the &&, ||, ! trio as "the" logical operator. :)

(In reply to Ross Kirsling from comment #40) > (In reply to Devin Rousso from comment #39) > > Comment on attachment 396500 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=396500&action=review > > > > >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2870 > > >> + case Operator::NullishEq: > > > > > > Wait, I missed that this got updated to NullishEq/NULLISHEQUAL somewhere along the line—can you change it back to Coalesce in a follow-up? > > > ?? and ?. together are the nullish operators, "coalesce" is really the word that's unique to ??. > > > > I changed it to "nullish" to match the filename of the tests in test262. Personally, I find "nullish" to be much clearer than "coalesce" (which I don't think is a good word for any of this), and as you said they're all related to "nullish", so this would be a new "nullish equal" among the already existing "nullish chain" and "nullish or". > > Many test262 filenames are quite odd, so I wouldn't worry about that part... ?? forms a CoalesceExpression in the spec (https://tc39.es/ecma262/#prod-CoalesceExpression) and this is the equals version of that, so it's quite important that the two match. > > Since "logical" means "truthy", "logical and operator" and "nullish coalesce operator" are in direct alignment in their naming. We don't refer to just one of the &&, ||, ! trio as "the" logical operator. :) Oh! I was not aware that it was also called that in the spec. I'll change it back :) Sorry for the trouble.

(In reply to Devin Rousso from comment #41) > (In reply to Ross Kirsling from comment #40) > > (In reply to Devin Rousso from comment #39) > > > Comment on attachment 396500 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=396500&action=review > > > > > > >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2870 > > > >> + case Operator::NullishEq: > > > > > > > > Wait, I missed that this got updated to NullishEq/NULLISHEQUAL somewhere along the line—can you change it back to Coalesce in a follow-up? > > > > ?? and ?. together are the nullish operators, "coalesce" is really the word that's unique to ??. > > > > > > I changed it to "nullish" to match the filename of the tests in test262. Personally, I find "nullish" to be much clearer than "coalesce" (which I don't think is a good word for any of this), and as you said they're all related to "nullish", so this would be a new "nullish equal" among the already existing "nullish chain" and "nullish or". > > > > Many test262 filenames are quite odd, so I wouldn't worry about that part... ?? forms a CoalesceExpression in the spec (https://tc39.es/ecma262/#prod-CoalesceExpression) and this is the equals version of that, so it's quite important that the two match. > > > > Since "logical" means "truthy", "logical and operator" and "nullish coalesce operator" are in direct alignment in their naming. We don't refer to just one of the &&, ||, ! trio as "the" logical operator. :) > > Oh! I was not aware that it was also called that in the spec. I'll change it back :) > > Sorry for the trouble. <https://webkit.org/b/210663> Rename NullishEq / NULLISHEQUAL to CoalesceEq / COALESCEEQUAL to match the spec