Improve excess property checks #28854
Conversation
| @@ -40,7 +50,7 @@ tests/cases/compiler/objectLiteralExcessProperties.ts(33,27): error TS2322: Type | |||
| var b2: Book | string = { foreward: "nope" }; | |||
| ~~~~~~~~~~~~~~~~ | |||
| !!! error TS2322: Type '{ foreward: string; }' is not assignable to type 'string | Book'. | |||
| !!! error TS2322: Object literal may only specify known properties, and 'foreward' does not exist in type 'string | Book'. | |||
There was a problem hiding this comment.
Side note: in this or another PR, we should make these both but
There was a problem hiding this comment.
It's probably most correct to have two separate sentences, as the subjects of the clauses are not well-connected. Object literal may only specify known properties. 'foreward' does not exist in type 'string | Book'. Two separate statements of fact - there is no real relationship between them, so a conjunction feels a bit extraneous to me. 🚲 🏚
I call in @sandersn as our resident linguist to weigh in.
|
Looks good, but it's not clear why we don't want to dive into conditional types. |
| // No excess property checks on generic types | ||
| const obj1: T = { name: "test" }; | ||
| ~~~~ | ||
| !!! error TS2322: Type '{ name: string; }' is not assignable to type 'T'. |
There was a problem hiding this comment.
Side note: I've seen a couple issues opened where people don't really understand constrained generics well and are surprised an assignment like this fails and don't "get" the error. A little more elaboration could help here (ie, when assignment to a bare type parameter fails), like A is assignable to T's constraint B, but not T itself., just to indicate that "we looked at what you're thinking of, and that isn't what's going on here".
Unrelated to this PR, though. Just a remark. @DanielRosenwasser you agree?
| !!! error TS2322: Type '{ name: string; prop: boolean; }' is not assignable to type 'T'. | ||
| // Excess property checks only on non-generic parts of unions | ||
| const obj3: T | { prop: boolean } = { name: "test", prop: true }; | ||
| ~~~~~~~~~~~~ |
There was a problem hiding this comment.
This makes sense in an assignment context, but
declare function foo<T>(x: T | { prop: boolean }): T | boolean;
foo({prop: true, name: "test"}); // probably shouldn't be excess, trivially satisfies `T`for calls it seems a little jank, perhaps? The union case just seems presumptuous to me, I guess.
There was a problem hiding this comment.
Does the excess error happen before inference of T to {prop: true, name: string}?
There was a problem hiding this comment.
@weswigham As @jack-williams points out, there's no error in your example because we infer and substitute a type for T before the assignment check.
With this PR we improve the logic for determining when to perform excess property checks. Specifically, we now perform excess property checks when the source type is a fresh object literal and the target type is not an empty object type (as determined by
isEmptyObjectType) and is an excess property check target. An excess property check target is defined asobjecttype,Fixes #28752 (in that the error messages are less confusing).