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

Improved regexp/strict rule #225

Merged
merged 3 commits into from
Jun 4, 2021
Merged

Improved regexp/strict rule #225

merged 3 commits into from
Jun 4, 2021

Conversation

RunDevelopment
Copy link
Collaborator

This adds the logic I was talking about here. Many reports are fixable. Only reports that are likely a but are not fixable.

However, there is still one problem with this: the tests don't pass because RegExpValidator says that /(?<foo>)\k<foo>/ isn't valid. I have no idea why it rejects that regex. Do you know why @ota-meshi?

@RunDevelopment RunDevelopment requested a review from ota-meshi June 2, 2021 14:27
@ota-meshi
Copy link
Owner

/(?<foo>)\k<foo>/ isn't valid

I think it's probably an invalid regular expression because of backward compatibility.

However, the named backreference syntax, /\k<foo>/, is currently permitted in non-Unicode RegExps and matches the literal string "k<foo>". In Unicode RegExps, such escapes are banned.

https://github.com/tc39/proposal-regexp-named-groups#backwards-compatibility-of-new-syntax

I'm not sure if it's valid as a strict non-Unicode regular expression.

@RunDevelopment
Copy link
Collaborator Author

RunDevelopment commented Jun 3, 2021

v8 will happily execute that regex. Example:

Node.js v14.15.4.
> /^(?<foo>A)\k<foo>$/.test("AA")
true

It should be valid because:

In this proposal, \k<foo> in non-Unicode RegExps will continue to match the literal string "k<foo>" unless the RegExp contains a named group, in which case it will match that group or be a syntax error [...]

And I think, I figured out why regexpp throws an error. It's a bug in regexpp's validator: mysticatea/regexpp#23.

Now we have two option:

  1. Wait until the bug is fixed.
  2. Publish regexp/strict without RegExpValidator and re-add it later when the bug is fixed.

Which one do we want to choose?

@ota-meshi
Copy link
Owner

I heard a rumor that mysticatea seems to be busy these days and doesn't have time to maintain OSS.
I don't know if he will reply, but I will try to send him a message via chat.

@RunDevelopment
Copy link
Collaborator Author

I see. Thank you @ota-meshi!

In that case, I'll disable the RegExpValidator-based check for patterns with named backreferences. We can re-add it as soon as the bug is fixed.

Copy link
Owner

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

Thank you very much!

@ota-meshi ota-meshi merged commit 637ee67 into strict Jun 4, 2021
@ota-meshi ota-meshi deleted the strict-improvements branch June 4, 2021 02:46
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.

2 participants