Infer unknown at value positions of objects inferred from keyof T
#55547
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
|
@DanielRosenwasser I think this is a pretty good break - should we just pull it in now, or should we wait until a different part of the release cycle? |
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at fa43c1c. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite on this PR at fa43c1c. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the regular perf test suite on this PR at fa43c1c. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite on this PR at fa43c1c. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at fa43c1c. You can monitor the build here. |
|
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@DanielRosenwasser Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@DanielRosenwasser Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @DanielRosenwasser, it looks like the DT test run failed. Please check the log for more details. |
|
@DanielRosenwasser Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
A minimal repro case of the above problem: TS playground. It looks fixable on the Vue's side. I think that it actually somewhat proves how this is an improvement because the return type of this function utilizes the inferred type of |
|
@DanielRosenwasser did we discuss this at the design meeting yet? I don't see a link to any particular meeting. |
|
Don't think we did - we probably should. |
|
@weswigham would you be able to talk about it at the Friday meeting? |
|
I guess the only thing I might say is that we could change this behavior based on |
|
Honestly, I kinda just wanna ship it - any breaks that this causes are almost certainly good breaks where you've done something very unsafe, likely unintentionally, since inference is oft inscrutable, and it's not always obvious your dominating inference source is a |
|
That's fine with me if we have a plan to fix Vue. |
I will prioritize investigating this. |
|
I dug into this case deeper and I think there are two things that could be done about this and both are quite independent from this. They could become prerequisites for this PR. One thing about the example above is that a pretty low priority inference wins over something better that could be inferred from the generic signature. I’ve opened this issue about it: #56931 and an experimental fix for it: #56939 . The extended community test suite didn’t fail there for Vue so likely the combination of that PR and this PR here wouldnt either. The second issue is that by inferring an object with properties typed as Now… is that a problem? Inferring anything here is quite unsound unless there are some other things at play (like a return context or smth). Going further… if a union of keys is passed not within an array or something… we can’t even say that those properties are required. If they are passed within an array we similarly can’t guarantee that this array contain all of the keys that we can see in the type system. So really we can only say that all properties are optional. The question is - what are the use cases for this inference? Why it was introduced? Are those use cases covered today by more sound alternatives? OTOH, similar unsound situations can always be achieved when supplying type arguments manually (and likely in other situations too). So perhaps the mental model is that it’s not the type system’s responsibility to ensure soundness here but rather the function/signature’s author. If the type system is meant to allow this regardless of constraints then declare function test1<T>(keys: keyof T): T;
const result1 = test1("" as "a" | "b"); // { a: any; b: any; }
declare function test2<T extends Record<string, never>>(keys: keyof T): T;
const result2 = test2("" as "a" | "b"); // Record<string, never>This case is far-fetched but conceptually the point stands - the better fit would be the constraint of each potential property. As @weswigham noted here, it’s not enough to call I'm not sure if that would actually happen though (I will try after we establish here how this whole thing should work). If not then I'd say that this could utilize the reverse typed machinery. Instead of creating an anonymous object here, we could create a reverse mapped type (with a fake source object) and depend on the fix from #56300 to get correct (and individual) constraints for each property. |
@Andarist I tested your minimal repro with #56939 (here is the playground): declare function defineComponent<Props extends Record<string, any>>(
setup: (props: Props) => void,
options?: {
props?: (keyof Props)[];
},
): (props: Props) => any;
defineComponent(
<T extends string>(_props: { msg: T }) => {
return () => {};
},
{
props: ["msg"],
},
);The inferred type of the call to function defineComponent<{
msg: T;
}>(setup: (props: {
msg: T;
}) => void, options?: {
props?: "msg"[] | undefined;
} | undefined): <T extends string>(props: {
msg: T;
}) => anyTherefore the |
|
@Andarist defineComponent(
<T extends string>(_props: { msg: T, test: T }) => {
return () => {};
},
{
props: ["msg"],
},
);If you see their documentation: it seems that they want to force you to manually declare the props inside the array before allowing you to use them as arguments of the Therefore it appears that their desired behavior is for the |
|
@Andarist Thanks to #55811 , if #56939 lands, they could use a definition similar to the following one to error out when someone destructures a property not declared in the declare function defineComponent<
Props extends Record<string, any>,
const RuntimeProps extends readonly string[]
>(
setup: (props: { [K in keyof Props & RuntimeProps[number]]: Props[K] }) => void,
options?: {
props?: RuntimeProps;
},
): (props: Props) => any; |
|
Yeah, I understand that - in general - this other PR would change the behavior for Vue today. It's great to know how it would affect them negatively though, that I didn't know, and thanks for that! Though, their whole signature is somewhat dubious and I don't find it to be a compelling use case that would prove that this other PR is worse than what we have today. Their docs mention:
But if we actually, try to utilize this then I'm not quite sure about what kind of generics support we are talking about: import { defineComponent, ref } from "vue";
import { h } from "@vue/runtime-dom";
const Comp = defineComponent(
<T extends string | number>(props: { msg: T; list: T[] }) => {
const count = ref(0);
return () => {
return h("div", count.value);
};
},
{
props: ["msg", "list"],
},
);
Comp;
// ^? const Comp: (props: { msg: any; list: any; } & {}) => anyMaybe I don't understand how their compiler uses this but TS-wise this leaves me quite confused since this "generics support" leaves the output totally unsafe. I also think that their API choice for this leaves some nasty runtime holes here and that they could do better. If they need to declare required keys~ then they really should use an object for that and not an array. An array typed as If they would use an object to declare those props then it would work with my change and it would improve the runtime safety. The location of the error would change but I don't think that's a big deal (TS playground): declare function defineComponent<Props extends Record<string, any>>(
setup: (props: Props) => void,
options?: {
props?: Record<keyof Props, unknown>
},
): (props: Props) => any;
const result = defineComponent(
<T extends string>(_props: { msg: T, list: T[] }) => {
return () => {};
},
{
props: { // error reported here when not all props are declared here
msg: true,
// list: true // uncomment to see the inferred generic signature
},
},
);
Nice ❤️ |
@Andarist historically, declaring just an array with props' names has always been the most concise way to declare props in Vue components, therefore I don't think they would be willing to drop it. The object syntax is supported as well, of course, as you can see here.
Me neither, paradoxically they would have better support thanks to #56939 because the returned function would be generic and bounded. |
I noticed this when investigating a completion problem. Using
anyhere might "accidentally leak" (which happens in the changed baselines) without the user noticing. I think it would be great to change this tounknownunless there is a strong reason why it should stay this way.Note that this function was introduced by @sandersn in #19227 (and
anywas used in its followup: #21271 ). This was in pre(historic)-3.0 times whenunknownwasn't even a thing in the language yet 😉 So back thenunknownwasn't even an option.