Improve best type matching for elementwise elaborations #57537
Conversation
typescript-bot
added
the
For Uncommitted Bug
src/compiler/checker.ts
Outdated
| const idx = getIndexedAccessTypeOrUndefined(target, nameType); | ||
| if (idx) { | ||
| return idx; | ||
| } | ||
| if (target.flags & TypeFlags.Union) { |
There was a problem hiding this comment.
swapping the order of operations here improves some errors quite a bit in my eyes since the union discrimination happens early, for example:
const obj6: {
prop:
| {
type: "foo";
prop: string;
}
| {
type: "bar";
prop: number;
};
} = {
prop: {
type: "foo",
// current: Type 'boolean' is not assignable to type 'string | number'.
// pr: Type 'boolean' is not assignable to type 'string'
prop: true,
},
};
const obj7: {
prop:
| {
type: "foo";
prop: string;
}
| {
type: "bar";
prop: number;
};
} = {
// current: Type '{ type: "foo"; prop: number; }' is not assignable to type '{ type: "foo"; prop: string; } | { type: "bar"; prop: number; }'.
// Types of property 'prop' are incompatible.
// Type 'number' is not assignable to type 'string'.
prop: {
type: "foo",
// pr: Type 'number' is not assignable to type 'string'.
prop: 42,
},
};
src/compiler/checker.ts
Outdated
| @@ -21983,8 +21984,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| } | |||
| } | |||
| if (reportErrors) { | |||
| // Elaborate only if we can find a best matching type in the target union | |||
There was a problem hiding this comment.
I removed this union from here because it was misleading - the containing function handles both unions and intersections. However, from what I can tell reportErrors: true is only ever passed in for unions - I can revert this if requested
src/compiler/checker.ts
Outdated
| // Elaborate only if we can find a best matching type in the target union | ||
| const bestMatchingType = getBestMatchingType(source, target, isRelatedTo); | ||
| // Elaborate only if we can find a best matching type in the target | ||
| const bestMatchingType = getBestMatchingType(source, target, /*matchSingleOverlappy*/ true, isRelatedTo); |
There was a problem hiding this comment.
Returning a single overlappy type is required here to make the non-elementwise union-based elaboration to elaborate about the "last" element in the union, see the "recovered" errors in this commit: e9d136a
src/compiler/checker.ts
Outdated
| @@ -50901,10 +50901,18 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| } | |||
| } | |||
|
|
|||
| function findBestTypeForObjectLiteral(source: Type, unionTarget: UnionOrIntersectionType) { | |||
| if (getObjectFlags(source) & ObjectFlags.ObjectLiteral && someType(unionTarget, isArrayLikeType)) { | |||
| return find(unionTarget.types, t => !isArrayLikeType(t)); | |||
There was a problem hiding this comment.
The referenced bug was a somewhat funny one because optional properties always include | undefined in their types and undefinedType is always kept at the beginning of unionType.types. So in such a situation this was always returning that undefinedType - making it impossible to create a good elementwise elaboration.
src/compiler/checker.ts
Outdated
| !(t.flags & TypeFlags.Primitive) && | ||
| !isArrayLikeType(t) && | ||
| !typeHasCallOrConstructSignatures(t) && | ||
| (everyContainedType(t, t => !t.symbol || !(t.symbol.flags & SymbolFlags.Class)) || !containsNonPublicProperties(getAugmentedPropertiesOfType(t)))); |
There was a problem hiding this comment.
this is quite related to #56183 - although completions and contextual types are orthogonal to elementwise elaborations. I only figured out that I should filter those nominal types here because I was working on that other PR in the past
There was a problem hiding this comment.
Changed errors here might feel a little bit questionable. I feel like it's a good change and that it indirectly matches some other situations.
Let's take a look at this one:
function test(_: { children: [string, number] | boolean[] }) {}
const children = [{}, ""] satisfies [unknown, unknown];
test({
// Type '[{}, string]' is not assignable to type '[string, number] | boolean[]'.
// Type '[{}, string]' is not assignable to type '[string, number]'.
// Type at position 0 in source is not compatible with type at position 0 in target.
// Type '{}' is not assignable to type 'string'.(2322)
children,
});Regardless of the order of this union we get the same error. The tuple gets priority selection here and that's exactly what happens with my changes here. Since getBestMatchingType gets called before getIndexedAccessTypeOrUndefined I now select the tuple through findMatchingTypeReferenceOrTypeAliasReference.
If you feel strongly about it I can work on special-casing tuples/arrays in this algorithm.
There was a problem hiding this comment.
Those errors were improved based on overlapping type selection. Even though the discriminant property has the error (so we can't quite know which type it was supposed to be) - the other properties indicate the user's intention.
| !!! error TS2322: Type '{ kind: "A"; n: { a: string; b: string; }; }' is not assignable to type 'AB'. | ||
| !!! error TS2322: Types of property 'n' are incompatible. | ||
| !!! error TS2322: Object literal may only specify known properties, and 'b' does not exist in type 'AN'. | ||
| !!! error TS2353: Object literal may only specify known properties, and 'b' does not exist in type 'AN'. |
There was a problem hiding this comment.
This feels like a straight improvement - this was supposed to look like this since #55152 (cc @RyanCavanaugh - maybe it would be worth rechecking if the approach used by that PR doesn't have any other holes)
Andarist
force-pushed
the
fix/elementwise-errors-best-type-obj-literal
branch
from
78fd6d9 to
e295774
Compare
|
@typescript-bot perf test this |
|
Heya @DanielRosenwasser, I've started to run the regular perf test suite on this PR at e295774. You can monitor the build here. Update: The results are in! |
| @@ -14,7 +14,7 @@ indirectDiscriminantAndExcessProperty.ts(22,5): error TS2322: Type 'string' is n | |||
| thing({ | |||
| type: foo1, | |||
| ~~~~ | |||
| !!! error TS2322: Type 'string' is not assignable to type '"foo" | "bar"'. | |||
| !!! error TS2322: Type 'string' is not assignable to type '"foo"'. | |||
There was a problem hiding this comment.
Heh - this is kind of better if you're already familiar with string literals, but I'd argue overall worse in that it doesn't give as much of a hint that type is really the discriminant. I don't think it's a blocker though.
There was a problem hiding this comment.
Note that this isn't related to discriminants here. This case just happens to have a discriminant target.
The reason this has changed is that previously getBestMatchIndexedAccessTypeOrUndefined would prefer the simple match done using getIndexedAccessTypeOrUndefined. Since type is a shared property in the union target that would return early and the error with the union type would be raised. Now getBestMatchingType is preferred so this new error here is the product of the best overlappy type matching.
This now matches more closely the non-elementwise error reported in the very same situation, see the playground here
| @@ -20,9 +20,9 @@ index.tsx(11,6): error TS2769: No overload matches this call. | |||
| ~~~ | |||
| !!! error TS2769: No overload matches this call. | |||
| !!! error TS2769: Overload 2 of 2, '(props: PropsType, context: any): Foo', gave the following error. | |||
| !!! error TS2769: Type 'unknown' is not assignable to type 'string | boolean'. | |||
| !!! error TS2769: Type 'unknown' is not assignable to type 'string'. | |||
There was a problem hiding this comment.
This does seem worse overall, but it feels like JSX children should have better messages.
There was a problem hiding this comment.
I touched on this here. Let me know if that makes any sense or if you think I should work more on improving this case.
| !!! error TS2322: Type 'number' is not assignable to type 'undefined'. | ||
| !!! related TS6500 objectLiteralNormalization.ts:11:19: The expected type comes from property 'b' which is declared here on type '{ a: number; b: number; } | { a: string; b?: undefined; } | { a?: undefined; b?: undefined; }' |
There was a problem hiding this comment.
Unlike the other regressions, it really feels like we've lost too much context here.
There was a problem hiding this comment.
Doesn't the related info contain the same information that was reported previously in the non-elementwise error? What would be your ideal outcome here? It could be nicer if the error would focus on the selected best type and not on the containing union.
I wouldn't call this bit a regression per se - the same information is still here, it's presented differently though. And, of course, the presentation does matter - I'm not claiming that it doesn't.
There was a problem hiding this comment.
FWIW: That error is confusing. It starts out saying number isn't assignable to undefined, which is true, but then goes on to claim that the "expected type" comes from b of { a: number; b: number; } | { a: string; b?: undefined; } | { a?: undefined; b?: undefined; }. Now, given that union as the target type, if the b in the source is the only problem... why can't it be a number? So the error by itself kinda-sorta contradicts itself and requires an additional elementwise error saying a is also the wrong type to clear things up.
tl;dr: the error message isn’t a complete description of the problem, you also need to know what the source type of a is to understand why there’s an error at all
There was a problem hiding this comment.
It makes sense. When rereading it now I see how this is confusing - perhaps the related information should mention the complete source type or it should somehow mention the selected best type based on which this elementwise elaboration is produced.
I'll look into improving this - thanks for the feedback ❤️
|
@DanielRosenwasser Here they are:
tscComparison 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: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
typescript-bot
added
For Backlog Bug
…s-best-type-obj-literal # Conflicts: # src/compiler/utilities.ts
fixes #57541
This PR improves error locations for elementwise error elaborations in a couple of situations. It often finds a better property candidate for the printed error - so the highlighted error is way closer to the actual problem, and it's easier to focus on it. At times, when finding a candidate property when previously it couldn't find any, it also reduces the error span - which is also less distracting.