visitCallExpression not traversing through all calls

[cyberhck]

Bug Report

• TSLint version: 5.11.0
• TypeScript version: Version 3.1.2
• Running TSLint via: CLI

TypeScript code being linted

const something = (cb: () => void) => {};

const selector = something(() => {
  const something = require("typescript");
});

with tslint.json configuration:

{
  "rules": {
        "no-var-requires": true
  }
}

Actual behavior

Passes

Expected behavior

Fails

But if I change the code for linting to:

const something = require("typescript");

It does fail.

I came across this while working on my own linting rule having a bug, turns out visitCallExpression doesn't call for anything inside a callback for some reason.

As of now we've to use normal walker and visit each child of a source file, which might result it in being slow.

I'm not sure if it's limitation of tslint, or typescript not reporting it or something, I'm not even sure if it's known to tslint team.

Please let me know if this is something tslint will be working on? Or is it actually not possible with visitCallExpression?

Other rules such as no-console actually works because they go through each child.

I did go to AST explorer, and it shows CallExpression.

[JoshuaKGoldberg]

Hey @cyberhck - exciting that you're working on your own linting rule, but this post is a little unclear on problem you're facing. Are you saying that no-var-requires has a bug because visitCallExpression doesn't visit anything, or that the RuleWalker class has a bug in visitCallExpression?

If it's a bug in no-var-requires, that is known and in PR at #2341.

If it's a bug in RuleWalker, that would be surprising given how widely used it is. It's generally recommended to use walker functions anyway: see #2522 (just opened #4298 to track marking the walkers as deprecated).

[cyberhck]

I'm saying RuleWalker might have a bug, (or I missed something),
when I traverse using visitCallExpression, it visits all the top level call expressions, but it doesn't traverse call expressions inside a callback for some reason.

I used visitCallExpression because I thought that's the recommended way of doing it, actually I don't know much about the terminology either, my rule is at: https://github.com/crazyfactory/tslint-rules/blob/master/src/languageRule.ts

If you see there, I'm using visitCallExpression, is that not supposed to be used? Did I miss something?

This languageRule is for our internal purpose only, but the thing it's supposed to detect is working everywhere except if it's inside a callback. Looking at #2341 , looks like it also changed from using visitCallExpression to walk, is this a new way of doing it?

is visitCallExpression not supposed to go through each CallExpression? I'm so confused right now, maybe I didn't understand AST properly or something.

If I use walker instead of visitCallExpression, it seems like it works, but visitCallExpression was so elegant, but for some reason it doesn't visit CallExpressions inside callback (maybe by design?)

[JoshuaKGoldberg]

it doesn't traverse call expressions

That's probably because you're returning before calling super.visitCallExpression(node); in the function. The parent class' visitCallExpression is what handles recursively going to the children of that call expression.

from using visitCallExpression to walk

Yup! This is preferred. Although visitCallExpression seems elegant, it comes with some pretty bad performance issues per that issue.

Search for applyWithFunction in the TSLint source code for examples of how to use walk style walking. classNameRule.ts is a pretty good example of one that only looks for a specific type of node.

Closing this as it's working as intended. Thanks for posting!

[cyberhck]

so you're saying, super.visitCallExpression is supposed to be called initially (or moving that call to first line will solve it)? I feel stupid, sorry for wasting your time 😄

[JoshuaKGoldberg]

Ha, no worries - this stuff isn't documented very heavily. As long as super.visitCallExpression is called, the approach you're taking should work.