Skip to content

Add fallback when both co- and contra-variant inference candidates exist #54072

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

Merged
merged 3 commits into from
May 2, 2023
Merged

Add fallback when both co- and contra-variant inference candidates exist #54072

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
966a5a8
Add fallback when both co- and contra-variant inferences exist
ahejlsberg Apr 30, 2023
f0f477e
Add regression test
ahejlsberg Apr 30, 2023
c377c44
Restore code from #52180
ahejlsberg May 2, 2023
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
35 changes: 19 additions & 16 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25154,22 +25154,24 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const inference = context.inferences[index];
if (!inference.inferredType) {
let inferredType: Type | undefined;
const signature = context.signature;
if (signature) {
const inferredCovariantType = inference.candidates ? getCovariantInference(inference, signature) : undefined;
if (inference.contraCandidates) {
// If we have both co- and contra-variant inferences, we use the co-variant inference if it is not 'never',
// it is a subtype of some contra-variant inference, and no other type parameter is constrained to this type
// parameter and has inferences that would conflict. Otherwise, we use the contra-variant inference.
const useCovariantType = inferredCovariantType && !(inferredCovariantType.flags & TypeFlags.Never) &&
let fallbackType: Type | undefined;
if (context.signature) {
const inferredCovariantType = inference.candidates ? getCovariantInference(inference, context.signature) : undefined;
const inferredContravariantType = inference.contraCandidates ? getContravariantInference(inference) : undefined;
if (inferredCovariantType || inferredContravariantType) {
// If we have both co- and contra-variant inferences, we prefer the co-variant inference if it is not 'never',
// all co-variant inferences are subtypes of it (i.e. it isn't one of a conflicting set of candidates), it is
// a subtype of some contra-variant inference, and no other type parameter is constrained to this type parameter
// and has inferences that would conflict. Otherwise, we prefer the contra-variant inference.
const preferCovariantType = inferredCovariantType && (!inferredContravariantType ||
!(inferredCovariantType.flags & TypeFlags.Never) &&
every(inference.candidates, t => isTypeSubtypeOf(t, inferredCovariantType)) &&
some(inference.contraCandidates, t => isTypeSubtypeOf(inferredCovariantType, t)) &&
every(context.inferences, other =>
other !== inference && getConstraintOfTypeParameter(other.typeParameter) !== inference.typeParameter ||
every(other.candidates, t => isTypeSubtypeOf(t, inferredCovariantType)));
inferredType = useCovariantType ? inferredCovariantType : getContravariantInference(inference);
}
else if (inferredCovariantType) {
inferredType = inferredCovariantType;
every(context.inferences, other => other === inference ||
getConstraintOfTypeParameter(other.typeParameter) !== inference.typeParameter ||
Copy link
Member

Choose a reason for hiding this comment

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

Part of #52180 was motivated by noticing that when we treat the "current" inference as special, we incur some order-dependent behavior. Where this came up (if my memory was correct) was when I was editing the strictFunctionTypes PR, where I could edit the file and see errors change around non-deterministically.

I tested restoring the change from #52180 and this PR still passes (i.e. putting back other !== inference && getConstraintOfTypeParameter(other.typeParameter) !== inference.typeParameter), which makes me feel like reverting it back to the state in #52123 that skips over other === inference will bring back that oddity (and we just don't have a test for it).

I'll try and see if I can make it break again like I did the first time.

Copy link
Member

Choose a reason for hiding this comment

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

It turns out we deleted the code where I was able to reproduce this, so, I guess I don't really know anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking carefully at the code you had in #52180 and the original code plus the line I added here, I've convinced myself they're just two ways of saying the same thing. So I'll restore the change from #52180.

Copy link
Member

Choose a reason for hiding this comment

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

Oh! You're right, I missed that new line.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, I'm happy with whichever is more performant or clearer. They're definitely equivalent now that I think closely.

every(other.candidates, t => isTypeSubtypeOf(t, inferredCovariantType))));
inferredType = preferCovariantType ? inferredCovariantType : inferredContravariantType;
fallbackType = preferCovariantType ? inferredContravariantType : inferredCovariantType;
}
else if (context.flags & InferenceFlags.NoDefault) {
// We use silentNeverType as the wildcard that signals no inferences.
Expand Down Expand Up @@ -25199,7 +25201,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (constraint) {
const instantiatedConstraint = instantiateType(constraint, context.nonFixingMapper);
if (!inferredType || !context.compareTypes(inferredType, getTypeWithThisArgument(instantiatedConstraint, inferredType))) {
inference.inferredType = inferredType = instantiatedConstraint;
// If the fallback type satisfies the constraint, we pick it. Otherwise, we pick the constraint.
inference.inferredType = fallbackType && context.compareTypes(fallbackType, getTypeWithThisArgument(instantiatedConstraint, fallbackType)) ? fallbackType : instantiatedConstraint;
}
}
}
Expand Down
66 changes: 66 additions & 0 deletions tests/baselines/reference/coAndContraVariantInferences5.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
=== tests/cases/compiler/coAndContraVariantInferences5.ts ===
type Thing = 'a' | 'b';
>Thing : Symbol(Thing, Decl(coAndContraVariantInferences5.ts, 0, 0))

function f(
>f : Symbol(f, Decl(coAndContraVariantInferences5.ts, 0, 23))

options: SelectOptions<Thing>,
>options : Symbol(options, Decl(coAndContraVariantInferences5.ts, 2, 11))
>SelectOptions : Symbol(SelectOptions, Decl(coAndContraVariantInferences5.ts, 17, 2))
>Thing : Symbol(Thing, Decl(coAndContraVariantInferences5.ts, 0, 0))

onChange: (status: Thing | null) => void,
>onChange : Symbol(onChange, Decl(coAndContraVariantInferences5.ts, 3, 34))
>status : Symbol(status, Decl(coAndContraVariantInferences5.ts, 4, 15))
>Thing : Symbol(Thing, Decl(coAndContraVariantInferences5.ts, 0, 0))

): void {
select({
>select : Symbol(select, Decl(coAndContraVariantInferences5.ts, 10, 1))

options,
>options : Symbol(options, Decl(coAndContraVariantInferences5.ts, 6, 12))

onChange,
>onChange : Symbol(onChange, Decl(coAndContraVariantInferences5.ts, 7, 16))

});
}

declare function select<KeyT extends string>(props: SelectProps<KeyT>): void;
>select : Symbol(select, Decl(coAndContraVariantInferences5.ts, 10, 1))
>KeyT : Symbol(KeyT, Decl(coAndContraVariantInferences5.ts, 12, 24))
>props : Symbol(props, Decl(coAndContraVariantInferences5.ts, 12, 45))
>SelectProps : Symbol(SelectProps, Decl(coAndContraVariantInferences5.ts, 12, 77))
>KeyT : Symbol(KeyT, Decl(coAndContraVariantInferences5.ts, 12, 24))

type SelectProps<KeyT extends string> = {
>SelectProps : Symbol(SelectProps, Decl(coAndContraVariantInferences5.ts, 12, 77))
>KeyT : Symbol(KeyT, Decl(coAndContraVariantInferences5.ts, 14, 17))

options?: SelectOptions<KeyT>;
>options : Symbol(options, Decl(coAndContraVariantInferences5.ts, 14, 41))
>SelectOptions : Symbol(SelectOptions, Decl(coAndContraVariantInferences5.ts, 17, 2))
>KeyT : Symbol(KeyT, Decl(coAndContraVariantInferences5.ts, 14, 17))

onChange: (key: KeyT) => void;
>onChange : Symbol(onChange, Decl(coAndContraVariantInferences5.ts, 15, 34))
>key : Symbol(key, Decl(coAndContraVariantInferences5.ts, 16, 15))
>KeyT : Symbol(KeyT, Decl(coAndContraVariantInferences5.ts, 14, 17))

};

type SelectOptions<KeyT extends string> =
>SelectOptions : Symbol(SelectOptions, Decl(coAndContraVariantInferences5.ts, 17, 2))
>KeyT : Symbol(KeyT, Decl(coAndContraVariantInferences5.ts, 19, 19))

| Array<{key: KeyT}>
>Array : Symbol(Array, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>key : Symbol(key, Decl(coAndContraVariantInferences5.ts, 20, 13))
>KeyT : Symbol(KeyT, Decl(coAndContraVariantInferences5.ts, 19, 19))

| Array<KeyT>;
>Array : Symbol(Array, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>KeyT : Symbol(KeyT, Decl(coAndContraVariantInferences5.ts, 19, 19))

53 changes: 53 additions & 0 deletions tests/baselines/reference/coAndContraVariantInferences5.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
=== tests/cases/compiler/coAndContraVariantInferences5.ts ===
type Thing = 'a' | 'b';
>Thing : "a" | "b"

function f(
>f : (options: SelectOptions<Thing>, onChange: (status: Thing | null) => void) => void

options: SelectOptions<Thing>,
>options : SelectOptions<Thing>

onChange: (status: Thing | null) => void,
>onChange : (status: Thing | null) => void
>status : Thing | null

): void {
select({
>select({ options, onChange, }) : void
>select : <KeyT extends string>(props: SelectProps<KeyT>) => void
>{ options, onChange, } : { options: SelectOptions<Thing>; onChange: (status: Thing | null) => void; }

options,
>options : SelectOptions<Thing>

onChange,
>onChange : (status: Thing | null) => void

});
}

declare function select<KeyT extends string>(props: SelectProps<KeyT>): void;
>select : <KeyT extends string>(props: SelectProps<KeyT>) => void
>props : SelectProps<KeyT>

type SelectProps<KeyT extends string> = {
>SelectProps : SelectProps<KeyT>

options?: SelectOptions<KeyT>;
>options : SelectOptions<KeyT> | undefined

onChange: (key: KeyT) => void;
>onChange : (key: KeyT) => void
>key : KeyT

};

type SelectOptions<KeyT extends string> =
>SelectOptions : SelectOptions<KeyT>

| Array<{key: KeyT}>
>key : KeyT

| Array<KeyT>;

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

type Thing = 'a' | 'b';

function f(
options: SelectOptions<Thing>,
onChange: (status: Thing | null) => void,
): void {
select({
options,
onChange,
});
}

declare function select<KeyT extends string>(props: SelectProps<KeyT>): void;

type SelectProps<KeyT extends string> = {
options?: SelectOptions<KeyT>;
onChange: (key: KeyT) => void;
};

type SelectOptions<KeyT extends string> =
| Array<{key: KeyT}>
| Array<KeyT>;