fix(eslint-plugin): [prefer-optional-chain] handle more cases

[bradzacher]

I imported my implementation to the fb codebase and ran it (because I figured this rule isn't actually TS specific), and I found a few cases that I missed in the original implementation.

To better handle the cases, I significantly reduced the "allowed" scope of the rule by only allowing certain AST structures (see the isValidChainTarget function).

Changes:

at detection time:

1. no longer false-positives on chains where the first variable name is a substring of the next in the chain:
   • foo && fooBar.baz
2. now can detect chains with some optionals already:
   • foo && foo.bar?.baz
3. no longer detcets arbitrarily "complex" computed cases, such as:
   foo && foo[bar as string] && foo[bar as string].baz, or
   foo && foo[1 + 2] && foo[1 + 2].baz.
   • I realised that in order to "normalise" the text for comparison, the rule needs to know how to normalise every allowed node.
   • There's probably a better algorithm than just comparing text directly that removes this limitation.

at fix time:

1. no longer modifies call expression arguments (fixes a bug where required spaces in unary operators and JSX was stripped):
   • foo && foo.map(bar => <Baz bar={bar} />)
     • fix to foo?.map(bar => <Baz bar={bar} />)
   • foo && foo.map(bar => typeof bar)
     • fix to foo?.map(bar => typeof bar)
   • foo && foo.bar(/* comment */a, )
     • fix to foo?.bar(/* comment */a, )
2. No longer strips spaces from computed member expression literals:
   • foo && foo['bar baz']
     • fix to foo?.['bar baz']
3. No longer drops trailing binary expressions:
   • foo != null && foo.bar != null
     • fix to foo?.bar != null
   • foo && foo.bar != null && baz
     • fix to foo?.bar != null && baz

[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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[codecov]

Codecov Report

Merging #1261 into master will decrease coverage by 0.02%.
The diff coverage is 93.42%.

@@            Coverage Diff             @@
##           master    #1261      +/-   ##
==========================================
- Coverage    94.1%   94.07%   -0.03%     
==========================================
  Files         130      131       +1     
  Lines        5681     5737      +56     
  Branches     1596     1616      +20     
==========================================
+ Hits         5346     5397      +51     
- Misses        182      184       +2     
- Partials      153      156       +3

Impacted Files	Coverage Δ
packages/eslint-plugin/src/util/misc.ts	88% <100%> (ø)	⬆️
packages/eslint-plugin/src/util/nullThrows.ts	100% <100%> (ø)
packages/eslint-plugin/src/util/index.ts	100% <100%> (ø)	⬆️
...s/eslint-plugin/src/rules/prefer-optional-chain.ts	95.14% <92.18%> (-4.86%)	⬇️
...nt-plugin/src/rules/no-untyped-public-signature.ts	100% <0%> (ø)	⬆️

[glen-84]

const x = {y: 0};
if (x["y"] !== undefined && x["y"] !== null) {
    //
}

... is currently being reported, fixed to:

const x = {y: 0};
if (x["y"]) {
    //
}

This doesn't seem right at all. My real-world example actually looks like this:

const field = `relatedArticleIds${i}`;

if (data[field] !== undefined && data[field] !== null) {
    relatedArticleIds.push(data[field]);
}

(i.e. it uses a dynamic index)

The fix doesn't actually use optional chaining, and the code is not equivalent (checking for not undefined and not null vs checking for truthy).

Will this be fixed here (has it already been?), or should I open a separate issue for it?

[bradzacher]

@glen-84 - that issue is fixed by this PR.
https://github.com/typescript-eslint/typescript-eslint/pull/1261/files#diff-9e91bdee9914244f3ff940760d3f59bfR119