feat(eslint-plugin): [prefer-optional-chain] handle cases where the first operands are unrelated to the rest of the chain and add type info

[bradzacher]

PR Checklist

• Addresses an existing open issue: fixes Enhancement: [prefer-optional-chain] handle cases where the first logical(s) are unrelated to the proceeding logicals #6332, fixes [prefer-optional-chain] Only checks the first occurrence in conditional #1461, fixes [eslint-plugin] Breaking: enable type information for prefer-optional-chain #4820, fixes Enhancement: [prefer-optional-chain] Suggest optional chain when the start of the expression is already optionally chained #5697
• Supersedes existing PR: closes feat(eslint-plugin): [prefer-optional-chain] use type checking for strict falsiness #6240
• That issue was marked as accepting prs
• Steps in CONTRIBUTING.md were taken

Overview

This is a big refactor of the lint rule so that it can be MUCH more robust and handle more cases.

The new algorithm is based off of flattening the logical expression to an array and then iterating over it. This new algorithm allows us to handle cases like this:

if (unrelated && foo && foo.bar) {...}

The new algorithm also includes a beefy recursive node comparison function which clearly compares two nodes semantically, instead of comparing them loosely based on source code text. This should lead to fewer weird cases, and also allows us to handle cases like this:

if (foo.bar && foo?.bar.baz) {...}
if (foo?.bar && foo.bar.baz) {...}

The TL;DR of the algorithm:

1. Convert the logical chain to a list of logical operators in order
   foo && foo.bar && foo.bar.baz --> [foo, foo.bar, foo.bar.baz]
2. Classify each operand based on whether or not it's in a form that does a "nullish comparison" that can be converted to optional chain.
   foo != null && foo.bar !== undefined && foo.bar.baz --> [foo<NotEqualNullOrUndefined>, foo.bar<NotStrictEqualUndefined>, foo.bar.baz<Boolean>]
3. Further analyse the chain looking for pairs like foo !== null && foo !== undefined and group those as one operand
4. Analyse the chain looking for contiguous sub-chains of "subset comparisons" (eg foo.bar is a subset of foo.bar.baz.bam, but not of foo.baz)
5. When a sub-chain ends, report on it.

TODO:

• Enhancement: [prefer-optional-chain] Suggest optional chain when the start of the expression is already optionally chained #5697 -> handle foo?.bar && foo.bar
• add generic logic for getting the "compared name" text in a standardised format (getComparedName)
• add logic for analysing && and || conditionals separately (analyzeAndChain and analyzeOrChain respectively)
• delete old selectors
• ensure all existing tests pass
• add tests for new cases
• ??????????? add back the fixer ???????????

[typescript-eslint]

Thanks for the PR, @bradzacher!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 3c387a9. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 47 targets

• Node 14 - nx test eslint-plugin --coverage=false
• Node 18 - nx test eslint-plugin
• Node 14 - nx test typescript-estree --coverage=false
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx test type-utils --coverage=false
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx test eslint-plugin-internal --coverage=false
• Node 18 - nx test visitor-keys
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx test utils
• Node 18 - nx run-many --target=build --parallel --exclude website
• nx run-many --target=build --parallel --exclude website
• Node 14 - nx test parser --coverage=false
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx test scope-manager --coverage=false
• Node 14 - nx test utils --coverage=false
• Node 18 - nx test type-utils
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx test ast-spec --coverage=false
• Node 14 - nx test visitor-keys --coverage=false
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx test eslint-plugin-tslint --coverage=false
• nx run-many --target=typecheck --parallel
• Node 14 - nx run-many --target=build --parallel --exclude website
• nx run-many --target=lint --parallel
• Node 18 - nx test parser
• nx run-many --target=build --parallel --exclude website
• Node 18 - nx test scope-manager
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx test eslint-plugin-internal
• Node 18 - nx test typescript-estree
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx run-many --target=build --parallel --exclude website
• nx run-many --target=build --parallel --exclude website
• Node 18 - nx test ast-spec
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx test eslint-plugin-tslint
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx run-many --target=build --parallel --exclude website
• nx run-many --target=build --parallel --exclude website
• nx run-many --target=build --parallel --exclude website

---

Sent with 💌 from NxCloud.

[netlify]

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	4c36891
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/64a7905efc36a000084beac7
😎 Deploy Preview	https://deploy-preview-6397--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[bradzacher]

This is now "check complete" - all existing tests are passing as are the new tests.
Need to rebuild the fixer logic though.

This is going to be a big chunk of the logic to figure out the correct places to put an optional chain 🤔

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 4c36891. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 31 targets

• Node 16 - nx test eslint-plugin --coverage=false
• Node 20 - nx test eslint-plugin
• nx run-many --target=lint --parallel
• nx run-many --target=typecheck --parallel
• Node 20 - nx test scope-manager
• Node 20 - nx test typescript-estree
• Node 20 - nx test visitor-keys
• Node 20 - nx run types:build
• Node 20 - nx test utils
• nx run types:build
• Node 20 - nx test repo-tools
• Node 20 - nx test type-utils
• Node 20 - nx test rule-schema-to-typescript-types
• Node 16 - nx test typescript-estree --coverage=false
• nx run integration-tests:test
• Node 20 - nx test parser
• Node 20 - nx test eslint-plugin-internal
• Node 20 - nx test ast-spec
• Node 20 - nx test eslint-plugin-tslint
• Node 16 - nx test eslint-plugin-internal --coverage=false
• Node 16 - nx test scope-manager --coverage=false
• Node 16 - nx test utils --coverage=false
• Node 16 - nx test eslint-plugin-tslint --coverage=false
• Node 16 - nx run types:build
• Node 16 - nx test repo-tools --coverage=false
• Node 16 - nx test ast-spec --coverage=false
• Node 16 - nx test type-utils --coverage=false
• Node 16 - nx test visitor-keys --coverage=false
• Node 16 - nx test rule-schema-to-typescript-types --coverage=false
• Node 16 - nx test parser --coverage=false
• nx run-many --target=build --parallel --exclude=website --exclude=website-eslint

---

Sent with 💌 from NxCloud.

[bradzacher]

@typescript-eslint/triage-team - this is functionally complete in terms of raw logic.
However it is missing the fixer logic because:

1. frankly it's not a small piece of work to re-implement it.
2. it's the bit that made the rule unsafe to begin with (x != null && x.y returns false | T, but x?.y returns T | undefined)

In some future state I can definitely see us including:

• an autofixer if we detect it's safe (eg if it doesn't change the types), or
• a suggestion fixer in other cases

But I don't know if it's worth "holding up" this change to re-implement. Happy to do it if we think it's necessary, if not then we can review and land this as is.

[JoshuaKGoldberg]

😞 I do dislike removing functionality. Maybe we could put this out as an experimental rule without deleting the existing one?

[bradzacher]

Maybe we could put this out as an experimental rule without deleting the existing one?

That would create a worse state, IMO - as we would have two nearly identical rules that are mostly differentiated by having a fixer or not. What would be the value prop for a user to switch to test the rule? We'd need a breaking change to remove the experimental rule name.

Also "experimental" implies instability or that it may not be adopted - but the rule is perfectly stable (passes all the existing tests, and more) and will be adopted - it just doesn't have the previously available (unsafe) fixer.

[JoshuaKGoldberg]

Yeah that's a good point.

Heh, I'm torn here. Expecting previously (perhaps slightly incorrect) satisfied users to be annoyed by the lack of autofixer.

Given that the autofixer today is unsafe, I think it's reasonable for us to say "we've rewritten and removed it for safety - welcoming community PRs to add it back".

[omril1]

It's really sad to see the fixer being removed, my team absolutely loved it when I turned the rule on since we worked on a code-base full of partial objects (and different patterns to deal with them).
I've never even once saw a case of false positive fix of the type object | false.
I did see a few false positives returning undefined instead of null (from nullish objects) but they only affected tests asserting on null, the real code had falsy checks catching both.

[JoshuaKGoldberg]

...maybe we could expose the old one under a name like "deprecated"?

[bradzacher]

I've never even once saw a case of false positive fix of the type object | false.

Also applies to 0 | object and '' | object.
It's rare, for sure, but we've had a few users report it - which means it's not as rare as you might hope in the wild.

The bigger issue is the conversion of return types - x != null && x.y returns false | T, whereas x?.y returns undefined | T. In a lot of cases this distinction isn't going to matter - eg in boolean locations like if or logical expressions, or in expression statements. But there are places where this can matter and the autofix will cause errors. Again we've had some reports of this!

Honestly I'd be happy with having a flag like allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing which enables people to opt-in to an unsafe fixer - the issue is that it's just a bigger chunk of work to build the fixer.

[bradzacher]

Fun edge case I just discovered:

declare const foo: { bar: number } | null;
foo === null || foo.bar;
foo !== null && !foo.bar;

In the first case foo.bar is seen by my code as a "truthy boolean" check - which isn't a valid operand in an "OR" chain.
Similarly in the second case !foo.bar is seen as a "falsy boolean" check - which isn't a valid operand in an "AND" chain.

Which means that my code will ignore it and try not to include it in the chain to report and fix.

Will need to add special case logic to handle this as it's obviously intended to be chained together.

---

This also opens up another question about how this rule functions (that we can probably punt on for now, but worth mentioning).
Currently the rule will ignore operands with "invalid" checks. For example:

foo && foo.bar && foo.bar.baz === 1
// rule will currently fix to
foo?.bar && foo.bar.baz === 1

Should the rule inspect through these "invalid" checks? Or should it ignore them entirely?
I think there's a subset of them that it should include

• !==/===/!=/== comparing against anything
• not >/>=/</<= because TS will hard error if one of the operands is nullish.
• not typeof because foo && typeof foo.bar will return undefined | typeof foo.bar whereas typeof foo?.bar will return typeof foo.bar | 'undefined' - which is VERY different

[JoshuaKGoldberg]

Should the rule inspect through these "invalid" checks? Or should it ignore them entirely?

...hmm. My instinct is to ignore "invalid" checks as an odd edge case. If users complain we can add it as a bugfix later on - but if the code is roughly not valid, I'd hesitate to feel confident in making changes.

[bradzacher]

Note: I hate the tests with a passion because it's impossible for anyone to understand them as written without checking out and running the test 😢
However without utilities it's really hard to write and maintain the 700 cases with lots of repeated code.
Sigh this is a big pet peeve I have with ESLint rule tests. There has to be a better way...

Snapshot output would make it much simpler to review as you can see the expected reports and fixes properly rather than doing it opaquely.

The test coverage should show how much is covered overall - which should be all of the code!

[JoshuaKGoldberg]

This is really impressive code. Seriously awesome PR @bradzacher!

I've gone over it a few times and think I generally understand it. Particular ups on the approach of separating out the collection and analysis bits.

I say let's get this in for v6!

[omril1]

type parameters were renamed to type arguments in this context, right?

[bradzacher]

[omril1]

Same