Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional chaining #6973

Draft
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

ShortDevelopment
Copy link
Contributor

@ShortDevelopment ShortDevelopment commented Apr 15, 2024

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

Copy link
Collaborator

@rhuanjl rhuanjl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

lib/Runtime/ByteCode/ByteCodeEmitter.cpp Outdated Show resolved Hide resolved
lib/Runtime/ByteCode/ByteCodeEmitter.cpp Outdated Show resolved Hide resolved
lib/Runtime/ByteCode/FuncInfo.h Outdated Show resolved Hide resolved
@ShortDevelopment
Copy link
Contributor Author

ShortDevelopment commented Apr 18, 2024

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
Copy link
Contributor Author

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

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

@ShortDevelopment ShortDevelopment force-pushed the feat-optional-chaining branch 2 times, most recently from aae4d9f to a9a1c70 Compare May 1, 2024 13:46
@ShortDevelopment ShortDevelopment force-pushed the feat-optional-chaining branch 2 times, most recently from b132685 to 9904051 Compare May 3, 2024 21:15
@ShortDevelopment ShortDevelopment force-pushed the feat-optional-chaining branch from d8841a9 to 1a46f30 Compare May 4, 2024 13:31
@ShortDevelopment ShortDevelopment force-pushed the feat-optional-chaining branch from 1a46f30 to 8baa9e6 Compare May 4, 2024 21:44
@ShortDevelopment ShortDevelopment force-pushed the feat-optional-chaining branch 2 times, most recently from 5958fb4 to 8c4eddf Compare May 10, 2024 11:03
EmitOptionalChainWrapper(pexpr->AsParseNodeUni(), byteCodeGenerator, funcInfo, [&](ParseNode *innerNode) {
EmitDelete(innerNode, innerNode, byteCodeGenerator, funcInfo);
});
pnode->location = pexpr->location;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// emit chain expression
// Every `?.` node will call `EmitNullPropagation`
// `EmitNullPropagation` short-circuits to `skipLabel` in case of a nullish value
emitChainContent(innerNode);
pnodeOptChain->location = innerNode->location;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)

pnodeOptChain->location = innerNode->location;

Comment on lines +11278 to +11285
default:
{
Emit(pexpr, byteCodeGenerator, funcInfo, false);
funcInfo->ReleaseLoc(pexpr);
byteCodeGenerator->Writer()->Reg2(
Js::OpCode::Delete_A, funcInfo->AcquireLoc(pnode), pexpr->location);
break;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JavascriptOperators::Delete does only return true. Why can't we just emit bytecode to load true?
Is this because of getter/setter side-effects?

Var JavascriptOperators::Delete(Var var, ScriptContext* scriptContext)
{
JIT_HELPER_NOT_REENTRANT_NOLOCK_HEADER(Op_Delete);
return scriptContext->GetLibrary()->GetTrue();
JIT_HELPER_END(Op_Delete);
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, I was confused when I first looked at this; it appears this OpCode is used only in this one location in the bytecodeemitter and is for a no_op delete, as far as I can see we could indeed replace it with LdTrue and delete the logic for OpCode:Delete_A and the relevant code paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement optional chaining
3 participants