Migrate to tslint 5 and adopt TS noUnusedLocals

[egamma]

Background

We were running with tslint 3.7 which is now over a year old and the latest version of tslint is 5.8.

The migration was blocked since one of the popular tslint rules no-unused-variables changed its implementation in tslint 4.0 so that it could no longer be supported by the vscode-tslint extension. The reason is that this rule now requires type information, this means, it needs a TypeScript project to run which is too costly (https://github.com/Microsoft/vscode-tslint/issues/70#issuecomment-241041929).

TypeScript noUnsuedLocals

TypeScript provides a compiler option noUnusedLocals that can replace the tslint rule. The issue we had with this option is that it reports false positives and until recently there was no way to suppress a false positive. This has changed in TypeScript 2.6. You can now suppress a check on the next line using the //@ts-ignore comment.

The noUsedLocals option is more powerful than the no-unused-variables tslint rule. It also recognizes unused properties/instance variables. This helps us to identify injected services that are not used.

An important difference between the tslint no-unused-variablesrule and the TypeScript compiler option is that TypeScript reports unused locals as an error and not as a warning. There is #37616 which adds the setting typescript.reportStyleChecksAsWarnings. to control whether style checks should be reported as warnings.

Switching to noUnusedLocals

I've added the noUnusedLocals to our top-level tsconfig.json and the tsconfig.json files of the bundled extensions. I've removed the no-unused-locals rule from the tslint.json.

To make the transition to tslint 5 smooth and to avoid committing code into git with errors, I've added //@ts-ignore comments for all the noUnusedLocals warnings. I've categorized these warnings as follows using a description in the comments:

• @ts-ignore @editorAction uses the class (68 instances) these are false positives that we cannot fix Decorated private propoerty flagged as unused with noUnusedLocals:true TypeScript#13120. We will likely remove the use of the @editorAction decorator to avoid polluting our code with ts-ignore comments.
• @ts-ignore unused injected service (164) instances) we have many places where a service is injected into a class but the service is not used. We must fix those warnings.
• @ts-ignore unused local (11 instances) these are most of the time correct and need to be reviewed and fixed
• @ts-ignore unused property (83 instances) some false positives, they need to be reviewed and fixed if possible.
• @ts-ignore unused type (23 instances) most of them are false positives but need to be reviewed
• @ts-ignore unused generic parameter (14 instances) these are all correct but the type parameter was added for consistency. Nothing we can do about them.

Action

As part of the migration tslint 5 you now have to review your code and remove these comments where possible by removing the unused local. There will still be some false positives that need the //@ts-ignore comment. If this is the case then we should describe the reason in the comment and file an issue against TypeScript.

To review the comments:

• Use search to find the @ts-ignore in the code.
• Remove the ignore comment and fix the code if possible
• If it is a false positive consider reporting an issue against TS if it doesn't exist yet.

To check whether you have removed the warnings you can run the Build VS Code task. This task has a problem matcher so that the TypeScript errors show up in the Problems panel. This requires that the locally installed TS version is 2.6 and picked up by gulp-tsb (we have switched to TS 2.6 recently and you should run an npm install).

Checklist

Reviewed the "no unused locals" errors:

• @alexandrudima
• @weinand
• @chrmarti
• @Tyriar
• @dbaeumer
• @bpasero
• @egamma
• @isidorn
• @joaomoreno
• @jrieken
• @kieferrm
• @aeschli
• @octref
• @rebornix
• @ramya-rao-a
• @roblourens
• @sandy081
• @mjbvz

[alexdima]

I think for the @editorAction we should move away from decorators. It does not appear to be something the TS compiler wishes to support: microsoft/TypeScript#13120 (comment)

now, for the decorator scenario, this is something that is happening outside the knowledge of the compiler. it can not attest to its correctness or validity. for these cases, my recommendation is to switch off --noUnusedLocals.

[bpasero]

@egamma good stuff, thanks for the prep work.

[isidorn]

@egamma very useful. Thanks a lot

[dbaeumer]

@egamma thanks for preparing this.

[joaomoreno]

This is great! Thanks! 0438b90

[egamma]

Closing - no more @ts-ignore!