-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Skip parent error when reporting excess property checks #55152
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.
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
Skip parent error when reporting excess property checks #55152
Conversation
# Conflicts: # tests/baselines/reference/jsxSpreadTag.js # tests/baselines/reference/jsxSpreadTag.symbols # tests/baselines/reference/jsxSpreadTag.types # tests/cases/compiler/jsxSpreadTag.ts
src/compiler/checker.ts
Outdated
@@ -20354,6 +20354,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
let overrideNextErrorInfo = 0; // How many `reportRelationError` calls should be skipped in the elaboration pyramid | |||
let lastSkippedInfo: [Type, Type] | undefined; | |||
let incompatibleStack: DiagnosticAndArguments[] | undefined; | |||
let skipParentCounter = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This currently never goes higher than 1, but if we ever had the notion of an EPC->EPC nested error, that would be how it could happen. Open to changing it to a boolean if wanted.
@typescript-bot pack this |
Heya @RyanCavanaugh, I've started to run the tarball bundle task on this PR at 9551e67. You can monitor the build here. |
Hey @RyanCavanaugh, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running |
Let's make sure this example doesn't regress and always reports on the discriminant. export type Blah =
| { type: "foo", abc: string }
| { type: "bar", xyz: number, extra: any };
declare function thing(blah: Blah): void;
let foo = "foo";
thing({
type: foo,
abc: "hello!"
});
thing({
type: foo,
abc: "hello!",
extra: 123,
});
let bar = "bar";
thing({
type: bar,
xyz: 123,
extra: 123,
}); |
!!! error TS2352: Object literal may only specify known properties, and 'foo' does not exist in type '{ id: number; }'. | ||
!!! error TS2352: Object literal may only specify known properties, and 'foo' does not exist in type '{ id: number; }'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error should never have been issued in the first place. It seems like a bug in our element-wise elaboration logic. Notice how in the following example one of these is has a missing property error and the other is an excess property error:
let okay = { foo: "" } as { id: 123 };
// ~~~~~~~~~~~~~~~~~~~~~~~~~~
// Conversion of type '{ foo: string; }' to type '{ id: 123; }' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
// Property 'id' is missing in type '{ foo: string; }' but required in type '{ id: 123; }'.
let waat = [{ foo: "" }] as { id: 123 }[];
// ~~~
// Conversion of type '{ foo: string; }[]' to type '{ id: 123; }[]' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
// Type '{ foo: string; }' is not comparable to type '{ id: 123; }'.
// Object literal may only specify known properties, and 'foo' does not exist in type '{ id: 123; }'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened up #55167
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I wouldn’t ever expect an EPC error on a type assertion.
@typescript-bot pack this |
Heya @jakebailey, I've started to run the tarball bundle task on this PR at e76b422. You can monitor the build here. |
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
Suggestions on wording from a user who was previously confused by it Priorities:
It's a bit tricky to give clear instructions on how to assign it given the many scenarios. Simpler option might be:
Really the important thing isnt how to assign but that the user understands that it is possible, to avoid misundersranding types. |
We can discuss here but I'd like to change the message (if we do) in a different PR Whether or not the error message should describe how to disable the error is a bit of a judgment call. Most errors today don't do this; usually we only do this when the error is low-confidence. EPCs are, in my experience, very high-confidence -- it's nearly always a typo or total user error to have an excess property, and the brevity of the error message can reflect this. Going off a concrete example type Dimensions = {
width: number;
height: number;
}
const m: Dimensions = { width: 0, length: 0, height: 0 }; The current error stack (as of this PR) is just
I liked the phrasing we switched to in #50471, where we're pointing out that something seems wrong rather than declaring some impossibility. Applying that style here, I'd maybe write something like
Advantages
We also have the typo-detected variant: type Dimensions = {
width: number;
height: number;
length?: number;
}
const m: Dimensions = { width: 0, lenghh: 0, height: 0 };
I would just shorten further:
|
@RyanCavanaugh Yeah I generally agree. No issues with going ahead with something along those lines. I based my suggestion on the type casting error which is similar in feeling, but with a suggestion on how to fix it. I think in the situation where you want to add properties, you are creating an object that conforms to some type standard, that is probably defined externally. For simply object creation, you could extend this type with an intersection to creating your own expanded type. The other scenario that I can think of is abstract properties, but the way that this is handled is currently very permissive, as previously discussed, so no EPC checking here. So the only scenario where EPC would come into play here is the Just thought I would lay that out incase it helps anyone. |
I like those. Nit: In the ones I quoted, I fixed them up to "appears to be unintentional" |
src/compiler/checker.ts
Outdated
@@ -20354,6 +20354,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
let overrideNextErrorInfo = 0; // How many `reportRelationError` calls should be skipped in the elaboration pyramid | |||
let lastSkippedInfo: [Type, Type] | undefined; | |||
let incompatibleStack: DiagnosticAndArguments[] | undefined; | |||
let skipParentCounter = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be captured in captureErrorCalculationState
and reset in resetErrorInfo
so it's set/unset when a speculative relationship elaboration pyramid is discarded. AFAIK, you'd need to do something like trigger EPC inside a conditional type or indexed access type where we end up using the constraint check rather than the normal check for validity for it to matter (and the bug would look something like us discarding elaboration layers for non-EPC errors). Definitely niche, but those functions are just there for capturing/restoring error pyramid state, so should also track this new variable.
Also, maybe a comment on how this differs from the nearby overrideNextErrorInfo
number. (overrideNextErrorInfo
skips the next N things in the pyramid, while when set this skips the N prior things in the pyramid.)
Case in point. In general, people see "X is not assignable to Y" and expect that to apply in all contexts. That issue, incidentally, also highlights another way the current error is misleading: it says "object literal may only specify known properties", but many defect reports still involve object literals, just without the contextual typing required for EPC to work. |
When we report an excess property error, the current error chain looks like this
As frequently noted by @fatcerberus, this gives the misleading impression that object types are sealed, since the way we printback a fresh object literal type is indistinguishable from a regular object type. It's also a waste of space since the top errorline prints back
T
when it's already in the EPC line, and the object literal source type is always anonymous (therefore long).This PR skips the parent line when reporting EPC errors, so you don't see the "x is not assignable to y" part. The implementation is set up to handle the tricky case
where we want to print back two EPC errors: