Uh oh!
There was an error while loading. Please reload this page.
Tweak logic that chooses co- vs. contra-variant inferences when covariant inference is an empty object type - #59772
Conversation
…iant inference is an empty object type
| // 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)) && |
There was a problem hiding this comment.
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)).
There was a problem hiding this comment.
Actually, there is an open question here as to what the inferred type here should be (what should be the result's type):
declarefunctioncall<P>(tag: (props: P)=>void,attributes: P): P;declarefunctionconsume(p: {a?: string;b?: number;c?: boolean}): void;declareconstobj: {a?: string};constresult=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:
declarefunctioncall<P>(tag: (props: P)=>void,attributes: P): P;declarefunctionconsume(p: {a?: string;b?: number;c?: boolean}): void;constobj: {a?: string;}={a: 'foo',b: 'bar'}constresult=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.
There was a problem hiding this comment.
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:
declarefunctioncall<P>(tag: (props: P)=>void,attributes: P): P;declarefunctionconsume(p: {a?: string;b?: number;c?: boolean}): void;declareconstobj1: {a?: string};constresult1=call(consume,obj1);constresult4=call(consume,{ ...obj1});And all in all, I'm just not sure what's right here. It all boils down to choosing some tradeoffs.
fed8b34 to
993dfb4CompareJake Bailey (jakebailey)
commented
Aug 28, 2024
TypeScript Bot (@typescript-bot) test it |
TypeScript Bot (typescript-bot)
commented
Aug 28, 2024
Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready. Everything looks the same! |
TypeScript Bot (typescript-bot)
commented
Aug 28, 2024
Jake Bailey (@jakebailey) Here are the results of running the user tests with tsc comparing Everything looks good! |
TypeScript Bot (typescript-bot)
commented
Aug 28, 2024
Jake Bailey (@jakebailey) Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
TypeScript Bot (typescript-bot)
commented
Aug 29, 2024
Jake Bailey (@jakebailey) Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Ryan Cavanaugh (RyanCavanaugh)
commented
Aug 30, 2024
I don't feel like this change is well-motivated enough to merge as-is. It seems like it's just detecting the repro in the linked issue and doing something else when it sees that, rather than a principled approach of what to do if all inference sites for a type parameter are intersections. Given the call on the linked issue it sounds like we're in agreement to not take this, but I'm definitely willing to have a discussion from first principles (just a log a new bug and we can take it from there). |
Mateusz Burzyński (Andarist)
commented
Sep 3, 2024
I agree. I think that maybe a patch closer to this one could improve some scenarios but I'm also not sure. It's hard for me to know which tradeoffs should be preferred by the overall TS design in cases like this. |
fixes#59765
cc Wesley Wigham (@weswigham) as the reviewer of #57909 by which this got affected