eslint-community / eslint-plugin-eslint-plugin

An ESLint plugin for linting ESLint plugins
MIT License
195 stars 26 forks source link

Rule banning complex ESQuery selectors #339

Open JoshuaKGoldberg opened 1 year ago

JoshuaKGoldberg commented 1 year ago

Moving discussion from https://github.com/typescript-eslint/typescript-eslint/pull/6444#discussion_r1102035077 here, and quoting @bradzacher:

now a days though I tend to feel that leaning too heavily into the custom DSL makes code less accessible and harder to understand / debug. [specifiers.0] is one of those parts of the DSL that's super unclear as to what it's doing, IMO.

I think leveraging super clear parts of the DSL (prop existence, parents) are great and easy to understand, but for anything more complex it's better to write the clearer logic.

Is there any consensus/desire here to limit ESQuery node types?

Proposed "bad":

return {
  ImportDeclaration[importKind!="type"][specifiers.0](node) {
}

Proposed "good":

return {
  ImportDeclaration[importKind!="type"](node) {
    if (!node.specifiers.length) {
      return;
    }
}
bmish commented 1 year ago

We'd welcome a rule for this, as some plugin authors may prefer to avoid complex selectors. However, I would lean toward any such rule to be optional and not recommended. It feels too opinionated to be in the recommended config. I wouldn't want to prevent plugin authors from taking full advantage of selectors if they prefer that style.

DMartens commented 9 months ago

I would like to contribute this rule but I think there needs to be discussion about what a complex selector is. Personally I would only allow node types (e.g. FunctionDeclaration) and node classes (e.g. :function) combined with , or an optional :exit.

Valid Examples:

Invalid examples:

But as basically none of the selector syntax is used, it should be clear what and what is not allowed (versus different rule options, e.g. allowing maximally one attribute selector, ...). This also makes other rules for node selectors unnecessary (e.g. formatting or whether a selector can be simplified). Further this allows the callback parameters to be easily typeable (see #340)

I tried the above rules on my own rules and found no cases where I did not find it preferrable. What do you think?

JoshuaKGoldberg commented 8 months ago

Yeah it's a good question. https://github.com/typescript-eslint/typescript-eslint/issues/4065 shows @auvred's really nifty library, magic-query. So I personally think "whatever is reasonable to parse in the type system" is a good delineation.