Skip to content

fix: implement early errors for record and tuple#11652

Merged
nicolo-ribaudo merged 5 commits intobabel:mainfrom
JLHwung:early-error-record-and-tuple
Jun 20, 2020
Merged

fix: implement early errors for record and tuple#11652
nicolo-ribaudo merged 5 commits intobabel:mainfrom
JLHwung:early-error-record-and-tuple

Conversation

@JLHwung
Copy link
Copy Markdown
Contributor

@JLHwung JLHwung commented May 30, 2020 •
edited
Loading

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
@JLHwung JLHwung requested a review from nicolo-ribaudo May 30, 2020 14:23
@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented May 30, 2020 •
edited
Loading

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
Copy Markdown
Collaborator

babel-bot commented May 30, 2020 •
edited
Loading

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

@nicolo-ribaudo
Copy link
Copy Markdown
Member

nicolo-ribaudo commented May 30, 2020

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

@JLHwung
Copy link
Copy Markdown
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
Copy Markdown
Member

nicolo-ribaudo commented Jun 1, 2020

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
UnexpectedReservedWord: "Unexpected reserved word '%0'",
UnexpectedSuper: "super is only allowed in object methods and classes",
UnexpectedToken: "Unexpected token '%'",
UnexpectedToken: "Unexpected token '%0'",
Copy link
Copy Markdown
Contributor Author

@JLHwung JLHwung Jun 1, 2020

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

@JLHwung JLHwung Jun 1, 2020

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
Copy Markdown
Contributor

rickbutton commented Jun 8, 2020

@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
Copy Markdown
Contributor Author

JLHwung commented Jun 19, 2020

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

@nicolo-ribaudo
Copy link
Copy Markdown
Member

nicolo-ribaudo commented Jun 20, 2020

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

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

+1 more reviewer

@rickbutton rickbutton rickbutton approved these changes

Reviewers whose approvals may not affect merge requirements

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.

4 participants

@JLHwung @babel-bot @nicolo-ribaudo @rickbutton