Bug 209716 – [ESNext] Implement logical assignment operators

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Devin Rousso]

<https://tc39.es/proposal-logical-assignment/>

[Devin Rousso]

Created attachment 394858 [details]
[Patch] WIP

Needs tests, but otherwise works! :)

[Devin Rousso]

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

[Ross Kirsling]

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

[Ross Kirsling]

(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

[Devin Rousso]

(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 🤦‍♂️

[Devin Rousso]

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.

[Devin Rousso]

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

[Devin Rousso]

Comment on attachment 394873 [details]
Patch

oh and:

```
    let x = null;
    (function() {
        x ??= 42;
    })();
```

with a `const` version too

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 395181 [details]
Patch

[Ross Kirsling]

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?

[Devin Rousso]

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.

[Ross Kirsling]

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

[Devin Rousso]

(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 😛

[Devin Rousso]

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 😅

[Devin Rousso]

Created attachment 396009 [details]
Patch

[Devin Rousso]

Created attachment 396017 [details]
Patch

[Devin Rousso]

Created attachment 396018 [details]
Patch

[Devin Rousso]

Created attachment 396025 [details]
Patch

we nede to throw an exception if the lhs is read-only _after_ emitting the rhs

[Saam Barati]

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.

[Ross Kirsling]

(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?

[Devin Rousso]

Created attachment 396458 [details]
Patch

rebase and confirm that existing tests cover the non-local variable resolve assignment path

[Devin Rousso]

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

[Ross Kirsling]

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. 🤔

[Devin Rousso]

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 🤦‍♂️

[Devin Rousso]

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.

[Devin Rousso]

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

[Devin Rousso]

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

[Devin Rousso]

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.

[Devin Rousso]

(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

[Devin Rousso]

Created attachment 396492 [details]
Patch

[Devin Rousso]

Created attachment 396493 [details]
Patch

fix dot/bracket too

[Ross Kirsling]

Comment on attachment 396493 [details]
Patch

Woohoo! Let's land this once EWS is green.

[Ross Kirsling]

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.

[Devin Rousso]

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` :(

[EWS]

Committed r260119: <https://trac.webkit.org/changeset/260119>

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

[Radar WebKit Bug Importer]

<rdar://problem/61811886>

[Ross Kirsling]

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 ??.

[Devin Rousso]

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

[Ross Kirsling]

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

[Devin Rousso]

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

[Devin Rousso]

(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