Skip to content

Tweak logic that chooses co- vs. contra-variant inferences when covariant inference is an empty object type #59772

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

Closed
wants to merge 3 commits into from
Closed

Tweak logic that chooses co- vs. contra-variant inferences when covariant inference is an empty object type #59772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2cb9b6
Tweak logic that chooses co- vs. contra-variant inferences when covar…
Andarist Aug 27, 2024
993dfb4
more tests
Andarist Aug 27, 2024
c149634
add extra test case
Andarist Aug 28, 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
2 changes: 1 addition & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26989,7 +26989,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// Similarly ignore co-variant `any` inference when both are available as almost everything is assignable to it
// and it would spoil the overall inference.
const preferCovariantType = inferredCovariantType && (!inferredContravariantType ||
!(inferredCovariantType.flags & (TypeFlags.Never | TypeFlags.Any)) &&
!(inferredCovariantType.flags & (TypeFlags.Never | TypeFlags.Any) || isEmptyAnonymousObjectType(inferredCovariantType) && isWeakType(inferredContravariantType)) &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a conservative change that aims to address #59765 .

I could imagine this might need some further tweaking to produce good error locations when both of the inferred types are weak or something. However, so far so good. This already produces good results and good error locations as far as I can tell - perhaps part of that can be attributed to the further checks like some(inference.contraCandidates, t => isTypeAssignableTo(inferredCovariantType, t)).

Copy link
Contributor Author

@Andarist Andarist Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, there is an open question here as to what the inferred type here should be (what should be the result's type):

declare function call<P>(tag: (props: P) => void, attributes: P): P;

declare function consume(p: { a?: string; b?: number; c?: boolean }): void;
declare const obj: { a?: string };

const result = call(consume, obj);

There is no clear answer to this - both satisfy the constraint and both of the arguments. I think that, in general, a covariant inference is safer in a situation like this. It's less likely to introduce problems like this one:

declare function call<P>(tag: (props: P) => void, attributes: P): P;

declare function consume(p: { a?: string; b?: number; c?: boolean }): void;
const obj: { a?: string; } = { a: 'foo', b: 'bar' }

const result = call(consume, obj);
obj.b // it's `number` in 5.5 but at runtime it could easily be a `string`

Nowadays the user can use NoInfer to guide what should be used as the inference source if they don't get the desired inferences for their use case - it's all a little bit situational after all.

And from this perspective... this whole PR could just be closed as the current behavior (5.6-rc) isn't necessarily wrong. Duh, it could even be seen as desirable. All in all, this requires a ruling on what's the better default and all from the team.

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 also experimented with an alternative patch:

diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index 954e45cdeb..f5e3d12778 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -26989,11 +26989,12 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
                     // Similarly ignore co-variant `any` inference when both are available as almost everything is assignable to it
                     // and it would spoil the overall inference.
                     const preferCovariantType = inferredCovariantType && (!inferredContravariantType ||
-                        !(inferredCovariantType.flags & (TypeFlags.Never | TypeFlags.Any) || isEmptyAnonymousObjectType(inferredCovariantType) && isWeakType(inferredContravariantType)) &&
+                        !(inferredCovariantType.flags & (TypeFlags.Never | TypeFlags.Any)) &&
                             some(inference.contraCandidates, t => isTypeAssignableTo(inferredCovariantType, t)) &&
                             every(context.inferences, other =>
                                 other !== inference && getConstraintOfTypeParameter(other.typeParameter) !== inference.typeParameter ||
-                                every(other.candidates, t => isTypeAssignableTo(t, inferredCovariantType))));
+                                every(other.candidates, t => isTypeAssignableTo(t, inferredCovariantType))) &&
+                            (some(inference.candidates, t => !isObjectOrArrayLiteralType(t)) || isTypeAssignableTo(inferredCovariantType, inferredContravariantType) && !isTypeAssignableTo(inferredContravariantType, inferredCovariantType)));
                     inferredType = preferCovariantType ? inferredCovariantType : inferredContravariantType;
                     fallbackType = preferCovariantType ? inferredContravariantType : inferredCovariantType;
                 }

All tests pass with it with no changes whatsoever but then it makes those 2 to behave differently:

declare function call<P>(tag: (props: P) => void, attributes: P): P;
declare function consume(p: { a?: string; b?: number; c?: boolean }): void;

declare const obj1: { a?: string };
const result1 = call(consume, obj1);
const result4 = call(consume, { ...obj1 });

And all in all, I'm just not sure what's right here. It all boils down to choosing some tradeoffs.

some(inference.contraCandidates, t => isTypeAssignableTo(inferredCovariantType, t)) &&
every(context.inferences, other =>
other !== inference && getConstraintOfTypeParameter(other.typeParameter) !== inference.typeParameter ||
Expand Down
56 changes: 56 additions & 0 deletions tests/baselines/reference/coAndContraVariantInferences12.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
coAndContraVariantInferences12.ts(18,19): error TS2353: Object literal may only specify known properties, and 'age' does not exist in type '{ name?: string | undefined; } & ElementAttributes'.
coAndContraVariantInferences12.ts(23,18): error TS2345: Argument of type '{}' is not assignable to parameter of type '{ name: string; } & ElementAttributes'.
Property 'name' is missing in type '{}' but required in type '{ name: string; }'.
coAndContraVariantInferences12.ts(24,20): error TS2353: Object literal may only specify known properties, and 'age' does not exist in type '{ name: string; } & ElementAttributes'.
coAndContraVariantInferences12.ts(25,18): error TS2345: Argument of type '{ idomKey: null; }' is not assignable to parameter of type '{ name: string; } & ElementAttributes'.
Property 'name' is missing in type '{ idomKey: null; }' but required in type '{ name: string; }'.


==== coAndContraVariantInferences12.ts (4 errors) ====
// https://github.com/microsoft/TypeScript/issues/59765

type FunctionComponent<P = any> = (props: P) => Element | null;

interface ElementAttributes {
idomKey?: string | null | number;
children?: unknown;
skip?: boolean;
}

declare function element<P>(
tag: FunctionComponent<P & ElementAttributes>,
attributes: P & ElementAttributes,
): Element;

declare function ElName(props: { name?: string }): Element;
element(ElName, {}); // ok
element(ElName, { age: 42 }); // error
~~~
!!! error TS2353: Object literal may only specify known properties, and 'age' does not exist in type '{ name?: string | undefined; } & ElementAttributes'.
element(ElName, { idomKey: null }); // ok
element(ElName, { idomKey: null, name: "Trevor" }); // ok

declare function ElName2(props: { name: string }): Element;
element(ElName2, {}); // error
~~
!!! error TS2345: Argument of type '{}' is not assignable to parameter of type '{ name: string; } & ElementAttributes'.
!!! error TS2345: Property 'name' is missing in type '{}' but required in type '{ name: string; }'.
!!! related TS2728 coAndContraVariantInferences12.ts:22:35: 'name' is declared here.
element(ElName2, { age: 42 }); // error
~~~
!!! error TS2353: Object literal may only specify known properties, and 'age' does not exist in type '{ name: string; } & ElementAttributes'.
element(ElName2, { idomKey: null }); // error
~~~~~~~~~~~~~~~~~
!!! error TS2345: Argument of type '{ idomKey: null; }' is not assignable to parameter of type '{ name: string; } & ElementAttributes'.
!!! error TS2345: Property 'name' is missing in type '{ idomKey: null; }' but required in type '{ name: string; }'.
!!! related TS2728 coAndContraVariantInferences12.ts:22:35: 'name' is declared here.

declare function ElEmpty(props: {}): Element;
element(ElEmpty, { name: "Trevor" }); // ok
element(ElEmpty, { age: 42 }); // ok
element(ElEmpty, { idomKey: null }); // ok
element(ElEmpty, { idomKey: null, name: "Trevor" }); // ok
declare const withOptionalName: { name?: string };
element(ElEmpty, withOptionalName); // ok
element(ElEmpty, { ...withOptionalName, idomKey: null }); // ok

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

=== coAndContraVariantInferences12.ts ===
// https://github.com/microsoft/TypeScript/issues/59765

type FunctionComponent<P = any> = (props: P) => Element | null;
>FunctionComponent : Symbol(FunctionComponent, Decl(coAndContraVariantInferences12.ts, 0, 0))
>P : Symbol(P, Decl(coAndContraVariantInferences12.ts, 2, 23))
>props : Symbol(props, Decl(coAndContraVariantInferences12.ts, 2, 35))
>P : Symbol(P, Decl(coAndContraVariantInferences12.ts, 2, 23))
>Element : Symbol(Element, Decl(lib.dom.d.ts, --, --), Decl(lib.dom.d.ts, --, --))

interface ElementAttributes {
>ElementAttributes : Symbol(ElementAttributes, Decl(coAndContraVariantInferences12.ts, 2, 63))

idomKey?: string | null | number;
>idomKey : Symbol(ElementAttributes.idomKey, Decl(coAndContraVariantInferences12.ts, 4, 29))

children?: unknown;
>children : Symbol(ElementAttributes.children, Decl(coAndContraVariantInferences12.ts, 5, 35))

skip?: boolean;
>skip : Symbol(ElementAttributes.skip, Decl(coAndContraVariantInferences12.ts, 6, 21))
}

declare function element<P>(
>element : Symbol(element, Decl(coAndContraVariantInferences12.ts, 8, 1))
>P : Symbol(P, Decl(coAndContraVariantInferences12.ts, 10, 25))

tag: FunctionComponent<P & ElementAttributes>,
>tag : Symbol(tag, Decl(coAndContraVariantInferences12.ts, 10, 28))
>FunctionComponent : Symbol(FunctionComponent, Decl(coAndContraVariantInferences12.ts, 0, 0))
>P : Symbol(P, Decl(coAndContraVariantInferences12.ts, 10, 25))
>ElementAttributes : Symbol(ElementAttributes, Decl(coAndContraVariantInferences12.ts, 2, 63))

attributes: P & ElementAttributes,
>attributes : Symbol(attributes, Decl(coAndContraVariantInferences12.ts, 11, 48))
>P : Symbol(P, Decl(coAndContraVariantInferences12.ts, 10, 25))
>ElementAttributes : Symbol(ElementAttributes, Decl(coAndContraVariantInferences12.ts, 2, 63))

): Element;
>Element : Symbol(Element, Decl(lib.dom.d.ts, --, --), Decl(lib.dom.d.ts, --, --))

declare function ElName(props: { name?: string }): Element;
>ElName : Symbol(ElName, Decl(coAndContraVariantInferences12.ts, 13, 11))
>props : Symbol(props, Decl(coAndContraVariantInferences12.ts, 15, 24))
>name : Symbol(name, Decl(coAndContraVariantInferences12.ts, 15, 32))
>Element : Symbol(Element, Decl(lib.dom.d.ts, --, --), Decl(lib.dom.d.ts, --, --))

element(ElName, {}); // ok
>element : Symbol(element, Decl(coAndContraVariantInferences12.ts, 8, 1))
>ElName : Symbol(ElName, Decl(coAndContraVariantInferences12.ts, 13, 11))

element(ElName, { age: 42 }); // error
>element : Symbol(element, Decl(coAndContraVariantInferences12.ts, 8, 1))
>ElName : Symbol(ElName, Decl(coAndContraVariantInferences12.ts, 13, 11))
>age : Symbol(age, Decl(coAndContraVariantInferences12.ts, 17, 17))

element(ElName, { idomKey: null }); // ok
>element : Symbol(element, Decl(coAndContraVariantInferences12.ts, 8, 1))
>ElName : Symbol(ElName, Decl(coAndContraVariantInferences12.ts, 13, 11))
>idomKey : Symbol(idomKey, Decl(coAndContraVariantInferences12.ts, 18, 17))

element(ElName, { idomKey: null, name: "Trevor" }); // ok
>element : Symbol(element, Decl(coAndContraVariantInferences12.ts, 8, 1))
>ElName : Symbol(ElName, Decl(coAndContraVariantInferences12.ts, 13, 11))
>idomKey : Symbol(idomKey, Decl(coAndContraVariantInferences12.ts, 19, 17))
>name : Symbol(name, Decl(coAndContraVariantInferences12.ts, 19, 32))

declare function ElName2(props: { name: string }): Element;
>ElName2 : Symbol(ElName2, Decl(coAndContraVariantInferences12.ts, 19, 51))
>props : Symbol(props, Decl(coAndContraVariantInferences12.ts, 21, 25))
>name : Symbol(name, Decl(coAndContraVariantInferences12.ts, 21, 33))
>Element : Symbol(Element, Decl(lib.dom.d.ts, --, --), Decl(lib.dom.d.ts, --, --))

element(ElName2, {}); // error
>element : Symbol(element, Decl(coAndContraVariantInferences12.ts, 8, 1))
>ElName2 : Symbol(ElName2, Decl(coAndContraVariantInferences12.ts, 19, 51))

element(ElName2, { age: 42 }); // error
>element : Symbol(element, Decl(coAndContraVariantInferences12.ts, 8, 1))
>ElName2 : Symbol(ElName2, Decl(coAndContraVariantInferences12.ts, 19, 51))
>age : Symbol(age, Decl(coAndContraVariantInferences12.ts, 23, 18))

element(ElName2, { idomKey: null }); // error
>element : Symbol(element, Decl(coAndContraVariantInferences12.ts, 8, 1))
>ElName2 : Symbol(ElName2, Decl(coAndContraVariantInferences12.ts, 19, 51))
>idomKey : Symbol(idomKey, Decl(coAndContraVariantInferences12.ts, 24, 18))

declare function ElEmpty(props: {}): Element;
>ElEmpty : Symbol(ElEmpty, Decl(coAndContraVariantInferences12.ts, 24, 36))
>props : Symbol(props, Decl(coAndContraVariantInferences12.ts, 26, 25))
>Element : Symbol(Element, Decl(lib.dom.d.ts, --, --), Decl(lib.dom.d.ts, --, --))

element(ElEmpty, { name: "Trevor" }); // ok
>element : Symbol(element, Decl(coAndContraVariantInferences12.ts, 8, 1))
>ElEmpty : Symbol(ElEmpty, Decl(coAndContraVariantInferences12.ts, 24, 36))
>name : Symbol(name, Decl(coAndContraVariantInferences12.ts, 27, 18))

element(ElEmpty, { age: 42 }); // ok
>element : Symbol(element, Decl(coAndContraVariantInferences12.ts, 8, 1))
>ElEmpty : Symbol(ElEmpty, Decl(coAndContraVariantInferences12.ts, 24, 36))
>age : Symbol(age, Decl(coAndContraVariantInferences12.ts, 28, 18))

element(ElEmpty, { idomKey: null }); // ok
>element : Symbol(element, Decl(coAndContraVariantInferences12.ts, 8, 1))
>ElEmpty : Symbol(ElEmpty, Decl(coAndContraVariantInferences12.ts, 24, 36))
>idomKey : Symbol(idomKey, Decl(coAndContraVariantInferences12.ts, 29, 18))

element(ElEmpty, { idomKey: null, name: "Trevor" }); // ok
>element : Symbol(element, Decl(coAndContraVariantInferences12.ts, 8, 1))
>ElEmpty : Symbol(ElEmpty, Decl(coAndContraVariantInferences12.ts, 24, 36))
>idomKey : Symbol(idomKey, Decl(coAndContraVariantInferences12.ts, 30, 18))
>name : Symbol(name, Decl(coAndContraVariantInferences12.ts, 30, 33))

declare const withOptionalName: { name?: string };
>withOptionalName : Symbol(withOptionalName, Decl(coAndContraVariantInferences12.ts, 31, 13))
>name : Symbol(name, Decl(coAndContraVariantInferences12.ts, 31, 33))

element(ElEmpty, withOptionalName); // ok
>element : Symbol(element, Decl(coAndContraVariantInferences12.ts, 8, 1))
>ElEmpty : Symbol(ElEmpty, Decl(coAndContraVariantInferences12.ts, 24, 36))
>withOptionalName : Symbol(withOptionalName, Decl(coAndContraVariantInferences12.ts, 31, 13))

element(ElEmpty, { ...withOptionalName, idomKey: null }); // ok
>element : Symbol(element, Decl(coAndContraVariantInferences12.ts, 8, 1))
>ElEmpty : Symbol(ElEmpty, Decl(coAndContraVariantInferences12.ts, 24, 36))
>withOptionalName : Symbol(withOptionalName, Decl(coAndContraVariantInferences12.ts, 31, 13))
>idomKey : Symbol(idomKey, Decl(coAndContraVariantInferences12.ts, 33, 39))

Loading