no-shadowed-variable: please add scope options

[rokoroku]

• TSLint version: v5.5.0
• TypeScript version: 2.4.1
• Running TSLint via: VSCode / gulp-tslint

Description

There is an update for no-shadowed-variable today #2598 .

Enhancement is really good, however, It breaks lots of our code.

It should be options for check interfaces, classes, type parameters, imports, etc so that we can granularly adapt to the rule with specific scopes.

[ajafff]

It should be options for check interfaces, classes, type parameters, imports, etc so that we can granularly adapt to the rule with specific scopes.

Makes sense. I'd rather ignore specific declarations instead of enabling the desired checks.

My idea is to ignore names of ignored declarations completely.
Let's assume you have variables enabled and classes disabled:

• variable shadowed by another variable -> error for the second variable
• variable shadowed by class -> no error
• class shadowed by variable -> no error
• variable shadowed by class shadowed by variable -> error for the second variable
• class shadowed by variable shadowed by class -> no error

[ajafff]

Currently the following declarations are checked:

• variables
• parameters
• all kinds of imports
• functions
• namespaces
• enums
• classes
• interfaces
• type parameters
• type aliases

For which of them do we need an option to ignore it? Do we want to add an option for each of them (except variables and parameters) or an option to ignore all types (enum through type alias)?

[rokoroku]

My main breakages are

1. same function name and callback argument

db.transaction((transaction) => { // Shadowed name: 'transaction'
  return model.doSomething({ transaction }); 
});

2. parameters in a class decorator

class SomeClass {
  @memoize({ normalizer: ([arg1, arg2]) => `...` })
  someMethod(arg1, arg2) { ... } // Shadowed name: 'arg1', 'arg2'
}

3. generics in class static methods

export class GenericClass<T = any> {
  data: T;
  static genericFunction<T>(args: T) { ... } // Shadowed name: 'T'
}

It seems like (1) can be achieved via ignoring function type, but (2) and (3) cannot be achieved in this way. (EDIT: (3) could be suppressed with ignoring type parameters, right? )

So I want to suggest specifying a scope to be checked.. e.g. block scope, function scope, class scope, namespace scope, and file scope.

[ajafff]

(1) I cannot reproduce this with the code provided. There has to be a a function or variable named transaction in a parent scope for the error to occur. But I get your point.

The following would be expected:

transaction((transaction) => { // Shadowed name: 'transaction'
  // do stuff
});

(2) Cannot reproduce with the provided code snippet. This code should not result in an error.

(3) Yeah, that could be suppressed by ignoring type parameters.

---

Seems like it's better to add an option for each kind of declaration.

[rokoroku]

(1) I found that I create a new variable named transaction below the function. It's my fault ;)

(2) Let me show you my screen:

let memoize: any;
class TestClass {
    @memoize({ normalizer: (arg1) => true }, )
    getArg1(arg1: any, arg2: any) {
        return JSON.stringify(arguments);
    }

    @memoize({ normalizer: (arg2) => false }, )
    getArg2(arg1: any, arg2: any) {
        return JSON.stringify(arguments);
    }
}

---

I'll appreciate if you provide any options for this rule, regardless of my problem 👍

[ajafff]

@rokoroku #3030 adds the ability to disable certain checks.
(2) is really a bug. My PR also fixes this.