Skip to content

Prefer higher-priority inferences from generic signatures over low-priority inferences from the first pass #56939

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Prefer higher-priority inferences from generic signatures over low-priority inferences from the first pass #56939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31d0e8b
Prefer higher-priority inferences from generic signatures over low-pr…
Andarist Jan 3, 2024
b89cb87
clone inference info
Andarist Jan 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38973,7 +38973,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// potentially add inferred type parameters to the outer function return type.
const returnType = context.signature && getReturnTypeOfSignature(context.signature);
const returnSignature = returnType && getSingleCallOrConstructSignature(returnType);
if (returnSignature && !returnSignature.typeParameters && !every(context.inferences, hasInferenceCandidates)) {
if (returnSignature && !returnSignature.typeParameters && !every(context.inferences, info => hasInferenceCandidates(info) && info.priority === InferencePriority.None)) {
// Instantiate the signature with its own type parameters as type arguments, possibly
// renaming the type parameters to ensure they have unique names.
const uniqueTypeParameters = getUniqueTypeParameters(context, signature.typeParameters);
Expand All @@ -38993,8 +38993,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// If the type parameters for which we produced candidates do not have any inferences yet,
// we adopt the new inference candidates and add the type parameters of the expression type
// to the set of inferred type parameters for the outer function return type.
if (!hasOverlappingInferences(context.inferences, inferences)) {
mergeInferences(context.inferences, inferences);
const merged = tryInferencesMerge(context.inferences, inferences);
if (merged) {
context.inferredTypeParameters = concatenate(context.inferredTypeParameters, uniqueTypeParameters);
return getOrCreateTypeFromSignature(instantiatedSignature);
}
Expand Down Expand Up @@ -39034,12 +39034,20 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return false;
}

function mergeInferences(target: InferenceInfo[], source: InferenceInfo[]) {
function tryInferencesMerge(target: InferenceInfo[], source: InferenceInfo[]) {
let merged = false;

const areOverlapping = hasOverlappingInferences(target, source);

for (let i = 0; i < target.length; i++) {
if (!hasInferenceCandidates(target[i]) && hasInferenceCandidates(source[i])) {
target[i] = source[i];
if (areOverlapping ? hasInferenceCandidates(source[i]) && hasInferenceCandidates(target[i]) && source[i].priority! < (target[i].priority! & ~InferencePriority.ReturnType) : hasInferenceCandidates(source[i])) {
const clonedInferenceInfo = cloneInferenceInfo(source[i]);
clonedInferenceInfo.priority = InferencePriority.None;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO for myself:

this line might not be needed at all or the assigned priority should be the source priority without InferencePriority.ReturnType (the latter sounds like a good idea if those inferences should stay "upgradable"/mergeable). IIUC, no new inferences without InferencePriority.ReturnType can be made here anyway - all of those were already collected before the ones from generic signatures start to be included.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, yeah, None definitely doesn't feel correct, since None is actually the highest priority inference possible - and I don't think simply adopting source's inference for target when they overlap should guarantee this is the best inference possible across all potential inference sites.

But also - this is mutating the inference object, which you should avoid, since it means if that inference is reused in multiple contexts, you'll be mangling the priority in all of them (...we probably shouldn't be reusing an inference in multiple contexts, but there are probably some edge cases we decided it was OK in that this might break the invariants of). If we can't reuse the source inference wholesale (because its priority already doesn't contain ReturnType), then we should duplicate the source inference object with a new priority.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially picked None here because it's the most conservative thing to do. If I'm reasoning about it correctly. Right now (on main) we iterate over those generic signatures (conceptually at least):

  • if the inferences are overlapping then we don't try to use any of the new ones
  • if the inferences are not overlapping then we use the new ones but that means that when we overlap again we won't pick any new ones

So my conservative change adjusts this only slightly - it only allows a single "upgrade" when inferences are overlapping (and in such a case, we only update the overlapping one, this is important - we can't assign inferences that are not overlapping).

I thought that this, both, is just the smallest change that could be done here if this direction is promising at all and that there might be an argument to be made here that in this scenario the inferences made from earlier generic signatures should be preferred (left ones having the higher priority over the right ones). The latter doesn't match how "regular" inferences are gathered but it is how it implicitly works on main (when overlap happens the new inferences are ignored after all, so they can't be replaced/updated).

I could certainly experiment with relaxing this behavior and allow those upgrades continuously.

I pushed out a change using cloneInferenceInfo but I'm not sure if you meant that or cloneInferenceContext. And perhaps it should also be applied conditionally? but then that would imply that cloneInferenceInfo is the right choice here since ReturnType is relevant to the info and not to the whole context.

If we can't reuse the source inference wholesale (because its priority already doesn't contain ReturnType)

Does it mean that when the inference info of the source contains ReturnType then it's OK to reuse it and avoid cloning?

Copy link
Member

Choose a reason for hiding this comment

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

Does it mean that when the inference info of the source contains ReturnType then it's OK to reuse it and avoid cloning?

If you're not going to mutate it, you can reuse it. Probably.

I pushed out a change using cloneInferenceInfo

Yeah, that's the one.

I could certainly experiment with relaxing this behavior and allow those upgrades continuously.

Yeah, I just don't think an inference site like this is the end-all inference source within a signature context (eg, a direct inference w/o return type inference or any other priority modifiers) - there may be a better one elsewhere, and for that one to be preferred, this priority needs to not be None.

This is spitballing, but it may be better to think of this, rather than as "merging" the inference infos, instead as using information from the contextual signature inference process to make new inferences in the original context - and that inference should have an appropriate priority that might not be the highest available. Honestly, in that regard, all this cloning may be a bit misleading - if we're allowing overlapping inference results in the two lists of inferences, mergeInferences is only used here, and it may not be the clearest approach. It may be better to instead create a new inference context to do these return type/contextual parameter inferences within, and then do a inferTypes(context.inferences, getInferredType(newContext, i), context.inferences[i].typeParameter, newContext.inferences[i].priority & ~InferencePriority.ReturnType) for each type parameter at index i, which should better handle merging inferences of differing priorities together, without needing bespoke or duplicated priority merging logic. I dunno though, there may be more complexity with this approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to chew on this for a bit. As an additional data point - I think this issue is quite related to this code and perhaps could help guiding us to the best solution: #57072 (comment) .

One extra problem with allowing further inferences here is that context.inferredTypeParameters would get polluted more and more with the type parameters that might not get used or something. I still don't quite understand all of the implications of this algorithm though so maybe I'm missing something. I already have seen this happening today when experimenting with some stuff:

declare function compose<A, B, C>(f: (a: A) => B, g: (b: B) => C): (a: A) => C

declare function f<A, B>(a: A): B
declare function g<B, C>(a: B): C

const result = compose(f, g)
//        ^? const result: <A, B>(a: A) => unknown

Since those inferredTypeParameters are simply concatenated there is no good way of even rejecting the unused ones~ at some point.

Copy link
Member

Choose a reason for hiding this comment

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

Since those inferredTypeParameters are simply concatenated there is no good way of even rejecting the unused ones~ at some point.

Yeah, at some point a filtering of that list to remove type parameters that definitely aren't used and logic for figuring out if the type parameter was actually used is probably needed for usability. It'll be a bit of a challenge, though, with all the deferred inferences we can make "maybe it's used later 🤷‍♂️" is probably a somewhat common case.

target[i] = clonedInferenceInfo;
merged = true;
}
}
return merged;
}

function getUniqueTypeParameters(context: InferenceContext, typeParameters: readonly TypeParameter[]): readonly TypeParameter[] {
Expand Down
60 changes: 60 additions & 0 deletions tests/baselines/reference/genericFunctionInference3.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
//// [tests/cases/compiler/genericFunctionInference3.ts] ////

=== genericFunctionInference3.ts ===
// https://github.com/microsoft/TypeScript/issues/56931

declare function defineComponent<Props extends Record<string, any>>(
>defineComponent : Symbol(defineComponent, Decl(genericFunctionInference3.ts, 0, 0))
>Props : Symbol(Props, Decl(genericFunctionInference3.ts, 2, 33))
>Record : Symbol(Record, Decl(lib.es5.d.ts, --, --))

setup: (props: Props) => void,
>setup : Symbol(setup, Decl(genericFunctionInference3.ts, 2, 68))
>props : Symbol(props, Decl(genericFunctionInference3.ts, 3, 10))
>Props : Symbol(Props, Decl(genericFunctionInference3.ts, 2, 33))

options?: {
>options : Symbol(options, Decl(genericFunctionInference3.ts, 3, 32))

props?: (keyof Props)[];
>props : Symbol(props, Decl(genericFunctionInference3.ts, 4, 13))
>Props : Symbol(Props, Decl(genericFunctionInference3.ts, 2, 33))

},
): (props: Props) => unknown;
>props : Symbol(props, Decl(genericFunctionInference3.ts, 7, 4))
>Props : Symbol(Props, Decl(genericFunctionInference3.ts, 2, 33))

const res1 = defineComponent(
>res1 : Symbol(res1, Decl(genericFunctionInference3.ts, 9, 5))
>defineComponent : Symbol(defineComponent, Decl(genericFunctionInference3.ts, 0, 0))

<T extends string>(_props: { msg: T }) => {
>T : Symbol(T, Decl(genericFunctionInference3.ts, 10, 3))
>_props : Symbol(_props, Decl(genericFunctionInference3.ts, 10, 21))
>msg : Symbol(msg, Decl(genericFunctionInference3.ts, 10, 30))
>T : Symbol(T, Decl(genericFunctionInference3.ts, 10, 3))

return () => {};
}
);

const res2 = defineComponent(
>res2 : Symbol(res2, Decl(genericFunctionInference3.ts, 15, 5))
>defineComponent : Symbol(defineComponent, Decl(genericFunctionInference3.ts, 0, 0))

<T extends string>(_props: { msg: T }) => {
>T : Symbol(T, Decl(genericFunctionInference3.ts, 16, 3))
>_props : Symbol(_props, Decl(genericFunctionInference3.ts, 16, 21))
>msg : Symbol(msg, Decl(genericFunctionInference3.ts, 16, 30))
>T : Symbol(T, Decl(genericFunctionInference3.ts, 16, 3))

return () => {};
},
{
props: ["msg"],
>props : Symbol(props, Decl(genericFunctionInference3.ts, 19, 3))

},
);

62 changes: 62 additions & 0 deletions tests/baselines/reference/genericFunctionInference3.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
//// [tests/cases/compiler/genericFunctionInference3.ts] ////

=== genericFunctionInference3.ts ===
// https://github.com/microsoft/TypeScript/issues/56931

declare function defineComponent<Props extends Record<string, any>>(
>defineComponent : <Props extends Record<string, any>>(setup: (props: Props) => void, options?: { props?: (keyof Props)[];}) => (props: Props) => unknown

setup: (props: Props) => void,
>setup : (props: Props) => void
>props : Props

options?: {
>options : { props?: (keyof Props)[] | undefined; } | undefined

props?: (keyof Props)[];
>props : (keyof Props)[] | undefined

},
): (props: Props) => unknown;
>props : Props

const res1 = defineComponent(
>res1 : <T extends string>(props: { msg: T; }) => unknown
>defineComponent( <T extends string>(_props: { msg: T }) => { return () => {}; }) : <T extends string>(props: { msg: T; }) => unknown
>defineComponent : <Props extends Record<string, any>>(setup: (props: Props) => void, options?: { props?: (keyof Props)[] | undefined; } | undefined) => (props: Props) => unknown

<T extends string>(_props: { msg: T }) => {
><T extends string>(_props: { msg: T }) => { return () => {}; } : <T extends string>(_props: { msg: T;}) => () => void
>_props : { msg: T; }
>msg : T

return () => {};
>() => {} : () => void
}
);

const res2 = defineComponent(
>res2 : <T extends string>(props: { msg: T; }) => unknown
>defineComponent( <T extends string>(_props: { msg: T }) => { return () => {}; }, { props: ["msg"], },) : <T extends string>(props: { msg: T; }) => unknown
>defineComponent : <Props extends Record<string, any>>(setup: (props: Props) => void, options?: { props?: (keyof Props)[] | undefined; } | undefined) => (props: Props) => unknown

<T extends string>(_props: { msg: T }) => {
><T extends string>(_props: { msg: T }) => { return () => {}; } : <T extends string>(_props: { msg: T;}) => () => void
>_props : { msg: T; }
>msg : T

return () => {};
>() => {} : () => void

},
{
>{ props: ["msg"], } : { props: "msg"[]; }

props: ["msg"],
>props : "msg"[]
>["msg"] : "msg"[]
>"msg" : "msg"

},
);

26 changes: 26 additions & 0 deletions tests/cases/compiler/genericFunctionInference3.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// @strict: true
// @noEmit: true

// https://github.com/microsoft/TypeScript/issues/56931

declare function defineComponent<Props extends Record<string, any>>(
setup: (props: Props) => void,
options?: {
props?: (keyof Props)[];
},
): (props: Props) => unknown;

const res1 = defineComponent(
<T extends string>(_props: { msg: T }) => {
return () => {};
}
);

const res2 = defineComponent(
<T extends string>(_props: { msg: T }) => {
return () => {};
},
{
props: ["msg"],
},
);