Rule request: forbid control flow statements in finally

[louy]

Adding a return statement in a finally block is a very confusing thing. Thrown errors get ignored, return statements in try blocks get ignored, and your mind explodes.

I'd love to have a rule to forbid this. Especially now that we're having async/await.

examples:

try {
  return 1;
} catch(err) {
  return 2;
} finally {
  return 3;
}
// > 3

try {
  throw new Error;
  return 1;
} catch(err) {
  return 2;
} finally {
  return 3;
}
// > 3

[mysticatea]

Thank you for this issue.

Indeed, ReturnStatements in a finally block may hide actual errors or results.
I think also ThrowStatements in a finally block are similar.

As far as I know Java Compiler is warning this ReturnStatements.

I like this idea, but we need to wait for opinions of other members.

[BYK]

I'm 👍 since this is a core language issue and a pitfall quite easy to fall into without realizing consequences. We should add this to eslint:recommended too IMO.

[nzakas]

Seems like a good idea. Does anyone want to champion this? @eslint/eslint-team

@BYK we only change eslint:recommended when we do a major release because it's a breaking change.

[michaelficarra]

If return is forbidden, all control flow operations should be forbidden: throw, break, and continue.

[platinumazure]

👍 to @michaelficarra. Perhaps the rule could be called something like no-finally-control-flow?

[BYK]

@nzakas - I know. I wanted to raise the flag to include this in 3.0.0 planning :)

I can champion the rule. @onurtemizkan interested in implementing?

[onurtemizkan]

Sure! 😄

[nzakas]

Okay @BYK is championing this rule. Please work with @onurtemizkan to implement the rule using the new format.

[BYK]

Did we decide on a rule name?

no-finally-control-flow sounds like a good one but a bit on the longer side. Other suggestions: no-control-finally, no-control-in-finally, no-unpredictable-finally.

@onurtemizkan has started working on this.

[platinumazure]

I could probably get behind any of those. I'll throw one more name into the
ring: no-interrupt-finally.
On Apr 15, 2016 8:39 AM, "Burak Yiğit Kaya" [email protected]
wrote:

Did we decide on a rule name?

no-finally-control-flow sounds like a good one but a bit on the longer
side. Other suggestions: no-control-finally, no-control-in-finally,
no-unpredictable-finally.

@onurtemizkan https://github.com/onurtemizkan has started working on
this.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#5808 (comment)

[nzakas]

"Control flow" refers to anything that changes where execution flows, Including if, while, etc., so it's a bit broad for this use case.

How about no-unsafe-finally?

[BYK]

no-unsafe-finally may be a bit too broad but I like it.

[nzakas]

Okay, we'll leave this to @BYK and @onurtemizkan to implement.

[onurtemizkan]

@michaelficarra I couldn't find a case for break and continue, creating unexpected behaviour in finally. Could you give an example?

[michaelficarra]

(function() {
  label: try {
    return 0;
  } finally {
    break label;
  }
  return 1;
})()

[onurtemizkan]

Ah, okay. Thanks!

[bakkot]

Currently this rule isn't actually checking that breaks and continues are crossing the boundary of the finally. In fact, one of the examples of correct code from the docs actually gives an error:

let foo = function(a) {
    try {
        return 1;
    } catch(err) {
        return 2;
    } finally {
        switch(a) {
            case 1: {
                console.log("hola!")
                break;
            }
        }
    }
};

This also trips this rule, I think incorrectly:

try {} finally {
  while(true) {
    break;
  }
}

I would not think the mere presence of control flow statements would be an issue: it's only control flow which crosses the boundary of the finally. Doing this right involves determining which statements are broken / continued.

I can open an issue for this if people agree.

[platinumazure]

@bakkot I agree with your logic. Please open a new issue and I will triage it as a bug right away. 😄

[onurtemizkan]

@bakkot Sorry, my bad. I can post a fix for this if you open an issue.

[bakkot]

Opened #5972.

[mk-pmb]

Does anyone have an in-the-wild example where flow control in a finally block caused an actual problem? (To me, the use of this rule seems intuitive, but I'd like to convince a skeptic.)