no-useless-return has a false negative with try/catch

[not-an-aardvark]

Tell us about your environment

• ESLint Version: 3.9.0
• Node Version: 6.9.1
• npm Version: 3.10.8

What parser (default, Babel-ESLint, etc.) are you using?

default

Please show your full configuration:

rules:
  no-useless-return: error

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

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

What did you expect to happen?

This should report an error, because the return statement can be safely removed.

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

No error.

[mysticatea]

Thank you for this issue.

In fact, this is caused by the current code path analyzer.
Since the analyzer cannot know the places which throw exceptions, it generates 2 code paths which go to the catch block from both the first statement and the last statement in the try block (edit: and from throw statements).
But in this case, the return; statement throws no exception clearly. I will investigate to improve the code path.

[not-an-aardvark]

When linting this code:

try { a; b; c; } catch (e) {}

I got this code path:

DOT

digraph {
node[shape=box,style="rounded,filled",fillcolor=white];
initial[label="",shape=circle,style=filled,fillcolor=black,width=0.25,height=0.25];
final[label="",shape=doublecircle,style=filled,fillcolor=black,width=0.25,height=0.25];
s1_1[label="Program\nTryStatement\nBlockStatement\nExpressionStatement\nIdentifier (a)\nIdentifier:exit (a)"];
s1_2[label="ExpressionStatement\nIdentifier (b)\nExpressionStatement\nIdentifier (c)\nExpressionStatement:exit\nIdentifier:exit (b)\nExpressionStatement:exit\nIdentifier:exit (c)\nExpressionStatement:exit\nBlockStatement:exit"];
s1_4[label="TryStatement:exit\nProgram:exit"];
s1_3[label="CatchClause\nIdentifier (e)\nBlockStatement\nIdentifier:exit (e)\nBlockStatement:exit\nCatchClause:exit"];
initial->s1_1->s1_2->s1_4;
s1_1->s1_3->s1_4->final;
}

Image

---

But I expected to get something like this:

DOT

digraph {
node[shape=box,style="rounded,filled",fillcolor=white];
initial[label="",shape=circle,style=filled,fillcolor=black,width=0.25,height=0.25];
final[label="",shape=doublecircle,style=filled,fillcolor=black,width=0.25,height=0.25];
s1_1[label="Program\nTryStatement\nBlockStatement\nExpressionStatement\nIdentifier (a)"];
s1_2[label="Identifier:exit (a)\nExpressionStatement:exit\nExpressionStatement\nIdentifier (b)"];
s1_3[label="Identifier:exit (b)\nExpressionStatement:exit\nExpressionStatement\nIdentifier (c)"];
s1_4[label="Identifier:exit (c)\nExpressionStatement:exit\nBlockStatement:exit"];
s1_5[label="CatchClause\nIdentifier (e)\nBlockStatement\nIdentifier:exit (e)\nBlockStatement:exit\nCatchClause:exit"];
s1_6[label="TryStatement:exit\nProgram:exit"];
initial->s1_1->s1_2->s1_3->s1_4->s1_6;
s1_1->s1_5->s1_6->final;
s1_2->s1_5;
s1_3->s1_5;
}

Image

---

Is it intentional that each try statement only contains one error point (and one exit point at the end), rather than having an exit point for every possible error?

[mysticatea]

Yes, it's intentional because I thought that making every throwable path is too many. #3559 (comment)

I'm sorry for I have not done this.

[not-an-aardvark]

I see, that makes sense.

• Do you think it would still be too many paths if we used scope analysis to avoid impossible ReferenceErrors? (It might still be complex because we would have to keep track of TDZ references.)
• As a smaller fix for this issue, maybe we could avoid adding the edge to the CatchClause if the TryStatement has no throwable paths.

[mysticatea]

It makes sense if we avoid making throwable path at references which never throw.
If we make every throwable path, I think we should measure impact to performance.

[not-an-aardvark]

Actually, I suppose the second solution wouldn't resolve this issue because something like this would still report an error:

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

[nzakas]

@mysticatea @not-an-aardvark are either of you interested in addressing this?

[kaicataldo]

Friendly ping

[guilhermejcgois]

I have free time until 1st december, can I go for it @mysticatea ? Any tip will be very appreciate 😄

Taking a look into this issue seems that in the current version we still have it, adding the example of @not-an-aardvark and no-useless-return.js#L308-L329 cause the test to fail

[nzakas]

@guilhermejcgois they have long since left the project. You are welcome to pick this up.

[eslint-github-bot]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

[ljharb]

This has an open PR, #16693

[sam3k]

Update:

• This might be the actual opened PR? #16593
• We're waiting to hear from @guilhermejcgois on one last edge case that needs to be addressed in the PR

[nzakas]

Looks like we will need help finishing up the PR for this.