Skip to content

Improve excess property checks #28854

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Dec 5, 2018
Merged

Improve excess property checks #28854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1d2df84
Improve excess property checking logic
ahejlsberg Dec 4, 2018
f1b3c6e
Accept new baselines
ahejlsberg Dec 4, 2018
e7bb562
Add additional tests
ahejlsberg Dec 4, 2018
6e022bd
Accept new baselines
ahejlsberg Dec 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11836,7 +11836,7 @@ namespace ts {
if (!noImplicitAny && getObjectFlags(target) & ObjectFlags.JSLiteral) {
return false; // Disable excess property checks on JS literals to simulate having an implicit "index signature" - but only outside of noImplicitAny
}
if (maybeTypeOfKind(target, TypeFlags.Object) && !(getObjectFlags(target) & ObjectFlags.ObjectLiteralPatternWithComputedProperties)) {
if (isExcessPropertyCheckTarget(target)) {
const isComparingJsxAttributes = !!(getObjectFlags(source) & ObjectFlags.JsxAttributes);
if ((relation === assignableRelation || relation === definitelyAssignableRelation || relation === comparableRelation) &&
(isTypeSubsetOf(globalObjectType, target) || (!isComparingJsxAttributes && isEmptyObjectType(target)))) {
Expand All @@ -11849,6 +11849,9 @@ namespace ts {
for (const prop of getPropertiesOfObjectType(source)) {
if (shouldCheckAsExcessProperty(prop, source.symbol) && !isKnownProperty(target, prop.escapedName, isComparingJsxAttributes)) {
if (reportErrors) {
// Report error in terms of object types in the target as those are the only ones
// we check in isKnownProperty.
const errorTarget = filterType(target, isExcessPropertyCheckTarget);
// We know *exactly* where things went wrong when comparing the types.
// Use this property as the error node as this will be more helpful in
// reasoning about what went wrong.
Expand All @@ -11857,7 +11860,7 @@ namespace ts {
// JsxAttributes has an object-literal flag and undergo same type-assignablity check as normal object-literal.
// However, using an object-literal error message will be very confusing to the users so we give different a message.
// TODO: Spelling suggestions for excess jsx attributes (needs new diagnostic messages)
reportError(Diagnostics.Property_0_does_not_exist_on_type_1, symbolToString(prop), typeToString(target));
reportError(Diagnostics.Property_0_does_not_exist_on_type_1, symbolToString(prop), typeToString(errorTarget));
}
else {
// use the property's value declaration if the property is assigned inside the literal itself
Expand All @@ -11871,17 +11874,17 @@ namespace ts {

const name = propDeclaration.name!;
if (isIdentifier(name)) {
suggestion = getSuggestionForNonexistentProperty(name, target);
suggestion = getSuggestionForNonexistentProperty(name, errorTarget);
}
}

if (suggestion !== undefined) {
reportError(Diagnostics.Object_literal_may_only_specify_known_properties_but_0_does_not_exist_in_type_1_Did_you_mean_to_write_2,
symbolToString(prop), typeToString(target), suggestion);
symbolToString(prop), typeToString(errorTarget), suggestion);
}
else {
reportError(Diagnostics.Object_literal_may_only_specify_known_properties_and_0_does_not_exist_in_type_1,
symbolToString(prop), typeToString(target));
symbolToString(prop), typeToString(errorTarget));
}
}
}
Expand Down Expand Up @@ -18595,20 +18598,23 @@ namespace ts {
return true;
}
}
else if (targetType.flags & TypeFlags.UnionOrIntersection) {
else if (targetType.flags & TypeFlags.UnionOrIntersection && isExcessPropertyCheckTarget(targetType)) {
for (const t of (targetType as UnionOrIntersectionType).types) {
if (isKnownProperty(t, name, isComparingJsxAttributes)) {
return true;
}
}
}
else if (targetType.flags & TypeFlags.Conditional) {
return isKnownProperty((targetType as ConditionalType).root.trueType, name, isComparingJsxAttributes) ||
isKnownProperty((targetType as ConditionalType).root.falseType, name, isComparingJsxAttributes);
}
return false;
}

function isExcessPropertyCheckTarget(type: Type): boolean {
return !!(type.flags & TypeFlags.Object && !(getObjectFlags(type) & ObjectFlags.ObjectLiteralPatternWithComputedProperties) ||
type.flags & TypeFlags.NonPrimitive ||
type.flags & TypeFlags.Union && some((<UnionType>type).types, isExcessPropertyCheckTarget) ||
type.flags & TypeFlags.Intersection && every((<IntersectionType>type).types, isExcessPropertyCheckTarget));
}

function checkJsxExpression(node: JsxExpression, checkMode?: CheckMode) {
if (node.expression) {
const type = checkExpression(node.expression, checkMode);
Expand Down
8 changes: 4 additions & 4 deletions tests/baselines/reference/conditionalTypesExcessProperties.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
tests/cases/conformance/types/conditional/conditionalTypesExcessProperties.ts(8,5): error TS2322: Type '{ test: string; arg: A; }' is not assignable to type 'Something<A>'.
Type '{ test: string; arg: A; }' is not assignable to type 'A extends object ? { arg: A; } : { arg?: undefined; }'.
tests/cases/conformance/types/conditional/conditionalTypesExcessProperties.ts(9,33): error TS2322: Type '{ test: string; arg: A; arr: A; }' is not assignable to type 'Something<A>'.
Object literal may only specify known properties, and 'arr' does not exist in type 'Something<A>'.
tests/cases/conformance/types/conditional/conditionalTypesExcessProperties.ts(9,5): error TS2322: Type '{ test: string; arg: A; arr: A; }' is not assignable to type 'Something<A>'.
Type '{ test: string; arg: A; arr: A; }' is not assignable to type 'A extends object ? { arg: A; } : { arg?: undefined; }'.


==== tests/cases/conformance/types/conditional/conditionalTypesExcessProperties.ts (2 errors) ====
Expand All @@ -17,8 +17,8 @@ tests/cases/conformance/types/conditional/conditionalTypesExcessProperties.ts(9,
!!! error TS2322: Type '{ test: string; arg: A; }' is not assignable to type 'Something<A>'.
!!! error TS2322: Type '{ test: string; arg: A; }' is not assignable to type 'A extends object ? { arg: A; } : { arg?: undefined; }'.
sa = { test: 'bye', arg: a, arr: a } // excess
~~~~~~
~~
!!! error TS2322: Type '{ test: string; arg: A; arr: A; }' is not assignable to type 'Something<A>'.
!!! error TS2322: Object literal may only specify known properties, and 'arr' does not exist in type 'Something<A>'.
!!! error TS2322: Type '{ test: string; arg: A; arr: A; }' is not assignable to type 'A extends object ? { arg: A; } : { arg?: undefined; }'.
}

57 changes: 50 additions & 7 deletions tests/baselines/reference/objectLiteralExcessProperties.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
tests/cases/compiler/objectLiteralExcessProperties.ts(9,18): error TS2322: Type '{ forword: string; }' is not assignable to type 'Book'.
Object literal may only specify known properties, but 'forword' does not exist in type 'Book'. Did you mean to write 'foreword'?
tests/cases/compiler/objectLiteralExcessProperties.ts(11,27): error TS2322: Type '{ foreward: string; }' is not assignable to type 'string | Book'.
Object literal may only specify known properties, and 'foreward' does not exist in type 'string | Book'.
Object literal may only specify known properties, but 'foreward' does not exist in type 'Book'. Did you mean to write 'foreword'?
tests/cases/compiler/objectLiteralExcessProperties.ts(13,53): error TS2322: Type '({ foreword: string; } | { forwards: string; })[]' is not assignable to type 'Book | Book[]'.
Type '({ foreword: string; } | { forwards: string; })[]' is not assignable to type 'Book[]'.
Type '{ foreword: string; } | { forwards: string; }' is not assignable to type 'Book'.
Expand All @@ -13,17 +13,27 @@ tests/cases/compiler/objectLiteralExcessProperties.ts(17,26): error TS2322: Type
Object literal may only specify known properties, but 'foreward' does not exist in type 'Book & Cover'. Did you mean to write 'foreword'?
tests/cases/compiler/objectLiteralExcessProperties.ts(19,57): error TS2322: Type '{ foreword: string; color: string; price: number; }' is not assignable to type 'Book & Cover'.
Object literal may only specify known properties, and 'price' does not exist in type 'Book & Cover'.
tests/cases/compiler/objectLiteralExcessProperties.ts(21,43): error TS2322: Type '{ foreword: string; price: number; }' is not assignable to type 'Book & number'.
Object literal may only specify known properties, and 'price' does not exist in type 'Book & number'.
tests/cases/compiler/objectLiteralExcessProperties.ts(21,5): error TS2322: Type '{ foreword: string; price: number; }' is not assignable to type 'Book & number'.
Type '{ foreword: string; price: number; }' is not assignable to type 'number'.
tests/cases/compiler/objectLiteralExcessProperties.ts(23,29): error TS2322: Type '{ couleur: string; }' is not assignable to type 'Cover | Cover[]'.
Object literal may only specify known properties, and 'couleur' does not exist in type 'Cover | Cover[]'.
tests/cases/compiler/objectLiteralExcessProperties.ts(25,27): error TS2322: Type '{ forewarned: string; }' is not assignable to type 'Book | Book[]'.
Object literal may only specify known properties, and 'forewarned' does not exist in type 'Book | Book[]'.
tests/cases/compiler/objectLiteralExcessProperties.ts(33,27): error TS2322: Type '{ colour: string; }' is not assignable to type 'Cover'.
Object literal may only specify known properties, but 'colour' does not exist in type 'Cover'. Did you mean to write 'color'?
tests/cases/compiler/objectLiteralExcessProperties.ts(37,25): error TS2304: Cannot find name 'IFoo'.
tests/cases/compiler/objectLiteralExcessProperties.ts(39,11): error TS2322: Type '{ name: string; }' is not assignable to type 'T'.
tests/cases/compiler/objectLiteralExcessProperties.ts(41,11): error TS2322: Type '{ name: string; prop: boolean; }' is not assignable to type 'T & { prop: boolean; }'.
Type '{ name: string; prop: boolean; }' is not assignable to type 'T'.
tests/cases/compiler/objectLiteralExcessProperties.ts(43,43): error TS2322: Type '{ name: string; prop: boolean; }' is not assignable to type 'T | { prop: boolean; }'.
Object literal may only specify known properties, and 'name' does not exist in type '{ prop: boolean; }'.
tests/cases/compiler/objectLiteralExcessProperties.ts(45,76): error TS2322: Type '{ name: string; prop: boolean; }' is not assignable to type '{ name: string; } | (T & { prop: boolean; })'.
Object literal may only specify known properties, and 'prop' does not exist in type '{ name: string; }'.
tests/cases/compiler/objectLiteralExcessProperties.ts(49,44): error TS2322: Type '{ z: string; }' is not assignable to type 'object & { x: string; }'.
Object literal may only specify known properties, and 'z' does not exist in type 'object & { x: string; }'.


==== tests/cases/compiler/objectLiteralExcessProperties.ts (10 errors) ====
==== tests/cases/compiler/objectLiteralExcessProperties.ts (16 errors) ====
interface Book {
foreword: string;
}
Expand All @@ -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'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: in this or another PR, we should make these both but

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

!!! error TS2322: Object literal may only specify known properties, but 'foreward' does not exist in type 'Book'. Did you mean to write 'foreword'?

var b3: Book | (Book[]) = [{ foreword: "hello" }, { forwards: "back" }];
~~~~~~~~~~~~~~~~
Expand All @@ -66,9 +76,9 @@ tests/cases/compiler/objectLiteralExcessProperties.ts(33,27): error TS2322: Type
!!! error TS2322: Object literal may only specify known properties, and 'price' does not exist in type 'Book & Cover'.

var b7: Book & number = { foreword: "hi", price: 10.99 };
~~~~~~~~~~~~
~~
!!! error TS2322: Type '{ foreword: string; price: number; }' is not assignable to type 'Book & number'.
!!! error TS2322: Object literal may only specify known properties, and 'price' does not exist in type 'Book & number'.
!!! error TS2322: Type '{ foreword: string; price: number; }' is not assignable to type 'number'.

var b8: Cover | Cover[] = { couleur : "non" };
~~~~~~~~~~~~~~~
Expand All @@ -91,4 +101,37 @@ tests/cases/compiler/objectLiteralExcessProperties.ts(33,27): error TS2322: Type
!!! error TS2322: Type '{ colour: string; }' is not assignable to type 'Cover'.
!!! error TS2322: Object literal may only specify known properties, but 'colour' does not exist in type 'Cover'. Did you mean to write 'color'?
!!! related TS6501 tests/cases/compiler/objectLiteralExcessProperties.ts:28:5: The expected type comes from this index signature.

// Repros inspired by #28752

function test<T extends IFoo>() {
~~~~
!!! error TS2304: Cannot find name 'IFoo'.
// No excess property checks on generic types
const obj1: T = { name: "test" };
~~~~
!!! error TS2322: Type '{ name: string; }' is not assignable to type 'T'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// No excess property checks on intersections involving generics
const obj2: T & { prop: boolean } = { name: "test", prop: true };
~~~~
!!! error TS2322: Type '{ name: string; prop: boolean; }' is not assignable to type 'T & { prop: boolean; }'.
!!! 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 };
~~~~~~~~~~~~
Copy link
Member

@weswigham weswigham Dec 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the excess error happen before inference of T to {prop: true, name: string}?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

!!! error TS2322: Type '{ name: string; prop: boolean; }' is not assignable to type 'T | { prop: boolean; }'.
!!! error TS2322: Object literal may only specify known properties, and 'name' does not exist in type '{ prop: boolean; }'.
// Excess property checks only on non-generic parts of unions
const obj4: T & { prop: boolean } | { name: string } = { name: "test", prop: true };
~~~~~~~~~~
!!! error TS2322: Type '{ name: string; prop: boolean; }' is not assignable to type '{ name: string; } | (T & { prop: boolean; })'.
!!! error TS2322: Object literal may only specify known properties, and 'prop' does not exist in type '{ name: string; }'.
// No excess property checks when union includes 'object' type
const obj5: object | { x: string } = { z: 'abc' }
// The 'object' type has no effect on intersections
const obj6: object & { x: string } = { z: 'abc' }
~~~~~~~~
!!! error TS2322: Type '{ z: string; }' is not assignable to type 'object & { x: string; }'.
!!! error TS2322: Object literal may only specify known properties, and 'z' does not exist in type 'object & { x: string; }'.
}

32 changes: 32 additions & 0 deletions tests/baselines/reference/objectLiteralExcessProperties.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,23 @@ interface Indexed {
var b10: Indexed = { 0: { }, '1': { } }; // ok

var b11: Indexed = { 0: { colour: "blue" } }; // nested object literal still errors

// Repros inspired by #28752

function test<T extends IFoo>() {
// No excess property checks on generic types
const obj1: T = { name: "test" };
// No excess property checks on intersections involving generics
const obj2: T & { prop: boolean } = { name: "test", prop: true };
// Excess property checks only on non-generic parts of unions
const obj3: T | { prop: boolean } = { name: "test", prop: true };
// Excess property checks only on non-generic parts of unions
const obj4: T & { prop: boolean } | { name: string } = { name: "test", prop: true };
// No excess property checks when union includes 'object' type
const obj5: object | { x: string } = { z: 'abc' }
// The 'object' type has no effect on intersections
const obj6: object & { x: string } = { z: 'abc' }
}


//// [objectLiteralExcessProperties.js]
Expand All @@ -46,3 +63,18 @@ var b8 = { couleur: "non" };
var b9 = { forewarned: "still no" };
var b10 = { 0: {}, '1': {} }; // ok
var b11 = { 0: { colour: "blue" } }; // nested object literal still errors
// Repros inspired by #28752
function test() {
// No excess property checks on generic types
var obj1 = { name: "test" };
// No excess property checks on intersections involving generics
var obj2 = { name: "test", prop: true };
// Excess property checks only on non-generic parts of unions
var obj3 = { name: "test", prop: true };
// Excess property checks only on non-generic parts of unions
var obj4 = { name: "test", prop: true };
// No excess property checks when union includes 'object' type
var obj5 = { z: 'abc' };
// The 'object' type has no effect on intersections
var obj6 = { z: 'abc' };
}
Loading