Fix: no-regex-spaces false positives and invalid autofix (fixes #12226)

[mdjermanovic]

What is the purpose of this pull request? (put an "X" next to item)

[X] Bug fix #12226 + 2 other bugs.

This PR fixes 3 unrelated bugs in no-regex-spaces:

1. False positives in character classes (e.g. /[ ]/ fixed to /[ {2}]/)
2. Invalid autofix for strings with escape sequences (e.g. RegExp('\\d ') fixed to RegExp('\\ {2} '))
3. Regex with invalid syntax were reported and fixed (e.g. RegExp('[ ') fixed to RegExp('[ {2}')). This might not be a bug, but I guess it's better to ignore invalid regex.

1. Character classes

The following tests were failing:

// valid[]
"var foo = /[  ]/;",
"var foo = /[   ]/;",
"var foo = / [  ] /;",
"var foo = new RegExp('[  ]');",
"var foo = new RegExp('[   ]');",
"var foo = new RegExp(' [  ] ');",

// invalid[]
"var foo = /[   ]  /;",
"var foo = new RegExp('[   ]  ');"

The code is now using regexpp parser to target only spaces with "Alternative" parent.

Example:

/* eslint no-regex-spaces:error */

var foo = /[  ]/;
var foo = new RegExp('[   ]  ');

Previous fix:

/* eslint no-regex-spaces:error */

var foo = /[ {2}]/; // incorrect
var foo = new RegExp('[ {3}] {2}'); // incorrect

New behavior:

/* eslint no-regex-spaces:error */

var foo = /[  ]/; // no error
var foo = new RegExp('[   ] {2}'); // error and autofix, but not inside []

2. Strings with escape sequences

It was assumed that the indexes are same as in the string's source code literal presentation.

The following tests were failing:

// valid[]
"var foo = new RegExp('bar \\ baz')",
"var foo = new RegExp('bar\\ \\ baz')",
"var foo = new RegExp('bar \\u0020 baz')",
"var foo = new RegExp('bar\\u0020\\u0020baz')",

// invalid[]
"var foo = new RegExp('\\\\d  ')"
"var foo = RegExp('\\u0041   ')"

The logic is now changed to check for consecutive spaces in the source code (Node.raw), and also don't fix if the raw presentation is not same as the internal.

Example:

/* eslint no-regex-spaces:error */

var foo = new RegExp('bar \ baz');
var foo = new RegExp('\\d  ');

Previous fix:

/* eslint no-regex-spaces:error */

var foo = new RegExp('bar {2} baz'); // incorrect, requires 3 spaces
var foo = new RegExp('\\ {2} '); // incorrect, missing 'd'

New behavior:

/* eslint no-regex-spaces:error */

var foo = new RegExp('bar \ baz'); // no error, there are no consecutive spaces in the source
var foo = new RegExp('\\d  '); // error, but no autofix

3. Invalid syntax

The following tests were failing:

// valid[]
"var foo = new RegExp('[  ');",
"var foo = new RegExp('{  ', 'u');"

Example:

/* eslint no-regex-spaces:error */

var foo = new RegExp('[  ');

Previous fix:

/* eslint no-regex-spaces:error */

var foo = new RegExp('[ {2}');

New behavior:

/* eslint no-regex-spaces:error */

var foo = new RegExp('[  '); // no error

What changes did you make? (Give an overview)

Use regexpp to parse, don't fix strings that have a different raw representation, don't report regex with invalid syntax.

Is there anything you'd like reviewers to focus on?

Everything please, I didn't use regexpp before.

The cases in 2. could be auto-fixed but that would require a utility to map indexes (that could be a future enhancement, I believe at the moment it's good enough to prevent invalid autofix).

There are some edge cases where the rule will report an error, e.g. RegExp('[ ]\\u0020\\u0020'), but that would require the same utility.

[mdjermanovic]

Marking as accepted as the bug 1. is accepted in #12226, just to note that this PR additionaly:

• Disables autofix in certain situations (2.)
• Ignores invalid regex (3.).

@platinumazure there is a TODO in the code, I'm not sure is it already fixed in #7053?

[platinumazure]

The TODO can be removed. Thanks!

It looks like the rule is intentionally only reporting one violation and relying on multipass autofix to find multiple violations, is that correct?

Otherwise LGTM, thanks!

[platinumazure]

You are right, this TODO can be removed. Thanks!

[mdjermanovic]

TODO is removed now :)

[mdjermanovic]

It looks like the rule is intentionally only reporting one violation and relying on multipass autofix to find multiple violations, is that correct?

Didn't want to change that, I think it was intentional because the messages can't be more descriptive and the whole range of the node (literal/call/new) is reported, so it could be confusing to see multiple similar messages for the same node.

[platinumazure]

Thanks @mdjermanovic. I think it would be awesome to try to report smaller ranges as a future enhancement.

I'll re-review shortly.

[platinumazure]

LGTM, thanks!