New: Rule class-methods-use-this option exceptMethods accepts regex

[derjones]

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

[X] 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?
class-methods-use-this

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

How will the change be implemented? (New option, new default behavior, etc.)?
Use regex for option 'exceptMethods'

Please provide some example code that this change will affect:

Examples of correct code for this rule when used with exceptMethodsForRegex:

/*eslint class-methods-use-this: ["error", { "exceptMethodsForRegex": ["^foo.*"] }] */

class A {
    foobar() {
    }
}

Examples of incorrect code for this rule when used with exceptMethodsForRegex:

/*eslint class-methods-use-this: ["error", { "exceptMethodsForRegex": ["^foo.*"] }] */

class A {
    fobar() {
    }
    afoobar() {
    }
}

What does the rule currently do for this code?
Ensures that class methods use this

What will the rule do after it's changed?
Ensures that class methods use this with regex support for exceptions

What changes did you make? (Give an overview)
Allow regex expression

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

[jsf-clabot]

All committers have signed the CLA.

[mdjermanovic]

Hi @derjones, thanks for the PR!

Would you mind updating the post with some example code that this change will affect? It would be much easier to evaluate the proposal, and it's also a required field.

The initial version of the code looks to me like a breaking change. For example, if someone has "abc" in the exceptMethods, after this change the rule would also allow "fooabc", "abcfoo", and all other names containing "abc"?

[ljharb]

accepting a regex opens the rule up to a bunch of CVEs. is there a reason this can’t be simple globs?

[derjones]

Hm okay, in my case globs would be enough. But other rules also use RegExp for exceptions: camelcase, lines-around-comment e.g.

[not-an-aardvark]

I don't think this is a security issue if the regexes come from a config. If someone has the ability to edit a config file and add malicious regexes, they probably also have the ability to create a JS file and do worse things anyway.

If inline config comments are enabled, someone could maybe create a piece of code that takes a very long time to lint, but I think that's an accepted risk at this point.

[ljharb]

That’s true, but that won’t stop tons of false positive CVEs.

[not-an-aardvark]

With an exception or two, it seems to have stopped them so far. False-positive CVEs are sometimes a problem, but it seems silly to not use a feature for fear of false positive security reports. (Maybe someone can convince V8 to use linear-time regex matching for regexes that don't have backreferences, and then we stop tons of real CVEs.)

[derjones]

I think it's a general question if regex is allowed in a rule config or not. Since some rules use a regex in the config already you would have to remove either all or allow in general

[derjones]

For consistency (#11275) I added an array. Maybe it's better to extend a rule anyway.

[mdjermanovic]

For consistency (#11275) I added an array. Maybe it's better to extend a rule anyway.

Thanks for making this change! Could you please update the example in the original post?

I'll champion this. An enhancement also needs 3 👍 from other teams members to be accepted.

[derjones]

Okay, thanks! I just updated the example.

[kaicataldo]

I wonder if there's a better alternative to exceptMethodsForRegex? Seeing this option without context, I think I would assume that it configures something related to RegExp syntax, rather than it accepting a RegExp as a configuration option.

One alternative idea - though this is different from other rules we have that accept a regex - could be to use the current exceptMethods option and allow both strings and RegExp, denoting RegExp using / characters. e.g.

Examples of correct code:

/*eslint class-methods-use-this: ["error", { "exceptMethodsForRegex": ["/^foo.*/", "baz"] }] */

class A {
    foobar() {}
    baz() {}
}

Examples of incorrect code:

/*eslint class-methods-use-this: ["error", { "exceptMethodsForRegex": ["/^foo.*/", "baz"] }] */

class A {
    fobar() {}
    afoobar() {}
}

[derjones]

There was the same discussion in this PR already (#11275)

The result was:

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.

[kaicataldo]

@mdjermanovic As the champion for this proposal, how would you like to proceed?

[kaicataldo]

Unfortunately, it looks like this proposal didn't get enough support from the team and so I'm closing it. While we wish we'd be able to accommodate everyone's requests, we do need to prioritize. We've found that proposals failing to reach consensus after a long time tend to never do it, and as such, we close those issues and pull requests. This doesn't mean the idea isn't interesting, just that it's not something the team can commit to. Thanks for your interest in improving ESLint!