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

YeeJone · 2022-07-24T15:15:58Z

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 · 2022-07-24T15:16:00Z

☁️ 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

Sent with 💌 from NxCloud.

typescript-eslint · 2022-07-24T15:16:02Z

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 · 2022-07-24T15:16:07Z

✅ 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 · 2022-07-24T15:31:37Z

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 · 2022-07-29T04:07:17Z

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 · 2022-07-31T05:02:38Z

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 · 2022-07-31T05:06:36Z

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 · 2022-08-19T02:08:28Z

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.

fix: no-non-null-asserted-optional-chain infinite loop problem

c847de9

bradzacher added the bug Something isn't working label Jul 25, 2022

bradzacher changed the title ~~fix(eslint-plugin): no-non-null-asserted-optional-chain infinite loop problem~~ fix(eslint-plugin): [no-non-null-asserted-optional-chain] infinite loop problem Aug 10, 2022

bradzacher requested changes Aug 17, 2022

View reviewed changes

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Aug 17, 2022

YeeJone requested a review from bradzacher August 18, 2022 08:00

fix: code modified to a cleaner way

ac7ccb4

JoshuaKGoldberg removed the awaiting response Issues waiting for a reply from the OP or another party label Oct 26, 2022

JoshuaKGoldberg requested changes Oct 26, 2022

View reviewed changes

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Oct 26, 2022

bradzacher mentioned this pull request Nov 2, 2022

feat: bump minimum supported TS version to 4.2.4 #5915

Merged

3 tasks

YeeJone closed this Nov 16, 2022

github-actions bot locked as resolved and limited conversation to collaborators Nov 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

YeeJone commented Jul 24, 2022

nx-cloud bot commented Jul 24, 2022 •

edited

Loading

typescript-eslint bot commented Jul 24, 2022

netlify bot commented Jul 24, 2022 •

edited

Loading

codecov bot commented Jul 24, 2022 •

edited

Loading

JoshuaKGoldberg commented Jul 29, 2022

YeeJone commented Jul 31, 2022

YeeJone commented Jul 31, 2022

bradzacher left a comment •

edited

Loading

YeeJone commented Aug 19, 2022

JoshuaKGoldberg left a comment

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

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

Conversation

YeeJone commented Jul 24, 2022

PR Checklist

Overview

nx-cloud bot commented Jul 24, 2022 • edited Loading

☁️ Nx Cloud Report

typescript-eslint bot commented Jul 24, 2022

netlify bot commented Jul 24, 2022 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

codecov bot commented Jul 24, 2022 • edited Loading

Codecov Report

JoshuaKGoldberg commented Jul 29, 2022

YeeJone commented Jul 31, 2022

YeeJone commented Jul 31, 2022

bradzacher left a comment • edited Loading

Choose a reason for hiding this comment

YeeJone commented Aug 19, 2022

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

nx-cloud bot commented Jul 24, 2022 •

edited

Loading

netlify bot commented Jul 24, 2022 •

edited

Loading

codecov bot commented Jul 24, 2022 •

edited

Loading

bradzacher left a comment •

edited

Loading