From 80dc5225b58de764d7f2b6e3fc9d0c7993a6c07a Mon Sep 17 00:00:00 2001 From: HamletDRC Date: Sat, 10 Oct 2015 06:14:14 +0200 Subject: [PATCH] [Issue #34] new promise-must-complete rule closes #34 --- README.md | 1 + src/promiseMustCompleteRule.ts | 179 ++++++++++ tests/PromiseMustCompleteRuleTests.ts | 455 ++++++++++++++++++++++++++ tslint.json | 1 + 4 files changed, 636 insertions(+) create mode 100644 src/promiseMustCompleteRule.ts create mode 100644 tests/PromiseMustCompleteRuleTests.ts diff --git a/README.md b/README.md index f7321d4cb..f9f41822b 100644 --- a/README.md +++ b/README.md @@ -70,6 +70,7 @@ Rule Name | Description | Since `no-unused-imports` | Remove unused imports | 0.0.1 `no-with-statement` | Do not use with statements. Assign the item to a new variable instead | 0.0.1 `prefer-array-literal` | Use array literal syntax when declaring arry types instead of the Array object with a generic parameter type. For example, prefer the Javascript from of string[] to the TypeScript form Array | 0.0.5 +`promise-must-complete` | When a Promise instance is created, then either the reject() or resolve() parameter must be called on it within all code branches in the scope. For more examples see the [feature request](https://github.com/Microsoft/tslint-microsoft-contrib/issues/34). | 0.0.5 `react-no-dangerous-html` | Do not use React's dangerouslySetInnerHTML API. This rule finds usages of the dangerouslySetInnerHTML API (but not any JSX references). For more info see the [react-no-dangerous-html Rule wiki page](https://github.com/Microsoft/tslint-microsoft-contrib/wiki/react-no-dangerous-html-Rule). | 0.0.2 `use-named-parameter` | Do not reference the arguments object by numerical index; instead, use a named parameter. This rule is similar to JSLint's [Use a named parameter](https://jslinterrors.com/use-a-named-parameter) rule. | 0.0.3 diff --git a/src/promiseMustCompleteRule.ts b/src/promiseMustCompleteRule.ts new file mode 100644 index 000000000..d0d99ddc5 --- /dev/null +++ b/src/promiseMustCompleteRule.ts @@ -0,0 +1,179 @@ +import ErrorTolerantWalker = require('./ErrorTolerantWalker'); +import AstUtils = require('./AstUtils'); +import Utils = require('./Utils'); + +/** + * Implementation of the promise-must-complete rule. + */ +export class Rule extends Lint.Rules.AbstractRule { + public static FAILURE_STRING = 'A Promise was found that appears to not have resolve or reject invoked on all code paths'; + + public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + return this.applyWithWalker(new PromiseAnalyzer(sourceFile, this.getOptions())); + } +} + +class PromiseAnalyzer extends ErrorTolerantWalker { + + private isPromiseDeclaration(node: ts.NewExpression): boolean { + if (node.expression.kind === ts.SyntaxKind.Identifier + && node.expression.getText() === 'Promise' + && node.arguments != null && node.arguments.length > 0) { + + let firstArg: ts.Expression = node.arguments[0]; + if (firstArg.kind === ts.SyntaxKind.ArrowFunction || firstArg.kind === ts.SyntaxKind.FunctionExpression) { + return true; + } + } + return false; + } + + private getCompletionIdentifiers(declaration: ts.SignatureDeclaration): ts.Identifier[] { + var result: ts.Identifier[] = []; + if (declaration.parameters == null || declaration.parameters.length === 0) { + return result; + } + + let arg1: ts.ParameterDeclaration = declaration.parameters[0]; + let arg2: ts.ParameterDeclaration = declaration.parameters[1]; + if (arg1 != null && arg1.name.kind === ts.SyntaxKind.Identifier) { + result.push(declaration.parameters[0].name); + } + if (arg2 != null && arg2.name.kind === ts.SyntaxKind.Identifier) { + result.push(declaration.parameters[1].name); + } + return result; + } + + protected visitNewExpression(node: ts.NewExpression): void { + if (this.isPromiseDeclaration(node)) { + let functionArgument: ts.FunctionLikeDeclaration = node.arguments[0]; + let functionBody = functionArgument.body; + let competionIdentifiers : ts.Identifier[] = this.getCompletionIdentifiers(functionArgument); + this.validatePromiseUsage(node, functionBody, competionIdentifiers); + } + super.visitNewExpression(node); + } + + private validatePromiseUsage(promiseInstantiation: ts.NewExpression, block: ts.Node, completionIdentifiers: ts.Identifier[]) : void { + let blockAnalyzer = new PromiseCompletionWalker(this.getSourceFile(), this.getOptions(), completionIdentifiers); + blockAnalyzer.visitNode(block); + if (!blockAnalyzer.isAlwaysCompleted()) { + var failure = this.createFailure(promiseInstantiation.getStart(), promiseInstantiation.getWidth(), Rule.FAILURE_STRING); + this.addFailure(failure); + } + } +} + +class PromiseCompletionWalker extends ErrorTolerantWalker { + + private completionIdentifiers: ts.Identifier[]; + private wasCompleted : boolean = false; + private allBranchesCompleted : boolean = true; // by default, there are no branches, so this is true + private hasBranches : boolean = false; + private walkerOptions: Lint.IOptions; + + constructor(sourceFile: ts.SourceFile, options: Lint.IOptions, completionIdentifiers: ts.Identifier[]) { + super(sourceFile, options); + this.walkerOptions = options; // we need to store this because this.getOptions() returns undefined even when this has a value + this.completionIdentifiers = completionIdentifiers; + } + + // need to make this public so it can invoked from parent walker + public visitNode(node: ts.Node): void { + super.visitNode(node); + } + + public isAlwaysCompleted() : boolean { + if (this.wasCompleted) { + return true; // if the main code path completed then it doesn't matter what the child branches did + } + if (!this.hasBranches) { + return false; // if there were no branches and it is not complete... then it is in total not complete. + } + return this.allBranchesCompleted; // if main path did *not* complete, the look at child branch status + } + + protected visitIfStatement(node: ts.IfStatement): void { + this.hasBranches = true; + + // an if statement is a branch, so we need to see if this branch completes. + let ifAnalyzer = new PromiseCompletionWalker(this.getSourceFile(), this.walkerOptions, this.completionIdentifiers); + let elseAnalyzer = new PromiseCompletionWalker(this.getSourceFile(), this.walkerOptions, this.completionIdentifiers); + + ifAnalyzer.visitNode(node.thenStatement); + + if (!ifAnalyzer.isAlwaysCompleted()) { + this.allBranchesCompleted = false; + } else if (node.elseStatement != null) { + elseAnalyzer.visitNode(node.elseStatement); + if (!elseAnalyzer.isAlwaysCompleted()) { + this.allBranchesCompleted = false; + } + } + // there is no need to call super.visit because we already took care of walking all the branches + } + + protected visitCallExpression(node: ts.CallExpression): void { + + if (node.expression.kind === ts.SyntaxKind.Identifier) { + if (this.isCompletionIdentifier(node.expression)) { + this.wasCompleted = true; + return; // this branch was completed, do not walk any more. + } + } + + let referenceEscaped : boolean = Utils.exists(node.arguments, (argument: ts.Expression) : boolean => { + return this.isCompletionIdentifier(argument); + }); + if (referenceEscaped) { + this.wasCompleted = true; + return; // this branch was completed, do not walk any more. + } + super.visitCallExpression(node); + } + + + protected visitArrowFunction(node: ts.FunctionLikeDeclaration): void { + // walk into function body but do not track any shadowed identifiers + var nonShadowedIdentifiers: ts.Identifier[] = this.getNonShadowedCompletionIdentifiers(node); + let analyzer = new PromiseCompletionWalker(this.getSourceFile(), this.walkerOptions, nonShadowedIdentifiers); + analyzer.visitNode(node.body); + if (analyzer.isAlwaysCompleted()) { + this.wasCompleted = true; + } + } + + protected visitFunctionExpression(node: ts.FunctionExpression): void { + // walk into function body but do not track any shadowed identifiers + var nonShadowedIdentifiers: ts.Identifier[] = this.getNonShadowedCompletionIdentifiers(node); + let analyzer = new PromiseCompletionWalker(this.getSourceFile(), this.walkerOptions, nonShadowedIdentifiers); + analyzer.visitNode(node.body); + if (analyzer.isAlwaysCompleted()) { + this.wasCompleted = true; + } + } + + private getNonShadowedCompletionIdentifiers(declaration: ts.FunctionLikeDeclaration): ts.Identifier[] { + + let result: ts.Identifier[] = []; + this.completionIdentifiers.forEach((identifier: ts.Identifier): void => { + // if this identifier is not shadowed, then add it to result + var isShadowed: boolean = Utils.exists(declaration.parameters, (parameter: ts.ParameterDeclaration): boolean => { + return AstUtils.isSameIdentifer(identifier, parameter.name); + }); + if (!isShadowed) { + result.push(identifier); + } + }); + + return result; + } + + private isCompletionIdentifier(sourceIdentifier: ts.Node): boolean { + return Utils.exists(this.completionIdentifiers, (identifier: ts.Identifier): boolean => { + return AstUtils.isSameIdentifer(sourceIdentifier, identifier); + }); + + } +} diff --git a/tests/PromiseMustCompleteRuleTests.ts b/tests/PromiseMustCompleteRuleTests.ts new file mode 100644 index 000000000..8461cedf0 --- /dev/null +++ b/tests/PromiseMustCompleteRuleTests.ts @@ -0,0 +1,455 @@ +/// +/// + +/* tslint:disable:quotemark */ +/* tslint:disable:no-multiline-string */ + +import TestHelper = require('./TestHelper'); + +/** + * Unit tests. + */ +describe('promiseMustCompleteRule', () : void => { + + var ruleName : string = 'promise-must-complete'; + + describe('should pass', () : void => { + + it('when promise completes', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + if (something) { + resolve('value'); + } else { + if (somethingElse) { + resolve('value'); + } else { + reject(); + } + } + })`; + + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + it('on resolve - lambda', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + resolve('value'); + })`; + + TestHelper.assertViolations(ruleName, script, []); + }); + + it('on resolve - function', () : void => { + var script : string = ` + new Promise(function (resolve, reject) { + resolve('value'); + })`; + + TestHelper.assertViolations(ruleName, script, []); + }); + + it('on resolve - alternative name', () : void => { + var script : string = ` + new Promise((someOtherName, reject) => { + someOtherName('value'); + })`; + + TestHelper.assertViolations(ruleName, script, []); + }); + + it('on reject', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + reject('value); + })`; + + TestHelper.assertViolations(ruleName, script, []); + }); + + it('on reject - function', () : void => { + var script : string = ` + new Promise(function (resolve, reject) { + reject('value); + })`; + + TestHelper.assertViolations(ruleName, script, []); + }); + + it('on reject - alternative name', () : void => { + var script : string = ` + new Promise((resolve, someOtherName) => { + someOtherName('value); + })`; + + TestHelper.assertViolations(ruleName, script, []); + }); + + it('when single branch is completed - with if-statement', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + if (something) { + resolve('value'); + } + })`; + + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + it('when single branch is completed - with if-else-statement', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + if (something) { + resolve('value'); + } else { + resolve('value'); + } + })`; + + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + it('when single branch is completed - with if-else-statement', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + if (something) { + if (somethingElse) { + resolve('value'); + } else { + reject(); + } + } else { + if (somethingElse) { + resolve('value'); + } else { + reject(); + } + } + })`; + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + it('with nested if-else statement', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + if (something) { + if (somethingElse) { + resolve('value'); + } else { + reject(); + } + } else { + if (somethingElse) { + somethingElse(); + } else { + reject(); + } + reject(); // branches are not even analyzed when main thread resolves + } + })`; + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + it('when resolved within a function', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + someCall(function () { + resolve('value'); + }); + })`; + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + it('when resolved within a lambda', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + someCall(() => { + resolve(); + }); + })`; + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + it('when resolved within a lambda - with extra parameter', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + someCall((someParm) => { + resolve('value'); + }); + })`; + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + it('when resolved within a for loop', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + for(var x = 0; x < something.length; x++) { + resolve('value'); + } + })`; + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + it('when resolved within a for in loop', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + for(var x in something) { + resolve('value'); + } + })`; + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + it('when resolved within a while loop', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + while (something) { + resolve(); + } + })`; + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + it('when resolve reference escaped into a function call', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + doSomething(resolve); // reference escapes and we assume it resolves + })`; + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + it('when reject reference escaped into a function call', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + doSomething(reject); // reference escapes and we assume it resolves + })`; + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + it('when non-shadowed parameter resolves within a function', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + someCall(function (arg1, reject) { + resolve('value'); + }); + })`; + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + it('when non-shadowed parameter rejects within a function', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + someCall(function (resolve, arg2) { + reject(); + }); + })`; + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + it('when non-shadowed parameter resolves within a lambda', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + someCall((arg1, reject) => { + resolve('value'); + }); + })`; + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + it('when non-shadowed parameter rejects within a lambda', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + someCall((resolve, arg2) => { + reject(); + }); + })`; + TestHelper.assertViolations(ruleName, script, [ ]); + }); + + }); + + describe('should fail', () : void => { + + it('when empty lambda', () : void => { + var script : string = ` + new Promise(() => { + })`; + + TestHelper.assertViolations(ruleName, script, [ + { + "failure": "A Promise was found that appears to not have resolve or reject invoked on all code paths", + "name": "file.ts", + "ruleName": "promise-must-complete", + "startPosition": { "character": 17, "line": 2 } + } + ]); + }); + + it('when empty function', () : void => { + var script : string = ` + new Promise(function { + })`; + + TestHelper.assertViolations(ruleName, script, [ + { + "failure": "A Promise was found that appears to not have resolve or reject invoked on all code paths", + "name": "file.ts", + "ruleName": "promise-must-complete", + "startPosition": { "character": 17, "line": 2 } + } + ]); + }); + + it('when has no complete', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + })`; + + TestHelper.assertViolations(ruleName, script, [ + { + "failure": "A Promise was found that appears to not have resolve or reject invoked on all code paths", + "name": "file.ts", + "ruleName": "promise-must-complete", + "startPosition": { "character": 17, "line": 2 } + } + ]); + }); + + it('when single branch is missing complete - with if-statement', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + if (something) { + someOtherFunction(); + } + })`; + + TestHelper.assertViolations(ruleName, script, [ + { + "failure": "A Promise was found that appears to not have resolve or reject invoked on all code paths", + "name": "file.ts", + "ruleName": "promise-must-complete", + "startPosition": { "character": 17, "line": 2 } + } + ]); + }); + + it('when single branch is missing complete - with if-else-statement', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + if (something) { + resolve('value'); + } else { + someOtherFunction() + } + })`; + + TestHelper.assertViolations(ruleName, script, [ + { + "failure": "A Promise was found that appears to not have resolve or reject invoked on all code paths", + "name": "file.ts", + "ruleName": "promise-must-complete", + "startPosition": { "character": 17, "line": 2 } + } + ]); + }); + + it('with nested if-else statement', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + if (something) { + if (somethingElse) { + resolve('value'); + } else { + reject(); + } + } else { + if (somethingElse) { + somethingElse(); + } else { + reject(); + } + } + })`; + TestHelper.assertViolations(ruleName, script, [ + { + "failure": "A Promise was found that appears to not have resolve or reject invoked on all code paths", + "name": "file.ts", + "ruleName": "promise-must-complete", + "startPosition": { "character": 17, "line": 2 } + } + ]); + }); + + it('when shadowed parameter resolved within a function', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + someCall(function (resolve) { // this parameter actually shadows the one in the enclosing scope + resolve(); + }); + })`; + TestHelper.assertViolations(ruleName, script, [ + { + "failure": "A Promise was found that appears to not have resolve or reject invoked on all code paths", + "name": "file.ts", + "ruleName": "promise-must-complete", + "startPosition": { "character": 17, "line": 2 } + } + ]); + }); + + it('when shadowed parameter rejects within a function', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + someCall(function (reject) { // this parameter actually shadows the one in the enclosing scope + reject(); + }); + })`; + TestHelper.assertViolations(ruleName, script, [ + { + "failure": "A Promise was found that appears to not have resolve or reject invoked on all code paths", + "name": "file.ts", + "ruleName": "promise-must-complete", + "startPosition": { "character": 17, "line": 2 } + } + ]); + }); + + it('when shadowed parameter resolved within a lambda', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + someCall((arg1, resolve) => { // this parameter actually shadows the one in the enclosing scope + resolve('value'); + }); + })`; + TestHelper.assertViolations(ruleName, script, [ + { + "failure": "A Promise was found that appears to not have resolve or reject invoked on all code paths", + "name": "file.ts", + "ruleName": "promise-must-complete", + "startPosition": { "character": 17, "line": 2 } + } + ]); + }); + + it('when shadowed parameter rejects within a lambda', () : void => { + var script : string = ` + new Promise((resolve, reject) => { + someCall((reject) => { // this parameter actually shadows the one in the enclosing scope + reject(); + }); + })`; + TestHelper.assertViolations(ruleName, script, [ + { + "failure": "A Promise was found that appears to not have resolve or reject invoked on all code paths", + "name": "file.ts", + "ruleName": "promise-must-complete", + "startPosition": { "character": 17, "line": 2 } + } + ]); + }); + }); + +}); +/* tslint:enable:quotemark */ +/* tslint:enable:no-multiline-string */ diff --git a/tslint.json b/tslint.json index 1d53f4dea..ceb6a8680 100644 --- a/tslint.json +++ b/tslint.json @@ -27,6 +27,7 @@ "no-unused-imports": true, "no-with-statement": true, "prefer-array-literal": true, + "promise-must-complete": true, "react-no-dangerous-html": true, "use-named-parameter": true,