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` does not allow "y"/"sticky" flag #1537

Closed
ljharb opened this Issue Dec 1, 2014 · 15 comments

Comments

Projects
None yet
2 participants
@ljharb
Contributor

ljharb commented Dec 1, 2014

Per https://people.mozilla.org/~jorendorff/es6-draft.html#sec-get-regexp.prototype.sticky "y" is a valid flag in ES6, and new RegExp('a', 'y') should not trigger no-invalid-regexp when the dev wants to allow that flag.

To avoid allowing "y" in pre-ES6 environments, perhaps this could be configurable in the rule, eg, a "flags" string/array?

(eslint v0.10.0)

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Dec 1, 2014

Member

This won't work in 0.10.0 because it doesn't have ES6 support. I'll make sure it works in the es6jsx branch. This is something the new validator should catch.

Member

nzakas commented Dec 1, 2014

This won't work in 0.10.0 because it doesn't have ES6 support. I'll make sure it works in the es6jsx branch. This is something the new validator should catch.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Dec 1, 2014

Contributor

Great, thanks. Will that branch be eventually merged with master?

Contributor

ljharb commented Dec 1, 2014

Great, thanks. Will that branch be eventually merged with master?

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Dec 1, 2014

Member

Yup. We just need an esprima fix before that can happen.

Member

nzakas commented Dec 1, 2014

Yup. We just need an esprima fix before that can happen.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Dec 13, 2014

Member

Slight change of plans, we're going to move to our own parser. I've opened an issue to add support for both y and u: eslint/espree#9

Member

nzakas commented Dec 13, 2014

Slight change of plans, we're going to move to our own parser. I've opened an issue to add support for both y and u: eslint/espree#9

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Jan 11, 2015

Member

You can now enable this using the ecmaFeatures property in your config file:
https://github.com/eslint/eslint/tree/master/docs/configuring

(Checked into master)

Member

nzakas commented Jan 11, 2015

You can now enable this using the ecmaFeatures property in your config file:
https://github.com/eslint/eslint/tree/master/docs/configuring

(Checked into master)

@nzakas nzakas closed this Jan 11, 2015

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Jan 13, 2015

Contributor

I'm running eslint off of the latest master, and I've added:

"ecmaFeatures": {
  "regexYFlag": true,
  "regexUFlag": true
}

to my .eslintrc file, but I'm still getting these errors. Is there something else I need to do to enable it?

Contributor

ljharb commented Jan 13, 2015

I'm running eslint off of the latest master, and I've added:

"ecmaFeatures": {
  "regexYFlag": true,
  "regexUFlag": true
}

to my .eslintrc file, but I'm still getting these errors. Is there something else I need to do to enable it?

@nzakas nzakas reopened this Jan 13, 2015

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Jan 13, 2015

Member

Hmmm, no, that should work. Reopening to investigate.

Member

nzakas commented Jan 13, 2015

Hmmm, no, that should work. Reopening to investigate.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Jan 13, 2015

Contributor

If it becomes necessary I can push a temporary branch for you to look at, but I'd rather not since it's still in progress :-)

Contributor

ljharb commented Jan 13, 2015

If it becomes necessary I can push a temporary branch for you to look at, but I'd rather not since it's still in progress :-)

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Jan 13, 2015

Member

Not necessary. Can you post what's output to the console and the line that has the regex on it?

Member

nzakas commented Jan 13, 2015

Not necessary. Can you post what's output to the console and the line that has the regex on it?

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Jan 13, 2015

Contributor
   95:15  error  Invalid flags supplied to RegExp constructor 'y'     no-invalid-regexp
   98:15  error  Invalid flags supplied to RegExp constructor 'u'     no-invalid-regexp
  119:15  error  Invalid flags supplied to RegExp constructor 'mygi'  no-invalid-regexp
  122:15  error  Invalid flags supplied to RegExp constructor 'mugi'  no-invalid-regexp

And the resulting lines:

 95: expect(new RegExp('a', 'y').flags).to.equal('y');
 98: expect(new RegExp('a', 'u').flags).to.equal('u');
119: expect(new RegExp('a', 'mygi').flags).to.equal('gimy');
122: expect(new RegExp('a', 'mugi').flags).to.equal('gimu');

(I don't actually have any /a/y literal regexes, because they'd be a syntax error in older browsers - those are inside Function('return /a/y') so I can catch the thrown error. While it would be awesome if eslint could report on function bodies inside Function, I doubt it's practical)

Contributor

ljharb commented Jan 13, 2015

   95:15  error  Invalid flags supplied to RegExp constructor 'y'     no-invalid-regexp
   98:15  error  Invalid flags supplied to RegExp constructor 'u'     no-invalid-regexp
  119:15  error  Invalid flags supplied to RegExp constructor 'mygi'  no-invalid-regexp
  122:15  error  Invalid flags supplied to RegExp constructor 'mugi'  no-invalid-regexp

And the resulting lines:

 95: expect(new RegExp('a', 'y').flags).to.equal('y');
 98: expect(new RegExp('a', 'u').flags).to.equal('u');
119: expect(new RegExp('a', 'mygi').flags).to.equal('gimy');
122: expect(new RegExp('a', 'mugi').flags).to.equal('gimu');

(I don't actually have any /a/y literal regexes, because they'd be a syntax error in older browsers - those are inside Function('return /a/y') so I can catch the thrown error. While it would be awesome if eslint could report on function bodies inside Function, I doubt it's practical)

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Jan 15, 2015

Member

Working on this.

Member

nzakas commented Jan 15, 2015

Working on this.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Jan 15, 2015

Member

Depends on #1648, so switching to that.

Member

nzakas commented Jan 15, 2015

Depends on #1648, so switching to that.

@nzakas nzakas closed this in c0a7be6 Jan 17, 2015

nzakas added a commit that referenced this issue Jan 17, 2015

Merge pull request #1664 from eslint/issue1537
Fix: Property regex flag checking (fixes #1537)
@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Jan 17, 2015

Contributor

Hooray, you fixed it!

Contributor

ljharb commented Jan 17, 2015

Hooray, you fixed it!

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Jan 17, 2015

Member

Yup, and it was just pushed.

Member

nzakas commented Jan 17, 2015

Yup, and it was just pushed.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Jan 17, 2015

Contributor

To clarify: I saw the push, I immediately tested it, confirmed that it's fixed, and posted my comment :-) Thanks!

Contributor

ljharb commented Jan 17, 2015

To clarify: I saw the push, I immediately tested it, confirmed that it's fixed, and posted my comment :-) Thanks!

Holixus pushed a commit to Holixus/eslint that referenced this issue Feb 12, 2015

ljharb added a commit to ljharb/eslint that referenced this issue Apr 14, 2015

ljharb added a commit to ljharb/eslint that referenced this issue Apr 14, 2015

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.