Implement Nullish Coalescing operator #6545

rhuanjl · 2020-12-19T11:55:35Z

This PR takes work begun by @boingoing in #6254 and completes it to implement the stage 4 tc39 proposal for a nullish coalescing operator.

Proposal repository: https://github.com/tc39/proposal-nullish-coalescing
The implementations passes most relevant test262 test cases - exception being ones also involving tail call optimisations which is a separate JS feature that CC does not support.

A switch ESNullishCoalescingOperator is included but set to true by default.

The basic logic of this implementation is fairly simple - though one key complexity is that a ?? b || c is not permitted without brackets around one of the two operations, this required some extra tracking in the parser.

Within the bytecode emitter the noalish coalescer is de-sugered so that:
result = node1 ?? node2;
becomes

result = node1;
if (result == null) {
  result = node2;
}

@ppenzin @Fly-Style please could one of you take a look?

This operator is similar to the logical or operator (`||`) but is meant to avoid unintended behavior which might arise when using logical or. Problem with logical or is that the left argument is coerced to a boolean which means that if it is something falsy (like `0`, `''`, or `false`) then the right argument will be used instead. The coalescing operator (`??`) is designed to solve this problem. It only assigns the right value when the left value is `null` or `undefined`. ```javascript null || 'something'; // 'something' undefined || 'something'; // 'something' '' || 'something'; // 'something' 0 || 'something'; // 'something' false || 'something'; // 'something' null ?? 'something'; // 'something' undefined ?? 'something'; // 'something' '' ?? 'something'; // '' 0 ?? 'something'; // 0 false ?? 'something'; // false ```

rhuanjl · 2020-12-19T22:05:00Z

lib/Parser/Parse.cpp

@@ -279,6 +279,7 @@ LPCWSTR Parser::GetTokenString(tokens token)
    case tkColon: return _u(":");
    case tkLogOr: return _u("||");
    case tkLogAnd: return _u("&&");
+    case tkCoalesce: return _u("??");


This switch converts parser tokens back into strings for error messages.

rhuanjl · 2020-12-19T22:05:44Z

lib/Parser/Parse.cpp

@@ -3237,7 +3238,8 @@ ParseNodePtr Parser::ParseTerm(BOOL fAllowCall,
    _Out_opt_ BOOL* pfCanAssign /*= nullptr*/,
    _Inout_opt_ BOOL* pfLikelyPattern /*= nullptr*/,
    _Out_opt_ bool* pfIsDotOrIndex /*= nullptr*/,
-    _Inout_opt_ charcount_t *plastRParen /*= nullptr*/)
+    _Inout_opt_ charcount_t *plastRParen /*= nullptr*/,
+    _Out_opt_ bool* looseCoalesce /*= nullptr*/)


The looseCoalesce out parameter enables tracking of unbracketed coalescing operators for the syntax error when they are combined with ?? or ||.

rhuanjl · 2020-12-19T22:07:03Z

lib/Parser/Parse.cpp

+
+            if (nop == knopLogAnd || nop == knopLogOr)
+            {
+                if (localCoalesce || coalescing)


Here's the check for the syntax error - localCoalesce will be true if the left hand side of this operation is an unbracketed ??, coalescing will be true if the right hand side is.

Aware the variable names aren't the best - open to alternatives.

rhuanjl · 2020-12-19T22:07:40Z

lib/Parser/Parse.cpp

+                localCoalesce = true;
+                if (looseCoalesce != nullptr)
+                {
+                    *looseCoalesce = true;


Here we mark that the current operation is a coalescing operation to feed up into the above check at a different level.

rhuanjl · 2020-12-19T22:08:29Z

lib/Parser/Parse.cpp

@@ -3440,6 +3442,11 @@ ParseNodePtr Parser::ParseTerm(BOOL fAllowCall,

        ChkCurTok(tkRParen, ERRnoRparen);

+        if (looseCoalesce != nullptr)
+        {
+            *looseCoalesce = false;


The last passed statement was wrapped in brackets - therefore it is not a "loose" coalesce.

rhuanjl · 2020-12-19T22:09:34Z

lib/Parser/perrors.h

+LSC_ERROR_MSG(1075, ERRInvalidAssignmentTarget, "Invalid destructuring assignment target")
+LSC_ERROR_MSG(1076, ERRFormalSame, "Duplicate formal parameter names not allowed in this context")
+LSC_ERROR_MSG(1077, ERRDestructNotInit, "Destructuring declarations cannot have an initializer")
+LSC_ERROR_MSG(1078, ERRCoalesce, "Coalescing operator '\?\?' not permitted unparenthesized inside '||' or '&&' expressions")


New error message for the new syntax error. The changes above in this file are just white space.

rhuanjl · 2020-12-19T22:11:25Z

lib/Runtime/ByteCode/ByteCodeEmitter.cpp

+    // In that case the right hand side is not evaluated
+    // If the left hand side is null or undefined it resolves to the right hand side
+    // PTNODE(knopCoalesce     , "??"        ,None    ,Bin  ,fnopBin)
+    case knopCoalesce:


This case in the bytecode emitter is the run time implementation - the command is fully de-sugared here, so no new opcodes were needed.

fatcerberus · 2020-12-19T22:40:56Z

Word of warning that per the DOM spec, document.all is falsy but is expected to be treated as truthy by operators like ?? - Edge doesn’t use Chakra anymore so this is less important but something to keep in mind since I don’t think embedders can emulate this behavior in userland.

rhuanjl · 2020-12-19T22:50:40Z

Word of warning that per the DOM spec, document.all is falsy but is expected to be treated as truthy by operators like ?? - Edge doesn’t use Chakra anymore so this is less important but something to keep in mind since I don’t think embedders can emulate this behavior in userland.

Falsely shouldn't be a problem - only if tests with == null if it does that's bizarre BUT I've implemented what the spec says and this isn't currently used in a browser anyway so I think we just roll with it.

fatcerberus · 2020-12-19T23:55:40Z

Yeah, oddly enough document.all == null is actually true even though it’s an object, verified in Chrome console. Apparently it’s some compatibility baggage from ancient Internet Explorer.

I don’t think it’s a major issue in practice, like you said. Just wanted to state it for the record since embedders have no way to emulate this behavior.

ppenzin · 2020-12-20T06:18:27Z

Doing the usual clopenning to clear persistent CI failure.

rhuanjl · 2020-12-20T11:51:36Z

Yeah, oddly enough document.all == null is actually true even though it’s an object, verified in Chrome console. Apparently it’s some compatibility baggage from ancient Internet Explorer.

I don’t think it’s a major issue in practice, like you said. Just wanted to state it for the record since embedders have no way to emulate this behavior.

Thinking about it - if any browser does embed this and if a developer wants to do document.all ?? something and is confused at getting something as the result that's on them - the sort of special casing required to avoid that is IMO not worth the performance cost it would entail for all other uses.

fatcerberus · 2020-12-20T13:37:14Z

It turns out that the special case under discussion is actually formally encoded in the ES spec, see https://tc39.es/ecma262/#sec-IsHTMLDDA-internal-slot

rhuanjl · 2020-12-20T13:41:25Z

It turns out that the special case under discussion is actually formally encoded in the ES spec, see https://tc39.es/ecma262/#sec-IsHTMLDDA-internal-slot

Yeah I read it - and I don't think it's worth supporting for now, to handle it in this operation could either:
a) implement a whole new opcode OR
b) use two strict equal comparisons instead of one normal comparison

(a) would be a lot of extra code and (b) would be a performance hit - if anyone actually makes a new webbrowser with CC and wants spec perfect support for window.all we can revisit.

fatcerberus · 2020-12-20T13:53:49Z

Alright, that makes sense. Just wanted to make sure everything was on the table first before a decision was made. I’ll duck out now 🦆

ppenzin · 2020-12-22T00:53:40Z

Closing and reopening in hopes to clear the CI failure.

CC @divmain ;)

rhuanjl · 2020-12-24T12:03:05Z

@ppenzin interestingly there's an option to override the CI and merge - I'm guessing there's no way to retrigger it now the repository has been transferred - considering it passed everything other than windows 7 - think we should just merge it?

ppenzin · 2020-12-26T05:36:37Z

@rhuanjl agree - we should merge it.

boingoing and others added 2 commits December 18, 2020 18:56

fix and complete nullish coalescing

b1981d8

rhuanjl force-pushed the nullish branch from f07f76b to b1981d8 Compare December 19, 2020 12:11

rhuanjl mentioned this pull request Dec 19, 2020

Implement nullish coalescing operator #6254

Closed

rhuanjl added this to In progress in Javascript Standards via automation Dec 19, 2020

rhuanjl added the ES Spec Changed label Dec 19, 2020

rhuanjl commented Dec 19, 2020

View reviewed changes

ppenzin approved these changes Dec 20, 2020

View reviewed changes

ppenzin closed this Dec 20, 2020

Javascript Standards automation moved this from In progress to Done Dec 20, 2020

ppenzin reopened this Dec 20, 2020

Javascript Standards automation moved this from Done to In progress Dec 20, 2020

ppenzin closed this Dec 22, 2020

Javascript Standards automation moved this from In progress to Done Dec 22, 2020

ppenzin reopened this Dec 22, 2020

Javascript Standards automation moved this from Done to In progress Dec 22, 2020

rhuanjl mentioned this pull request Dec 22, 2020

Implement optional chaining #6349

Open

ppenzin merged commit 052db4c into chakra-core:master Dec 26, 2020

Javascript Standards automation moved this from In progress to Done Dec 26, 2020

rhuanjl deleted the nullish branch December 26, 2020 09:59

rhuanjl mentioned this pull request Mar 18, 2021

Support logical assignment #6658

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Nullish Coalescing operator #6545

Implement Nullish Coalescing operator #6545

rhuanjl commented Dec 19, 2020 •

edited

rhuanjl Dec 19, 2020

rhuanjl Dec 19, 2020

rhuanjl Dec 19, 2020

rhuanjl Dec 19, 2020

rhuanjl Dec 19, 2020

rhuanjl Dec 19, 2020

rhuanjl Dec 19, 2020

fatcerberus commented Dec 19, 2020

rhuanjl commented Dec 19, 2020

fatcerberus commented Dec 19, 2020

ppenzin commented Dec 20, 2020

rhuanjl commented Dec 20, 2020

fatcerberus commented Dec 20, 2020

rhuanjl commented Dec 20, 2020 •

edited

fatcerberus commented Dec 20, 2020

ppenzin commented Dec 22, 2020

rhuanjl commented Dec 24, 2020

ppenzin commented Dec 26, 2020

Implement Nullish Coalescing operator #6545

Implement Nullish Coalescing operator #6545

Conversation

rhuanjl commented Dec 19, 2020 • edited

rhuanjl Dec 19, 2020

Choose a reason for hiding this comment

rhuanjl Dec 19, 2020

Choose a reason for hiding this comment

rhuanjl Dec 19, 2020

Choose a reason for hiding this comment

rhuanjl Dec 19, 2020

Choose a reason for hiding this comment

rhuanjl Dec 19, 2020

Choose a reason for hiding this comment

rhuanjl Dec 19, 2020

Choose a reason for hiding this comment

rhuanjl Dec 19, 2020

Choose a reason for hiding this comment

fatcerberus commented Dec 19, 2020

rhuanjl commented Dec 19, 2020

fatcerberus commented Dec 19, 2020

ppenzin commented Dec 20, 2020

rhuanjl commented Dec 20, 2020

fatcerberus commented Dec 20, 2020

rhuanjl commented Dec 20, 2020 • edited

fatcerberus commented Dec 20, 2020

ppenzin commented Dec 22, 2020

rhuanjl commented Dec 24, 2020

ppenzin commented Dec 26, 2020

rhuanjl commented Dec 19, 2020 •

edited

rhuanjl commented Dec 20, 2020 •

edited