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

no-invalid-regexp rule doesn't error with example from its documentation #10861

Closed
FloEdelmann opened this issue Sep 16, 2018 · 15 comments

Comments

@FloEdelmann
Copy link

commented Sep 16, 2018

Tell us about your environment

  • ESLint Version: 5.6.0
  • Node Version: 8.12.0
  • npm Version: 6.3.0

What parser (default, Babel-ESLint, etc.) are you using? default

Please show your full configuration:

Configuration
{
  "env": {
    "es6": true,
    "node": true
  },
  "parserOptions": {
    "ecmaVersion": 2017
  },
  "rules": {
    "no-invalid-regexp": "error"
  }
}

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

// like in the no-invalid-regexp documentation:
// https://github.com/eslint/eslint/blob/master/docs/rules/no-invalid-regexp.md#rule-details
new RegExp('\\');
eslint file.js

What did you expect to happen?

eslint failing with an error from the no-invalid-regexp rule.

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

eslint passed with no output.

@eslint eslint bot added the triage label Sep 16, 2018

@FloEdelmann

This comment has been minimized.

Copy link
Author

commented Sep 16, 2018

It was still working with eslint 4.15.0. Could it be a regression introduced in #10062?

@sstern6

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2018

Following up on this, can this be reproduced by an eslint member to be a confirmed bug? If so, Ill work on this.

@not-an-aardvark not-an-aardvark added accepted and removed evaluating labels Oct 3, 2018

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

Thanks for the report, I can reproduce this issue.

@sstern6

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2018

Thanks @not-an-aardvark ill work on this. Might have some questions.

@platinumazure

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

Is this really a bug in the rule? I'm reading the example as a regex which matches against a backslash anywhere in the string.

My understanding is that escaping the backslash would be required in both the RegExp constructor and in a regexp literal:

new RegExp("\\");

/\\/;

If I'm misunderstanding something, I would be grateful to learn what that is. Otherwise, I think this should be a documentation change (to remove the example), not a bugfix.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

When I run that code, I get an error:

SyntaxError: Invalid regular expression: /\/: \ at end of pattern
@mysticatea

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

This is not a bug.

The // is valid Pattern, so new RegExp('\\'); is valid.

Nevermind, I misread it.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

@sstern6

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

@mysticatea im not understanding, how is it a valid pattern when new RegExp("\\"); returns a syntax error?

@mysticatea

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

I'm sorry, I misread it as // 😅

@FloEdelmann

This comment has been minimized.

Copy link
Author

commented Oct 5, 2018

/\/ is an invalid RegExp, as the backslash is not escaped. /\\/ is fine.
In new RegExp('\\'), the backslash is just escaped in the string (in a string, e.g. \n is a valid character), so to escape it in both the string and the RegExp, one has to use new RegExp('\\\\').

@mysticatea

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

So this is a bug in regexpp; it hasn't followed the tc39/ecma262#910 spec update yet because I had overlooked it. I will update it.

@mysticatea mysticatea added the blocked label Oct 5, 2018

mysticatea added a commit to mysticatea/regexpp that referenced this issue Oct 5, 2018

mysticatea added a commit that referenced this issue Oct 5, 2018

@mysticatea mysticatea removed the blocked label Oct 5, 2018

@sstern6

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

@mysticatea I was going to ask if this was a regexpp issue! Thanks for the PR, so when this gets merged in we will need to update the regexpp package and then I was thinking that we should add a test to the test suite to avoid regressions for this use case?

Is this correct?

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

@sstern6 It looks like the fix has already been published as a new version of regexpp. #10920 has the effect of upgrading regexpp and adding a test, as you suggested.

mysticatea added a commit that referenced this issue Oct 9, 2018

@FloEdelmann

This comment has been minimized.

Copy link
Author

commented Oct 9, 2018

Thanks for fixing this!

@eslint eslint bot locked and limited conversation to collaborators Apr 8, 2019

@eslint eslint bot added the archived due to age label Apr 8, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.