prefer-regex-literals: should report regex literals passed to RegExp constructor

[lo1tuma]

What rule do you want to change?

prefer-regex-literals

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

It will produce more warnings.

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

I think this makes sense as default behavior.

Please provide some example code that this change will affect:

const foo = new RegExp(/foo/);

What does the rule currently do for this code?

It is not recognized as a problem.

What will the rule do after it's changed?

The rule should report this as problem and suggest the following autofix:

-const foo = new RegExp(/foo/);
+const foo = /foo/;

Are you willing to submit a pull request to implement this change?

Yes

[mdjermanovic]

I think this makes sense. Not sure should it be new default behavior (breaking?) or an option.

Should the rule also report new RegExp(/foo/, "u")?

The rule should report this as problem and suggest the following autofix:

Did you mean "suggest" as the new Suggestions feature, or just the usual autofix?

[kaicataldo]

I support this proposal. I wonder if we should put this behind an option turned off by default even if we land this in the v7 major release and make it the default in a future major release.

[ljharb]

An option in v6, enabled in v7 would be ideal :-) waiting until v8 seems like a long time.

[kaicataldo]

@ljharb We're already in the alpha stage of releasing v7 and don't have any plans to do any more v6 releases.

[lo1tuma]

Did you mean "suggest" as the new Suggestions feature, or just the usual autofix?

I was referring to the usual autofix.

Should the rule also report new RegExp(/foo/, "u")?

I would say this should be reported as well. As long as both arguments are literals. const flags = 'u'; new RegExp(/foo/, flags); should be not reported.

---

Regarding whether this should be the default behavior or an option: I was really surprised to figure out the rule doesn’t already recognize this pattern, so maybe this could be even considered a bug.

[mdjermanovic]

Regarding whether this should be the default behavior or an option: I was really surprised to figure out the rule doesn’t already recognize this pattern, so maybe this could be even considered a bug.

I guess we didn't consider disallowing new RegExp(/foo/) because it looks more like a redundant use of the RegExp constructor rather than a matter of preference.

This is a prefer-* rule so it could be argued that reporting new RegExp(/foo/) might not be its purpose. Though disallowing new RegExp(/foo/, "u") in favor of /foo/u does look to me like something that could be the responsibility of this rule (and then it probably wouldn't make sense to not report new RegExp(/foo/) as well), so I vote for this proposal 👍

[mdjermanovic]

I'd also vote to start with an option instead of the default behavior. Although it isn't likely maybe there could be some use case for these.

[lo1tuma]

I’ve updated the PR and implemented the change behind a new option disallowRedundantWrapping.

[mdjermanovic]

I'll champion this, only one more 👍 needed.

[kaicataldo]

@eslint/eslint-team We need one more 👍 to accept this.

[nzakas]

Done. Marked as accepted.