Rewrite no-unsafe-finally #2346

andy-hanson · 2017-03-15T01:21:17Z

PR checklist

Addresses an existing issue: #0000
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Just a rewrite. Should be simpler now.

adidahiya

just some minor comments

adidahiya · 2017-03-25T03:08:21Z

src/rules/noUnsafeFinallyRule.ts

-function isBreakBoundary(scope: IFinallyScope, node: ts.BreakStatement): boolean {
-    return node.label ? scope.labels.indexOf(node.label.text) >= 0 : scope.isBreakBoundary;
+type JumpStatement = ts.BreakStatement | ts.ContinueStatement | ts.ThrowStatement | ts.ReturnStatement;
+function showKind(node: JumpStatement): string {


how about naming this printJumpKind

adidahiya · 2017-03-25T03:09:05Z

src/rules/noUnsafeFinallyRule.ts

-            if (isControlFlowBoundary(currentScope, node)) {
-                return false;
+function jumpIsLocalToFinallyBlock(jump: JumpStatement): boolean {
+    const isBreakContinue = jump.kind === ts.SyntaxKind.BreakStatement || jump.kind === ts.SyntaxKind.ContinueStatement;


isBreakOrContinue

adidahiya · 2017-03-25T03:13:17Z

src/rules/noUnsafeFinallyRule.ts

 }

-function isBreakBoundary(scope: IFinallyScope, node: ts.BreakStatement): boolean {
-    return node.label ? scope.labels.indexOf(node.label.text) >= 0 : scope.isBreakBoundary;
+type JumpStatement = ts.BreakStatement | ts.ContinueStatement | ts.ThrowStatement | ts.ReturnStatement;


let's move this type to just before function walk since it's used above too

adidahiya · 2017-03-25T03:16:35Z

src/rules/noUnsafeFinallyRule.ts

+            case ts.SyntaxKind.TryStatement:
+                const { tryBlock, catchClause, finallyBlock } = node as ts.TryStatement;
+                ts.forEachChild(tryBlock, cb);
+                if (catchClause) {


try to avoid implicit boolean coercion, check catchClause !== undefined instead. same a few lines down

We do have a lint rule for that (strict-boolean-expressions), would you want to enable that in this repo?

yeah, I'd like to enable it, I remember having some issues when trying to use it elsewhere but I haven't put in the time to investigate it fully.

ajafff

Your implementation is missing checks to prevent crashes on invalid code and proper handling of function boundaries. Added comments at the relevant sections of code.

ajafff · 2017-03-27T08:44:37Z

src/rules/noUnsafeFinallyRule.ts

+                break;
+
+            default:
+                ts.forEachChild(node, cb);


if (isFunctionScopeBoundary(node)) { const old = inFinally; inFinally = false; ts.forEachChild(node, cb); inFinally = old; break; } else { return ts.forEachChild(node, cb); }

I don't think that's necessary. jumpIsLocalToFinallyBlock checks for function scopes.

ajafff · 2017-03-27T08:47:59Z

src/rules/noUnsafeFinallyRule.ts

+
+    let node: ts.Node = jump;
+    // This should only be called inside a finally block, so we'll eventually reach the TryStatement case and return.
+    while (true) {


Avoid property access on undefined by ending the loop at SourceFile.
You should also stop at function scope boundary.

while(node.kind !== ts.SyntaxKind.SourceFile && !isFunctionScopeBoundary(node))

We'll always exit this loop, as we only get here if we saw a TryStatement above.

ajafff · 2017-03-27T08:52:00Z

src/rules/noUnsafeFinallyRule.ts

+            case ts.SyntaxKind.ClassExpression:
+            case ts.SyntaxKind.FunctionDeclaration:
+            case ts.SyntaxKind.FunctionExpression:
+            case ts.SyntaxKind.ArrowFunction:


this does not include all function scope boundaries. use isFunctionScopeBoundary(parent) instead, see comment above

ajafff · 2017-03-27T08:52:15Z

src/rules/noUnsafeFinallyRule.ts

-                return false;
+function jumpIsLocalToFinallyBlock(jump: JumpStatement): boolean {
+    const isBreakOrContinue = jump.kind === ts.SyntaxKind.BreakStatement || jump.kind === ts.SyntaxKind.ContinueStatement;
+    const label = isBreakOrContinue ? (jump as ts.BreakStatement | ts.ContinueStatement).label : undefined;


nit: ts.BreakOrContinueStatement

ajafff · 2017-03-27T08:55:00Z

src/rules/noUnsafeFinallyRule.ts

+                    ctx.addFailureAtNode(node, Rule.FAILURE_STRING(printJumpKind(node as JumpStatement)));
+                }
+                ts.forEachChild(node, cb);
+                break;


nit: return ts.forEachChild(...)

ajafff · 2017-03-27T08:57:47Z

src/rules/noUnsafeFinallyRule.ts

+            case ts.SyntaxKind.ClassExpression:
+            case ts.SyntaxKind.FunctionDeclaration:
+            case ts.SyntaxKind.FunctionExpression:
+            case ts.SyntaxKind.ArrowFunction:
                return true;


shouldn't this return false?

If you're climbing parents and see a function before you reach the try statement, that means there is a function inside the finally block. Jump statements can't leave a function, so the jump is definitely local to that finally block (because it's local to the function and the function is inside the finally block).

ajafff

just one change request and a comment.
other than that the code looks good.

ajafff · 2017-03-28T07:56:26Z

src/rules/noUnsafeFinallyRule.ts

+
+    let node: ts.Node = jump;
+    // This should only be called inside a finally block, so we'll eventually reach the TryStatement case and return.
+    while (true) {


you still need to check if you reached ts.SourceFile to guard against invalid code crashing this rule.
take this code for example:

try { } finally { break; // break is not allowed here, tsc will complain but this rule will crash } // or for(;;) { try { } finally { break foo; // label does not exist, tsc will complain but this rule will crash } }

Added a test case. (It doesn't crash.)

ajafff · 2017-03-28T08:06:40Z

src/rules/noUnsafeFinallyRule.ts

+                // falls through
+
+            default:
+                return ts.forEachChild(node, cb);


you're right, you don't need to check for function boundaries here, but it would avoid doing unnecessary time spent in jumpIsLocalToFinallyBlock and would make the code clearer.
You can of course avoid doing the additional check if inFinally === false.

if (inFinally && isFunctionScopeBoundary(node)) { inFinally = false; ts.forEachChild(node, cb); inFinally = true; } else { return ts.forEachChild(node, cb); }

jumpIsLocalToFinallyBlock is only activated if a jump statement appears in a finally block, which with 99% certainty won't happen if people are linting with this rule. (Who on earth would put a loop with a break statement inside of a finally block?) So I don't think it's worth doing more work in cb (which runs on every node) to try to save time in jumpIsLocalToFinallyBlock (which will almost never be called).

Rewrite no-unsafe-finally

51178f8

adidahiya reviewed Mar 25, 2017

View reviewed changes

Respond to PR comments

0779311

andy-hanson mentioned this pull request Mar 25, 2017

Lint for strict-boolean-expressions #2408

Merged

4 tasks

ajafff suggested changes Mar 27, 2017

View reviewed changes

andy-hanson added 2 commits March 27, 2017 18:48

Clean up code

e914f42

Add comment

6d1ec57

ajafff suggested changes Mar 28, 2017

View reviewed changes

andy-hanson mentioned this pull request Apr 8, 2017

Using (Block/)ScopeAwareRuleWalker with typed programs #2514

Closed

Prove that it won't crash on invalid code

7322185

andy-hanson force-pushed the no-unsafe-finally branch from e0ae485 to 7322185 Compare April 8, 2017 16:58

adidahiya approved these changes Apr 12, 2017

View reviewed changes

adidahiya merged commit 39e3f20 into palantir:master Apr 12, 2017

andy-hanson deleted the no-unsafe-finally branch April 13, 2017 00:23

renovate-bot mentioned this pull request May 3, 2017

Update dependency tslint to version 5.2.0 singapore/lint-condo#282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite no-unsafe-finally #2346

Rewrite no-unsafe-finally #2346

andy-hanson commented Mar 15, 2017

adidahiya left a comment

adidahiya Mar 25, 2017

adidahiya Mar 25, 2017

adidahiya Mar 25, 2017

adidahiya Mar 25, 2017

andy-hanson Mar 25, 2017

adidahiya Mar 25, 2017

ajafff left a comment

ajafff Mar 27, 2017

andy-hanson Mar 28, 2017

ajafff Mar 27, 2017 •

edited

Loading

andy-hanson Mar 28, 2017

ajafff Mar 27, 2017

andy-hanson Mar 28, 2017

ajafff Mar 27, 2017

ajafff Mar 27, 2017

ajafff Mar 27, 2017

andy-hanson Mar 28, 2017

ajafff left a comment

ajafff Mar 28, 2017

andy-hanson Apr 8, 2017 •

edited

Loading

ajafff Mar 28, 2017

andy-hanson Apr 8, 2017 •

edited

Loading

Rewrite no-unsafe-finally #2346

Rewrite no-unsafe-finally #2346

Conversation

andy-hanson commented Mar 15, 2017

PR checklist

Overview of change:

adidahiya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-hanson Apr 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-hanson Apr 8, 2017 • edited Loading

Choose a reason for hiding this comment

ajafff Mar 27, 2017 •

edited

Loading

andy-hanson Apr 8, 2017 •

edited

Loading

andy-hanson Apr 8, 2017 •

edited

Loading