Rule request: don't use the in operator on nodes

[JoshuaKGoldberg]

Continuing conversation from here typescript-eslint/typescript-eslint#5610 (comment): We (typescript-eslint) generally try not to use string in checking pattern on AST nodes, as it's a little imprecise. We normally check for specific types.

- if ('async' in node && node.async) {
+ if (isFunctionLike(node) && node.async) {

How about making this an ESLint rule?

cc @eliasm307 since we were chatting 🙂

[eliasm307]

@JoshuaKGoldberg cool! So from the title mentioning "nodes" specifically, can I confirm if this rule is meant to be specific to the typescript-eslint/eslint code bases?

Could have a general "no-in" or something which triggers on all usages of "in"? But it could be a bit heavy handed so it would need some options to configure when it should trigger?

[JoshuaKGoldberg]

Oh sorry yes, to be clear: I do mean specific to typescript-eslint/eslint code bases, and specifically on instances of ESTree.Node or TSESTree.Node. Banning the in operator altogether is too heavyhanded, yeah.

[eliasm307]

Ah ok, in that case it sounds technically interesting. Assuming it's a rule with auto fixing, it might be challenging sometimes to be able to determine what predicates an "in" usage on a node should be replaced with e.g. Doing "body" in node where node is a generic node could be a number of things that can't be expressed with one predicate, as far as I know.

Not sure what you had in mind, but I've done a quick search across the monorepo and there are about 71 matches for ' in which isn't too much. So imo I don't know if its worth implementing the auto fixing. I think just a rule that triggers when "in" is used on a Node would be sufficient, then its up to the dev to choose what predicates to replace it with, and this also prevents future "in" usages.

I can't commit to making a PR for this but I'm interested to see what kind of solution someone comes up with.

[bmish]

I agree that it's safer and clearer to check the node type instead of whether the node has a particular property. And it sounds like the in operator is the primary avenue of abuse for this in TypeScript ESLint rules.

Possible rule names that come to mind:

• no-property-in-node (my preference given it's likely entirely focused on in)
• no-node-has-property

I don't think an autofixer would be practical for this, and I also don't think it would be necessary anyway.

---

Side note: I was considering whether we could make this applicable to JavaScript ESLint rules too, but so far, it doesn't seem like it would add much value. Here's my reasoning:

We could also make this semi-applicable to JavaScript ESLint rules by checking for the following examples, with cruder static analysis to ensure that node is from a visitor function parameter since we wouldn't have the variable type information available:

• node.hasOwnProperty('foo')
• Object.prototype.hasOwnProperty.call(node, 'foo')

However, this probably wouldn't be very useful since in JavaScript, people typically just use node.foo to check if a node has a property, and we obviously can't disallow that since we don't know if they're checking whether the node has the property or just whether the property is truthy.

[JoshuaKGoldberg]

@bmish I started working on this, but: this would be the first rule in the plugin that depends on types. Which means we'd have to settle on a strategy for introducing that concept I think. Proposal:

• Add @typescript-eslint/utils as an explicit dependency
• Create a new plugin analogous to recommended that also includes type-checked rules: maybe, recommended-type-checked to match typescript-eslint?

The tricky thing here is that we now have two places where "type checking" is cared about:

• Whether a rule's functionality is acting on type-checked rules (e.g. this rule only being meant for rules that act with type info)
• Whether a rule's implementation uses type information (parserServices.getTypeAtLocation(...))

My hunch is that it's ok to intentionally conflate the two. As in, if a codebase is using types, it probably wants to write rules with intentional property type checking.

Does that sound reasonable?

[bmish]

Adding the dependency sounds fine.

Is there any way we could avoid adding additional configs? I'm generally not a big fan of adding configs as most people just use recommended and thus miss out on everything else. Could this just be part of recommended (in the next major version) and just no-op for users that aren't using TypeScript or don't have type information available?

If it's not possible for this to be seamless and there's a strong reason to add the config still, I'm open to considering it...

[JoshuaKGoldberg]

no-op for users that aren't using TypeScript or don't have type information available?

In theory, yes. In practice it's kind of tricky. Roughly quoting typescript-eslint/typescript-eslint#7440 -> https://typescript-eslint.io/developers/custom-rules#typed-rules: we (typescript-eslint) have typically recommend against changing rule logic based solely on whether type information exists. In our experience, users are generally surprised when rules behave differently with or without type information. Additionally, if they misconfigure their ESLint config, they may not realize why the rule started behaving differently. Our recommendation has been to either gate type checking behind an explicit option for the rule or creating two versions of the rule instead.

Not saying that that's a certainty forever and always 😄 just the current recommendation and reasoning behind it.

A lot of folks' first instinct is to then have the rules behave differently based on file extension: .js & co for untyped, .ts & co for typed. But some projects have type checking enabled in JS files via JSDoc so that falls apart. 😞

[bmish]

I do think it's a good point that having automatic rule logic based on environmental factors is not necessarily a great practice and can theoretically cause confusion or reduce the user's ability to achieve their desired behavior.

I will admit to violating this principle in some rules I've written, for example, in eslint-plugin-ember where test rules no-op if test isn't in the current file path. This has allowed us to offer streamlined configuration/setup in the form of a single recommended config that users can simply enable for their entire project once and then forget about, without the need to enable separate configs for source code vs. tests, and without sacrificing runtime performance or the risk that test rules will mistakenly flag non-test code. Realistically, this has worked out well for us, even though I realize there are some theoretical downsides.

Perhaps there's a balance to strike between making everything clear and explicit through extra configuration vs. having some things "just work"...

Getting back to the situation where there's a rule that could optionally take advantage of type information, I lean toward sticking with a single rule and just adding the explicit type-checking option you mentioned. Then the user or a recommended-type-checked config could simply enable the rule with that option enabled to opt-in to the smarter type-checked behavior.

What I prefer to avoid is having to duplicate every rule for this. Having to create a new rule just to start taking advantage of type information is a very high barrier to entry, and having additional versions of rules creates a significant maintenance burden for us and unnecessarily multiples the API surface of the plugin...

So TLDR I think your original proposal is probably fine :)