Repository navigation
error-prone: un-typed parameters of function types #5187
Description
There was a bug in Angular where we had a signature
static forEachWithIndex<T>(array: T[], fn: (T, number) => void) {}
which I'm fixing:
angular/angular@7de9979
because of course without --noImplicitAny it's the same as
static forEachWithIndex<T>(array: T[], fn: (T: any, number: any) => void) {}
which looks completely silly. And the callback may have a large body with many dereferences from these parameters which no one realizes are unchecked.
This is very unfortunate of course, but I think we could statically detect cases where the name of an untyped parameter in a position like this collides with a type expression, and warn/error to save the user from this.
It's not an isolated case, I'm sure. In fact for Java, I started the error-prone project (http://errorprone.info) which augments the Java compiler with third-party type checks, including mis-usages of common libraries. Eddie Aftandilian (@eaftan) and Liam Miller-Cushon (@cushon) did most of the work and made that very successful, running it in every Java compilation at Google and promoting most checks to compile errors (of course using criteria for making these into type errors, which we document http://errorprone.info/docs/criteria)
I wonder what the TypeScript team thinks about further static analysis beyond the TypeScript spec. Do you think it's valuable to extend the compiler with a third-party plugin that catches classes of errors outside the language spec?
Activity
DanielRosenwasser commentedon Oct 9, 2015
Check out #3081.
I am strongly against the syntax as it stands, but the general consensus is that it'd too big of a breaking change.
RyanCavanaugh commentedon Oct 9, 2015
We ❤️ TSLint (we just integrated it into our build pipeline and have written custom rules to apply to our own codebase). If there were a
tscltool that ran TSC + TSLint, I think it'd be a great thing to point people at.That'd be a fantastic TSLint rule.
The problem in question is definitely one of the most common errors ("I wrote x:
(string, number) => voidand I can pass(42, 'hello)!) we see from transitioning users. It was probably the most important use case that drove thenoImplicitAnyflag.We had assumed the intersection of people who would run a linter or other static analysis tool that wouldn't already be running under
noImplicitAnywould be very small. Our model was that anyone doing more "strict" compilation would start withnoImplicitAny, then move to more complex tools. It'd be great to hear about why you would go to a static analysis tool before enabling that flag as it might allow us to fine-tune the rules.tinganho commentedon Oct 9, 2015
Ryan Cavanaugh (@RyanCavanaugh) What about #3003?
alexeagle commentedon Oct 9, 2015
Agree that
--noImplicitAnywould have caught this. For angular codebase, we have a couple weeks of work to add typings everywhere, and most of the remaining ones are lower value (functions with no return statement which are clearlyvoid, test code, private methods).We 💚 TSLint as well, and I've written custom checks to migrate us part of the way to
--noImplicitAny. I think it's better to have finer control on the 'typings coverage required' knob, so that we can make incremental steps to get there. I'm sure other codebases migrating from JS would benefit from the same.But, while I could write a TSLint check for this particular issue, it's only possible because I could require types on every parameter in a function type. TSLint is purely syntactic, as you know, so if I wanted to write other static analysis checks, I would be stuck without the typechecker and symbol table. For example, in this case the bug is when the parameter name matches a type that's available in this scope. I would not want to break compiles if people choose optional typings, that's a feature of TypeScript. For example, that would disallow
function funcFactory(): (_) => void { ... }which is a useful shorthand for users who are bought into the optional/gradual typing sales pitch.So I think #3003 looks interesting, and would allow what I'm thinking.
mhegazy commentedon Oct 21, 2015
#3003 is something that has been on the radar for a while. the problem is it is not a simple undertaking, we need to have a whole infrastructure about discovering, loading, and executing these analyzers from the command line and in the language service. we will also need a way to expose them to the user and possible a way to report perf issues in an analyzer.
As for the OP, as discussed in #3081, it would be a breaking change to require type annotations for all call signatures. catching the simple cases where the parameter name is a name of a primitive type (i.e. doing it syntactically only) would not catch other places where the type name is user defined type. i would agree with Ryan Cavanaugh (@RyanCavanaugh) that this looks like a suitable place for a linter rule.
chore(tslint): disallow variable names that look like keywords.
chore(tslint): disallow variable names that look like keywords.