diff --git a/src/rules/noUnsafeFinallyRule.ts b/src/rules/noUnsafeFinallyRule.ts index 6c84729b15b..4aa1ad5a2df 100644 --- a/src/rules/noUnsafeFinallyRule.ts +++ b/src/rules/noUnsafeFinallyRule.ts @@ -17,6 +17,7 @@ import * as utils from "tsutils"; import * as ts from "typescript"; + import * as Lint from "../index"; export class Rule extends Lint.Rules.AbstractRule { @@ -40,171 +41,110 @@ export class Rule extends Lint.Rules.AbstractRule { }; /* tslint:enable:object-literal-sort-keys */ - public static FAILURE_TYPE_BREAK = "break"; - public static FAILURE_TYPE_CONTINUE = "continue"; - public static FAILURE_TYPE_RETURN = "return"; - public static FAILURE_TYPE_THROW = "throw"; - public static FAILURE_STRING_FACTORY = (name: string) => { - return `${name} statements in finally blocks are forbidden.`; + public static FAILURE_STRING(name: string): string { + return `'${name}' statements in finally blocks are forbidden.`; } public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { - return this.applyWithWalker(new NoReturnInFinallyScopeAwareWalker(sourceFile, this.getOptions())); + return this.applyWithFunction(sourceFile, walk); } } -/** - * Represents details associated with tracking finally scope. - */ -interface IFinallyScope { - /** - * A value indicating whether the current scope is a `break` boundary. - */ - isBreakBoundary: boolean; - - /** - * A value indicating whether the current scope is a `continue` boundary. - */ - isContinueBoundary: boolean; - - /** - * A value indicating whether the current scope is within a finally block. - */ - isFinallyBlock: boolean; - - /** - * A value indication whether the current scope is a `return` and `throw` boundary. - */ - isReturnsThrowsBoundary: boolean; - - /** - * A collection of `break` or `continue` labels in this scope. - */ - labels: string[]; -} - -/** - * Represents a block walker that identifies finally blocks and walks - * only the blocks that do not change scope for return statements. - */ -class NoReturnInFinallyScopeAwareWalker extends Lint.ScopeAwareRuleWalker { - - protected visitBreakStatement(node: ts.BreakOrContinueStatement) { - if (!this.isControlFlowWithinFinallyBlock(isBreakBoundary, node)) { - super.visitBreakStatement(node); - return; - } - - this.addFailureAtNode(node, Rule.FAILURE_STRING_FACTORY(Rule.FAILURE_TYPE_BREAK)); - } - - protected visitContinueStatement(node: ts.BreakOrContinueStatement) { - if (!this.isControlFlowWithinFinallyBlock(isContinueBoundary, node)) { - super.visitContinueStatement(node); - return; +type JumpStatement = ts.BreakStatement | ts.ContinueStatement | ts.ThrowStatement | ts.ReturnStatement; + +function walk(ctx: Lint.WalkContext): void { + let inFinally = false; + ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void { + switch (node.kind) { + case ts.SyntaxKind.TryStatement: + const { tryBlock, catchClause, finallyBlock } = node as ts.TryStatement; + ts.forEachChild(tryBlock, cb); + if (catchClause !== undefined) { + ts.forEachChild(catchClause, cb); + } + if (finallyBlock !== undefined) { + const old = inFinally; + inFinally = true; + cb(finallyBlock); + inFinally = old; + } + break; + + case ts.SyntaxKind.BreakStatement: + case ts.SyntaxKind.ContinueStatement: + case ts.SyntaxKind.ThrowStatement: + case ts.SyntaxKind.ReturnStatement: + if (inFinally && !jumpIsLocalToFinallyBlock(node as JumpStatement)) { + ctx.addFailureAtNode(node, Rule.FAILURE_STRING(printJumpKind(node as JumpStatement))); + } + // falls through + + default: + return ts.forEachChild(node, cb); } + }); +} - this.addFailureAtNode(node, Rule.FAILURE_STRING_FACTORY(Rule.FAILURE_TYPE_CONTINUE)); - } - - protected visitLabeledStatement(node: ts.LabeledStatement) { - this.getCurrentScope().labels.push(node.label.text); - - super.visitLabeledStatement(node); - } - - protected visitReturnStatement(node: ts.ReturnStatement): void { - if (!this.isControlFlowWithinFinallyBlock(isReturnsOrThrowsBoundary)) { - super.visitReturnStatement(node); - return; - } - - this.addFailureAtNode(node, Rule.FAILURE_STRING_FACTORY(Rule.FAILURE_TYPE_RETURN)); - } - - protected visitThrowStatement(node: ts.ThrowStatement): void { - if (!this.isControlFlowWithinFinallyBlock(isReturnsOrThrowsBoundary)) { - super.visitThrowStatement(node); - return; - } - - this.addFailureAtNode(node, Rule.FAILURE_STRING_FACTORY(Rule.FAILURE_TYPE_THROW)); - } - - public createScope(node: ts.Node): IFinallyScope { - const isScopeBoundary = super.isScopeBoundary(node); - - return { - isBreakBoundary: isScopeBoundary || isLoopBlock(node) || isCaseBlock(node), - isContinueBoundary: isScopeBoundary || isLoopBlock(node), - isFinallyBlock: isFinallyBlock(node), - isReturnsThrowsBoundary: isScopeBoundary, - labels: [], - }; - } - - protected isScopeBoundary(node: ts.Node): boolean { - return super.isScopeBoundary(node) || - isFinallyBlock(node) || - isLoopBlock(node) || - isCaseBlock(node); - } - - private isControlFlowWithinFinallyBlock( - isControlFlowBoundary: (scope: IFinallyScope, node?: TNode) => boolean, node?: TNode): boolean { - const scopes = this.getAllScopes(); - - let currentScope = this.getCurrentScope(); - let depth = this.getCurrentDepth(); - - while (currentScope) { - if (isControlFlowBoundary(currentScope, node)) { - return false; - } - - if (currentScope.isFinallyBlock) { - return true; +function jumpIsLocalToFinallyBlock(jump: JumpStatement): boolean { + const isBreakOrContinue = utils.isBreakOrContinueStatement(jump); + const label = isBreakOrContinue ? (jump as ts.BreakOrContinueStatement).label : undefined; + + 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) { + const parent = node.parent!; + switch (parent.kind) { + case ts.SyntaxKind.TryStatement: + if ((parent as ts.TryStatement).finallyBlock === node) { + return false; + } + break; + + case ts.SyntaxKind.SwitchStatement: + if (jump.kind === ts.SyntaxKind.BreakStatement && !label) { + return true; + } + break; + + case ts.SyntaxKind.ForInStatement: + case ts.SyntaxKind.ForOfStatement: + case ts.SyntaxKind.ForStatement: + case ts.SyntaxKind.WhileStatement: + case ts.SyntaxKind.DoStatement: + if (isBreakOrContinue && !label) { + return true; + } + break; + + case ts.SyntaxKind.LabeledStatement: { + const { text } = (parent as ts.LabeledStatement).label; + if (label && label.text === text) { + return true; + } + break; } - currentScope = scopes[--depth]; + default: + if (utils.isFunctionScopeBoundary(parent)) { + // Haven't seen TryStatement yet, so the function is inside it. + // No jump statement can escape a function, so the jump is local. + return true; + } } - return false; - } -} -function isLoopBlock(node: ts.Node): boolean { - const parent = node.parent; - - return parent !== undefined && - node.kind === ts.SyntaxKind.Block && - (parent.kind === ts.SyntaxKind.ForInStatement || - parent.kind === ts.SyntaxKind.ForOfStatement || - parent.kind === ts.SyntaxKind.ForStatement || - parent.kind === ts.SyntaxKind.WhileStatement || - parent.kind === ts.SyntaxKind.DoStatement); -} - -function isCaseBlock(node: ts.Node): boolean { - return node.kind === ts.SyntaxKind.CaseBlock; -} - -function isFinallyBlock(node: ts.Node): boolean { - const parent = node.parent; - - return parent !== undefined && - node.kind === ts.SyntaxKind.Block && - utils.isTryStatement(parent) && - parent.finallyBlock === node; -} - -function isReturnsOrThrowsBoundary(scope: IFinallyScope) { - return scope.isReturnsThrowsBoundary; -} - -function isContinueBoundary(scope: IFinallyScope, node: ts.ContinueStatement): boolean { - return node.label ? scope.labels.indexOf(node.label.text) >= 0 : scope.isContinueBoundary; + node = parent; + } } -function isBreakBoundary(scope: IFinallyScope, node: ts.BreakStatement): boolean { - return node.label ? scope.labels.indexOf(node.label.text) >= 0 : scope.isBreakBoundary; +function printJumpKind(node: JumpStatement): string { + switch (node.kind) { + case ts.SyntaxKind.BreakStatement: + return "break"; + case ts.SyntaxKind.ContinueStatement: + return "continue"; + case ts.SyntaxKind.ThrowStatement: + return "throw"; + case ts.SyntaxKind.ReturnStatement: + return "return"; + } } diff --git a/test/rules/no-unsafe-finally/test.js.lint b/test/rules/no-unsafe-finally/test.js.lint index 394d3729689..508a0b4a474 100644 --- a/test/rules/no-unsafe-finally/test.js.lint +++ b/test/rules/no-unsafe-finally/test.js.lint @@ -2,7 +2,7 @@ function() { try { } finally { return; - ~~~~~~~ [no-return-in-finally] + ~~~~~~~ [return] } } @@ -11,7 +11,7 @@ function() { } catch { } finally { return 1; - ~~~~~~~~~ [no-return-in-finally] + ~~~~~~~~~ [return] } } @@ -21,7 +21,7 @@ function() { } finally { { return 1; - ~~~~~~~~~ [no-return-in-finally] + ~~~~~~~~~ [return] } } } @@ -33,7 +33,7 @@ function() { for (let i = 0; i < 5; ++i) { if (i % 2 === 0) { return 1; - ~~~~~~~~~ [no-return-in-finally] + ~~~~~~~~~ [return] } } } @@ -47,7 +47,7 @@ function() { while (i < 5) { if (i % 2 === 0) { return i; - ~~~~~~~~~ [no-return-in-finally] + ~~~~~~~~~ [return] } ++i; @@ -63,7 +63,7 @@ function() { do { if (i % 2 === 0) { return i; - ~~~~~~~~~ [no-return-in-finally] + ~~~~~~~~~ [return] } ++i; @@ -79,7 +79,7 @@ function() { } catch { } finally { return 2; - ~~~~~~~~~ [no-return-in-finally] + ~~~~~~~~~ [return] } } } catch { @@ -87,7 +87,7 @@ function() { for (const i of [1, 2, 3, 4, 5]) { if (i % 2 === 0) { return 1; - ~~~~~~~~~ [no-return-in-finally] + ~~~~~~~~~ [return] } } } @@ -98,7 +98,7 @@ function foo() { try { } finally { return 1; - ~~~~~~~~~ [no-return-in-finally] + ~~~~~~~~~ [return] } } catch { } @@ -108,7 +108,7 @@ function foo() { try { } finally { throw "error"; - ~~~~~~~~~~~~~~ [no-throw-in-finally] + ~~~~~~~~~~~~~~ [throw] } } @@ -117,7 +117,7 @@ function foo() { try { } finally { continue; - ~~~~~~~~~ [no-continue-in-finally] + ~~~~~~~~~ [continue] } } } @@ -127,7 +127,7 @@ function foo() { try { } finally { break; - ~~~~~~ [no-break-in-finally] + ~~~~~~ [break] } } } @@ -137,7 +137,7 @@ function foo() { try { } finally { continue; - ~~~~~~~~~ [no-continue-in-finally] + ~~~~~~~~~ [continue] } } } @@ -147,7 +147,7 @@ function foo() { try { } finally { continue; - ~~~~~~~~~ [no-continue-in-finally] + ~~~~~~~~~ [continue] } } while (i > 0); } @@ -157,7 +157,7 @@ function foo() { try { } finally { continue; - ~~~~~~~~~ [no-continue-in-finally] + ~~~~~~~~~ [continue] } } } @@ -167,7 +167,7 @@ function foo() { try { } finally { break; - ~~~~~~ [no-break-in-finally] + ~~~~~~ [break] } } while (i > 0); } @@ -177,7 +177,7 @@ function foo() { try { } finally { break; - ~~~~~~ [no-break-in-finally] + ~~~~~~ [break] } } } @@ -189,14 +189,14 @@ function foo() { return; } finally { break; - ~~~~~~ [no-break-in-finally] + ~~~~~~ [break] } break; default: try { } finally { break; - ~~~~~~ [no-break-in-finally] + ~~~~~~ [break] } break; } @@ -207,7 +207,7 @@ function foo() { try { } finally { break label; - ~~~~~~~~~~~~ [no-break-in-finally] + ~~~~~~~~~~~~ [break] } } @@ -217,7 +217,7 @@ function foo() { try { } finally { break label; - ~~~~~~~~~~~~ [no-break-in-finally] + ~~~~~~~~~~~~ [break] } } } @@ -230,7 +230,7 @@ function foo() { inner: for (let j = 0; j < 2; ++j) { break outer; - ~~~~~~~~~~~~ [no-break-in-finally] + ~~~~~~~~~~~~ [break] } } } @@ -242,7 +242,7 @@ function foo() { try { } finally { continue label; - ~~~~~~~~~~~~~~~ [no-continue-in-finally] + ~~~~~~~~~~~~~~~ [continue] } } } @@ -255,7 +255,7 @@ function foo() { inner: for (let j = 0; j < 2; ++j) { continue outer; - ~~~~~~~~~~~~~~~ [no-continue-in-finally] + ~~~~~~~~~~~~~~~ [continue] } } } @@ -355,7 +355,7 @@ function() { } } -[no-return-in-finally]: return statements in finally blocks are forbidden. -[no-throw-in-finally]: throw statements in finally blocks are forbidden. -[no-break-in-finally]: break statements in finally blocks are forbidden. -[no-continue-in-finally]: continue statements in finally blocks are forbidden. +[return]: 'return' statements in finally blocks are forbidden. +[throw]: 'throw' statements in finally blocks are forbidden. +[break]: 'break' statements in finally blocks are forbidden. +[continue]: 'continue' statements in finally blocks are forbidden. diff --git a/test/rules/no-unsafe-finally/test.ts.lint b/test/rules/no-unsafe-finally/test.ts.lint index 394d3729689..b64d8f7408c 100644 --- a/test/rules/no-unsafe-finally/test.ts.lint +++ b/test/rules/no-unsafe-finally/test.ts.lint @@ -2,7 +2,7 @@ function() { try { } finally { return; - ~~~~~~~ [no-return-in-finally] + ~~~~~~~ [return] } } @@ -11,7 +11,7 @@ function() { } catch { } finally { return 1; - ~~~~~~~~~ [no-return-in-finally] + ~~~~~~~~~ [return] } } @@ -21,7 +21,7 @@ function() { } finally { { return 1; - ~~~~~~~~~ [no-return-in-finally] + ~~~~~~~~~ [return] } } } @@ -33,7 +33,7 @@ function() { for (let i = 0; i < 5; ++i) { if (i % 2 === 0) { return 1; - ~~~~~~~~~ [no-return-in-finally] + ~~~~~~~~~ [return] } } } @@ -47,7 +47,7 @@ function() { while (i < 5) { if (i % 2 === 0) { return i; - ~~~~~~~~~ [no-return-in-finally] + ~~~~~~~~~ [return] } ++i; @@ -63,7 +63,7 @@ function() { do { if (i % 2 === 0) { return i; - ~~~~~~~~~ [no-return-in-finally] + ~~~~~~~~~ [return] } ++i; @@ -79,7 +79,7 @@ function() { } catch { } finally { return 2; - ~~~~~~~~~ [no-return-in-finally] + ~~~~~~~~~ [return] } } } catch { @@ -87,7 +87,7 @@ function() { for (const i of [1, 2, 3, 4, 5]) { if (i % 2 === 0) { return 1; - ~~~~~~~~~ [no-return-in-finally] + ~~~~~~~~~ [return] } } } @@ -98,7 +98,7 @@ function foo() { try { } finally { return 1; - ~~~~~~~~~ [no-return-in-finally] + ~~~~~~~~~ [return] } } catch { } @@ -108,7 +108,7 @@ function foo() { try { } finally { throw "error"; - ~~~~~~~~~~~~~~ [no-throw-in-finally] + ~~~~~~~~~~~~~~ [throw] } } @@ -117,7 +117,7 @@ function foo() { try { } finally { continue; - ~~~~~~~~~ [no-continue-in-finally] + ~~~~~~~~~ [continue] } } } @@ -127,7 +127,7 @@ function foo() { try { } finally { break; - ~~~~~~ [no-break-in-finally] + ~~~~~~ [break] } } } @@ -137,7 +137,7 @@ function foo() { try { } finally { continue; - ~~~~~~~~~ [no-continue-in-finally] + ~~~~~~~~~ [continue] } } } @@ -147,7 +147,7 @@ function foo() { try { } finally { continue; - ~~~~~~~~~ [no-continue-in-finally] + ~~~~~~~~~ [continue] } } while (i > 0); } @@ -157,7 +157,7 @@ function foo() { try { } finally { continue; - ~~~~~~~~~ [no-continue-in-finally] + ~~~~~~~~~ [continue] } } } @@ -167,7 +167,7 @@ function foo() { try { } finally { break; - ~~~~~~ [no-break-in-finally] + ~~~~~~ [break] } } while (i > 0); } @@ -177,7 +177,7 @@ function foo() { try { } finally { break; - ~~~~~~ [no-break-in-finally] + ~~~~~~ [break] } } } @@ -189,14 +189,14 @@ function foo() { return; } finally { break; - ~~~~~~ [no-break-in-finally] + ~~~~~~ [break] } break; default: try { } finally { break; - ~~~~~~ [no-break-in-finally] + ~~~~~~ [break] } break; } @@ -207,7 +207,7 @@ function foo() { try { } finally { break label; - ~~~~~~~~~~~~ [no-break-in-finally] + ~~~~~~~~~~~~ [break] } } @@ -217,7 +217,7 @@ function foo() { try { } finally { break label; - ~~~~~~~~~~~~ [no-break-in-finally] + ~~~~~~~~~~~~ [break] } } } @@ -230,7 +230,7 @@ function foo() { inner: for (let j = 0; j < 2; ++j) { break outer; - ~~~~~~~~~~~~ [no-break-in-finally] + ~~~~~~~~~~~~ [break] } } } @@ -242,7 +242,7 @@ function foo() { try { } finally { continue label; - ~~~~~~~~~~~~~~~ [no-continue-in-finally] + ~~~~~~~~~~~~~~~ [continue] } } } @@ -255,7 +255,7 @@ function foo() { inner: for (let j = 0; j < 2; ++j) { continue outer; - ~~~~~~~~~~~~~~~ [no-continue-in-finally] + ~~~~~~~~~~~~~~~ [continue] } } } @@ -355,7 +355,22 @@ function() { } } -[no-return-in-finally]: return statements in finally blocks are forbidden. -[no-throw-in-finally]: throw statements in finally blocks are forbidden. -[no-break-in-finally]: break statements in finally blocks are forbidden. -[no-continue-in-finally]: continue statements in finally blocks are forbidden. +function() { + try { + } finally { + function f() { + return; + } + } +} + +try { +} finally { + break; + ~~~~~~ [break] +} + +[return]: 'return' statements in finally blocks are forbidden. +[throw]: 'throw' statements in finally blocks are forbidden. +[break]: 'break' statements in finally blocks are forbidden. +[continue]: 'continue' statements in finally blocks are forbidden.