Fix excess property checking for unions with index signatures #34927
Conversation
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 386e550. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 386e550. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 386e550. You can monitor the build here. It should now contribute to this PR's status checks. |
|
@ahejlsberg Here they are:Comparison Report - master..34927
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Performance is unaffected, so we're fine there. RWC tests have a few error elaboration changes, but they look fine. DT tests reveal genuine errors in the tests of three packages, |
| !!! error TS2200: The types of 'icon.props' are incompatible between these types. | ||
| !!! error TS2200: Type '{ INVALID_PROP_NAME: string; ariaLabel: string; }' is not assignable to type 'ITestProps'. | ||
| !!! error TS2200: Object literal may only specify known properties, and 'INVALID_PROP_NAME' does not exist in type 'ITestProps'. | ||
| !!! error TS2345: Argument of type '{ icon: { props: { INVALID_PROP_NAME: string; ariaLabel: string; }; }; }' is not assignable to parameter of type '(TestProps & { children?: number; }) | ({ props2: { x: number; }; } & { children?: number; })'. |
There was a problem hiding this comment.
Should we try and keep this line of elaboration suppressed? I think the filtering we were doing before had the effect of suppressing it.
| type Thing3 = number | { toFixed: null, toString: undefined } | object; | ||
| const l: Thing3 = { toString: undefined }; // error, toFixed isn't null | ||
| ~ | ||
| !!! error TS2741: Property 'toFixed' is missing in type '{ toString: undefined; }' but required in type '{ toFixed: null; toString: undefined; }'. |
There was a problem hiding this comment.
IMO, this should still be an error - #31708 came up because we unconditionally treated object as a type with no properties for excess property checking (so all usages of object used to trigger excess property errors like this), which caused problems in these primitive only unions (where there really isn't any hinting as to what properties an object literal should contain, really), however in case like this one, where there's an explicit object type, using the hints from the specified object types for excess properties is more desirable (and prevents you form passing total garbage and typo'ing the property names from the provided literal).
There was a problem hiding this comment.
This PR doesn't change anything with respect to #31708. That logic is still in place and we still filter out primitives from unions containing object before we do excess property checking. Also, there was never any logic in place to remove object when actual object types are present. Rather, when object is present in a union it effectively turns off excess property checking, just like {}. However, we had some very odd behavior when it came to using toString and the other property names from Object. For example:
let x1: { a: string, b: number } | object = { a: 'test' };
let x2: { a: 'test', b: number } | object = { a: 'test' };
let x3: { toString: string, b: number } | object = { toString: 'test' };
let x4: { toString: 'test', b: number } | object = { toString: 'test' }; // Error!Above, we would error on x4, but not on any of the other ones. That just doesn't make sense, and we now permit it. (Keep in mind here that by design we don't give special treatment to toString and the other Object properties in assignment checking.)
There was a problem hiding this comment.
As I said in person - {} disables excess property checking because it has baggage whereupon it was used as a general top type. object has no such compunctions. Otherwise there's no way to have an excess property checked empty object.
There was a problem hiding this comment.
What I'm pointing out is that, except for the logic that filters out primitives, object and {} behave the same for purposes of excess property checking in the current compiler. It's existing behavior and unrelated to the issue I'm fixing in this PR.
|
Adding the Breaking Change label since we now catch more issues. |
This PR revises our excess property checking logic to properly handle union types with index signatures. For example, both of the following now error where previously they didn't:
Fixes #34611.