no-unused-variable configuration request

[scriby]

As the no-unused-variable is implemented now, I find it has too many false positives to really be useful. However, if I could just enable a portion of it that would check only local variables, that would be really useful.

[ashwinr]

i see. can you please give a couple of examples of these false positives?

[csnover]

The big problem I think is “private class members”. Imports, variables, and functions can all never be accessed dynamically without using eval, but private class members can easily be accessed dynamically using the array operator. So, private class members should be excluded from no-unused-variable or at least configurable.

[ashwinr]

I don't buy that. private members are used to prevent access; sure javascript has no semantics for this, but typescript does. if you're accessing a private member only using the array operator, then i'd suggest making it public and use it directly.

[csnover]

private members are used to prevent access

What does the access control model for class properties in TypeScript have to do with preventing the accidental creation of unused variables? They’re two different things.

Unused private class methods can be discovered easily during unit testing of the public APIs using code coverage analysis, by identifying methods that are never called, so it’s not critical to lint them; this is not the case for unused variables whose declarations will be covered in testing even when the variables are never actually accessed.

Additionally, in the design of the language itself, variables are guaranteed to be unused if they are never accessed within the scope where they are defined; as we have already discussed, the same is not true about object properties, which both JavaScript and TypeScript allow to be accessed using array operators. This leads to false positives:

class A {
  private _handles:Handle[];

  constructor(emitter:EventEmitter) {
    this._handles = [
      emitter.on('response', lang.hitch(this, '_handleResponse'));
    ];
  }

  destroy():void {
    var handle:Handle;
    while ((handle = this._handles.pop()) {
      handle.remove();
    }
  }

  // this method is used, but tslint says it isn’t
  private _handleResponse(event:Event) {}
}

sure javascript has no semantics for this, but typescript does

As mentioned above, TypeScript does not prescribe that you must access a property of a class using dot operators. a.foo and a['foo'] are both accepted.

if you're accessing a private member only using the array operator, then i'd suggest making it public

1. Public properties introduce new interface properties where private properties do not, which makes such a change inappropriate in most/all cases;
2. Projects that need to support ES3 environments without modifying global objects don’t have Function.prototype.bind available, so use convenience functions like lang.hitch(this, '_foo') in order to generate a new function with the correctly bound this context when passing to event listeners instead of hand-rolling closures every time. This is a very common pattern which relies on the normal practice of using an array operator instead of dot operator for property access. Because this convenience function is generic it also supports late binding of other objects that can have their properties mutated at runtime.

Thanks for your consideration,

[JoshuaKGoldberg]

Sorry if bumping this is against the rules - I've come across the same problem.

class Test {
    private done: boolean;

    constructor() {
        (function (): void {
            this.doPrivateThing();
            (<Test>this).doPrivateThing();
        }).bind(this)();
    }

    private doPrivateThing(): void {
        this.done = false;
    }
}

...gives the following error:

>> test.ts[11, 13]: unused variable: 'doPrivateThing'

It looks like tslint isn't checking inside the bound function. It's understandable that function.bind erases some information, but should <Test>this.doPrivateThing let it know that the variable's being used?

[senhongo]

I have a similar problem maybe?
A variable used for type declaration will trigger the no-unused-variable rule.

import Foo = this.that.Foo;

module some.module.blah {
 export  class bar {
    private foo: Foo;
  }
}

returns the error unused variable: 'Foo

[nikklassen]

The use of an import alias is also not detected when it's used with extends

import DateTimeOpts = Intl.DateTimeFormatOptions;

interface MyDateTimeOpts extends DateTimeOpts {
    timezoneOffset: number;
}

let opts: MyDateTimeOpts;
console.log(opts.timezoneOffset - 1);

[adidahiya]

@SenHeng, @nikklassen: those look like related issues; I've filed a new ticket for those false positives: #613.

as for the other issues in this thread, I think we could address them by adding a new option that makes the no-unused-variable rule skip private class members: #614. I don't want to do anything much more complex / involved than that; I really don't want to encourage any coding style that references private members with dynamic access as suggested in some of the examples above.

[adidahiya]

closing this for now. I can reopen if someone wants to discuss further... or feel free to file a new, more specific issue

[JoshuaKGoldberg]

I think we could address them by adding a new option that makes the no-unused-variable rule skip private class members

Forgive me if I'm misunderstanding, but does that not solve a different problem? Caring whether private variables are unused is different from correctly reporting whether they're unused, yes?

For further clarification, in the below example, waste should be flagged as unused, but done should not be.

class Test {
    private waste: boolean;
    private done: boolean;

    constructor() {
        (function (): void {
            this.doPrivateThing();
            (<Test>this).doPrivateThing();
        }).bind(this)();
    }

    private doPrivateThing(): void {
        this.done = false;
    }
}

[adidahiya]

ok, I think I see what you mean with the difference between waste and done there... but I don't see a straightforward approach that would correctly mark waste as unused while not also marking doPrivateThing() as unused. any ideas here?

basically the option I've suggested in #614 is a less strict version of the no-unused-variable rule for users who want to use dynamic access of private members/methods at all in their codebase.

[JoshuaKGoldberg]

Unfortunately I'm not familiar with the tslint codebase, so that brings up a question. Should the following line above mark doPrivateThing as used?

            (<Test>this).doPrivateThing();

...which brings up another question: does tslint know the anonymous function is being executed?

    constructor() {
        (function (): void {
            (<Test>this).doPrivateThing();
        }).bind(this)();
    }

[adidahiya]

No, we don't do much control flow analysis to know things like "is this anonymous function being executed". I'm hesitant to do that unless we have really good reasons to do so (it complicates the codebase).

(<Test>this).doPrivateThing() is the kind of dynamic access I'm referring to, and I don't even think the compiler recognizes that as a static reference to the private class method (@jkillian -- can you confirm please?). It mainly has to do with Function.prototype.bind, which erases some type information. So if the compiler doesn't count it as a reference, tslint will not do so either.

[jkillian]

@JoshuaKGoldberg
There are currently proposals on supporting typing for this, but until then, TSLint won't be able to recognize that this.doPrivateThing(); from your example refers to an instance of Test and is using its method. And you can see why this would be tricky: the this in the function could be anything. (Just as a note, when directly in a class method, this is typed.)

However, I think (<Test>this).doPrivateThing() should be recognized as a reference to the method by TypeScript and thus and should not be marked as unused by TSLint. Is TSLint marking the method as unused? If so I'll look into it.

[jkillian]

@JoshuaKGoldberg If I run TSLint on your example, I get an unused variable warning for waste but not for anything else. This is how it should work, right? 😃 Are you getting differing results?

[JoshuaKGoldberg]

However, I think (this).doPrivateThing() should be recognized as a reference to the method by TypeScript and thus and should not be marked as unused by TSLint. Is TSLint marking the method as unused? If so I'll look into it.

The incorrect mark was the behavior in July, but I just ran it locally and seems to be working. Thanks for taking the time to look into it, @adidahiya and @jkillian ! Everything seems to be fixed :)

[jkillian]

No problem! Always nice when bugs disappear haha