prefer-regex-literals suggestions

[Yash-Singh1]

What rule do you want to change?

prefer-regex-literals

Does this change cause the rule to produce more or fewer warnings?

No

How will the change be implemented? (New option, new default behavior, etc.)?

Adds fixer

Please provide some example code that this change will affect:

new RegExp('\\.')

What does the rule currently do for this code?

Reports error

What will the rule do after it's changed?

Fix it to:

/\./

[nzakas]

Seems possible to do.

Are you willing to submit a pull request?

[Yash-Singh1]

Sure, I will give this a shot.

[mdjermanovic]

It would be a useful enhancement.

@Yash-Singh1 any ideas on how this could be implemented?

We generally suggest that contributors wait for the issue to be accepted before working on it, but in this case I think it would be good to evaluate this enhancement on a prototype of the solution.

This is a nontrivial code transformation that should be aware of all differences between the syntax of regular expression literals and the syntax of RegExp constructor input patterns.

For example:

new RegExp("\n");

It's a valid RegExp input pattern consisting of a single code point for \n, but in regular expression literals it would be invalid:

/
/

One of the possible solutions is to use the built-in RegExp.prototype.toString. By the specification, it should be safe:

The returned String has the form of a RegularExpressionLiteral that evaluates to another RegExp object with the same behaviour as this object.

[...new RegExp("\n").toString()]; // ['/', '\', 'n', '/']

Some downsides of this solution would be:

• It depends on the execution environment - if the environment doesn't support syntax used in the given regular expression, we can't transform.
• It doesn't preserve syntactic choices from the original string literal:

[...new RegExp("\t").toString()]; // ['/', '\t', '/'] - this would be /    / in the source code, valid but unusual

[...new RegExp("\u1234").toString()]; // ['/', 'ሴ', '/']

[nzakas]

I think we can simplify this proposal such that we eliminate any regular expressions that would be problematic. Maybe to start we just say anything with \t, \u, \n, etc. won't be autofixed?

[lydell]

/ also sometimes needs to be escaped.

Another thing: For example RegExp("+") would throw a runtime error. Converting that to /+/ would now be a syntax error. Is it ok for an autofix to introduce syntax errors?

Similarly, RegExp("*") -> /*/ would start a block comment, which – in the worst case – could be closed by some other */ sequence and still produce “valid” code. Confusing!

So the autofix might need to know regex syntax. Just thinking out loud.

We also need to be careful so that 5/RegExp("a") isn’t converted to 5//a/. That’s a single-line comment now.

[nzakas]

@lydell good points!

Given all the edge cases we’ve identified, so we feel like it’s worth autofixing at this point?

[Yash-Singh1]

Sorry for the late reply, I was working on the changes and doing a bit of research on how to implement this. So far I got the basic functionality written, I just need to work on the edge cases.

So the autofix might need to know regex syntax.

Yes, there should be some form of validation for the regular expression. Since ESLint already depends on regexpp and it seems to have a validator function. There is an option on the ecmaVersion:

https://github.com/mysticatea/regexpp/blob/master/src/validator.ts#L137

And I think we can pass parserOptions.ecmaVersion to the function and validate the regular expression string.

We also need to be careful so that 5/RegExp("a") isn’t converted to 5//a/. That’s a single-line comment now.

Yes, that will ensure checking for division and not autofixing when there is division. But should your example: 5/RegExp("abc") autofix to 5/RegExp(/abc/) or 5/(/abc/)?

[lydell]

But should your example: 5/RegExp("abc") autofix to 5/RegExp(/a/) or (/a/)?

Or 5/ /a/ (space)

[ljharb]

Hopefully it'd autofix to 5/(/abc/), since /a/ doesn't match the original pattern.

[lydell]

That was an obvious typo to me. Of course the discussion is about which solution to use for the “division plus regex should not become a singleline comment” edge case.

[ljharb]

Right - but what would be the point of converting RegExp('abc') to RegExp(/abc/)? The RegExp constructor is redundant there. I think it'd convert to /abc/, and only when necessary, would be parenthesized (like when it's the divisor, or when the next line has an identifier that might be interpreted as flags, or something)

[mdjermanovic]

We can make a list of safe preceding tokens, e.g., = and ( will cover most common cases. We could add to the list other tokens for which we are sure that scanning cannot interpret our regex literal as something else. In all other cases we can wrap the literal in (), it doesn't have to be perfect.

As for the right side, I think the only problem is adjacent words like in. This can be handled by adding a space.

[mdjermanovic]

I think this feature should be suggestion, rather than autofix. Often there are multiple valid patterns with the same behavior, so if user doesn't like the suggested one, they can opt to not apply it. The suggested fix should still produce valid and equivalent code, but it doesn't necessarily have to be the preferred variant, since we might not always be able to figure out which one is it.

Here's a babel repl with an example of this transformation (babel-plugin-transform-regexp-constructors). /ሴ/ is a valid replacement for RegExp("\u1234"), but probably not the preferred one.

Also, writing regex patterns in string literals is generally error-prone. For example, RegExp("a\.b") is a common mistake reported by no-useless-escape. If we autofix this to /a.b/, the error will be hidden.

[Yash-Singh1]

A few more edge cases: /abc/*something would start a block comment. /abc//something would start a single-line comment.

[mdjermanovic]

A few more edge cases: /abc/*something would start a block comment. /abc//something would start a single-line comment.

I think this can happen only if the leftmost / was already interpreted as division, which depends only on the code before it, so the adjacent *something or /something on the right side are not causing the problem.

[mdjermanovic]

I'm not too concerned about the part where we are inserting /foo/ in the place of RegExp("foo") in the source code. We can make a list of tokens that cannot be at the end of expressions (that's where / would be seen as division) and check if the token before RegExp("foo") is in the list. If it is, then it's safe to insert /foo/ without parentheses, and astUtils.canTokensBeAdjacent can tell us if it needs spaces on both ends. If it isn't in the list, we'll insert (/foo/). This check doesn't have to be exhaustive (I believe only =, ( and , would cover the majority of cases where regular expressions appear in a source code), the worst case is that we'll sometimes insert a pair of parentheses that aren't really necessary.

I'm more concerned about how to get /foo/ from RegExp("foo"). We have a string value of the pattern and a raw string literal, but neither of them can be used directly as the content between /.../

[mdjermanovic]

Another thing: For example RegExp("+") would throw a runtime error. Converting that to /+/ would now be a syntax error. Is it ok for an autofix to introduce syntax errors?

That wouldn't be ok, RegExp("+") can be in a dead code or wrapped in try-catch, while /+/ breaks parsing and thus the whole script (or at least it should by the spec, in V8 it's a runtime error). We should only fix valid regular expressions.

[Yash-Singh1]

Should RegExp(1) autofix to /1/? Currently, the prefer-regex-literals rule only detects strings.

[ljharb]

Yes, I’d think so.

[Yash-Singh1]

I'm not too concerned about the part where we are inserting /foo/ in the place of RegExp("foo") in the source code. We can make a list of tokens that cannot be at the end of expressions (that's where / would be seen as division) and check if the token before RegExp("foo") is in the list. If it is, then it's safe to insert /foo/ without parentheses
...
If it isn't in the list, we'll insert (/foo/). This check doesn't have to be exhaustive (I believe only =, ( and , would cover the majority of cases where regular expressions appear in a source code), the worst case is that we'll sometimes insert a pair of parentheses that aren't really necessary.

I am just curious, instead of whitelisting tokens, why can't we disallow the / token instead, because that would cover more edge case tokens.

, and astUtils.canTokensBeAdjacent can tell us if it needs spaces on both ends.

Can you help me with an example where we would require space on both ends?

[mdjermanovic]

Should RegExp(1) autofix to /1/? Currently, the prefer-regex-literals rule only detects strings.

No, if it isn't reported then it shouldn't be fixed. We could discuss in a separate issue whether or not this should be reported.

[mdjermanovic]

, and astUtils.canTokensBeAdjacent can tell us if it needs spaces on both ends.

Can you help me with an example where we would require space on both ends?

a/RegExp("foo")in b would need spaces on both ends: a/ /foo/ in b

Without spaces, it would be a//foo/in b - which is a followed by a line comment. Because of the adjacent / on the left side, we have to add a space on the left, and then it becomes a/ /foo/in b, but this would be interpreted as a regular expression /foo/in ("SyntaxError: Invalid regular expression flags"), so we have to add a space on the right side too.

[mdjermanovic]

I am just curious, instead of whitelisting tokens, why can't we disallow the / token instead, because that would cover more edge case tokens.

/ can be in the list of allowed tokens, it just needs a space.

Disallowed preceding tokens should be ), literals, identifiers etc., for example:

x = y
RegExp("foo").test(x) ? bar() : baz()

When autofixed to /foo/, this becomes:

x = y
/foo/.test(x) ? bar() : baz()

This is interpreted as x = y / foo...

Now I realize that parentheses wouldn't solve the problem as they would be interpreted as function call, so it's probably best to not autofix the RegExp if it isn't preceded by one of the allowed tokens (=, (, ;, ,, ...).

[github-actions]

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

[ljharb]

There's an open PR: #15077

[nzakas]

@mdjermanovic seems like you were driving this. How should we proceed?

[mdjermanovic]

By the proof of concept #15077, this looks feasible and safe to me. I support this change, and I'm willing to champion it.

[nzakas]

I’ll 👍