Rule Change: Suggestions for require-unicode-regexp

[Yash-Singh1]

What rule do you want to change?

require-unicode-regexp

What change to do you want to make?

Implement suggestions

How do you think the change should be implemented?

A new default behavior

Example code

/abc/

What does the rule currently do for this code?

Reports error

What will the rule do after it's changed?

Give user suggestion prompt to convert into:

/abc/u

Participation

• I am willing to submit a pull request to implement this change.

Additional comments

N/A

[ljharb]

This seems very dangerous, unless it only does the autofix when the u flag would not change semantics. Are you envisioning it being able to detect the "same semantic" cases?

[Yash-Singh1]

Are you envisioning it being able to detect the "same semantic" cases?

I didn’t take in the fact that this flag has side effects when I was writing this issue, but yes, as long as adding the u flag won’t change the functionality of the regular expression.

[mdjermanovic]

This is doable, but seems very complicated.

Pattern should not contain anything that matches surrogate code units. That includes ., \D, \S, \W, negated character classes, etc. Ranges should be checked, too.

The i flag has a different mapping when u flag is set.

Also, some patterns would have completely different semantics with the u flag, e.g. /\u{1}/.

[Yash-Singh1]

So, for the fact that it disables Annex B extensions, that should mean that we have to check the regular expression and make sure it isn't something like /\w{1, 2/u, and that could be done by running regexpp in strict mode. Also, about the surrogate code units, we can probably traverse through the AST generated by regexpp and look for built-in character sets with the negate boolean set to true and validate character classes.

[nzakas]

This seems like a risky thing to autofix. Maybe a suggestion would be better?

[nzakas]

@eslint/eslint-tsc do we want to make a change here?

[btmills]

I’m 👍 to suggestion instead of fix.

[mdjermanovic]

Ideally, the rule would autofix regular expressions that will have the same behavior with the u flag. For those that will have different behavior, the rule could provide suggestions with a note that the behavior will change after the suggestion is applied.

If we're not confident that we can determine whether or not the behavior will change, then providing suggestions in both cases might be safer. However, I think that the suggestion message (or the error message itself) should state whether the behavior will change. Otherwise, user may just apply it without thinking about if there's anything that should be additionally changed in the pattern or in the way the regex is used. But, that brings basically the same question as with the autofix - can we reliably know if the behavior will change when the u flag is added?

[nzakas]

However, I think that the suggestion message (or the error message itself) should state whether the behavior will change.

I think this is asking too much. The point of suggestions is to provide potential fixes that might change behavior, but I think it’s beyond our scope to indicate how behavior might change. At some point we need to trust that developers have tests that will help them identify runtime errors.

[mdjermanovic]

However, I think that the suggestion message (or the error message itself) should state whether the behavior will change.

I think this is asking too much. The point of suggestions is to provide potential fixes that might change behavior, but I think it’s beyond our scope to indicate how behavior might change. At some point we need to trust that developers have tests that will help them identify runtime errors.

In that case, the suggestion message would always warn that the suggested fix may change behavior?

[ljharb]

Isn’t that implied?

[nzakas]

In that case, the suggestion message would always warn that the suggested fix may change behavior?

No. This is always implied with suggestions. If they didn’t change behavior, then they would be an autofix. The implication of suggestions is that it might change behavior and you should only apply it if you’re sure of what you’re doing. There isn’t any need to add this into every suggestion.

[mdjermanovic]

It isn't implied. Suggestion can retain the same behavior, but it isn't autofix when it doesn't seem that the behavior was intended. In core rules, we have 3 suggestions that don't change behavior:

eslint/lib/rules/no-unsafe-negation.js

Lines 79 to 80 in 60f6a06

	suggestNegatedExpression: "Negate '{{operator}}' expression instead of its left operand. This changes the current behavior.",
	suggestParenthesisedNegation: "Wrap negation in '()' to make the intention explicit. This preserves the current behavior."

eslint/lib/rules/no-nonoctal-decimal-escape.js

Lines 46 to 47 in 60f6a06

	refactor: "Replace '{{original}}' with '{{replacement}}'. This maintains the current functionality.",
	escapeBackslash: "Replace '{{original}}' with '{{replacement}}' to include the actual backslash character."

eslint/lib/rules/no-useless-escape.js

Lines 96 to 97 in 60f6a06

	removeEscape: "Remove the `\\`. This maintains the current functionality.",
	escapeBackslash: "Replace the `\\` with `\\\\` to include the actual backslash character."

[nzakas]

Sorry, I’m not quite sure what you’re arguing here. Suggestions don’t need to change behavior, we just know that they might. This is from our website (https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions):

In some cases fixes aren't appropriate to be automatically applied, for example, if a fix potentially changes functionality or if there are multiple valid ways to fix a rule depending on the implementation intent (see the best practices for applying fixes listed above). In these cases, there is an alternative suggest option on context.report() that allows other tools, such as editors, to expose helpers for users to manually apply a suggestion.

My overall point is that many suggestions change behavior. We can’t start adding this message into one suggestion because then it implies all other suggestions without that message won’t change behavior, which is clearly wrong. So then we would have to update all suggestions to clearly state whether they might change behavior, which we could do for core rules but not for plugin rules. I’d prefer we just not put it in the message and just make sure our documentation about what suggestions are is accurate.

[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.

[mdjermanovic]

Since we have accepted the same change in the no-misleading-character-class rule (#15279), it makes the most sense to accept this change as well.

[nzakas]

Sounds good.