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

V8intrinsic syntax plugin #10148

Merged
merged 13 commits into from Sep 6, 2019

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jun 29, 2019

Q                       A
Fixed Issues? Fixes #10104
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? ✔️
Tests Added + Pass? ✔️
Documentation PR Link
Any Dependency Changes? No
License MIT

Authored a new syntax plugin v8intrinsic to support V8 CallRuntime syntax

CallRuntime ::
  '%' Identifier Arguments

The original implementation in V8 parser peek() twice to check whether a modulo token is immediately followed by an identifier and a parenL token.

As in Babel lookahead() does not support multiple steps, here we implicitly track the execution branch inside parseSubscript to see if parseCallExpressionArguments is immediately called in the first round of parseExprSubscripts. If so we conclude that the modulo token is a valid V8Intrinsic marker.

It is also interesting to note that a V8Intrinsic call inside a new expression does not raise a SyntaxError in V8. i.e.

node --allow-natives-syntax -e "new %DebugPrint(null)"
// throws TypeError: (intermediate value) is not a constructor

node -e "new %DebugPrint(null)"
// throws SyntaxError: Unexpected token %

@mathiasbynens I don't think there are any V8 intrinsics supposed to work with new expression, aren't there?

If so, it should be reasonable to prevent v8intrisic from new expression, as is the behavior of this plugin. Otherwise we still need to take care on the new expression.

Action Items

@babel-bot
Copy link
Collaborator

babel-bot commented Jun 29, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11486/

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

I was thinking that maybe we can remove this.state.V8IntrinsicStart and put all the parsing logic in a single method: we can overwrite parseExprAtom and add something like this:

  1. If eat %%,
    i. Parse an identifier
    ii. Set that identifier's type to V8IntrinsicIdentifier
    iii. If the upcoming token isn't (, throw an error
  2. Else, return super.parseExprAtom.

This is similar to what we do for super() and import().

@nicolo-ribaudo nicolo-ribaudo added pkg: parser PR: New Feature 🚀 A type of pull request used for our changelog categories labels Jun 30, 2019
@JLHwung
Copy link
Contributor Author

JLHwung commented Jul 1, 2019

I was thinking that maybe we can remove this.state.V8IntrinsicStart and put all the parsing logic in a single method

I agree, actually my first attack is on rewriting parseIdentifier but I find out it would mess up with the token position internal state. Apparently overriding parseExprAtom is way better than my previous approach. I have pushed the rewritten version.

Can you also add a test for yield?

Great observation! The parseYield will check if a token can startsExpr, if not it ends up with an empty yield statement, which fails under this case where % can actually start an expression. I would try to figure out a way to do this in the plugin land.

@mathiasbynens
Copy link
Contributor

Thanks for your work on this!

@mathiasbynens I don't think there are any V8 intrinsics supposed to work with new expression, aren't there?

That's correct. Here's how V8 implements parsing support based on the flag.

@JLHwung
Copy link
Contributor Author

JLHwung commented Jul 1, 2019

That's correct. Here's how V8 implements parsing support based on the flag.

The implementation of V8 parsing V8Intrinsic will lead to the fact that new %StringParseInt("42", 10) will not throw a syntax error, instead it throws a runtime type error.

However as I have rewritten the plugin and now it closely follow the way how V8 parse. I have added this newExpression test case to the success test suites.

@JLHwung
Copy link
Contributor Author

JLHwung commented Jul 1, 2019

I've talked to Nicolo offline a bit. Here we add { startsExpr: true } configuration to modulo token in parser/types since

  • It is only used in parseYield. The impact is under control.
  • There have been some plugin-defined tokens inside this file, i.e. braceBarL, braceBarR. It is acceptable to place plugin-defined token configurations here.

Copy link
Member

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

Awesome!

@nicolo-ribaudo nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Jul 4, 2019
@nicolo-ribaudo nicolo-ribaudo added this to the v7.6.0 milestone Jul 4, 2019
Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

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

👍

@jridgewell
Copy link
Member

@nicolo-ribaudo: Is this waiting for a release, or can it be merged?

@nicolo-ribaudo
Copy link
Member

We just released v7.5 so this is waiting for the next minor, but if it is urgent we could do it earlier than the usual time between two minor releases.

@@ -19,3 +23,12 @@ defineType("Placeholder", {
},
},
});

defineType("V8IntrinsicIdentifier", {
Copy link
Member

Choose a reason for hiding this comment

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

I just realized that we should add this to the allowed callees of CallExpression

Implement V8 Intrinsic Syntax Extension. Here we check the execution branch inside the parseSubscript to make sure the V8IntrinsicIdentifier is immediately followed by a call expression.
@nicolo-ribaudo nicolo-ribaudo merged commit da0af5f into babel:master Sep 6, 2019
@JLHwung JLHwung deleted the v8intrinsic-syntax-plugin branch September 6, 2019 18:42
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Dec 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add optional parsing support for V8 intrinsics
6 participants