Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule: Don't 'in' check AST nodes? #416

Closed
JoshuaKGoldberg opened this issue Jan 8, 2024 · 5 comments
Closed

Rule: Don't 'in' check AST nodes? #416

JoshuaKGoldberg opened this issue Jan 8, 2024 · 5 comments

Comments

@JoshuaKGoldberg
Copy link
Contributor

From typescript-eslint/typescript-eslint#8226:

Suggestion

Sometimes folks will use an 'in' property check in a rule to narrow the type of a node:

declare const node: ts.BinaryExpression | ts.ConditionalExpression;

if ('condition' in node) {
  node
  // ^? const node: ts.ConditionalExpression
}

I've been advising to prefer using less "dynamic" logical gates: existing type predicate functions, direct .kind/.type checks, and/or having different functions for different node selectors.

declare const node: ts.BinaryExpression | ts.ConditionalExpression;

// or:
// if (node.kind === ts.SyntaxKind.ConditionalExpression) {
if (ts.isConditionalExpression(condition)) {
  node
  // ^? const node: ts.ConditionalExpression
}

My reasoning has been that "dynamic" logical gates, beyond being a potential deoptimization in JS perf, are susceptible to the types changing unexpectedly. E.g. a new version of TypeScript might add a new node type to a built-in union that adds a case previously meant to be caught by the rule.

My understanding of in checks is that they're generally reserved for cases where the more "static" ways of checking aren't available.

Shall we encode this as a lint rule?

Is this generally sound advice for lint rules altogether?

@aladdin-add
Copy link
Contributor

I recognize its value, but it seems to be the type of information needed?

Otherwise there could be a lot of false positives (if obj is not an ast node):

if( key in obj) {....}

@JoshuaKGoldberg
Copy link
Contributor Author

JoshuaKGoldberg commented Jan 9, 2024

Yeah, this would have to be a typescript-eslint typed rule (typescript-eslint > Custom Rules > Typed Rules) to be useful in non-trivial cases.

@aladdin-add
Copy link
Contributor

Currently no similar rules are included in this plugin, but I think it's possible, as long as it doesn't affect js users.
/ cc @bmish, would like to see your thoughts. 😄

@bmish
Copy link
Member

bmish commented Jan 10, 2024

It sounds useful to me.

Is this different from this one?

@JoshuaKGoldberg
Copy link
Contributor Author

🤦 dear heavens. Sorry haha. I swear I did search for duplicates!

@JoshuaKGoldberg JoshuaKGoldberg closed this as not planned Won't fix, can't repro, duplicate, stale Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants