error-prone: un-typed parameters of function types

[alexeagle]

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. @eaftan and @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?

[DanielRosenwasser]

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]

I wonder what the TypeScript team thinks about further static analysis beyond the TypeScript spec

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 tscl tool that ran TSC + TSLint, I think it'd be a great thing to point people at.

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.

That'd be a fantastic TSLint rule.

The problem in question is definitely one of the most common errors ("I wrote x: (string, number) => void and I can pass (42, 'hello) !) we see from transitioning users. It was probably the most important use case that drove the noImplicitAny flag.

We had assumed the intersection of people who would run a linter or other static analysis tool that wouldn't already be running under noImplicitAny would be very small. Our model was that anyone doing more "strict" compilation would start with noImplicitAny, 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]

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 tscl tool that ran TSC + TSLint, I think it'd be a great thing to point people at.

@RyanCavanaugh What about #3003?

[alexeagle]

Agree that --noImplicitAny would 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 clearly void, 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]

#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 @RyanCavanaugh that this looks like a suitable place for a linter rule.