From 2638af9b376719877ac0d03d612fdb2ff516b344 Mon Sep 17 00:00:00 2001 From: Manuel Lopez Date: Sat, 10 Dec 2016 02:12:44 -0600 Subject: [PATCH] [fix] handle-callback-err (closes #146) Using a similar technique as in prefer-arrow-functions to determine if a function uses its first parameter. We keep track of the function scopes to determine if the parameter is being used. --- src/rules/handleCallbackErrRule.ts | 102 ++++++++-- src/test/rules/handleCallbackErrRuleTests.ts | 190 +++++++++++++++++++ 2 files changed, 274 insertions(+), 18 deletions(-) diff --git a/src/rules/handleCallbackErrRule.ts b/src/rules/handleCallbackErrRule.ts index 2ede33d..3dcc62e 100644 --- a/src/rules/handleCallbackErrRule.ts +++ b/src/rules/handleCallbackErrRule.ts @@ -5,18 +5,22 @@ export class Rule extends Lint.Rules.AbstractRule { public static FAILURE_STRING = 'Expected error to be handled'; public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { - const languageService = Lint.createLanguageService(sourceFile.fileName, sourceFile.getFullText()); - return this.applyWithWalker(new ErrCallbackHandlerWalker(sourceFile, this.getOptions(), languageService)); + return this.applyWithWalker(new ErrCallbackHandlerWalker(sourceFile, this.getOptions())); } } +interface IFunctionScope { + firstParamName: string | undefined; + hasFirstParam: boolean; +} + class ErrCallbackHandlerWalker extends Lint.RuleWalker { - private languageService: ts.LanguageService; + private stack: IFunctionScope[] = []; private errorCheck: (name: string) => boolean; + private firstParameterName: (node: ts.FunctionLikeDeclaration) => string | undefined; - constructor(sourceFile: ts.SourceFile, options: Lint.IOptions, languageService: ts.LanguageService) { + constructor(sourceFile: ts.SourceFile, options: Lint.IOptions) { super(sourceFile, options); - this.languageService = languageService; const errorArgument = options.ruleArguments[0] || 'err'; if (errorArgument.charAt(0) === '^') { @@ -24,33 +28,95 @@ class ErrCallbackHandlerWalker extends Lint.RuleWalker { } else { this.errorCheck = (name) => name === errorArgument; } + this.firstParameterName = (node: ts.FunctionLikeDeclaration) => { + const param = node.parameters[0]; + return param ? param.name.getText(sourceFile) : undefined; + }; } - public visitFunctionExpression(node: ts.FunctionExpression) { - this.validateFunction(node); - super.visitFunctionExpression(node); + /** + * Pushes new function scope with its first parameter name and the assumption that it is not + * used in its body. + */ + private enterScope(firstParamName?: string): void { + this.stack.push({ + firstParamName, + hasFirstParam: false + }); + } + + /** + * Pops a function scope from the stack. + */ + private exitScope(): IFunctionScope { + return this.stack.pop(); + } + + protected visitSourceFile(node: ts.SourceFile) { + // Reset internal state. + this.stack = []; + super.visitSourceFile(node); } - public visitFunctionDeclaration(node: ts.FunctionDeclaration) { - this.validateFunction(node); + protected visitFunctionDeclaration(node: ts.FunctionDeclaration) { + this.enterScope(this.firstParameterName(node)); super.visitFunctionDeclaration(node); + this.exitFunction(node); + } + + protected visitFunctionExpression(node: ts.FunctionExpression) { + this.enterScope(this.firstParameterName(node)); + super.visitFunctionExpression(node); + this.exitFunction(node); } public visitArrowFunction(node: ts.ArrowFunction) { - this.validateFunction(node); + this.enterScope(this.firstParameterName(node)); super.visitArrowFunction(node); + this.exitFunction(node); } - private validateFunction(node: ts.FunctionLikeDeclaration) { - const parameter = node.parameters[0]; + protected visitCatchClause(node: ts.CatchClause) { + // A catch clause creates another scope in which the first paramter of the parent function + // may no longer be valid. Lets create another scope + this.enterScope(node.variableDeclaration ? node.variableDeclaration.name.getText() : undefined); + super.visitCatchClause(node); + // Just exit the scope since this is not a function + this.exitScope(); + } - if (parameter && this.errorCheck(parameter.name.getText())) { - const fileName = this.getSourceFile().fileName; - const highlights = this.languageService.getDocumentHighlights(fileName, parameter.pos, [fileName]); + private exitFunction(node: ts.FunctionLikeDeclaration) { + const scopeInfo = this.exitScope(); + const param = scopeInfo.firstParamName; + if (param && this.errorCheck(param) && !scopeInfo.hasFirstParam) { + const name = node.parameters[0].name; + const failure = this.createFailure( + name.getStart(this.getSourceFile()), + name.getWidth(this.getSourceFile()), + Rule.FAILURE_STRING + ); + this.addFailure(failure); + } + } - if (!highlights || highlights[0].highlightSpans.length <= 1) { - this.addFailure(this.createFailure(parameter.name.getStart(), parameter.name.getWidth(), Rule.FAILURE_STRING)); + protected visitNode(node: ts.Node) { + if ( + this.stack.length > 0 && + node.parent.kind !== ts.SyntaxKind.Parameter && + node.kind === ts.SyntaxKind.Identifier + ) { + const text = (node as ts.Identifier).text; + // traverse through the function scopes (starting with the inner most) + // until we see one function that uses the current identifier + let i = this.stack.length; + while (i--) { + const info = this.stack[i]; + if (text === info.firstParamName) { + info.hasFirstParam = true; + break; + } } } + super.visitNode(node); } } diff --git a/src/test/rules/handleCallbackErrRuleTests.ts b/src/test/rules/handleCallbackErrRuleTests.ts index 723968a..7a2af9b 100644 --- a/src/test/rules/handleCallbackErrRuleTests.ts +++ b/src/test/rules/handleCallbackErrRuleTests.ts @@ -31,6 +31,22 @@ ruleTester.addTestGroup('standard-pass', 'should pass with standard config', [ cb(err) }) }`, + dedent` + app.listen(PORT, err => { + if (err) { + console.log(err); + return; + } + } + `, + dedent` + app.listen(PORT, (err) => { + if (err) { + console.log(err); + return; + } + } + `, `function handle (arg, err) {}` ]); @@ -98,4 +114,178 @@ ruleTester.addTestGroup('custom-error-regex-fail', 'should fail with custom erro } ]); +ruleTester.addTestGroup('eslint-valid', 'should pass eslint valid tests', [ + 'function test(error) {}', + 'function test(err) {console.log(err);}', + "function test(err, data) {if(err){ data = 'ERROR';}}", + 'var test = function(err) {console.log(err);};', + 'var test = function(err) {if(err){/* do nothing */}};', + 'var test = function(err) {if(!err){doSomethingHere();}else{};}', + 'var test = function(err, data) {if(!err) { good(); } else { bad(); }}', + 'try { } catch(err) {}', + { + code: dedent` + function test(err) { + try { + console.log('error', err); + throw Error('some error'); + } catch (err) { + console.log('Did you handle the original error?', err); + } + }` + }, + dedent` + getData(function(err, data) { + if (err) {} + getMoreDataWith(data, function(err, moreData) { + if (err) {} + getEvenMoreDataWith(moreData, function(err, allOfTheThings) { + if (err) {} + }); + }); + });`, + 'var test = function(err) {if(! err){doSomethingHere();}};', + dedent` + function test(err, data) { + if (data) { + doSomething(function(err) { + console.error(err); + }); + } else if (err) { + console.log(err); + } + }`, + dedent` + function handler(err, data) { + if (data) { + doSomethingWith(data); + } else if (err) { + console.log(err); + } + }`, + 'function handler(err) {logThisAction(function(err) {if (err) {}}); console.log(err);}', + 'function userHandler(err) {process.nextTick(function() {if (err) {}})}', + dedent` + function help() { + function userHandler(err) { + function tester() { + err; + process.nextTick(function() { + err; + }); + } + } + }`, + "function help(done) { var err = new Error('error'); done(); }", + { code: 'var test = err => err;' }, + { code: 'var test = err => !err;' }, + { code: 'var test = err => err.message;' }, + { code: 'var test = function(error) {if(error){/* do nothing */}};', options: ['error'] }, + { code: 'var test = (error) => {if(error){/* do nothing */}};', options: ['error'] }, + { code: 'var test = function(error) {if(! error){doSomethingHere();}};', options: ['error'] }, + { code: 'var test = function(err) { console.log(err); };', options: ['^(err|error)$'] }, + { code: 'var test = function(error) { console.log(error); };', options: ['^(err|error)$'] }, + { code: 'var test = function(anyError) { console.log(anyError); };', options: ['^.+Error$'] }, + { code: 'var test = function(any_error) { console.log(anyError); };', options: ['^.+Error$'] }, + { + code: 'var test = function(any_error) { console.log(any_error); };', + options: ['^.+(e|E)rror$'] + } +]); + +ruleTester.addTestGroup('eslint-invalid', 'should fail eslint invalid tests', [ + { code: 'function test(err) {}', errors: [error] }, + { code: 'function test(err, data) {}', errors: [error] }, + { code: 'function test(err) {errorLookingWord();}', errors: [error] }, + { code: 'function test(err) {try{} catch(err) {}}', errors: [error] }, + { + code: dedent` + function test(err) { + try{} + catch(err) { + console.log('did not handle the test err', err); + } + }`, + errors: [error] + }, + { code: dedent` + function test(err, callback) { + foo(function(err, callback) { + }); + }`, + errors: [error, error] + }, + { code: 'var test = (err) => {};', errors: [error] }, + { code: 'var test = function(err) {};', errors: [error] }, + { code: 'var test = function test(err, data) {};', errors: [error] }, + { code: 'var test = function test(err) {/* if(err){} */};', errors: [error] }, + { code: dedent` + function test(err) { + doSomethingHere(function(err){ + console.log(err); + }) + }`, + errors: [error] + }, + { code: 'function test(error) {}', options: ['error'], errors: [error] }, + { + code: dedent` + getData(function(err, data) { + getMoreDataWith(data, function(err, moreData) { + if (err) {} + getEvenMoreDataWith(moreData, function(err, allOfTheThings) { + if (err) {} + }); + }); + });`, + errors: [error] + }, + { code: dedent` + getData(function(err, data) { + getMoreDataWith(data, function(err, moreData) { + getEvenMoreDataWith(moreData, function(err, allOfTheThings) { + if (err) {} + }); + }); + });`, + errors: [error, error] + }, + { code: dedent` + function userHandler(err) { + logThisAction(function(err) { + if (err) { + console.log(err); + } + }) + }`, + errors: [error]}, + { code: dedent` + function help() { + function userHandler(err) { + function tester(err) { + err; + process.nextTick(function() { + err; + }); + } + } + }`, + errors: [error] + }, + { + code: 'var test = function(anyError) { console.log(otherError); };', + options: ['^.+Error$'], + errors: [error] + }, + { + code: 'var test = function(anyError) { };', + options: ['^.+Error$'], + errors: [error]}, + { + code: 'var test = function(err) { console.log(error); };', + options: ['^(err|error)$'], + errors: [error] + } +]); + ruleTester.runTests();