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

Introduce parser error codes #13033

Conversation

sosukesuzuki
Copy link
Member

Q                       A
Fixed Issues? Fixes #13007
Patch: Bug Fix? Maybe yes
Minor: New Feature? Maybe no
Tests Added + Pass? Yes
License MIT

If you have an idea for better interface, please comment!

@babel-bot
Copy link
Collaborator

babel-bot commented Mar 21, 2021

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 21, 2021

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

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

// These errors have a different error code than the key
export const SourceTypeModuleErrorMessages: ErrorTemplates = Object.freeze({
ImportMetaOutsideModule: {
code: "BABEL_PARSER_SOURCETYPE_MODULE_REQUIRED",
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

AccessorIsGenerator: {
code: "AccessorIsGenerator",
template: "A %0ter cannot be a generator",
},
Copy link
Member

Choose a reason for hiding this comment

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

It's just a stylistic preference, but I don't really like that we have to repeat so much boilerplate (duplicating every code, and the code/template keys for every message).

Maybe we could do something like this?

export const ErrorMessages = makeErrorTemplates({
  AccessorIsGenerator: "A %0ter cannot be a generator",
  ...
});

export function makeErrorTemplates(messages) {
  Object.keys(messages).forEach(code => {
    messages[code] = { code, template: messages[code] };
  });
}

it's a one-time cost only paid when importing @babel/parser.

/* eslint sort-keys: "error" */

/**
* @module parser/error-message
*/

// The Errors key follows https://cs.chromium.org/chromium/src/v8/src/common/message-template.h unless it does not exist
export const ErrorMessages = Object.freeze({
export const ErrorMessages: ErrorTemplates = makeErrorTemplates({
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This is probably not needed anymore, since it's inferred from makeErrorTemplates's return type.

Suggested change
export const ErrorMessages: ErrorTemplates = makeErrorTemplates({
export const ErrorMessages = makeErrorTemplates({

Comment on lines 164 to 167
throw this.raise(pos != null ? pos : this.state.start, {
code: "UnexpectedToken",
template: messageOrType,
});
Copy link
Member

Choose a reason for hiding this comment

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

I think here it would be better if the signature was

unexpected(pos: ?number, messageOrType: TokenType | ErrorTemplate)

and if we get an error template we use the correct code rather than UnexpectedToken.

nicolo-ribaudo
nicolo-ribaudo previously approved these changes Mar 24, 2021
existentialism
existentialism previously approved these changes Mar 24, 2021
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.

👍

@nicolo-ribaudo
Copy link
Member

Should this wait for a minor?

@JLHwung
Copy link
Contributor

JLHwung commented Mar 25, 2021

Note that the code of parsing error is now a contract between Babel and users. We may want to review our current error code to see if there is any typo / wording inconsistency. Then it should be good to land.

`Unexpected token \`${char}\`. Did you mean \`${htmlEntity}\` or \`{'${char}'}\`?`,
);
this.raise(this.state.pos, {
code: "UnexpectedToken",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we prefix error code with BABEL_PARSER like we did in BABEL_PARSER_SOURCETYPE_MODULE_REQUIRED? Should we use SNAKE_CASE instead?

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe we could always set code to BABEL_PARSER_ERROR, and add a new property (like errorCode, or parserError) using these new codes we are introducing.

Copy link
Contributor

@JLHwung JLHwung Mar 25, 2021

Choose a reason for hiding this comment

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

I prefer a prefixed error code but don't have a strong opinion. The Node.js recommends error.code to be the identifier of the error. Conventionally they use SNAKE_CASE. I don't have strong opinion between camelCase and SNAKE_CASE, as long as we have consensus.

https://nodejs.org/api/errors.html#errors_error_code

I would like to maintain the same stability policy as Node.js: The error.code thrown from @babel/parser is considered stable: It will only be changed in a major release, but not for error.message which we will keep polishing between patches/minors.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah but I wonder how useful these specific error messages are.

Except for a few (and they already have specific codes), I think often I would want do do something like this:

try {
  somethingThatCallsBabel();
} catch (e) {
  if (e.code === "BABEL_PARSER_ERROR") {
    console.warn("The file contains invalid syntax!");
  } else if (e.code === "BABEL_PARSER_MISSING_PLUGIN") {
    console.warn("You might want to enable a parser plugin!");
  } else if (e.code === "BABEL_PARSER_SOURCETYPE_MODULE_REQUIRED") {
    console.warn("You are parsing a module as a script!");
  } else if (e.code === "ENOENT") {
    console.warn("You are trying to parse a non-existing file!");
  } else {
    throw e;
  }
}

Imho the specific syntax error is a detail at a deeper leverl.

I would like to maintain the same stability policy as Node.js: The error.code thrown from @babel/parser is considered stable

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

The new example looks good to me. In that case we may consider place current detailed error code to syntaxError or reasonCode.

I think BABEL_PARSER_ERROR is too general, maybe BABEL_PARSER_INVALID_SYNTAX or BABEL_PARSER_SYNTAX_ERROR?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I like that (I don't have a preference about the exact names).

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to maintain the same stability policy as Node.js: The error.code thrown from @babel/parser is considered stable

👍

To summarize, the interface will look like this?:

{
  code: "BABEL_PARSER_SYNTAX_ERROR",
  reasonCode: "ArgumentsInClass",
  message: "'arguments' is only allowed in functions and class methods"
}

Copy link
Member

Choose a reason for hiding this comment

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

Yes 👍

@nicolo-ribaudo nicolo-ribaudo dismissed their stale review March 25, 2021 19:48

Dismissing my approval for now, since there will be more changes.

@sosukesuzuki
Copy link
Member Author

Also, code and reasonCode are now contract between Babel and users so we should introduce testing for it.

@sosukesuzuki
Copy link
Member Author

sosukesuzuki commented Mar 26, 2021

This PR includes too large changes for updating snapshots:

  • 105b817 : Modify runFixtureText to write serialized errors to snapshots.
  • 59f07f7 , 9828d62 : Actually update snapshots following the above commit.
    • the updating is executed via OVERWRITE=true TEST_ONLY=babel-parser make test-only

@nicolo-ribaudo
Copy link
Member

Wow this PR is getting huge! 🚀😂

Could you also run BABEL_8_BREAKING=true OVERWRITE=true TEST_ONLY=babel-parser make test-only?

@nicolo-ribaudo nicolo-ribaudo added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Mar 27, 2021
@nicolo-ribaudo
Copy link
Member

Maybe we could add a new test file (packages/babel-parser/test/error-codes.js) that asserts that:

  • import "foo" in a script throws BABEL_PARSER_SOURCETYPE_MODULE_REQUIRED
  • a b throws BABEL_PARSER_SYNTAX_ERROR
  • a b's error has a reasonCode property

@@ -73,7 +110,7 @@ export default class ParserError extends CommentsParser {
}
}
}
return this._raise({ loc, pos }, message);
return this._raise({ code, loc, pos }, message);
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be good to add params as a property.
Use case: in Prettier, Unexpected reserved word 'yield' should be suppressed, Unexpected reserved word 'for' should be rethrown. prettier/prettier#10603

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to split the error between UnexpectedKeyword and UnexpectedContextualKeyword if that works too, and avoid exposing too much internal details about how we build errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's a better solution for that use case.

Copy link
Member

Choose a reason for hiding this comment

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

@thorn0 I was going to implement this, but I noticed that we already have two different errors:

  • UnexpectedKeyword for things like for, if, while, ...
  • UnexpectedReservedWord for things like yield, await, enum, ...

@sosukesuzuki
Copy link
Member Author

Maybe we could add a new test file (packages/babel-parser/test/error-codes.js) that asserts that:

I added simple tests! f07223d

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.

Thanks!

I'll merge this to a feature branch (since we don't merge "New feature" PRs until we are ready for a minor version), so that you or someone else can work on #13033 (comment) on top of this one.

@nicolo-ribaudo nicolo-ribaudo changed the base branch from main to feat-7.14.0/parser-updates April 3, 2021 17:30
@nicolo-ribaudo nicolo-ribaudo changed the title babel-parser: introduce error code Introduce parser error codes Apr 3, 2021
@nicolo-ribaudo nicolo-ribaudo merged commit 0240981 into babel:feat-7.14.0/parser-updates Apr 3, 2021
@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Apr 3, 2021
9 tasks
nicolo-ribaudo pushed a commit that referenced this pull request Apr 9, 2021
@sosukesuzuki sosukesuzuki deleted the introduce-error-code branch April 12, 2021 22:41
nicolo-ribaudo pushed a commit that referenced this pull request Apr 13, 2021
JLHwung pushed a commit that referenced this pull request Apr 16, 2021
JLHwung pushed a commit that referenced this pull request Apr 17, 2021
JLHwung pushed a commit that referenced this pull request Apr 19, 2021
nicolo-ribaudo pushed a commit that referenced this pull request Apr 20, 2021
nicolo-ribaudo pushed a commit that referenced this pull request Apr 22, 2021
nicolo-ribaudo pushed a commit that referenced this pull request Apr 23, 2021
nicolo-ribaudo pushed a commit that referenced this pull request Apr 26, 2021
nicolo-ribaudo pushed a commit that referenced this pull request Apr 28, 2021
@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 Jul 14, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: errors 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

babel-parser: Introduce error code
6 participants