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

Detect lone or unescaped brackets and braces when the unicode flag is set #100

Merged
merged 2 commits into from
Feb 14, 2020

Conversation

pygy
Copy link
Contributor

@pygy pygy commented Feb 13, 2020

Hi @jviereck

This fixes mathiasbynens/regexpu-core#36 and thus babel/babel#11138

Edit: force push for code styling reasons.

@pygy
Copy link
Contributor Author

pygy commented Feb 13, 2020

Actually, wait... This will cause a Regexpocalypse that will not be easy to fix once this is live (because a codemod would rely on Babel, which would catch the update automatically because the way both Babel and regexpu-core rely on semver).

@nicolo-ribaudo
Copy link
Collaborator

I don't think that the situation is as tragic as you picture it, for a few reasons:

  1. This is an edge case.
  2. This is a normal bug: if someone relies on the buggy behavior, the burden of avoiding breakages is on them. Otherwise, every bugfix should be considered a breaking change.
  3. This package already had some bugfixes which caused more errors, like Check value of unicode code point escapes #91, and no one complained
  4. When people will stop transpiling /u (hopefully soon, since it's supported by every browser version with > 1.5% market share), their code will be broken regardless of this PR.

@pygy
Copy link
Contributor Author

pygy commented Feb 13, 2020

Let's roll with it then :-)

@jviereck jviereck merged commit 0f22de3 into jviereck:gh-pages Feb 14, 2020
@jviereck
Copy link
Owner

Thanks @pygy for providing this!

I agree with the arguments outlined by @nicolo-ribaudo and therefore accepted this PR.

@jviereck
Copy link
Owner

Published new release to npm as v0.6.3 - https://www.npmjs.com/package/regjsparser/v/0.6.3.

Thanks once again @pygy !

@jens-duttke
Copy link

Please see issue #101 . This change introduced a bug.

@jviereck
Copy link
Owner

jviereck commented Mar 6, 2020

Thanks for reporting this. I most likely have some time over the weekend to look into this.

@nicolo-ribaudo, @pygy, @jens-duttke Do you think it's better in the meantime to make a new release with the change removed for now?

@nicolo-ribaudo
Copy link
Collaborator

I think that it's ok to wait a few days. If you end up not having time to fix it please ping me!

@jviereck
Copy link
Owner

jviereck commented Mar 7, 2020

Digging into this, when reading the JS RegExp gramma, I am not sure why /}/u is supposed to be rejected. That is, I think this should parse as:

...
Atom
PatternCharacter
SourceCharacter

But SourceCharacter has no quantification on the unicode u flag. See the documentation here:

https://www.ecma-international.org/ecma-262/10.0/index.html#prod-PatternCharacter

Can someone give me a pointer what I am missing here?

Once it's clear to me why /}/u is supposed to be rejected, I hope to be able to fix the regression in #101 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rewritePattern('}', 'u') should throw, but it doesn't
4 participants