False positive on handle-callback-err when not using parentheses

[fxlemire]

tslint: 4.0.2
tslint-eslint-rules: 3.1.0
typescript: 2.1.4
vscode: 1.7.2
vscode-tslint: 0.7.1

Following code displays a handle-callback-err inside VS Code, but does not if err is wrapped with parentheses.

app.listen(PORT, err => {
  if (err) {
    console.log(err);
    return;
  }
}

rule: "handle-callback-err": [true, "^(err|error)$"]

[jmlopez-rod]

I have a fix for the rule (rewrote the logic to find out if we use the parameter) and I have also added the eslint tests. I wanted to see if the original code would pass the missing eslint tests and ...

they do. With the exception of the cases that @fxlemire mentioned. For instance:

test = err => err

fails but

test = (err) => err

is fine.

I think there might be a bug with typescript. It all comes down to this line:

const highlights = this.languageService.getDocumentHighlights(fileName, parameter.pos, [fileName]);

When we don't use parentheses highlights is undefined. If we follow the definition of getDocumentHighlights we see this call:

var node = ts.getTouchingWord(sourceFile, position);
console.log('node:', [node.getText()]);

In this case we would expect the node to the one corresponding to the parameter err. But instead when we don't use parentheses it gives us the whole statement:

// output for 'test = (err) => err'
node: [ 'err' ]

// output for 'test = err => err'
node: [ 'test = err => err' ]

Using

$ tsc --version
Version 2.0.10

Just tried the latest version but the error is still there:

*$ tsc --version
Version 2.2.0-dev.20161210

EDIT: Here is the reason:

parameter.pos !== parameter.name.getStart()

When adding parentheses it works because .pos and .name.getStart() are the same. When avoiding parentheses the two numbers are different. Now I feel dumb for rewriting the rule. I guess it is time to do performance test.

[jmlopez-rod]

Original Solution With Quick Fix:

New Solution in which we manually keep track of the function scope instead of using the typescript service to highlight the document for occurrences of the error parameter.

Sorry about the crappy screenshots. This is the modified solution:

https://github.com/buzinas/tslint-eslint-rules/blob/handle-callback-err/src/rules/handleCallbackErrRule.ts

Not as short and elegant as the first one but, more efficient? I'll let you guys decide which solution you want.

[jmlopez-rod]

v3.2.0 has been released. It includes this fix and arrow rules dealing with their style.

In case anyone is wondering why the build is failing, it is because debug@2.4.0 was released minutes before our release and a spelling mistake broke the build.

debug-js/debug#347