microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.31k stars 12.53k forks source link

Improvement/Bug in contextual inference where the call-site is generic. #45035

Open devanshj opened 3 years ago

devanshj commented 3 years ago

Feature request / Bug report

šŸ” Search Terms

Contextual inference, generic call-site

āœ… Viability Checklist

My suggestion meets these guidelines:

ā­ Suggestion / Problem

Let's first study contextual inference in different scenarios. Playground for code below. Highly recommended to view it in the playground so that you can see where are the red underlines, what is the inference and what are the completions.

type X<T = never> = { $: "foo" | T }
declare const $1: <Z>(x: Z) => { $: Z }
declare const $2: <Z>(x: NoInfer<Z>) => { $: Z }
type NoInfer<T> = [T][T extends any ? 0 : never]

let _: X;
_ = $1("")               // s1t1 - Z is inferred as "" and does not compile ........................ ok
_ = $2("")               // s1t2 - Z is inferred as "foo" and does not compile ..................... nice
_ = $1("foo")            // s1t3 - Z is inferred as "foo" and compiles ............................. ok
_ = $2("foo")            // s1t4 - Z is inferred as "foo" and compiles ............................. nice

declare const m1: (x: X) => void
m1($1(""))               // s2t1 - Z is inferred as "" and does not compile ........................ ok
m1($2(""))               // s2t2 - Z is inferred as "foo" and does not compile ..................... nice
m1($1("foo"))            // s2t3 - Z is inferred as "foo" and compiles ............................. ok
m1($2("foo"))            // s2t4 - Z is inferred as "foo" and compiles ............................. nice

declare const m2: <T>(x: InferStringLiteral<T>, _: X<NoInfer<T>>) => void
type InferStringLiteral<T> = T extends string ? T : string
m2("a", $1(""))          // s3t1 - Z is inferred as "" and does not compile ........................ ok
m2("a", $2(""))          // s3t2 - Z is inferred as "foo" | "a" and does not compile ............... nice
m2("a", $1("foo"))       // s3t3 - Z is inferred as "foo" and compiles ............................. ok
m2("a", $2("a"))         // s3t4 - Z is inferred as "foo" | "a" and compiles ....................... nice

declare const m3: (_: { x: X }) => void
m3({ x: $1("") })        // s4t1 - Z is inferred as "" and does not compile ........................ ok
m3({ x: $2("") })        // s4t2 - Z is inferred as "foo" and does not compile ..................... nice
m3({ x: $1("foo") })     // s4t3 - Z is inferred as "foo" and compiles ............................. ok
m3({ x: $2("foo") })     // s4t4 - Z is inferred as "foo" and compiles ............................. nice

declare const m4: <T extends { a?: number, x: X<keyof T> }>(_: T) => void
m4({ x: $1(""), a: 1 })  // s5t1 - Z is inferred as "" and does not compile ........................ ok 
m4({ x: $2(""), a: 1 })  // s5t2 - Z is inferred as string | number | symbol and does not compile... ugh
                         //        (quickinfo is incorrect see #44879)
m4({ x: $1("a"), a: 1 }) // s5t3 - Z is inferred as "a" and does not compile ....................... ok
m4({ x: $2("a"), a: 1 }) // s5t4 - Z is inferred as string | number | symbol and does not compile .. ughhh
                         //        (quickinfo is incorrect see #44879)

declare const m5: <T, U extends X<T>>(a: InferStringLiteral<T>, x: U) => void
m5("a", $1(""))          // s6t1 - Z is inferred as "" and does not compile ........................ ok 
m5("a", $2("foo"))       // s6t2 - Z is inferred as unknown and does not compile.................... ugh
                         //        (quickinfo is incorrect see #44879)
m5("a", $1("a"))         // s6t3 - Z is inferred as "a" and does not compile ....................... ok
m5("a", $2("a"))         // s6t4 - Z is inferred as unknown and does not compile ................... ughhh
                         //        (quickinfo is incorrect see #44879)
  1. The contextual inference is expected and consistent for s1, s2, s3 & s4. But weird yet consistent for s5 & s6.

  2. I think the following should be the expected behavior...

    s5t2 - Z is inferred as "foo" | "a" | "x" and does not compile
    s5t4 - Z is inferred as "foo" | "a" | "x" and compiles
    s6t2 - Z is inferred as "foo" | "a" and does not compile
    s6t4 - Z is inferred as "foo" | "a" and compiles
  3. In s5t4 & s6t4, "a" is one of the completions but they are incorrect because the language server infers the type parameter different from the compiler. This is most probably a bug which might be accommodated in #44879. So all in all the completions are useless and when the language server is fixed there would be no completions.

  4. Generally speaking, a sugar-like abstraction should not result in compromises in developer experience. If you inline $*("") as { $: "" } in s5 & s6 then the completions and the compiler would work as expected.

  5. My analysis is that when the type expected from calling $* is generic itself (ie s5 & s6) then compiler can't infer the type parameter Z correctly. It resolves all other generics to unknown hence the type parameter Z is inferred as keyof unknown and unknown in s5 and s6, respectively.
    Aside: Though I would expected other generics to resolve to their constraint meaning I'd have expected (like not ultimately but considering the weirdness itself) the type parameter of $2 in the following scenario as "a" | "b" instead of unknown. Playground.

    type X<T = never> = { $: T | "foo" }
    declare const m6: <T extends "a" | "b", U extends X<T>>(a: T, x: U) => void
    m6("a", $2("a")) // Z is inferred as unknown
    
    declare const $2: <Z>(x: NoInfer<Z>) => { $: Z }
    type NoInfer<T> = [T][T extends any ? 0 : never]
  6. #44999 is most probably the consequence of this weirdness as it fits the precondition and it's marked as a bug, so imo this should be also be considered as a bug. It's not apparent because the repros are vague but hopefully the following examples would make it more clear how essential it is to get this right.

  7. Maybe $1should work same as $2

šŸ“ƒ Motivating Example

Library authors provide sugar-like abstractions all the time. Take the following as an example. Playground.

createColors({
  base: { primary: "red" },
  derived: {
    primary400: lightenWithNoInfer("primary", 0.4), // does not compile
    primary500: lightenWithoutNoInfer("primary", 0.5), // does not compile
    primary600: lightenWithoutNoInfer("primary" as const, 0.6),
    primary700: lightenWithoutNoInferWithInferStringLiteral("primary", 0.7)
  }
})

declare const createColors: <C extends string>(theme:
  { base: { [colorIdentifier in C]: Color }
  , derived: 
      { [derivedColor in string]: 
          { base: NoInfer<C> // `NoInfer` so that `C` is inferred only from `base`
          , operation: (c: Color) => Color
          }
      }
  }) => "TODO"

declare const lightenWithNoInfer:
  <Z>(base: NoInfer<Z>, weight: number) => 
    { base: Z
    , operation: (c: Color) => Color
    }

declare const lightenWithoutNoInfer:
  <Z>(base: Z, weight: number) => 
    { base: Z
    , operation: (c: Color) => Color
    }

declare const lightenWithoutNoInferWithInferStringLiteral:
  <Z>(base: InferStringLiteral<Z>, weight: number) => 
    { base: Z
    , operation: (c: Color) => Color
    }

type Color = string;
type NoInfer<T> = [T][T extends any ? 0 : never]
type InferStringLiteral<T> = T extends string ? T : never

None of the lighten version is good.

  1. lightenWithNoInfer has completions but are incorrect and doesn't compile
  2. lightenWithoutNoInfer infers Z as string instead of "primary" hence doesn't compile
  3. lightenWithoutNoInferWithInferStringLiteral compiles but has no completions

If I were to be frank, there is no rocket science going on here, lightenWithoutNoInfer (or at least lightenWithNoInfer) should "just work" with completions and compilation.

And the problem isn't about string literals per se. Here's another real world example from xstate. Playground.

createMachine({
  schema: { event: createSchema<{ type: "FETCH" }>() },
  initial: "idle",
  states: {
    idle: {
      entry: send({ type: "FETCH" }), // does not compile
      on: {
        FETCH: "fetching"
      }
    },
    fetching: {}
  }
})

declare const createMachine: <State extends string, Event extends { type: string }>(m:
  { schema: { event: Event } // we want `Event` to be inferred only from here
  , initial: NoInfer<State>
  , states:
    { [S in State]: // we want `State` to be inferred only from here
        { entry?: 
          | { type: "xstate.send"
            , event: NoInfer<Event>
            }
      , on?:
          { [E in Event["type"]]?: NoInfer<State>
          }
      }
    }
  }) => void

declare const send:
  <E>(event: NoInfer<E>) => { type: "xstate.send", event: E }

declare const createSchema: <T>() => T
type NoInfer<T> = [T][T extends any ? 0 : never];

The problem exactly is same as above. And here you can even inline send({ type: "FETCH" }) to { type: "xstate.send", type: "FETCH" } and it compiles and even provides completions.

Aside: If we add { type: string } to the entry union send("") compiles but { type: "xstate.send", event: "" } doesn't, which is kinda weird too because both are equivalent.

šŸ’» Use Cases

Any case where the type parameters of a function are to be inferred from the return type instead of parameters AND location of the function call is a generic; is a use case. I suspect this improvement/bugfix will have a huge impact especially for library authors. Probably there are some folks out there banging theirs heads to make the completions work when they should probably "just work" without having to do anything.

Some "workarounds"

For s5

declare const $:
  < R
  , Z extends R extends { $: infer X } ? X : never>
    (x: Z) => R & { $: Z }

For lighten

declare const lighten:
  < R
  , C extends R extends { base: infer X } ? X : never>
    (base: C, weight: number) => 
      & R
      & { base: C
        , operation: (c: Color) => Color
        }

For send

declare const send:
  < R
  , E extends R extends { event: infer X } ? X : never>
    (event: E) =>
      & R
      & { type: "xstate.send", event: E }

All the above provide completions and compile too. Though complex cases like s4 don't work with this workaround.

Thanks for reading!

devanshj commented 3 years ago

Here's a slightly modified but super compact version of my "feature request" -

declare const m: <T extends { $: "a" | "b" }>(x: T) => T
declare const $: <Z>(x: Z) => { $: Z }

let x = m($(""))
// `Z` should be `"a" | "b"` (corollary - user should get "a" and "b" as completions)
// `typeof x` should be `{ $: "a" | "b" }`

let y = m($("a"))
// `Z` should be `"a"`
// `typeof y` should be `{ $: "a" }`

I missed an important nuance in the original proposal that Z should derive the constraint from the return type BUT should be derived from the parameter when it satisfies the constraint. This is not possible right now even with NoInfer so 0 out of the 24 cases match this desired behavior. The rational is that this exactly how it works if you inline $ -

declare const m: <T extends { $: "a" | "b" }>(x: T) => T

let x = m({ $: "" })
// `typeof $` is `"a" | "b"` and user gets "a" and "b" as completions
// `typeof x` is `{ $: "a" | "b" }`

let y = m({ $: "a" })
// `typeof $` is `"a"`
// `typeof y` is `{ $: "a" }`

All in all, I am, if I may, "just" :P asking the compiler to understand that $("") is an sugar for { $: "" } and it need not change the behavior in any way

RyanCavanaugh commented 3 years ago
let x = m($(""))
// `Z` should be `"a" | "b"` (corollary - user should get "a" and "b" as completions)
// `typeof x` should be `{ $: "a" | "b" }`

The only plausible implementations of $ and m (identity functions) mean that the value "" would inhabit a reference of type "a" | "b". Some justification is needed here

devanshj commented 3 years ago

First off - Let me make myself clear that this was only a "TLDR version" and a minimal "test case" to show the desired behavior. It's not a replacement for the full proposal in the original description (especially the real world examples, in fact they are more telling than anything else). So please do give it a read just in case you haven't :)

identity functions

Though it's not always the case, for example lighten in the above example isn't an "identity" function. Or to put in better terms it's not a sugar, the operation implementation is written library-land that comes with lighten.

the value "" would inhabit a reference of type "a" | "b"

I'm not sure what you mean by this? Are you saying the user would mostly never write literals and it would be $(x) where x would have already have a type "a" | "b"? Frankly I'm clueless what are you trying to ask šŸ˜…

fatcerberus commented 3 years ago

I think he's pointing out that since $() is necessarily an identity function per its typing (Z has no constraint, therefore could be anything, so there's very little you can do with it other than just pass it along as-is), it wouldn't make sense for the result of $("") to be used as input for something expecting "a" | "b".

devanshj commented 3 years ago

I don't think Ryan meant that. But to answer that...

First I'm assuming you meant to write "something expecting { $: "a" | "b" }" instead because that's what m is expecting (as it says T extends { $: "a" | "b" }).

so there's very little you can do with it other than just pass it along as-is

Right but only when there it's not used in-place.

let x = $("") // `Z` is inferred as `string` as expected

But when in cases where contextual inference can be applied typescript does infer Z, like here...

let x: { $: "a" | "b" } = $("") // Z is inferred as "a" | "b" as expected

Oof wait Z isn't inferred as "a" | "b" xD (was about to continue this answer but see my next comment)

RyanCavanaugh commented 3 years ago

it wouldn't make sense for the result of $("") to be used as input for something expecting "a" | "b".

That is indeed what I meant.

devanshj commented 3 years ago

That is indeed what I meant.

Yeah but it expects { $: "a" | "b" } right? T extends { $: "a" | "b" }. The only two possible values of the first parameter of $ are "a" and "b" so why not constraint it via inferring Z as "a" | "b" via contextual inference? What am I missing here?

I was about to edit my previous comment that let x: { $: "a" | "b" } = $("") is an even more minimal test case for my the desired behavior. (I was under impression that typescript infers Z to "a" | "b" but it doesn't. I already knew it via s1t1 just forgot because of confusing $2's behavior for $1)

Edit: Tbc of course after inferring Z as "a" | "b" there should be compile error and red underlines at "" (and now they are at x)

fatcerberus commented 3 years ago

Yeah AFAIK, TS infers type parameters only from the function call itself. The assignment target is not used for type inference except in specific cases, e.g. callback parameters.

devanshj commented 3 years ago

Continuing from here...

Though if I were to make a critic, I think the foundational problem is TypeScript (to put it naively) doesn't leverage or understand that some functions are essentially "sugars", it can be very easily identified if I write assign as (f: F) => { type: "xstate.assign", exec: F }. The fact that inlining assign compiles says that compiler doesn't really understand fully that assign is essentially just a sugar. I also lay this out in depth in #45035 (I would LOVE it you can share your thoughts on it, it's a little messy in the beginning because I couldn't point my finger on it but later in the comments it's clear).

It would be cool if TypeScript perhaps uses some heuristic to identity sugar-like functions, or if I were to propose a solution it would be that TypeScript provides a type that allows the user to mark an inference site to be of lesser priority so that we can type assign like \<F>(f: LowInfer\<F>) => { type: "xstate.assign", exec: F }. (It's important that it's LowInfer and not NoInfer I'll add a comment in #45035 to point out how it's different)

If I use NoInfer, it partially allows me to attain my desired behavior

declare const m: <T extends { $: "a" | "b" }>(x: T) => T
declare const $: <Z>(x: NoInfer<Z>) => { $: Z }
type NoInfer<T> = [T][T extends any ? 0 : never]

let x = m($(""))
// Desired behaviour:
// `Z` should be `"a" | "b"` (corollary - user should get "a" and "b" as completions)
// `typeof x` should be `{ $: "a" | "b" }`
// Actual behaviour:
// Same

let y = m($("a"))
// Desired behaviour
// `Z` should be `"a"`
// `typeof y` should be `{ $: "a" }`
// Actual behaviour
// `Z` in unknown (don't rely on quickinfo it's buggy #44879)
// `typeof y` is `{ $: "a" | "b" }`

I think a LowInfer instead of NoInfer would allow me to attain the desired behavior. It would be super cool if typescript is smart enough to identify sugar-like functions and can behave in the desired way out of the box but if not that then the LowInfer is the second ideal solution.

devanshj commented 3 years ago

@RyanCavanaugh I see you haven't labelled this I assume that's because you haven't got time to look into it and not because something is pending from my side, right? Just want to make sure I'm right in thinking that the ball is in your court šŸ˜…

RyanCavanaugh commented 3 years ago

Here's my current understanding of the issue, summarized in a minimal-yet-not-degenerately-minimal form:

type Box<T> = { value: T }
declare function box<T>(x: T): Box<T>;

declare function eatZeroBox(x: Box<0>): void;
// Passes
eatZeroBox(box(0));

declare function eatZeroBoxG<T extends Box<0>>(x: T): void;
// Passes
eatZeroBoxG(box(0 as const));
// Errors, should pass
eatZeroBoxG(box(0));

Is that right?

devanshj commented 3 years ago

Yep that's right. But it doesn't test or explain the desired behavior fully, this one does (I can elaborate how if you want). Your version is perhaps the most unsound-looking subset of that.

And also yeah I should have adopted a non-hipster box instead of $ :P

RyanCavanaugh commented 3 years ago

It's incomplete to error more often than you should, not unsound

devanshj commented 3 years ago

It's incomplete to error more often than you should, not unsound

Yeah correct excuse my lack of savviness when it comes to wording such things senpai, what I probably meant was "most horrible-looking" or perhaps "most unfortunate-looking" idk xD

devanshj commented 3 years ago

LowInfer would also improve types for popular libraries like zustand, a minimal version:

const create = <T>(f: (set: (t: Partial<T>) => void) => T): T => {
  let v = f(nV => v = { ...v, ...nV })
  return v;
}

let x = create(set => ({
  foo: 0,
  bar: () => set({
    foo: "lol" // compiles, shouldn't
  })
}))
x.foo // doesn't compile, should

Here T is inferred as unknown because the compiler gives priority to the T in Partial<T> instead of the T in the return type. With LowInfer we can write Partial<LowInfer<T>> and then T would be inferred as { foo: number, bar: () => void }

As of now, user's have to explicitly pass the type parameter which could have been inferred.

Andarist commented 1 year ago

The autocomplete issue mentioned here was fixed in 4.7 - despite the fact that the autocomplete argument didn't end up type-checking correctly: TS playground. This was likely fixed by this PR: https://github.com/microsoft/TypeScript/pull/48410

This started to typecheck OK in 4.8 though - and the same can be said about @RyanCavanaugh 's example from this comment and about the lightenWithoutNoInfer case from the main post. You can verify them in this TS playground. Those cases were fixed by this PR: https://github.com/microsoft/TypeScript/pull/49086

So overall, some cases from here were fixed - but some still aren't. It would be cool to distill the non-working cases to a minimal repro to get a better understanding of the difference between all of those cases and why some were fixed when some remain broken~.

An additional interesting fact is that when the inner call has an argument that doesn't satisfy the outer constraint then the inner type param isn't fixed with the information from the outer inference context. @devanshj was suggesting that it should still get fixed with the outer one (TS playground):

declare const m: <T extends { $: "a" | "b" }>(x: T) => T;
declare const $: <Z>(x: Z) => { $: Z };

// actual Z: ""
// expected Z: "a" | "b"
let x = m($("")); // errors correctly

I think that this is totally OK as it doesn't really matter what is inferred as Z here, as long as we end up with an error.

devanshj commented 1 year ago

I think that this is totally OK as it doesn't really matter what is inferred as Z here, as long as we end up with an error.

No it's important because what Z is inferred as will decide the contextual type for the argument of $, in some sense that's the whole point of the issue. I think you're viewing this as a seperative case when the fact is it's a distilled case of what should happen in all the above cases.

To give you an example let's take this case. Here the problem is the type parameter of send get's inferred as { type: string } (equivalent of "" from the distilled case) instead of { type: "FETCH" } (equivalent of "a" | "b" from the distilled case). And hence typescript doesn't have the correct contextual type to infer the "FETCH" value as "FETCH" type and not string type and hence the code doesn't compile. So the fact it infers "" instead of "a" | "b" changes everything.

As you said there are improvements in the compiler so let me revisit the distilled case and tick off what's done...

declare const m: <T extends { $: "a" | "b" }>(x: T) => T
declare const $: <Z>(x: Z) => { $: Z }

let x = m($(""))
// 1. `Z` should be `"a" | "b"` (corollary - user should get "a" and "b" as completions)
// 2. `typeof x` should be `{ $: "a" | "b" }`

let y = m($("a"))
// 3. `Z` should be `"a"`
// 4. `typeof y` should be `{ $: "a" }`

When the issue was written ie in 4.4.0-beta only (2) was fulfilled (the quickinfo might deceive that (3) is also fulfilled but it's not). Now with 4.9.4 along with (2), (3) & (4) also have been fulfilled and (1) remains unfulfilled (the corollary of (1) is now fulfilled but that's an irrelevant side-effect as it's only fulfilled for this case and not in general, eg there's no completion for "FETCH" in the above send case). So some cases got fixed because (3) & (4) got fulfilled and the other ones remained unfixed because (1) remained unfulfilled.

So we already have the distilled minimal repro you're talking about since day one ;)

Andarist commented 1 year ago

No it's important because what Z is inferred as will decide the contextual type for the argument of $

Right, this makes sense - although that only (?) prevents some extra errors to be reported. For example the error at acceptStr(arg) here:

declare const m: <T extends { $: "a" | "b" }>(x: T) => T;
declare const $: <Z>(x: Z, cb: (arg: Z) => void) => { $: Z };

declare function acceptStr(a: string): void

let x = m(
  $(100, (arg) => {
    acceptStr(arg);
  })
);

So the improvement here would be purely related to DX etc - or I'm missing what you are saying.

But either way - we are talking here about the case that shouldn't typecheck. It would still be interesting to get a minimal repro for cases that should typecheck but which still don't.

devanshj commented 1 year ago

No you're entirely missing what I'm saying xD. Okay let me make myself clear by answering this question of yours...

It would still be interesting to get a minimal repro for cases that should typecheck but which still don't.

Okay so let's start with a non-minimal repro of the case that should type check but doesn't, here (this is the same send case from above). Now let's make it minimal...

declare const eatEvent: <T extends { type: string }, U extends T>(t: T, u: U) => U
declare const createEvent: <E>(e: E) => E

eatEvent({ type: "FETCH" as const }, { type: "FETCH" }) // compiles, ok
eatEvent({ type: "FETCH" as const }, createEvent({ type: "FETCH" })) // doesn't compile, should

The second call doesn't typecheck because E gets inferred as { type: string } instead of { type: "FETCH" }. This behavior is same as Z getting inferred as "" and not "a" | "b". That is if you make Z infer as "a" | "b" it would automatically make E infer as { type: "FETCH" }, because the implementation responsible for both behaviors is same afaict. Hence I said the Z case is already a minimal distilled repro for the all cases not just this send one.

Now sure the Z repro is not great because it doesn't indicate in itself what is wrong and hence it would also rely on the result of .types file of the test result instead of .errors.txt but that's kinda besides the point. (Edit: perhaps not even .types as it doesn't print the inferences made for type parameters.)

I hope I made myself clearer.