Comments and review

Author: ShortDevelopment

lib/Parser/Parse.cpp

@@ -4192,19 +4192,20 @@ ParseNodePtr Parser::ParsePostfixOperators(
             ParseNodePtr name = nullptr;
             OpCode opCode = knopDot;
 
+            // We don't use separate knops for optional-chains
+            // Instead mark nodes as null-propagating
             bool isNullPropagating = tkOptChain == m_token.tk;
             if (isNullPropagating)
             {
                 isOptionalChain = true;
             }
-            // We don't use a custom token but rather tell that knopDot is null-propagating
-            // opCode = knopOptChain;
 
             this->GetScanner()->Scan();
             if (!m_token.IsIdentifier())
             {
                 if (isNullPropagating)
                 {
+                    // We don't need an identifier for an Index `?.[` or Call `?.(`
                     switch (m_token.tk)
                     {
                     case tkLParen:
@@ -4316,7 +4317,7 @@ ParseNodePtr Parser::ParsePostfixOperators(
         default:
             if (buildAST && isOptionalChain)
             {
-                // Wrap as optional chain
+                // Wrap the whole expression as an optional-chain
                 return CreateUniNode(knopOptChain, pnode);
             }
             return pnode;

lib/Runtime/ByteCode/ByteCodeEmitter.cpp

@@ -20,18 +20,23 @@ void EmitUseBeforeDeclaration(Symbol *sym, ByteCodeGenerator *byteCodeGenerator,
 void EmitUseBeforeDeclarationRuntimeError(ByteCodeGenerator *byteCodeGenerator, Js::RegSlot location);
 void VisitClearTmpRegs(ParseNode * pnode, ByteCodeGenerator * byteCodeGenerator, FuncInfo * funcInfo);
 
-/**
- * This function generates the common code for null-propagation / optional-chaining.
- */
+/// <summary>
+/// This function generates the common code for null-propagation / optional-chaining.
+/// If the targetObject is nullish this will short-circuit(skip) to the end of the chain-expression.
+/// 
+/// It should be called on every <c>?.</c> location.
+/// A call to this function is only valid from a node-emission inside a `knopOptChain` node.
+/// </summary>
 static void EmitNullPropagation(Js::RegSlot targetObjectSlot, ByteCodeGenerator *byteCodeGenerator, FuncInfo *funcInfo, bool isNullPropagating) {
     if (!isNullPropagating)
         return;
 
-    Assert(funcInfo->currentOptionalChain != 0);
+    // Ensure we've setup the skipLabel in the emission of a `knopOptChain`
+    Assert(funcInfo->currentOptionalChainSkipLabel >= 0);
 
-    // if (targetObject == null) goto chainEnd;
+    // if (targetObject == null) goto skipLabel;
     byteCodeGenerator->Writer()->BrReg2(
-        Js::OpCode::BrEq_A, funcInfo->currentOptionalChain->skipLabel,
+        Js::OpCode::BrEq_A, funcInfo->currentOptionalChainSkipLabel,
         targetObjectSlot, funcInfo->nullConstantRegister
     );
 }
@@ -11621,31 +11626,35 @@ void Emit(ParseNode* pnode, ByteCodeGenerator* byteCodeGenerator, FuncInfo* func
         byteCodeGenerator->Writer()->Element(
             Js::OpCode::LdElemI_A, pnode->location, protoLocation, pnode->AsParseNodeBin()->pnode2->location);
 
-
         ENDSTATEMENET_IFTOPLEVEL(isTopLevel, pnode);
         break;
     }
-
+    // The whole optional-chain expression will be wrapped in a UniNode with `knopOptChain`.
     case knopOptChain: {
-        FuncInfo::OptionalChainInfo *previousChain = funcInfo->currentOptionalChain;
+        Js::ByteCodeLabel previousSkipLabel = funcInfo->currentOptionalChainSkipLabel;
 
+        // Create a label that can skip the whole chain and store in `funcInfo`
         Js::ByteCodeLabel skipLabel = byteCodeGenerator->Writer()->DefineLabel();
-        FuncInfo::OptionalChainInfo currentOptionalChain = FuncInfo::OptionalChainInfo(skipLabel);
-        funcInfo->currentOptionalChain = &currentOptionalChain;
+        funcInfo->currentOptionalChainSkipLabel = skipLabel;
 
+        // Acquire slot for the result value
+        // Prefill it with `undefined` (Fallback for short-circuiting)
         Js::RegSlot resultSlot = funcInfo->AcquireLoc(pnode);
-        byteCodeGenerator->Writer()->Reg1(Js::OpCode::LdUndef, resultSlot); // result = undefined
+        byteCodeGenerator->Writer()->Reg1(Js::OpCode::LdUndef, resultSlot);
 
-        // emit chain
+        // emit chain expression
+        // Every `?.` node will call `EmitNullPropagation`
+        // `EmitNullPropagation` short-circuits to `skipLabel` in case of a nullish value
         ParseNodePtr innerNode = pnode->AsParseNodeUni()->pnode1;
         Emit(innerNode, byteCodeGenerator, funcInfo, false);
 
-        // Copy result
+        // Copy the expression result
+        // Only reached if we did not short-circuit
         byteCodeGenerator->Writer()->Reg2(Js::OpCode::Ld_A, resultSlot, innerNode->location);
         funcInfo->ReleaseLoc(innerNode);
 
         byteCodeGenerator->Writer()->MarkLabel(skipLabel);
-        funcInfo->currentOptionalChain = previousChain;
+        funcInfo->currentOptionalChainSkipLabel = previousSkipLabel;
         break;
     }
     // this is MemberExpression as rvalue

lib/Runtime/ByteCode/FuncInfo.cpp

@@ -34,7 +34,7 @@ FuncInfo::FuncInfo(
     outArgsCurrentExpr(0),
     innerScopeCount(0),
     currentInnerScopeIndex((uint)-1),
-    currentOptionalChain(nullptr),
+    currentOptionalChainSkipLabel(-1),
 #if DBG
     outArgsDepth(0),
 #endif

lib/Runtime/ByteCode/FuncInfo.h

@@ -96,13 +96,7 @@ class FuncInfo
     Js::RegSlot outArgsCurrentExpr; // max number of out args accumulated in the current nested expression
     uint        innerScopeCount;
     uint        currentInnerScopeIndex;
-    struct OptionalChainInfo {
-        Js::ByteCodeLabel skipLabel;
-
-        OptionalChainInfo(Js::ByteCodeLabel skipLabel) {
-            this->skipLabel = skipLabel;
-        }
-    } *currentOptionalChain;
+    Js::ByteCodeLabel currentOptionalChainSkipLabel;
 #if DBG
     uint32 outArgsDepth; // number of calls nested in an expression
 #endif