Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Rewrite no-unsafe-finally #2346

Merged
merged 5 commits into from
Apr 12, 2017
Merged

Rewrite no-unsafe-finally #2346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
51178f8
Rewrite no-unsafe-finally
andy-hanson Mar 15, 2017
0779311
Respond to PR comments
andy-hanson Mar 25, 2017
e914f42
Clean up code
andy-hanson Mar 28, 2017
6d1ec57
Add comment
andy-hanson Mar 28, 2017
7322185
Prove that it won't crash on invalid code
andy-hanson Apr 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
246 changes: 93 additions & 153 deletions src/rules/noUnsafeFinallyRule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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<IFinallyScope> {

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>): 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Contributor Author

@andy-hanson andy-hanson Apr 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}
});
}

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<TNode>(
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
    }
}

Copy link
Contributor Author

@andy-hanson andy-hanson Apr 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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";
}
}
Loading