no-unused-variable rules incorrectly marks required import

[alexeagle]

Bug Report

• TSLint version: 3.15.1 and HEAD
• TypeScript version: 2.0
• Running TSLint via: (please choose one) CLI

TypeScript code being linted

tsconfig.json

{ "compilerOptions": { "declaration": true } }

modA.ts

export class OpaqueToken {
  private _desc: string;
}

modB.ts

import {OpaqueToken} from './modA';
export class B {
  static BB = new OpaqueToken();
}

use.ts

import {OpaqueToken} from './modA';
import {B} from './modB';

export class Use {
  static PROP = [B.BB];
}

with tslint.json configuration:

{
    "rules": {
      "no-unused-variable": true
    }
}

Actual behavior

The import of OpaqueToken is flagged as unused in use.ts. This causes my PR #1568 to break type-checking Angular when I auto-remove unused imports, with the error

[ts] Public static property 'PROP' of exported class has or is using name 'OpaqueToken' from external module "/Users/alexeagle/tmp123/modA" but cannot be named.
(property) Use.PROP: OpaqueToken[]

Expected behavior

Since we are producing .d.ts files, the OpaqueToken type appears in use.d.ts even though it does not appear in use.ts. Therefore TS requires that the symbol be imported so it knows what import to write in the .d.ts.
This means the import is not unused, even though a naive syntactic walk of use.ts doesn't show the usage.

[alexeagle]

I believe that to fix this, we'll first need to make it possible for non-Lint.Rules.TypedRules to optionally use the typechecker if present. Then we'll be able to fix this bug in the "linterOptions": { "typeCheck": true } case.
I suspect in the other non-typechecker case, fixing this is not feasible.

[alexeagle]

I have a working fix in 43f204a but I want to check with someone on TS team if there's a better API for this. (@paulvanbrenk ?)

The general strategy is, anytime we produce a fix, verify that it doesn't introduce new diagnostics. This way we don't have to repeat logic inside typescript, and can use this in general.

[mhegazy]

The core of the issue here is that the declaration emitter imposes this restriction that a re-exported name has to be imported. For tslint to catch these, it will need to duplicate the logic of the declaration emitter to ensure a name will be needed. this seems a very complicated path. A better and simpler fix can be done on the declaration emitter side, ensuring that it does inject these imports itself. We already have an issue tracking this in microsoft/TypeScript#9944.

As for the proposed fix for checking for a "valid" fix. I can not speak for tslint, but this does not scale for the general case. a large project can take tens of seconds to do a full type check. that means that a user applying this fix in an editor for example has to wait all that time. Also doing this for multiple changes would not scale either.

[alexeagle]

In this case, we only need the preEmitDiagnostics for this single file. I think my case is just like an editor: you delete the import with the squiggly "this import is unused" underline, then immediately you see the red squiggly from the LS and realize that deleting it was the wrong action.

@chuckjaz says that C# refactorings also verify that the result doesn't introduce new diagnostics.

#9944 looks like it will solve this case, but I'm thinking ahead for other cases where tslint doesn't fully reproduce the logic in typescript. I believe we'll be better off not trying to duplicate logic, instead just re-use it by asking the LS for diagnostics.

[mhegazy]

@chuckjaz says that C# refactorings also verify that the result doesn't introduce new diagnostics.

We do not really do what C# does. C# goes to extreme lengths to ensure that refactoring are safe. we do not make these grantees for rename for instance.

#9944 looks like it will solve this case, but I'm thinking ahead for other cases where tslint doesn't fully reproduce the logic in typescript. I believe we'll be better off not trying to duplicate logic, instead just re-use it by asking the LS for diagnostics.

I do not think the general solution scales really. you have to do a full type check. this is the most expensive part of the compilation process. a refactoring should be done in less than 200ms. that is not possible if you run type check. This would be extremely obvious in a project of the size of angular or vscode. I think a better approach here is to ensure that errors are reliable, and that fixes are as conservative as possible. and if there are multiple fixes, show them to the user and let her choose.

[alexeagle]

Okay thanks for your judgement. I'll go ahead with this as a temp. fix for unused import, linking to #9944 as the proper solution, and avoid following this pattern in any other fixes/refactorings.

[chuckjaz]

I would prefer a option between validate vs. unvalidated refactoring instead of having to validate a list of changes myself.

There are cases where the programmer would need the refactoring to be validated. One such example is a wide ranging refactoring that might touch code of which the programmer is unfamiliar. There are also cases where an unvalidated refactoring is valid such as refactoring is local or the project is small or the refactoring done frequently and the programmer has developed a level of trust and understanding of what the refactoring will do. For the unvalidated option the 200ms is valid and reasonable number. In the latter, the programmer would be willing to wait as correctness is more important than responsiveness.

For refactorings across a large or code-base containing potentially many projects for which the programmer requesting the refactoring is unfamiliar and for which the maintainers are unavailable or unresponsive, paying the extra cost of validation is highly desirable.

For validation, we should consider also validating that the result of findReferences() is the expected value after a refactoring for symbol in the affected area. This validates, for example, a refactoring produced valid code but now references different symbols than it did before (e.g. similar to the the capture-substitution problem of lambda calculus).