-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Improvement/Bug in contextual inference where the call-site is generic. #45035
Comments
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 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 |
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 |
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 :)
Though it's not always the case, for example
I'm not sure what you mean by this? Are you saying the user would mostly never write literals and it would be |
I think he's pointing out that since |
I don't think Ryan meant that. But to answer that... First I'm assuming you meant to write "something expecting
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 let x: { $: "a" | "b" } = $("") // Z is inferred as "a" | "b" as expected Oof wait Z isn't inferred as |
That is indeed what I meant. |
Yeah but it expects I was about to edit my previous comment that Edit: Tbc of course after inferring |
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. |
Continuing from here...
If I use 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 |
@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 π |
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? |
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 |
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 |
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 As of now, user's have to explicitly pass the type parameter which could have been inferred. |
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: #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 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 |
No it's important because what To give you an example let's take this case. Here the problem is the type parameter of 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 So we already have the distilled minimal repro you're talking about since day one ;) |
Right, this makes sense - although that only (?) prevents some extra errors to be reported. For example the error at 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. |
No you're entirely missing what I'm saying xD. Okay let me make myself clear by answering this question of yours...
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 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 Now sure the I hope I made myself clearer. |
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.
The contextual inference is expected and consistent for s1, s2, s3 & s4. But weird yet consistent for s5 & s6.
I think the following should be the expected behavior...
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.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.My analysis is that when the type expected from calling
$*
is generic itself (ie s5 & s6) then compiler can't infer the type parameterZ
correctly. It resolves all other generics tounknown
hence the type parameterZ
is inferred askeyof unknown
andunknown
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 ofunknown
. Playground.#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.
Maybe
$1
should work same as$2
π Motivating Example
Library authors provide sugar-like abstractions all the time. Take the following as an example. Playground.
None of the
lighten
version is good.lightenWithNoInfer
has completions but are incorrect and doesn't compilelightenWithoutNoInfer
infersZ
asstring
instead of"primary"
hence doesn't compilelightenWithoutNoInferWithInferStringLiteral
compiles but has no completionsIf I were to be frank, there is no rocket science going on here,
lightenWithoutNoInfer
(or at leastlightenWithNoInfer
) 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.
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 theentry
unionsend("")
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
For
lighten
For
send
All the above provide completions and compile too. Though complex cases like s4 don't work with this workaround.
Thanks for reading!
The text was updated successfully, but these errors were encountered: