Update: Allow regex in no-param-reassign ignore option array

[lbennett-stacki]

Clickbait title. Not actually regex. Just a string that looks like a regex.

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

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What rule do you want to change?

no-param-reassign: https://eslint.org/docs/rules/no-param-reassign

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

No difference.

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

Change how an existing option is used.

Please provide some example code that this change will affect:

function foo(fooBar) {
  fooBar.aaa++;
}

What does the rule currently do for this code?

Disallow Reassignment of Function Parameters, using strings

What will the rule do after it's changed?

Disallow Reassignment of Function Parameters, using strings and regex-like strings

What changes did you make? (Give an overview)

https://eslint.org/docs/rules/no-param-reassign

I didn't really like matching string literals for the ignorePropertyModificationsFor configuration array for no-param-reassign. If you want to avoid adding disable comments for this rule, you have to agree on a set of property names. These will have to be generic names that could be more descriptive and useful from a glance, or the array will continuously have more useful property names added to it and it will continue to grow.

Now we can agree on a naming pattern, rather than an explicit name. i.e. /^mutable.*$/. Now instead of just element, I can have mutableSettingsButton.

Understanding the code:

• Parse all ignorePropertyModificationsFor array string items into RegExp objects.

  • If this item is a string surrounded in forward slashes (/), it will be parsed into /$1/$2, which will be used to instantiate a RegExp object with new RegExp($1, $2).
  • If this item is not surrounded in forward slashes, it will be parsed using new RegExp('^$1$') so it is backwards compatible.

• Adds tests for valid and invalid regex cases.

• Adds to a documentation example to demonstrate using a regex string item.

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

Personally, I couldn't decide the best way to approach this as my first eslint contribution. I personally would rather have separate and explicit rule configuration properties for regular expressions and string literals, but this seemed simpler than finding out what to call that configuration property. 😅I hope it's OK! Thanks a bunch for your time and efforts reviewing this ❤️

[kaicataldo]

@LukeeeeBennett So sorry this fell through the cracks. I'll refrain from reviewing the implementation until consensus has been reached, but this seems like a reasonable enhancement to me, especially since it's backwards compatible.

[platinumazure]

I'm 👍 as long as it's a new property. This would otherwise be a breaking change. (Open to being persuaded otherwise but that's where I stand on this right now.)

[g-plane]

It may not be a breaking change, because an identifier doesn't contain the character /.

[platinumazure]

Apologies, I misread this as saying the term would just be thrown into the RegExp constructor:

If this item is not surrounded in forward slashes, it will be parsed using new RegExp('^$1$') so it is backwards compatible.

I understand this is a backwards compatible change.

I'm slightly concerned at the approach to using forward slashes as delimiters, not because it's a bad idea in this case (it's a fine idea), but because most/all of our other rules that accept regular expressions do so without requiring forward slash delimiters. I'm worried about inconsistency and potential user confusion. (That said, I actually wish we would require delimiters in all of those rules as it's a lot more clear, given RegExp literals are not themselves available in json and yaml configuration formats.)

[lbennett-stacki]

@platinumazure I was considering a new configuration property initially; I have no strong opinion either way so if you'd rather lean on consistency let's do it that way, I'm more than happy to make the change. I see the pattern you talk about in this rule https://eslint.org/docs/rules/id-match.

[platinumazure]

@LukeeeeBennett I would love to see us make a broader change around supporting slash-delimited strings as regex across the board, so we have consistency. Until then, I think we should just create a new option. Would you mind updating the original issue post with a new option proposal? Thanks so much! (Also, please accept my apologies for the delayed reply.)

[lbennett-stacki]

(Also, please accept my apologies for the delayed reply.)

@platinumazure Hah, not a problem! Here is a much more delayed response.

We now have an additional option and this is ready for review.

[lbennett-stacki]

Feel free to let me know if anything is required for this to be merged. :)

[platinumazure]

We need a champion (from the team) and one more 👍 to accept the issue. That said, I'm still thinking we might need to have a broader discussion on handling user-supplied "regex" input.

@eslint/eslint-team Anyone want to champion this issue?

[mdjermanovic]

I'll champion this, and there are already 3 👍 , so marking as accepted.

A design question. The current proposal is an additional array of regexes. There is a similar PR #12305 for another rule, with the proposal to add a flag that would turn the existing array into an array of regexes.

Both proposals are avoiding a breaking change, but it would be good to have consistency, so the question is - is an additional array preferred over an additional flag, as a way to avoid breaking changes?

Edit: Disregard this post, this proposal is already accepted in its current form. Also, the other PR is updated to be consistent with this one.

[mdjermanovic]

LGTM in general! Left some notes about the docs and an additional test.

It would be also nice to update the commit message and PR title with the name of the new option.

[mdjermanovic]

Could we please also add for-in and for-of examples? Just to make this example consistent with other examples from the actual version.

[mdjermanovic]

It would be good to have an additional valid test to show that the two options do not intentionally (or unintentionally) disable each other.

For example, a test where props from two different params are modified, one modification is allowed because of ignorePropertyModificationsFor, the other because of ignorePropertyModificationsForRegex.

[platinumazure]

Mostly unrelated to this PR, but I wonder if we should have an RFC on handling user-supplied regex in ESLint core rules in general.

The RFC process would be useful for helping to outline which rules would benefit and which rules need breaking or large changes, as well as ensuring both the community and the ESLint team have a greater understanding of what changes are needed.

[kaicataldo]

To piggyback on what @platinumazure says above, it would be great if we could find a way to make this kind of configuration consistent across rules (related discussion here).

[mdjermanovic]

Just for my understanding, these are two different things?

One is an idea is to make a consistent but probably breaking change in the major version, the other is to have consistent non-breaking changes until the major version?

[platinumazure]

@mdjermanovic Agreed, but I also want to avoid having too many changes too quickly in this area. Part of me thinks we should just take the time to design this right and be consistent across all rules. If we tell users to use regex one way in a semver-minor release, then tell them to use it in a different way in the next semver-major release, that doesn't look like a good design (in my opinion).

[mdjermanovic]

Thanks for the clarification! I didn't understand the status of this PR then, thought that the additional option in its current form is accepted as the solution for semver-minor releases, and is waiting to be implemented for this particular rule.

Then, it looks like this PR (and #12305) should wait for RFC?

[platinumazure]

Looks good to me. Thanks for the extraordinary patience on this one.

I think the changes identified by @mdjermanovic are mostly nice-to-have and not critical, so I'm okay with merging this in as-is. I'm already planning to open a follow-up PR on this to address a minor performance concern, so I can put in those changes at that time as well.

This PR has been open for a very long time and I'd like to merge this in. I will address the changes brought up in this review in a follow-up PR