Tools: ☂️ Implement Regex Rules

[Kelbie]

Followed is a list of ESLint rules that we should implement.

• [x] groupopen Unmatched opening parenthesis.
• [x] groupclose Unmatched closing parenthesis.
• [x] setopen Unmatched opening square bracket.
  Example @baryla #176
• [x] rangerev Range values reversed. Start char code is greater than end char code.
• [x] quanttarg Invalid target for quantifier. @CaffeinateOften #189
• [x] quantrev Quantifier minimum is greater than maximum. @KevinKelbie #179 #249
• [x] esccharopen Dangling backslash. @diokey #181 #218
• [ ] esccharbad Unrecognized or malformed escape character. @ParkourKarthik
• [ ] esccharbad Invalid escape sequence. @ParkourKarthik
• [ ] unicodebad Unrecognized unicode category or script. @zinyando
• [ ] posixcharclassbad Unrecognized POSIX character class. @wgardiner
• [ ] posixcharclassnoset POSIX character class must be in a character set. @wgardiner
• [ ] notsupported The "{{~getLabel()}}" feature is not supported in this flavor of RegEx.
• [x] extraelse Extra else in conditional group. @zinyando #170
• [x] unmatchedref Reference to non-existent group "{{name}}". @diokey #197
• [x] modebad Unrecognized mode flag "{{errmode}}". @macovedj #195
• [ ] badname Group name can not start with a digit. @tatchi
• [x] dupname Duplicate group name. @tatchi #166
• [ ] branchreseterr Branch Reset. Results will be ok, but RegExr's parser does not number branch reset groups correctly.
• [ ] timeout The expression took longer than 250ms to execute.
• [ ] jsfuture The "{{~getLabel()}}" feature may not be supported in all browsers.
• [ ] infinite The expression can return empty matches, and may match infinitely in some use cases. @1ntEgr8
• [ ] fwdslash Unescaped forward slash. This may cause issues if copying/pasting this expression into code.

Please comment if you want to work on one of these and I'll mark you to avoid duplicating work. Mentions next to rules indicate that someone else is working on it. If a PR isn't opened within a few days then it will be up for grabs.

A checkmark indicates an open PR.

A reference implementation of these features can be found here

[Kelbie]

@sebmck let me know if there are any rules we shouldn't implement that are listed here.

[zinyando]

@KevinKelbie I'd love to help work on these but I'm not sure what they are, can you give me a quick summary and a link to an example implementation in Rome. Which file do we put the rules in also?

[tatchi]

Hey,

I would like to work on dupname :)

[Kelbie]

@zinyando I think the main file you want to look at is here because that's were the other rules are located such as Invalid capture group modifier, Unclosed group and Unclosed character set. Additionally there is another project which has implemented a lot of these features which you can use as a guide. I've not implemented a regex rule myself and am just peicing together what I can so if anyone knows more than I do please correct me.

[sebmck]

@tatchi Tip: Use the RegExpGroupCapture node and name property

[sebmck]

Note that there are also some missing syntax implementation in the regex parser. Notably some unicode escapes and backreferences. Happy to accept PRs if anyone wants to implement them.

[baryla]

I'll have a go at the fwdslash one 🙂

[sebmck]

Unclear how the fwdslash one would work since it will be a JS syntax error. I believe that rule only exists in regexr because you paste in regex bodies.

[baryla]

@sebmck that is a fair shout. Should that be taken out then since it may not apply to Rome?

[baryla]

If we're unsure how to tackle fwdslash or if it should be implemented into the regex rules, I can look at setopen in the meantime :)

[stephenwf]

Are RegExp constructor calls (new Regex('...')) ignored?

[sebmck]

@stephenwf They are. I think we should leave them as-is for the timebeing. We should figure out some strategy to extract the regex, explode it into a RegExpLiteral, run the compiler over it, and then reserialize it. It would allow us to have transforms and lints "just work" and avoid having to special case it and risk performance regressions by having to parse it all over the place.

[tatchi]

Hey,

I can work on the badname one :)

[diokey]

Hi there,
I'd like to try esccharopen

[zinyando]

I'll try tackling extraelse

[zinyando]

I'll take on unicodebad

[Kelbie]

I'll do quantrev

[wgardiner]

@KevinKelbie are POSIX character classes supported in javascript regex? It looks like Regexr only supports them for the PCRE engine.

[Kelbie]

@wgardiner that does seem to be the case. Could we still add some linting to tell the user that its not a supported feature in the javascript version of regex?

[wgardiner]

@KevinKelbie Sure, I can add a lint rule to throw an error for POSIX character classes and POSIX collating sequences

[ghost]

I'll take a stab at quanttarg.

I need to check to see if the unquantifiable types are defined but if not hopefully I can add those.

[Kelbie]

@CaffeinateOften I'll probably make a PR for quantrev today. Just so you know there are currently some bugs with the quantifier node but I've created fixes for them so that should help with your lint rule.

[diokey]

I'd like to try unmatchedref next

[ParkourKarthik]

I'd love to contribute. I'll pick up esccharbad. I need some help on where I could start and other specific references if any.

[macovedj]

Guess I'll jump in. Can I claim modebad?

[macovedj]

I might not understand badmode correctly, but it appears that this function actually does what I expected this issue to be about.

[Kelbie]

@macovedj I think your right however I find the error message to not be that helpful. It would be nice if the underline was under the incorrect flags rather than the start of the regex. What do you think?

[1ntEgr8]

I can pick up timeout!

[ghost]

I never commented here but I opened a PR for quanttarg #189 a few days ago!

[macovedj]

@KevinKelbie I think that mode-bad became obsolete as the underline no longer appears after changes made by @sebmck, and instead there is a color change in the text that correctly is applied to the first problematic flag. If you agree then we can close https://github.com/facebookexperimental/rome/pull/195 and if there's something else you think should be done then let me know.

[Kelbie]

@macovedj sorry for the late response, the underline still appears for me on master. The colour change is the autofixer and both appear for me.

[macovedj]

@KevinKelbie Have you seen that is ready for review again? Also, is my understanding of branchreseterr correct in that you basically just want us to throw an error whenever anybody attempts to use a branch reset group? If so I could pick that up, or maybe another after a question or two. Finally, do we have a way of knowing which items have been claimed but aren't being acted on? Also curious if this is knowable for eslint @sebmck. Asking mostly because I'd like to pick up more work, but don't feel that I have sufficient context on the formatter and don't know that I see other issues that I know enough about to grab, rather than because I'd like people to hurry up. I'd hate for anybody to feel rushed by this comment. Still learning proper open source etiquette.

[Kelbie]

Have you seen that is ready for review again?

Awesome!

Finally, do we have a way of knowing which items have been claimed but aren't being acted on?

It's been almost a month since we have heard anything from some of the assignees so I'd be happy to re-assign someone else to some of these issues. If anyone is stuggling with implementing their rule they have taken I'm sure some people on the discord would be able to point you in the right direction.

If you are taking on an already assigned rule then perhaps you could tag the assignee to let them know and wait a day to let them respond.

[sebmck]

Feel free to take on rules that have been assigned but have had no movement on.

[macovedj]

Cool I'll probably start with some of the non-regex ones since they're older.

[VictorHom]

~Can I work on rangerev? (I see the checkmark, but not sure if there is a pr)~

turns out the rule was completed already

[sebmck]

Copying what I mentioned in #20

I think this can be closed out in favor of more specific issues. I don't think we should focus on other general JS lint rules at this time. Let's move towards a release! Thanks everyone for your contributions so far. I encourage you to get involved in #341 (React lint rules), #18 (Published release) or #496 (Website for release).

Thank you everyone!