TypeScriptWarnings.md

Summary

TypeScript meeting notes discussing microsoft/TypeScript#13408 resulted in microsoft/TypeScript#19126 and I'm concerned this is the wrong direction.

Current behavior is broken because lint-like diagnostics are treated as errors.

Proposed PR makes them warnings.

My suggestion is to move these to TSLint.

Problems caused by TypeScript warnings

Warnings should not be presented in compiler output

We have a paper being published in Communications of the ACM in April 2018. (I can provide an advance copy on request). This describes our learning from operating static analysis tooling at Google for the last 6 years. We found conclusively that engineers do not act on a long list of existing warnings. They become "warning blind", we believe this is due to "survivor bias" - existing warnings were already in production, and are therefore mostly false positives. Engineers do not notice if the warning count increases from 100 to 101. And the warnings make it harder to grok the compiler output when there are also errors (sorting them helps).

At Google, we went to great trouble to remove warnings from the compiler outputs in every language. In Java, where I was most involved, we re-implemented these warnings in Error Prone (http://ErrorProne.info) so they do not appear in the compiler output

Then we instead run warnings-producing tools (eg. linters) in the code review process instead. This naturally presents only newly introduced warnings, as users only visit changed regions during a code review. (with the caveat that non-local introduction of warnings is possible, users have the option of seeing them all) Even our command line tool, equivalent of git lint runs all linters across all languages and only shows newly introduced, unless you pass a flag requesting all of them.

We will have a bunch of work to make tsc work in our warnings workflow

As I said above, we only produce warnings during code review. Currently we have taught the systems involved how to run tslint (including with type-check rules). Teaching it how to run tsc as well is a lot of work for us and makes maintenance more complicated. I suspect it would be less work to just re-implement checks like noUnusedLocals in TSLint and use our existing integration to execute them, rather than run tsc.

We may not be the only ones affected in this way.

Governance: does TS-team have time to support existing lint-like diagnostics, will you add new ones, etc.

If we establish the precedent that TypeScript does some linting, where does this end? What is the stance on future PRs that add new lint-like diagnostics? What guidance will you give about whether these belong in the TSLint repository instead? What about the related user support?

Warnings should have a suppression mechanism

We now have // @ts-ignore, however this is designed to suppress type-check errors. We should be careful about teaching users about this mechanism. It further conflates whether an unused variable is a type-check error or not. Also this mechanism is not as feature-rich as https://palantir.github.io/tslint/usage/rule-flags/

Worse, a given file may have a mix of // tslint:ignore-next-line and // @ts-ignore comments, and when a user needs to suppress a new warning, they'll have to understand which tool implements the warning to know which comment to use.

Anyone producing .ts files might have to be warnings clean

Angular generates code, but users choose the TypeScript flags this is compiled with. We must therefore generate code that conforms with the strictest of the settings they choose. We don't want to get bug reports from users that we are generating either errors or warnings in their build. If warnings were produced for .d.ts inputs, this would also apply to any library.

ERROR in src/app/app.module.ngfactory.ts(19,5): error TS6133: '_decl0_11' is declared but never used.
src/app/app.module.ngfactory.ts(20,5): error TS6133: '_decl0_41' is declared but never used.
src/app/app.module.ngfactory.ts(21,5): error TS6133: '_decl0_42' is declared but never used.
src/app/app.module.ngfactory.ts(23,5): error TS6133: '_decl0_44' is declared but never used.
src/app/app.module.ngfactory.ts(31,5): error TS6133: '_decl0_65' is declared but never used.
src/app/app.module.ngfactory.ts(32,5): error TS6133: '_decl0_75' is declared but never used.
src/app/app.module.ngfactory.ts(33,5): error TS6133: '_decl0_76' is declared but never used.

Proposal: move lint-like diagnostics to TSLint

Why?

1. TSLint already has the Program, and the TypeChecker, so it is capable of producing the same diagnostics
2. TSLint team already handles the governance, user support, reviewing PRs, release cycle, etc.
3. Simpler mental model for toolchain maintainers and developers. tsc is the type-checker, tslint is the linter.
4. Better code re-use - TypeScript shouldn't take a dependency on tsutils but this has re-usable logic to help implement these rules

How?

1. Find contributor(s) to port the current "lint-like" checks to TSLint. Google's TS team can help here, and I can probably find more help.
2. TSLint release with feature-parity with deprecated flags (we should coordinate with them to schedule this release)
3. Announce deprecation of flags such as "--noUnusedLocals"
4. (opt) Help users migrate: TypeScript release that prints a Warning (yes, a warning...) if these flags are used, or requires some new option --deprecatedLintFlags to continue using them
5. TypeScript release; it either becomes an error to use these flags, or they are a no-op.