Discussion: Parameter type inference from function body

[masaeedu]

This PR is not intended to be merged as-is, it is an attempt at demonstrating the feasibility of #15114, and is a request for revisiting this topic.

The change facilitates inference of parameter types from usage as arguments within the function body. Some motivating examples and the inferred types are shown below:

// CASE 1
function foo(s) {
    Math.sqrt(s)
}
// => function foo(s: number): void

// CASE 2
declare function swapNumberString(n: string): number;
declare function swapNumberString(n: number): string;

function subs(s) {
  return swapNumberString(s);
}
// => function subs(s: string): number

// NOTE: Still broken, needs to deal with overloads. Should have been inferred as:
// => (s: string) => number & (s: number) => string

// CASE 3
function f(x: number){
   return x;
}

function g(x){ return f(x); };
// => function g(x: number): number

[masaeedu]

Need to find robust mechanism for resolving the node corresponding to the invoked function to a function signature. Right now things like interface Foo{ (bar: Bar): Baz } can mess it up.

[masaeedu]

Down to one failed testcase now.

[masaeedu]

All tests should be passing now. In terms of implementation details, I still need to figure out how to robustly:

• Retrieve the parameter symbol associated with a given argument in a CallExpression
• Obtain a workable signature from things like interface Foo { (bar: Bar): Baz } so I can retrieve the parameters associated with arguments

That aside, I think it would be good to start looking at examples of tricky cases where either the implementation or the proposed semantics blow up.

[HerringtonDarkholme]

getIntersectionType cannot handle flow sensitive typing. For example,

function test(a) {
  if (someCond) roundNumber(a)
  else trimString(a)
}

[masaeedu]

@HerringtonDarkholme Could you provide a concrete example of this? For which reference is someCond affecting the type?

[masaeedu]

@HerringtonDarkholme I understand what you mean now; in the case you highlighted a should be inferred as number | string instead of number & string. number & string is an excessively conservative inference, but it is a sound one: number & string is assignable to number | string.

Some options are to bail and infer any for parameters used in branched code, keep the existing behavior and expect the user to manually supply typings when they realize number & string is unsatisfiable, or to actually analyze the flow graph and generate the appropriate union type.

[HerringtonDarkholme]

How does this work with recursive function? I guess it need some guard statement.

[masaeedu]

getTypeOfParameter calls getTypeOfSymbol, which internally has a stack checking for circularity and ejecting with "unknown". I believe we can rely on this, but I need to add tests.

[HerringtonDarkholme]

What if the parameterType is a generic type parameter? Should it be propagated to the inferred function?

[masaeedu]

Yes, probably it should be ensured that the function currently under inference does not already have an explicit type parameter list, and a type parameter to add polymorphism wrt the parameter under inference. If there is already a type parameter list we should just bail.

[HerringtonDarkholme]

Clearly, only collecting invocation isn't enough. But I wonder how other expressions can be handled. For example, how arg.assigment = 123 is handled.

[masaeedu]

@HerringtonDarkholme I've put out some initial thoughts in #15114 (comment). We can exploit co and contravariance to get sensible rules here. If something assigns to a parameter, the sensible thing is for the parameter to be inferred as having a supertype of said assignment. Unfortunately TypeScript can't represent this.

This means we should restrict ourselves to inference in covariant usages of the parameter variable, i.e. wherever it is used as a source of information (assignment of the parameter to well-typed reference, invocation of the parameter with well-typed argument list, accessing properties on parameter, etc.)

[HerringtonDarkholme]

Thanks for the pull request!

But I feel the quality of parameter inference doesn't match other inference already in TS.

However, it's another problem whether implementing full inference is worthy

[masaeedu]

@HerringtonDarkholme Thanks for the feedback! This is not intended to be a mergeable implementation of parameter inference; the goal is to demonstrate that it is possible to sensibly infer parameter types recursively without breaking everything in the compiler, and that it's possible to do so incrementally, reverting to noImplicitAny when a sensible inference cannot be made.

This is useful because when converting a new project to TypeScript, you often end up either disabling noImplicitAny and getting any everywhere, or manually propagating types up the call graph by adding parameter types to every function expression. Some kind of middle-ground would be useful, especially if this allows you to automatically generate explicit types for top-level APIs more accurately.

I covered only invocation expressions here because these seemed easiest to implement for a POC (and mirror the example from #15114), but the same concept extends to assignment from parameter, operator usage, etc.

[mhegazy]

I have recently worked on something similar to this in #14786. I started off with inferring from function body, but putting it to usage, changed my mind. most functions do not just use the parameter as is, they usually look at one ore more properties from it.. the result of the inference from the function body is always more of a constraint than a type really.. you end up with things like { kind: number } instead of Node, or { length: number } instead of Array which correct.. but not what a human would have thought about.. it gets complicated as the type gets more complex, and then you have these anonymous types that really mean nothing to the developer..

The other problem is this disables checking on the parameter all together. true that an implicit any inference does that as well, but at least you have a way to flag these as errors in --noImplicitAny.

I think the right place for this work is quick fix/ refactoring to help the user migrate to TS. The bar is low here for accuracy, since the user is doing an explicit action and will be reviewing the result anyways. Feel free to look at #14786, and contribute changes.

[masaeedu]

@mhegazy The overall goal of this and #17428 is to have the type <T extends { length }>(a: T) => T["length"] ascribed to a => a.length. This type means nothing to the developer when they really meant to talk about Array#length, but this is fine: the inferred type is sufficiently polymorphic that applying it to [1, 2, 3] will correctly produce number, and incorrectly applying it will produce an error.

Inferring things in this way ensures the function demands no more specificity in its parameters, and promises no less specificity in its return type, than what its implementation dictates.

It is true that inferring from the body will defer discovery of any type errors in the function implementation to the point where the function is invoked, but this is a worthwhile tradeoff for the significant decrease in verbosity and the increased ease of converting existing codebases to TypeScript.

As an example, if you do inference in this manner, it becomes more feasible for the typechecker to interact directly with plain JS modules that don't have any explicit typings.

[mhegazy]

#17428 is a breaking change.. a big one mind you. inferring constraints from the body of functions makes it less so, but still not completely. I would like to see a full proposal for this if you have one.

[masaeedu]

@mhegazy I'm going to be on vacation in a bit, and will spend some time to see if can come up with a proposal/reference implementation. That way you'll have a concrete list of thousands of broken testcases to point to when you reject the proposal 😉

[mhegazy]

That way you'll have a concrete list of thousands of broken testcases to point to when you reject the proposal

This is not the intention of my earlier comment.. I think if we are going to consider a major change, we should have the full picture, rather than having it spread on multiple smaller issues. In general this has a better chance of a positive outcome.

It is also important to acknowledge the proposal in #17428 is a breaking change, and a viable proposal needs to consider how it can be added/phased in with limited disruption to the system. Assuming we can make a fundamental change of behavior like this is with no mitigation is not realistic.. we have experience with similar changes with limiting any type evolution to under --noImplicitAny, and we have experimented with doing out-of-band inference in #14786. So having a proposal with this in mind would be very helpful.

[masaeedu]

@mhegazy I totally agree with you regarding the necessity of a proper proposal, my last comment was in jest. Exactly how much of a breaking change it would be, and whether it is possible to come up with a compromise that doesn't break 90% of existing TypeScript code is something I'm interested in myself.