Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infer type parameters from indexes on those parameters #20126

Closed

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Nov 18, 2017

This alters the inference info to accumulate a set of partial inferences created by inferences to indexes on a type parameter. These partial inferences are then combined into one real inference when candidates are created. The partial inferences for S to T[K] take the form of a mapped type of {[Key in K]: S}. I'm unsure if the priority of such an inference needs modification in some way. Creating such a type without any backing parse tree nodes required some slight modification to how a handful of attributes of a mapped type were looked up (namely, adding a cache for anything normally looked up on the declaration).

@sandersn and @DanielRosenwasser and I were talking the other day about how inference like this allows you to replace a large list of type parameters with an unordered bag of type parameters, like so:

interface Args {
    TA: object,
    TY: object
}

function foo<T extends Args>(
    a: T["TA"],
    b: T["TY"]): T["TA"] & T["TY"] {
    return undefined!;
}

const x = foo({
    x: {
        j: 12,
        i: 11
    }
}, { y: 42 });

without this change, x is instantiated as the intersections of the base constraints on T["TA"] and T["TY"] and so is object; but with this change we infer {x: {j: number, i: number}} for T["TA"] and {y: number} for T["TY"], creating an aggregate inference for T of {TA: {x: {j: number, i: number}}, TY: {y: number}}, making the return type {x: {j: number, i: number}} & {y: number}.

This could be particularly useful in Vue, as then they could introduce a bag for the type parameters passed into their many generic functions. This would enable them to add more type arguments as needed for improved inference or for future features, without breaking existing consumers who are manually specifying a subset of parameters (simply make new arguments in the bag optional) and also allowing that bag to be extended (via interface reopening) by any vue plugins, such as vuex (which then enables them to add overloads to vue-core methods which can carry through the new type information they provide).

I'm sure there's a bunch of refinements to be made, but does anyone have thoughts on adding inference of this kind? It doesn't seem to obviously interact with or overwrite any other inferences we do (certainly no common ones).

@@ -11252,6 +11251,27 @@ namespace ts {
}
return;
}
else if (target.flags & TypeFlags.IndexedAccess) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The odd location of this branch within inferTypes is to work around #20124 - if I placed it below the other IndexedAccess case (as I'd prefer to do), it would never be executed because of that bug.

map.templateType = source;
map.constraintType = (<IndexedAccessType>target).indexType;
map.typeParameter = <TypeParameter>createType(TypeFlags.TypeParameter);
// TODO (weswigham): Ensure the name chosen for the unused "K" does not shadow any other type variables in the given scope, so as to not have a chance of breaking declaration emit
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the correct way to do this is to set a flag on the symbol which tells the node builder to use an autogenerated identifier for the name, to just ensure there's no conflicts with anything in the surrounding scopes.

@weswigham
Copy link
Member Author

weswigham commented Nov 22, 2017

I have greatly simplified the creation of the partial inference types by introducing a type alias that we lookup and instantiate into the lib file. This also causes us to share instantiations between all partial inferences globally, which should be good for perf, and also allows it to have an alias symbol to name the somewhat complicated type by in declaration emit, should it remain generic.

Additionally, I have altered the shape of the partial inference type to be much more precise (in exchange it's a bit more complicated). Previously it was, for some S to T[K], {[Key in K]: S}. Now it is {[SomeK in K]: {[OneK in SomeK]: S}}[K]. This means that if K is instantiated as "A" | "B" we now produce {"A": S} | {"B": S} instead of {"A": S, "B": S}, which is much more correct (we only know we have to fulfill one of those members with our inference, not all of them).

Lastly, I've added way more test coverage. They show that this approach can give some excellent results, even under complex higher-order conditions, while also showing that the basic T["A" | "B"] could use some work... (though that might be a cascading failure from the failing in higher order indexed access inference that there's another PR our for; in any case it begets looking-into) Naturally even more tests are warranted (esp. in the case of multiple inference sites for one member - this is mostly a note for myself), but preferably in the direction that they are likely to be used.

@DanielRosenwasser as this currently is, can you see a way for the vue ecosystem to adopt a pattern involving inferences like this; do you know what such type definitions would look like, approximately (so they can be tested)?

@mhegazy
Copy link
Contributor

mhegazy commented Dec 1, 2017

this does not seem to work for me;

type Options = { k: "a", a: number } | { k: "b", b: string };
declare function f<T extends Options>(p: T["k"]): T;
const x = f("a");  // expect it to be `{ k: "a" }` but it is Options

@weswigham
Copy link
Member Author

weswigham commented Dec 1, 2017

@mhegazy In your example, x is inferred as Options because it can't infer just {k: "a"} because that doesn't satisfy the constraint on T. We actually do create that inference, we just have to toss it out because it alone isn't enough to satisfy the constraint. I could add constraint discrimination based on available partial inferences into getInferredType, if it interests you.

@weswigham
Copy link
Member Author

weswigham commented Dec 1, 2017

@mhegazy Done - a partial inference result which is not assignable to the constraint now discriminates the constraint based on the fields available in the partial inference, if possible, then creates a spread with the selected constraint and the partial inference. Pretty nifty; good idea.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 8, 2018

@weswigham can you get @ahejlsberg to take a look at this. i do think this is a good addition to make.

@typescript-bot
Copy link
Collaborator

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

@jcalz
Copy link
Contributor

jcalz commented Apr 11, 2022

I wonder why this never made it into the language, or if there was a reported issue it was meant to solve. I occasionally see people trying this sort of inference and I don't know where to point them for a canonical answer as to why it doesn't work this way.

@weswigham
Copy link
Member Author

Uhh... No real reason other than lack of a driver, I guess? As you can see, there's no backing issue, so it never really got prioritized for discussion.

@jcalz
Copy link
Contributor

jcalz commented Aug 28, 2022

@jcalz
Copy link
Contributor

jcalz commented Sep 21, 2022

Looks like this SO question is also running afoul of this, maybe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants