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

RegExp parse errors in regexpp are not tolerated and crash eslint #12169

Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules

Comments

@Jessidhia
Copy link

Jessidhia commented Aug 26, 2019

Tell us about your environment

  • ESLint Version: 6.2.1
  • Node Version: 12.6.0
  • npm Version: yarn 1.17.3

What parser (default, Babel-ESLint, etc.) are you using?
@typescript-eslint/parser, but the crash is in the regexpp instance used by the no-misleading-character-class rule.

Please show your full configuration:

Configuration
// too big to share, but this in any file should be enough
/* eslint no-misleading-character-class: 1 */

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

/{/u

It happens on the command line as well but this is particularly bothersome with editor plugins (e.g. vscode integration), as you get plugin crash messages while typing up regexps.

What did you expect to happen?
No crash, at worst just an "invalid regexp" lint.

What actually happened? Please include the actual, raw output from ESLint.

[Error - 1:37:37 PM] SyntaxError: Invalid regular expression: /{/u: Lone quantifier brackets
Occurred while linting .../test.js:9
    at RegExpValidator.raise (.../node_modules/regexpp/index.js:5643:15)
    at RegExpValidator.disjunction (.../node_modules/regexpp/index.js:5747:18)
    at RegExpValidator.pattern (.../node_modules/regexpp/index.js:5681:14)
    at RegExpValidator.validatePattern (.../node_modules/regexpp/index.js:5461:14)
    at RegExpParser.parsePattern (.../node_modules/regexpp/index.js:6889:25)
    at verify (.../node_modules/eslint/lib/rules/no-misleading-character-class.js:134:40)
    at Literal[regex] (.../node_modules/eslint/lib/rules/no-misleading-character-class.js:168:17)
    at listeners.(anonymous function).forEach.listener (.../node_modules/eslint/lib/linter/safe-emitter.js:45:58)
    at Array.forEach (<anonymous>)
    at Object.emit (.../node_modules/eslint/lib/linter/safe-emitter.js:45:38)

To be clear, this is indeed a syntax error, it's not a false positive on my input code; rather, it's an unhandled error happening inside the rule when a regexp is invalid.

Are you willing to submit a pull request to fix this bug?
Maybe, I don't know how this should be handled.

@Jessidhia Jessidhia added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Aug 26, 2019
@platinumazure
Copy link
Member

Hi @Jessidhia, thanks for the issue.

I think at the very least, we should wrap the regex parse in a try/catch statement and then bail out gracefully if we can.

I'm very curious to know what happens in the default parser. Seems to me this should be a parse error, even before any rules are run.

@platinumazure platinumazure added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Aug 26, 2019
@mdjermanovic
Copy link
Member

It is indeed parsing error in the default parser (demo)

I believe this is a bug in @typescript-eslint/parser, it shouldn't allow invalid code.

@kaicataldo
Copy link
Member

Sounds like this should be filed in the @typescript-eslint/parser repo.

@mysticatea
Copy link
Member

mysticatea commented Aug 27, 2019

I agree that this is a problem in @typescript-eslint/parser for now. But once RFC 19 was implemented, it will be a problem in that rule. (because RegExp's syntax error is recoverable.)

@platinumazure
Copy link
Member

platinumazure commented Aug 27, 2019

Based on @mysticatea's last comment, I think we should accept this bug and put a try/catch around the regex parse.

We would need to figure out how best to handle these cases: Either ignore that regex, or emit a lint warning that the regex can't be parsed.

@mysticatea mysticatea added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Aug 27, 2019
@mysticatea
Copy link
Member

mysticatea commented Aug 27, 2019

I think that ignoring syntax errors makes sense. Syntax errors should be reported by parsers and the purpose of no-misleading-character-class rule is to report the character classes that behave unexpectedly due to multiple code unit characters.

@mdjermanovic
Copy link
Member

The rule will also crash with the default parser on this:

/*eslint no-misleading-character-class: error */

new RegExp("{", "u");

so this isn't just an issue with @typescript-eslint/parser

The other 3 rules that use regexpp (no-control-regex, no-invalid-regexp and prefer-named-capture-group) already have try/catch.

@Jessidhia would you like to submit a PR for this fix?

@kaicataldo kaicataldo added the help wanted The team would welcome a contribution from the community for this issue label Sep 30, 2019
@mdjermanovic mdjermanovic removed the help wanted The team would welcome a contribution from the community for this issue label Oct 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.