Implement rule to forbid return statements in finally blocks #1097 #1349
Conversation
|
Hmm, looking at this again and while I believe the code is easily understandable and simple, it does require 2 passes over finally blocks, which is not ideal. I am thinking of an alternative approach to do it in a single pass by tracking current scope and flagging if the current scope is in a finally by taking advantage of scopeAwareRuleWalker. |
|
Good idea for a rule! I wasn't aware of the weird semantics until I looked it up:
|
|
Could you add this rule to |
| ruleName: "no-return-in-finally", | ||
| description: "Disallows return statements in finally blocks.", | ||
| descriptionDetails: "", | ||
| rationale: "Return statements inside finally blocks have confusing semantics.", |
There was a problem hiding this comment.
You could add a link (example) or some more details about what these confusing semantics are. Not a blocking issue though
There was a problem hiding this comment.
Would it be acceptable to link to eslint.org (seems a bit weird)? Or maybe: "Using control-flow statements such as return, continue, break and throws inside finally blocks overrides any control-flow statements in the same try ... catch scope. This is considered confusing and unexpected behavior."
There was a problem hiding this comment.
@lowkay looks good, here are a few small edit suggestions:
When used inside
finallyblocks, control flow statements such asreturn,continue,breakandthrowsoverride any other control flow statements in the same try/catch scope. This is confusing and unexpected behavior.
|
Should we broaden this rule to watch for other statements beyond just |
| return super.isScopeBoundary(node) || this.isFinallyBlock(node); | ||
| } | ||
|
|
||
| private isFinallyBlock(node: ts.Node): boolean { |
There was a problem hiding this comment.
You can move isFinallyBlock and isTryStatement outside of the class as they don't reference any class members. Very minor though, not a blocking issue for merging this
|
Code looks really good @lowkay! Only major thing here is to decide if the rule should handle more than just |
|
I agree with you that it should handle the same as eslint, take the rule name from theirs "no-unsafe-finally"? |
|
I like that it's consistent with ESLint (easier for users who use both), but I feel like "unsafe" implies the wrong idea. It's not so much unsafe as unexpected behavior. I'll ponder the name some more, feel free to make changes under the current name for now |
|
Cool... Looking at this further, the new semantics mean that handling continue/break need additional scopes :( So for example if we see a continue/break inside a switch inside a finally that's fine, but see a return anywhere outside a new scope boundary and we need to fail. Then how about handling breaking out of labels? We need to track where the label is with respect to the finally block. Essentially handling the different scoping of continue/break over return makes this rule much more complex to implement. |
|
I think I know how to handle it though - each scope indicates it's scoping impact (full boundary for return/throws vs partial boundary for continue/break) then we walk up the scopes:
|
|
Naming-wise I thought perhaps no-control-flow-in-finally but that's awkward... |
|
@lowkay That implementation strategy seems good! If some weird edge case is overly complex (for example, tracking labels) we can opt for code simplicity for this initial PR and enhance later. @adidahiya any thoughts on rule names? |
|
using the same rule name as eslint ("no-unsafe-finally") seems fine here since it has the same semantics |
|
Seems good to me, let's go with |
…k, continue, throws)
Conflicts: src/configs/latest.ts
|
@jkillian @adidahiya I believe I have made all the changes mentioned, additionally I managed to get label support in too, so |
|
Thanks @lowkay! Solid work on this which turned out to be trickier than expected |
…#1097 (palantir#1349) * Implement rule to forbid return statements in finally blocks palantir#1097 * explore the try statement fully to pick up violations in nested scopes * switch to a scope aware rule walker to reduce number of passes over the AST * Add support for other control flow statements in finally blocks (break, continue, throws) * improve the rationale description for the no unsafe finally rule * rename rule to no-unsafe-finally to be inline with eslint * add new rule to latest config * pull out helper functions from the walker class. * fix tslint violations (which didn't occur locally)
No description provided.