typescript-eslint/typescript-eslintPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star15.8k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

YeeJone wants to merge2 commits intotypescript-eslint:mainfromYeeJone:main

Closed

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

YeeJone wants to merge2 commits intotypescript-eslint:mainfromYeeJone:main

Conversation

Copy link

YeeJone commentedJul 24, 2022

PR Checklist

Addresses an existing open issue:fixes[no-non-null-asserted-optional-chain] Possible Infinite Loop #2521
That issue was marked asaccepting prs
Steps inCONTRIBUTING.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){caseAST_NODE_TYPES.MemberExpression:if(current.optional){// found an optional chain! stop traversingbreak;}current=current.object;continue;caseAST_NODE_TYPES.CallExpression:if(current.optional){// found an optional chain! stop traversingbreak;}current=current.callee;continue;default:// something that's not a ChainElement, which means this is not an optional chain we want to checkreturn;}}

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

c847de9

Copy link

nx-cloudbot commentedJul 24, 2022•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commitac7ccb4. 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 💌 fromNxCloud.

Copy link

Contributor

typescript-eslintbot commentedJul 24, 2022

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 visitorsper day.

Copy link

netlifybot commentedJul 24, 2022•
edited
Loading

✅ Deploy Preview fortypescript-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 yourNetlify site settings.

Copy link

codecovbot commentedJul 24, 2022•
edited
Loading

Codecov Report

Merging#5367 (ac7ccb4) intomain (f82727f) willincrease coverage by2.46%.
The diff coverage is0.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%> (ø)`
... and152 more

bradzacher added the bugSomething isn't working label

Jul 25, 2022

Copy link

Member

JoshuaKGoldberg commentedJul 29, 2022

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. 🤔

Copy link

Author

YeeJone commentedJul 31, 2022

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

Copy link

Author

YeeJone commentedJul 31, 2022

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

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

Copy link

Member

bradzacher left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 thebreak applies to the outerwhile, but instead it's applying to theswitch.

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

consthasOptionalChain=(()=>{letcurrent=child;while(current){switch(current.type){caseAST_NODE_TYPES.MemberExpression:if(current.optional){// found an optional chain! stop traversingreturntrue;}current=current.object;continue;caseAST_NODE_TYPES.CallExpression:if(current.optional){// found an optional chain! stop traversingreturntrue;}current=current.callee;continue;default:// something that's not a ChainElement, which means this is not an optional chain we want to checkreturnfalse;}}})();if(!hasOptionalChain){return;}

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

bradzacher added the awaiting responseIssues waiting for a reply from the OP or another party label

Aug 17, 2022

YeeJone requested a review frombradzacher

August 18, 2022 08:00

fix: code modified to a cleaner way

ac7ccb4

Copy link

Author

YeeJone commentedAug 19, 2022

explicitly

It has been modified, please help to review the code

JoshuaKGoldberg removed the awaiting responseIssues waiting for a reply from the OP or another party label

Oct 26, 2022

JoshuaKGoldberg requested changes

Oct 26, 2022

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.