Skip to content
This repository has been archived by the owner on Feb 21, 2022. It is now read-only.

Commit

Permalink
[fix] handle-callback-err (closes #146)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
Manuel Lopez committed Dec 10, 2016
1 parent bd7301e commit 2638af9
Show file tree
Hide file tree
Showing 2 changed files with 274 additions and 18 deletions.
102 changes: 84 additions & 18 deletions src/rules/handleCallbackErrRule.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,52 +5,118 @@ 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) === '^') {
this.errorCheck = RegExp.prototype.test.bind(new RegExp(errorArgument));
} 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);
}
}
190 changes: 190 additions & 0 deletions src/test/rules/handleCallbackErrRuleTests.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}`
]);

Expand Down Expand Up @@ -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();

0 comments on commit 2638af9

Please sign in to comment.