feat: fix no-useless-return false negative in try statement

[guilhermejcgois]

Promote up the useless returns in BlockStatement inside TryStatement when ESLint is going up in the tree during traversing the AST.

Fixes #7481

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

Address false-negative errors for no-useless-return rule inside Try statements.

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Tell us about your environment:

• ESLint Version: 8.28.0
• Node Version: 16.18.1
• npm Version: 8.19.2

What parser (default, @babel/eslint-parser, @typescript-eslint/parser, etc.) are you using?
default

Please show your full configuration:

Configuration

no-useless-return: error

What did you do? Please include the actual source code causing the issue.

From the issue, the following code is not triggering an error:

function foo() {
  try {
    return;
  } catch (err) {
    foo();
  }
}

What did you expect to happen?

The useless return is marked as error and the fixer can remove it.

What actually happened? Please include the actual, raw output from ESLint.

What changes did you make? (Give an overview)

The useless return is properly marked and being pushed to scopeInfo.uselessReturns when analyzing the ReturnStatement, but not present when the code path analysis ends. I found a way to fix that promoting up the assignaled useless return node in BlockStatement:exit, so we've the return node to be removed in the TryStatement's scope info.

Is there anything you'd like reviewers to focus on?

Correctness and if the fix is being made in the correct place.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: guilhermejcgois / name: Gois (8fa5756)

[netlify]

✅ Deploy Preview for docs-eslint ready!

Name	Link
🔨 Latest commit	414c472
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/638be9ef006b310008abf7ae
😎 Deploy Preview	https://deploy-preview-16593--docs-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.

[mdjermanovic]

scopeInfo.upper represents the upper function. If we push the return to useless returns there, it will never be removed, so this would introduce false positives:

function foo() {
    try {
        bar();
        return; // false positive, this return is not useless
    } catch {}
    baz();
}

[guilhermejcgois]

Indeed, will go back to work and add that case to tests

[guilhermejcgois]

@mdjermanovic do you think is better to found another way of fixing it? I can think about using TryStatement:exit to check if the try statement is the last block within the function, otherwise remove the false positive - but this sound just like a workaround for the problem. 🤔

[mdjermanovic]

The root cause of the problem seems to be that code path analysis considers segments containing returns from the try block as possible previous segments for segments in the catch block. This is logically incorrect, as the execution after return; cannot go into the catch block, but fixing this in the code path analysis would be a breaking change.

Also, even if we fix this in the code path analysis, there would still be false negatives with finally blocks.

/* eslint no-useless-return: ["error"] */

function foo() {
  try {
    return; // false negative
  } finally {
    bar();
  }
}

In this case, code path analysis is logically correct because bar(); is executed after the return;, but this return; is still useless.

I think we could try something like this in the no-useless-return rule:

1. Maintain a list of try blocks for which their catch/finally blocks are currently being traversed:
   • In "TryStatement > BlockStatement.block:exit", push that block statement to an array in scopeInfo.
   • In TryStatement:exit, pop.
2. When removing useless returns, keep those that are in range of blocks from 1.

[nzakas]

@mdjermanovic can you open an issue describing the code path analysis bug? We should try to fix that in the next major release.

[mdjermanovic]

I'll open an issue for code path analysis. Either way, try-catch-finally might require special-casing in this particular rule anyways, so I think it's also fine to try to fix #7481 directly in the rule in a minor release, as intended by this PR.

[nzakas]

@guilhermejcgois are you still working on this?

[guilhermejcgois]

Sorry, I don't get that I need to do something else. In the last commit I tried to add the case mentioned by @mdjermanovic and fix it, I should have pinged him explicitly about it, my mistake! 🤦🏻‍♂️ Did you guys needs a gas on it this year?

[mdjermanovic]

Looks like I missed the last commit. I tried it now and it does fix the false positive from #16593 (comment), but there are still other false positives, for example:

function foo() {
    if (something) {
        try {
            bar();
            return; // false positive, this return is not useless
        } catch {}
    }
    baz();
}

In the above example, TryStatement is the last node in its parent, but it isn't the last in the function. Trying to determine if return; is useless only by checking last nodes is tricky, and this rule already has all the logic about code paths, so I think we should try the solution I suggested at the end of #16593 (comment) - to just exclude from marking as not-useless returns in a try block while its catch and finally blocks are being traversed.

[guilhermejcgois]

Hey guys, happy new year! Just to update you, I'm having trouble dealing with the false positive pointed by @mdjermanovic , cause the unused returns is being removed (https://github.com/eslint/eslint/pull/16593/files#diff-9415ebfc11cef8ab76dee5f8b2fc7cf0199a44d4067776c749aed4b1deb1f1f4L170) and when I can make it to work, some other cases broken. In the next days I can sit down again to figured out some way to make it all working together

[github-actions]

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

[nzakas]

@guilhermejcgois just checking back to see if you still intend to finish this up?

[guilhermejcgois]

@nzakas sorry, I couldn't solve it the false positive mentioned by @mdjermanovic . In fact, I even managed to solve it (I couldn't do it without checking the last node), but I ran into a case that still doesn't work:

        {
            code: `
              function foo() {
                if (something) {
                  try {
                    bar();
                    return; // useless due to if being the last node from function
                  } catch (err) {}
                }
              }
            `,
            output: `
              function foo() {
                if (something) {
                  try {
                    bar();
                    
                  } catch (err) {}
                }
              }
            `
        },

Unfortunately, I have little time to dedicate myself to it right now. =/

[nzakas]

Okay, thanks for the update. We can see if someone else is willing to pick up the work to finish it up.

[snitin315]

I'll finish it.

[nzakas]

Thanks @snitin315!

[snitin315]

closing in favor of #16996