Skip to content
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

Avoid expensive relationship checking in mapped type member resolution #36754

Merged
merged 2 commits into from
Feb 12, 2020
Merged

Avoid expensive relationship checking in mapped type member resolution #36754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
868b7b9
Avoid expensive relationship checking in mapped type member resolution
ahejlsberg Feb 12, 2020
283118c
Accept new baselines
ahejlsberg Feb 12, 2020
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
4 changes: 2 additions & 2 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9589,7 +9589,7 @@ namespace ts {
// When creating an optional property in strictNullChecks mode, if 'undefined' isn't assignable to the
// type, we include 'undefined' in the type. Similarly, when creating a non-optional property in strictNullChecks
// mode, if the underlying property is optional we remove 'undefined' from the type.
prop.type = strictNullChecks && isOptional && !isTypeAssignableTo(undefinedType, propType) ? getOptionalType(propType) :
prop.type = strictNullChecks && isOptional && !maybeTypeOfKind(propType, TypeFlags.Undefined | TypeFlags.Void) ? getOptionalType(propType) :
strictNullChecks && !isOptional && modifiersProp && modifiersProp.flags & SymbolFlags.Optional ? getTypeWithFacts(propType, TypeFacts.NEUndefined) :
propType;
if (modifiersProp) {
Expand Down Expand Up @@ -13681,7 +13681,7 @@ namespace ts {
const templateMapper = combineTypeMappers(mapper, createTypeMapper([getTypeParameterFromMappedType(type)], [key]));
const propType = instantiateType(getTemplateTypeFromMappedType(<MappedType>type.target || type), templateMapper);
const modifiers = getMappedTypeModifiers(type);
return strictNullChecks && modifiers & MappedTypeModifiers.IncludeOptional && !isTypeAssignableTo(undefinedType, propType) ? getOptionalType(propType) :
return strictNullChecks && modifiers & MappedTypeModifiers.IncludeOptional && !maybeTypeOfKind(propType, TypeFlags.Undefined | TypeFlags.Void) ? getOptionalType(propType) :
strictNullChecks && modifiers & MappedTypeModifiers.ExcludeOptional && isOptional ? getTypeWithFacts(propType, TypeFacts.NEUndefined) :
propType;
}
Expand Down
12 changes: 6 additions & 6 deletions tests/baselines/reference/definiteAssignmentOfDestructuredVariable.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,20 @@ class C<T extends Options> {
>method : () => void

let { a, b } = this.foo;
>a : T["a"]
>a : T["a"] | undefined
>b : T["b"]
>this.foo : { [P in keyof T]: T[P]; }
>this : this
>foo : { [P in keyof T]: T[P]; }

!(a && b);
>!(a && b) : false
Copy link
Member

Choose a reason for hiding this comment

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

so.... this was just... wrong before? a was T["a"] which should have been constrained to number | object | undefined, which means a can be falsey, which means the new result is correct.

Do we fail to include undefined constraints in our analysis somewhere in control flow?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe it's the old issue of not having a good representation for the falsy parts of a higher-order type. For example:

interface Options {
    a: unknown;
    b: unknown;
}

function foo<T extends Options>(a: T['a'], b: T['b']) {
    const x = a && b;  // Type T['b']
}

The type of x really should be T['b'] unioned with the falsy parts of T['a'], but even with negated types this would be a pretty ugly type. IIRC we previously decided to leave this as a known design limitation.

Copy link
Member

Choose a reason for hiding this comment

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

Aw, yeah :(

>(a && b) : T["b"]
>a && b : T["b"]
>a : T["a"]
>!(a && b) : boolean
>(a && b) : T["b"] | undefined
>a && b : T["b"] | undefined
>a : T["a"] | undefined
>b : T["b"]

a;
>a : T["a"]
>a : T["a"] | undefined
}
}
8 changes: 4 additions & 4 deletions tests/baselines/reference/strictNullNotNullIndexTypeNoLib.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ class Test<T extends A> {
this.attrs.params!.name;
>this.attrs.params!.name : any
>this.attrs.params! : T["params"]
>this.attrs.params : T["params"]
>this.attrs.params : T["params"] | undefined
>this.attrs : Readonly<T>
>this : this
>attrs : Readonly<T>
>params : T["params"]
>params : T["params"] | undefined
>name : any
}
}
Expand Down Expand Up @@ -66,10 +66,10 @@ class Test2<T extends A> {

return this.attrs.params!; // Return type should maintain relationship with `T` after being not-null-asserted, ideally
>this.attrs.params! : T["params"]
>this.attrs.params : T["params"]
>this.attrs.params : T["params"] | undefined
>this.attrs : Readonly<T>
>this : this
>attrs : Readonly<T>
>params : T["params"]
>params : T["params"] | undefined
}
}
8 changes: 4 additions & 4 deletions tests/baselines/reference/strictNullNotNullIndexTypeShouldWork.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ class Test<T extends A> {
this.attrs.params!.name;
>this.attrs.params!.name : string
>this.attrs.params! : NonNullable<T["params"]>
>this.attrs.params : T["params"]
>this.attrs.params : T["params"] | undefined
>this.attrs : Readonly<T>
>this : this
>attrs : Readonly<T>
>params : T["params"]
>params : T["params"] | undefined
>name : string
}
}
Expand Down Expand Up @@ -63,10 +63,10 @@ class Test2<T extends A> {

return this.attrs.params!; // Return type should maintain relationship with `T` after being not-null-asserted, ideally
>this.attrs.params! : NonNullable<T["params"]>
>this.attrs.params : T["params"]
>this.attrs.params : T["params"] | undefined
>this.attrs : Readonly<T>
>this : this
>attrs : Readonly<T>
>params : T["params"]
>params : T["params"] | undefined
}
}
6 changes: 3 additions & 3 deletions tests/baselines/reference/typeVariableTypeGuards.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ class A<P extends Partial<Foo>> {
>doSomething : () => void

this.props.foo && this.props.foo()
>this.props.foo && this.props.foo() : void
>this.props.foo : P["foo"]
>this.props.foo && this.props.foo() : void | undefined
>this.props.foo : P["foo"] | undefined
>this.props : Readonly<P>
>this : this
>props : Readonly<P>
>foo : P["foo"]
>foo : P["foo"] | undefined
>this.props.foo() : void
>this.props.foo : () => void
>this.props : Readonly<P>
Expand Down
8 changes: 4 additions & 4 deletions tests/baselines/reference/voidReturnIndexUnionInference.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,19 +43,19 @@ function bad<P extends Props>(props: Readonly<P>) {
safeInvoke(props.onFoo, "blah");
>safeInvoke(props.onFoo, "blah") : boolean | undefined
>safeInvoke : <A1, R>(func: ((arg1: A1) => R) | null | undefined, arg1: A1) => R | undefined
>props.onFoo : P["onFoo"]
>props.onFoo : P["onFoo"] | undefined
>props : Readonly<P>
>onFoo : P["onFoo"]
>onFoo : P["onFoo"] | undefined
>"blah" : "blah"

// ERROR HERE!!!
// Type R in signature of safeInvoke incorrectly inferred as {} instead of void!
safeInvoke(props.onBar, "blah");
>safeInvoke(props.onBar, "blah") : void | undefined
>safeInvoke : <A1, R>(func: ((arg1: A1) => R) | null | undefined, arg1: A1) => R | undefined
>props.onBar : P["onBar"]
>props.onBar : P["onBar"] | undefined
>props : Readonly<P>
>onBar : P["onBar"]
>onBar : P["onBar"] | undefined
>"blah" : "blah"
}