Optional chaining

[ShortDevelopment]

This PR aims to add support for the stage-4 proposal optional-chaining.
It's inspired by the work of @rhuanjl but uses a more hacky approach to parsing.

Goals

• Minimize amount of changes
• (Hopefully) Performance

ToDo

• Add tests
  • Unused expression result
  • Return expression
  • Root optional-call (e.g. eval?.('a'))
  • Scoped method load (e.g. inside eval)
  • delete
• implement optional-deletion
• optional call invoked on eval function should be indirect eval
• Simple function calls
• Preserve this
(a.b)().c should be equivalent to a.b().c
• short circuit embedded expressions like a?.[++x]
++x should not be evaluated if a is null or undefined
• Don't tokenize a?.3:0 (ternary) as tkOptChain (?.)
• Tagged templates are not allowed in optional chain
• fix tmp-renaming for eval result
  eval("foo?.()") or eval("foo?.property")
• Works with optimizations
  • What about the apply call optimization?
  • Loop inversion (AST cloning)
    Only triggered for 2 nested for loops with assignment
    

    ChakraCore/test/loop/loopinversion.js

    Lines 60 to 66 in e26c81f

    	for (var j = 0; j < 1; ++j) {
    	for (var i = 0; i < 1; ++i) {
    	1;
    	c = 2;
    	1;
    	}
    	};

Out of scope

• Remove EmitInvoke (seems unused)
• Better error message for a call on a non-function

---

Spec: Optional Chains
Fixes #6349

[rhuanjl]

I think this looks neater than my version was, though struggling to follow how it handles every branch. What happens if you do a function call in an optional chain does it crash?

(Not going to merge until we have some significant test coverage)

[ShortDevelopment]

I'm currently working on function calls.
At the moment it would just be handled as if no optional-chaining would be used
⇾ crash in js land

Edit: Function call should work now but this is not always propagated correctly (yet)
Edit: Function calls should now work

[ShortDevelopment]

The jitted code currently crashes at this assertion (causing the test-failures)

ChakraCore/lib/Backend/GlobOpt.cpp

Line 11384 in 1f6e17c

Assert(block->globOptData.IsTypeSpecialized(varSym));

[ShortDevelopment]

We "copy" the value from the knopOptChain node to the knopDelete node

[ShortDevelopment]

Instead of acquiring a tmp I copy the location of pnodeOptChain to innerNode
(In case the caller has already aquired a tmp)

ChakraCore/lib/Runtime/ByteCode/ByteCodeEmitter.cpp

Line 63 in 05790ae

innerNode->location = pnodeOptChain->location;

And copy it back after emitting innerNode
(In case the location was NoRegister and the emission of innerNode aquired a tmp)

ChakraCore/lib/Runtime/ByteCode/ByteCodeEmitter.cpp

Line 69 in 05790ae

pnodeOptChain->location = innerNode->location;

[ShortDevelopment]

@rhuanjl Just in time for the first aniversary (🎉) of this PR, I'll mark it as ready for review!

• All feature work should be complete
• I'm likely missing some test cases (Do you have ideas?)
• You had some special handling for apply calls but I did not manage to come up with a working test case
• The pipeline is failing again due to a now unsupported CMake version 😢
  [Build]: MacOS ci does not support CMake < 3.5 #7031

[sapdragon]

I see that there has been more activity in the repository. Is there any chance of merging this PR?

[rhuanjl]

I see that there has been more activity in the repository. Is there any chance of merging this PR?

I'll take a look in the next week, back when we were last working on this I was worried about a couple of potentially untested code paths.

FYI: the only people who contribute to this repository now do it as a hobby and have unrelated jobs hence the glacial progress. I'm happy to give a bit of time to help any new contributor learn the ropes if you're aware of interest it would be good to see more happening here.

[rhuanjl]

@ShortDevelopment do you think this is complete? Should I re-review?

[ShortDevelopment]

@rhuanjl That would be great! 😀

Now that CI is running again, I had a second look at the apply-call optimization.
This optimization seems like a niche edge-case to optimize .apply(this, arguments) in very specific circumstances.

Everything else should be done.

As adding support for this code-path will only be a small addition, you could already review the whole PR (If that's okay for you). I would then add a small commit to finish this during this week.

[ShortDevelopment]

Okay, so here's my writeup on the apply-call optimization:

TL;DR: I would argue, it is very hard to trigger this code-path, potentially impossible. In fact, ci passes just fine when EmitApplyCall is replaced with std::abort (See d1abb8f). While this is no prove of cause, it's at least reassuring.

Definition

The apply-call optimization aims to optimize calls of the following form:

lhs.apply(this, arguments)

CC's test-suite contains this pattern pattern in a decent amount of tests.

Emission

EmitApplyCall is called instead of EmitCall if two heuristics pass; a syntactic and a semantic one:

ChakraCore/lib/Runtime/ByteCode/ByteCodeEmitter.cpp

Lines 11575 to 11583 in 97e68c9

	if (pnodeCall->isApplyCall && funcInfo->GetApplyEnclosesArgs())
	{
	// TODO[ianhall]: Can we remove the ApplyCall bytecode gen time optimization?
	EmitApplyCall(pnodeCall, byteCodeGenerator, funcInfo, fReturnValue);
	}
	else
	{
	EmitCall(pnodeCall, byteCodeGenerator, funcInfo, fReturnValue, /*fEvaluateComponents*/ true);
	}

Syntactic check

Checking for functions without nesting that contain a call of the following pattern: .apply(this, arguments).

ChakraCore/lib/Runtime/ByteCode/ByteCodeGenerator.cpp

Lines 5317 to 5339 in 97e68c9

	bool ApplyEnclosesArgs(ParseNode* fncDecl, ByteCodeGenerator* byteCodeGenerator)
	{
	if (byteCodeGenerator->IsInDebugMode())
	{
	// Inspection of the arguments object will be messed up if we do ApplyArgs.
	return false;
	}
	if (!fncDecl->HasVarArguments()
	&& fncDecl->AsParseNodeFnc()->pnodeParams == nullptr
	&& fncDecl->AsParseNodeFnc()->pnodeRest == nullptr
	&& fncDecl->AsParseNodeFnc()->nestedCount == 0)
	{
	ApplyCheck applyCheck;
	applyCheck.matches = true;
	applyCheck.sawApply = false;
	applyCheck.insideApplyCall = false;
	VisitIndirect<ApplyCheck>(fncDecl->AsParseNodeFnc()->pnodeBody, byteCodeGenerator, &applyCheck, &CheckApplyEnclosesArgs, &PostCheckApplyEnclosesArgs);
	return applyCheck.matches&&applyCheck.sawApply;
	}
	return false;
	}

ChakraCore/lib/Runtime/ByteCode/ByteCodeEmitter.cpp

Lines 5928 to 5970 in 97e68c9

	void CheckApplyEnclosesArgs(ParseNode* pnode, ByteCodeGenerator* byteCodeGenerator, ApplyCheck* applyCheck)
	{
	if ((pnode == nullptr) || (!applyCheck->matches))
	{
	return;
	}
	switch (pnode->nop)
	{
	case knopName:
	{
	Symbol* sym = pnode->AsParseNodeName()->sym;
	if (sym != nullptr)
	{
	if (sym->IsArguments())
	{
	if (!applyCheck->insideApplyCall)
	{
	applyCheck->matches = false;
	}
	}
	}
	break;
	}
	case knopCall:
	if ((!pnode->isUsed) && IsApplyArgs(pnode->AsParseNodeCall()))
	{
	// no nested apply calls
	if (applyCheck->insideApplyCall)
	{
	applyCheck->matches = false;
	}
	else
	{
	applyCheck->insideApplyCall = true;
	applyCheck->sawApply = true;
	pnode->AsParseNodeCall()->isApplyCall = true;
	}
	}
	break;
	}
	}

ChakraCore/lib/Runtime/ByteCode/ByteCodeEmitter.cpp

Lines 5880 to 5906 in 97e68c9

	bool IsApplyArgs(ParseNodeCall* callNode)
	{
	ParseNode* target = callNode->pnodeTarget;
	ParseNode* args = callNode->pnodeArgs;
	if ((target != nullptr) && (target->nop == knopDot))
	{
	ParseNode* lhsNode = target->AsParseNodeBin()->pnode1;
	if ((lhsNode != nullptr) && ((lhsNode->nop == knopDot) || (lhsNode->nop == knopName)) && !IsArguments(lhsNode))
	{
	ParseNode* nameNode = target->AsParseNodeBin()->pnode2;
	if (nameNode != nullptr)
	{
	bool nameIsApply = nameNode->AsParseNodeName()->PropertyIdFromNameNode() == Js::PropertyIds::apply;
	if (nameIsApply && args != nullptr && args->nop == knopList)
	{
	ParseNode* arg1 = args->AsParseNodeBin()->pnode1;
	ParseNode* arg2 = args->AsParseNodeBin()->pnode2;
	if ((arg1 != nullptr) && ByteCodeGenerator::IsThis(arg1) && (arg2 != nullptr) && (arg2->nop == knopName) && (arg2->AsParseNodeName()->sym != nullptr))
	{
	return arg2->AsParseNodeName()->sym->IsArguments();
	}
	}
	}
	}
	}
	return false;
	}

Semantic check

Based on my intuition, I would expect the semantic check to detect the special arguments keyword being shadowed by a var, let or const, but the implementation clearly checks for variables which are not the arguments keyword.
If it detects a variable not equal to arguments (like the special *this* variable required by the syntax check), it prevents the optimization:

ChakraCore/lib/Runtime/ByteCode/ByteCodeEmitter.cpp

Lines 2825 to 2855 in 97e68c9

	if ((pnodeFnc->nop == knopFncDecl) && (funcInfo->GetHasHeapArguments()) && (!funcInfo->GetCallsEval()) && ApplyEnclosesArgs(pnodeFnc, this))
	{
	bool applyEnclosesArgs = true;
	for (ParseNode* pnodeVar = funcInfo->root->pnodeVars; pnodeVar; pnodeVar = pnodeVar->AsParseNodeVar()->pnodeNext)
	{
	Symbol* sym = pnodeVar->AsParseNodeVar()->sym;
	if (sym->GetSymbolType() == STVariable && !sym->IsArguments())
	{
	applyEnclosesArgs = false;
	break;
	}
	}
	auto constAndLetCheck = [](ParseNodeBlock *pnodeBlock, bool *applyEnclosesArgs)
	{
	if (*applyEnclosesArgs)
	{
	for (auto lexvar = pnodeBlock->pnodeLexVars; lexvar; lexvar = lexvar->AsParseNodeVar()->pnodeNext)
	{
	Symbol* sym = lexvar->AsParseNodeVar()->sym;
	if (sym->GetSymbolType() == STVariable && !sym->IsArguments())
	{
	*applyEnclosesArgs = false;
	break;
	}
	}
	}
	};
	constAndLetCheck(funcInfo->root->pnodeScopes, &applyEnclosesArgs);
	constAndLetCheck(funcInfo->root->pnodeBodyScope, &applyEnclosesArgs);
	funcInfo->SetApplyEnclosesArgs(applyEnclosesArgs);
	}

Also, this optimization is only considered if jit or apply-inlining is disabled:

ChakraCore/lib/Runtime/ByteCode/ByteCodeEmitter.cpp

Line 2823 in 97e68c9

if (byteCodeFunction->IsInlineApplyDisabled() || this->scriptContext->GetConfig()->IsNoNative())

Conclusion

We could...

• fix the optimization and add support for OptChain
• remove the optimization as proposed by this existing comment
  

  ChakraCore/lib/Runtime/ByteCode/ByteCodeEmitter.cpp

  Line 11577 in 97e68c9

  // TODO[ianhall]: Can we remove the ApplyCall bytecode gen time optimization?