no-unused-variable: Create a duplicate SourceFile

[andy-hanson]

PR checklist

• Addresses an existing issue: no-unused-variable interferes with other type checked rules #2736, no-unsafe-any: should not complain about function calls which parameters are using any #2728, The implementation of the 'no-unused-variable' rule breaks the tslint language service plugin #2649
• New feature, bugfix, or enhancement
  • Includes tests
• Documentation update

Overview of change:

I tested out on https://github.com/CSchulz/tslint-no-unsafe-any-showcase and this fixed that issue. Hopefully this can fix the errors in VSCode too, but I'd need confirmation. CC @egamma

CHANGELOG.md entry:

[bugfix] no-unused-variable: Create a duplicate SourceFile

[egamma]

@andy-hanson having this problem fixed would be great.

To verify the fix you need to run on case insensitive system like Windows.

Is there a build of tslint in npm that I could try to verify the fix?

If desired I can come up with steps for how to verify the fix with the tslint-language-service plugin.

[nchen63]

microsoft/TypeScript#15344

[andy-hanson]

@egamma You could try with a linked copy? Clone my branch, yarn, yarn compile, and the binary is in ./bin/tslint, with .js files in ./lib.

[CSchulz]

Works for me on Windows.

[egamma]

@andy-hanson with the fix I can no longer reproduce the problem in the TS server plugin, which is great.

However, I have a question about the implementation of this rule in general. One goal of running tslint as a TypeScript server plugin is to reuse/share the Program that the TS server already maintains. Doesn't the current implementation of the no-unused-variable rule create another Program with copies of Source files for each run of the rule? While this is OK when the program is run from the command line, when the rule is run in the TypeScript server as part of the tslint-plugin, then the rule is run as the user types. Will this implementation scale with large Programs?

Also I'm still concerned about the implementation of getCaseSensitiveFileNames which is hardcoded to:

useCaseSensitiveFileNames: () => true,

Won't this bite at some point?

[andy-hanson]

@egamma
Yeah, that could end up doing a lot of work as it just calls getPreEmitDiagnostics and filters out the diagnostics that aren't from an unused variable, meaning it will need to get type information from other files.
I added a commit that limits the Program to a single source file, meaning all imports will fail to resolve. That should prevent it from doing a lot of extra work.

[egamma]

@andy-hanson goodness.

I added a commit that limits the Program to a single source file, meaning all imports will fail to resolve.

Does the rule still need access to the original Program? In other words does the rule still need to be a TypeRule?

Stepping back, the trick with using the compiler options to temporarily enable the TS noUnusedLocal option is a clever way to reuse the TS implementation of this check. It looks like the tslint rule does some additional work on top of what the TS option provides. Wouldn't it make sense to improve the original TS implementation so that there is no need for a temporary program with different compiler options? Reparsing a large single file can still be costly.

[andy-hanson]

The TS implementation marks symbols as used while checking them, so it can't be run independently of a Program.
The rule still uses the checker to filter out some bogus errors (microsoft/TypeScript#9944), so still needs to be a TypedRule.
The real solution to this problem is to fix microsoft/TypeScript#9448 and not need a lint rule for this. Same for --noFallthroughCasesInSwitch vs no-switch-case-fall-through.

[egamma]

The TS implementation marks symbols as used while checking them, so it can't be run independently of a Program .

@andy-hanson got it, actually not a problem when tslint is run as a language server plugin. In this setup we always pass a Program to tslint tslint-language-service.

So what I will do for now is to add a setting for the tslint-language-service plugin that allows a user to supress this rule when running as a plugin.

The real solution to this problem is to fix microsoft/TypeScript#9448 and not need a lint rule for this. >Same for --noFallthroughCasesInSwitch vs no-switch-case-fall-through .

#9448 has lots of comments, but your point is that you want to have some comment like // tslint:ignore ... for disabling a check for the lint style TS checks.

Interestingly TS has added a such a comment to disable type checking in JS files with // @ts-ignore, so the time is good to discuss this again.

[adidahiya]

looks like there are real build failures on Windows (appveyor CI)

[andy-hanson]

Looks like there is still a TypeScript blug blocking this. The checker tries to grab a node.symbol that is undefined. I can "fix" the bug by using the old sourceFile instead of creating a new one. This will have to wait on a solution to microsoft/TypeScript#15344.

[adidahiya]

@ajafff would you be able to take a look here and update the PR as necessary?

[ajafff]

I resolved the merge conflicts. The changes regarding case sensitivity from #2819 are still included.

I had to revert the change that created a program consisting of only one source file. That produced exceptions in getSymbolFlags while type checking. We now copy all source files.

I verified that it no longer has side effects on other rules by linting https://github.com/CSchulz/tslint-type-check-rule-collision
Note that it took 6 seconds to lint this small sample project. That's caused by copying every source file (including declaration files of node_modules) for every linted file.

Re: windows CI failures:
Here's the result of AppVeyor on my branch https://ci.appveyor.com/project/ajafff/tslint/build/1.0.228

[adidahiya]

Note that it took 6 seconds to lint this small sample project

That sounds like a lot. We only take this performance hit when no-unused-variable is enabled, right?

[ajafff]

We only take this performance hit when no-unused-variable is enabled, right?

You're right. This might make the rule unusable for larger projects or as a language service plugin, though.

[egamma]

You're right. This might make the rule unusable for larger projects or as a language service plugin, though.

Agreed, when used in language service plugin this will happen as the user types.

[giladgray]

@andy-hanson please update this branch so we can review it, or close it if not relevant.

[johnwiseheart]

Closing due to age and inactivity. Feel free to re-open or create a new pull request if you decide to continue working on this.