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

Jump to bottom

fix: implement early errors for record and tuple #11652

Merged
merged 5 commits into from Jun 20, 2020
Merged

fix: implement early errors for record and tuple #11652

merged 5 commits into from Jun 20, 2020

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented May 30, 2020 •
edited

Q                       A
Fixed Issues? Implements early errors for record and tuple
Patch: Bug Fix? Yes
Tests Added + Pass? Yes
License MIT

Implements early syntax errors specified in https://github.com/tc39/proposal-record-tuple#syntax-errors

  • Allow only ObjectProperty and SpreadElement in RecordExpression.
  • Disallow holes in TupleExpression
    I don't have a good idea on throwing recoverable errors other than passing through isTuple everywhere.They are now recoverable.
  • Disallow __proto__ as key in RecordExpression.

@rickbutton BTW do you think we can deprecate syntaxType: bar? It seems to me {| is longer proposed because I don't see a spec text on that.

@JLHwung JLHwung added PR: Spec Compliance 👓 A type of pull request used for our changelog categories Spec: Record Tuple labels May 30, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented May 30, 2020 •
edited

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9705b1e:

Sandbox Source
goofy-ganguly-ur7vq Configuration
mystifying-khayyam-256l7 Configuration

@babel-bot
Copy link
Collaborator

babel-bot commented May 30, 2020 •
edited

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

@nicolo-ribaudo
Copy link
Member

What if in parseExprListItem we always report "Empty holes are not allowed here" when allowEmpty is false?

@JLHwung
Copy link
Contributor Author

JLHwung commented May 31, 2020

@nicolo-ribaudo parseExprListItem(false, ?) is also used in parseCallExpressionArguments. Do we have mental concept of holes in call arguments?

@JLHwung JLHwung force-pushed the early-error-record-and-tuple branch from 436153c to 42f44ea Compare May 31, 2020 21:55
@JLHwung JLHwung force-pushed the early-error-record-and-tuple branch from 42f44ea to 5d91a42 Compare June 1, 2020 14:22
@nicolo-ribaudo
Copy link
Member

Do we have mental concept of holes in call arguments?

You are right, we don't. However, it's an error we can easily recover from: we just need to find a good error message for it. Maybe A comma can only be directly followed by another comma in array literals, creating a hole?

JLHwung
JLHwung commented Jun 1, 2020
View reviewed changes
packages/babel-parser/src/parser/error-message.js
@@ -163,7 +166,7 @@ export const ErrorMessages = Object.freeze({
"Private names can only be used as the name of a class element (i.e. class C { #p = 42; #m() {} } )\n or a property of member expression (i.e. this.#p).",
UnexpectedReservedWord: "Unexpected reserved word '%0'",
UnexpectedSuper: "super is only allowed in object methods and classes",
UnexpectedToken: "Unexpected token '%'",
UnexpectedToken: "Unexpected token '%0'",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️

JLHwung
JLHwung commented Jun 1, 2020
View reviewed changes
packages/babel-parser/src/parser/expression.js
if (allowEmpty && this.match(tt.comma)) {
if (this.match(tt.comma)) {
if (!allowEmpty) {
this.raise(this.state.pos, Errors.UnexpectedToken, ",");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I resort to the most generic error message. I still feel awkward mentioning holes when throwing f(,). Any suggestions are definitely welcome.

At least the error is now recoverable.

@rickbutton
Copy link
Contributor

@JLHwung thank you for pinging me on this! The changes you have listed look good to me. On syntax options, there is still some discussion in tc39/proposal-record-tuple#10 on which exact sigils to use for the literal syntax. I would be fine with removing the bar syntaxType option, but it is possible that the syntax will still change (although the champion group prefers #).

@JLHwung JLHwung changed the base branch from master to main June 19, 2020 13:27
@JLHwung
Copy link
Contributor Author

JLHwung commented Jun 19, 2020

@nicolo-ribaudo Can we consider this PR has also approval from Rick Button?

@nicolo-ribaudo
Copy link
Member

Yes 👍

@nicolo-ribaudo nicolo-ribaudo merged commit 30835f1 into babel:main Jun 20, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the early-error-record-and-tuple branch June 20, 2020 00:35
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Sep 19, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rickbutton rickbutton rickbutton approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees
No one assigned
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Spec Compliance 👓 A type of pull request used for our changelog categories Spec: Record Tuple
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JLHwung @babel-bot @nicolo-ribaudo @rickbutton