fix(eslint-plugin): [no-non-null-asserted-optional-chain] infinite loop problem

[YeeJone]

PR Checklist

• Addresses an existing open issue: fixes [no-non-null-asserted-optional-chain] Possible Infinite Loop #2521
• That issue was marked as accepting prs
• Steps in CONTRIBUTING.md were taken

Overview

Prior to typescript 3.9, the code in the no-non-null-asserted-optional-chain rule had an infinite loop.
Causes the ESLint process to fail to end

while (current) {
    switch (current.type) {
            case AST_NODE_TYPES.MemberExpression:
              if (current.optional) {
                // found an optional chain! stop traversing
                break;
              }

              current = current.object;
              continue;

            case AST_NODE_TYPES.CallExpression:
              if (current.optional) {
                // found an optional chain! stop traversing
                break;
              }

              current = current.callee;
              continue;

            default:
              // something that's not a ChainElement, which means this is not an optional chain we want to check
              return;
      }
}

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit ac7ccb4. 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 12 - nx test @typescript-eslint/eslint-plugin --coverage=false
• Node 18 - nx test @typescript-eslint/eslint-plugin
• nx run-many --target=build --all --parallel --exclude website
• Node 18 - nx test @typescript-eslint/utils
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx test @typescript-eslint/visitor-keys
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx test @typescript-eslint/type-utils
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• nx run-many --target=typecheck --all --parallel
• Node 12 - nx test @typescript-eslint/ast-spec --coverage=false
• Node 12 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx test @typescript-eslint/scope-manager
• Node 12 - nx test @typescript-eslint/parser --coverage=false
• Node 12 - nx test @typescript-eslint/type-utils --coverage=false
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 12 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 12 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 12 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 12 - nx test @typescript-eslint/utils --coverage=false
• Node 12 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx test @typescript-eslint/eslint-plugin-tslint
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx test @typescript-eslint/typescript-estree
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• nx run-many --target=build --all --parallel --exclude website --exclude website
• nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 12 - nx test @typescript-eslint/eslint-plugin-internal --coverage=false
• nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 12 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 12 - nx test @typescript-eslint/scope-manager --coverage=false
• Node 12 - nx test @typescript-eslint/visitor-keys --coverage=false
• Node 12 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx test @typescript-eslint/parser
• Node 12 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx test @typescript-eslint/ast-spec
• Node 12 - nx test @typescript-eslint/typescript-estree --coverage=false
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 18 - nx test @typescript-eslint/eslint-plugin-internal
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 12 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 12 - nx test @typescript-eslint/eslint-plugin-tslint --coverage=false
• Node 18 - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 12 - nx run-many --target=build --all --parallel --exclude website --exclude website
• nx run-many --target=build --all --parallel --exclude website --exclude website

---

Sent with 💌 from NxCloud.

[typescript-eslint]

Thanks for the PR, @YeeJone!

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.

[netlify]

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	ac7ccb4
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/62feefcbd9024a0009fdf6eb
😎 Deploy Preview	https://deploy-preview-5367--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 settings.

[codecov]

Codecov Report

Merging #5367 (ac7ccb4) into main (f82727f) will increase coverage by 2.46%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #5367      +/-   ##
==========================================
+ Coverage   91.36%   93.82%   +2.46%     
==========================================
  Files         132      290     +158     
  Lines        1494     9993    +8499     
  Branches      226     3009    +2783     
==========================================
+ Hits         1365     9376    +8011     
- Misses         65      336     +271     
- Partials       64      281     +217     

Flag	Coverage Δ
unittest	93.82% <0.00%> (+2.46%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...n/src/rules/no-non-null-asserted-optional-chain.ts	41.17% <0.00%> (ø)
.../eslint-plugin/src/rules/triple-slash-reference.ts	100.00% <0.00%> (ø)
...ages/eslint-plugin/src/rules/no-empty-interface.ts	100.00% <0.00%> (ø)
.../eslint-plugin/src/rules/member-delimiter-style.ts	94.73% <0.00%> (ø)
...lugin/src/rules/padding-line-between-statements.ts	93.83% <0.00%> (ø)
...ges/eslint-plugin/src/rules/space-before-blocks.ts	100.00% <0.00%> (ø)
...ackages/eslint-plugin/src/rules/prefer-includes.ts	97.72% <0.00%> (ø)
...rc/rules/indent-new-do-not-use/BinarySearchTree.ts	100.00% <0.00%> (ø)
...-plugin/src/rules/restrict-template-expressions.ts	100.00% <0.00%> (ø)
...eslint-plugin/src/rules/no-parameter-properties.ts	94.11% <0.00%> (ø)
... and 152 more

[JoshuaKGoldberg]

In an ideal world, this PR would be blocked on #1752, since it adds a couple of logic branches that aren't tested right now. But getting multi-TS-version testing support is going to be pretty tricky. 🤔

[YeeJone]

In an ideal world, this PR would be blocked on #1752, since it adds a couple of logic branches that aren't tested right now. But getting multi-TS-version testing support is going to be pretty tricky. 🤔

Indeed, adding multi-TS-version testing capabilities is tricky
Now I don't know how to add unit tests to validate my code...

But this is an obvious problem🐛, and I did some case verification locally with a lower version of TS, and it's fine

[YeeJone]

Now I use this package in a project with a low version of TS, and it often gets stuck, which is too uncomfortable😣

[bradzacher]

this code seems like it's pretty wrong anyways - I don't quite know how (or even if) it ever actually worked to begin with.
the intention is that the break applies to the outer while, but instead it's applying to the switch.

The code should instead be like this to explicitly exit the loop.

const hasOptionalChain = (() => {
  let current = child;
  while (current) {
    switch (current.type) {
      case AST_NODE_TYPES.MemberExpression:
        if (current.optional) {
          // found an optional chain! stop traversing
          return true;
        }

        current = current.object;
        continue;

      case AST_NODE_TYPES.CallExpression:
        if (current.optional) {
          // found an optional chain! stop traversing
          return true;
        }

        current = current.callee;
        continue;

      default:
        // something that's not a ChainElement, which means this is not an optional chain we want to check
        return false;
    }
  }
})();

if (!hasOptionalChain) {
  return;
}

this is much clearer than the existing code and it explicitly breaks from the loop rather than implicitly exiting

[YeeJone]

explicitly

It has been modified, please help to review the code

[JoshuaKGoldberg]

Could you add some unit tests verifying the behavior please? There are a lot of "line ... was not covered by tests" comments, which are indicating we can't be sure in CI this won't break in the future.