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 23 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)
  • 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
  • What about the apply call optimization?
  • 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")

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

Comment on lines +105 to +108
// ToDo: How can I run async tests?
// simpleObj.nothing?.[await Promise.reject()];
// simpleObj.null?.[await Promise.reject()];
// simpleObj.undefined?.[await Promise.reject()];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can I run async tests?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are a few ways, have a look at test/es7/async-generator-functionality.js for one example.

Note, I'd recommend putting the async tests in a separate file - I think it's easier/better to manage them separately.

@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 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);
}

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