Skip to content

[P in keyof T]: T[P] not accepting inferred base type via extends #42382

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

Closed
jonhue wants to merge 5 commits into from
Closed

[P in keyof T]: T[P] not accepting inferred base type via extends #42382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b510419
[P in keyof T]: T[P] not accepting inferred base type via extends
jonhue Jan 16, 2021
03a33c5
accept baselines
jonhue Jan 16, 2021
5b1570e
accept baselines of failing tests
jonhue Jan 17, 2021
bdf3096
review
jonhue Jan 29, 2021
4977339
add negative test case
jonhue Jan 30, 2021
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
8 changes: 5 additions & 3 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17883,9 +17883,11 @@ namespace ts {
const template = getTemplateTypeFromMappedType(target);
const modifiers = getMappedTypeModifiers(target);
if (!(modifiers & MappedTypeModifiers.ExcludeOptional)) {
if (template.flags & TypeFlags.IndexedAccess && (<IndexedAccessType>template).objectType === source &&
(<IndexedAccessType>template).indexType === getTypeParameterFromMappedType(target)) {
return Ternary.True;
if (template.flags & TypeFlags.IndexedAccess && (<IndexedAccessType>template).indexType === getTypeParameterFromMappedType(target)) {
const objectType = (<IndexedAccessType>template).objectType;
if (objectType === source) return Ternary.True;
const objectTypeConstraint = getConstraintOfType(objectType);
Copy link
Contributor

@Nathan-Fenner Nathan-Fenner Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think you don't want to look at the target's constraints at all - knowing that e.g. T is a subtype of {x: string} doesn't help you assign anything to a T, since T could have stricter requirements (like x: "yes" | "no").

We don't actually care about any of the constraint's attributes/properties for the assignability relation. The only reason we even need the T extends { foo: string } constraint is so that it's possible to write T["foo"] in the signature, which is handled elsewhere. So I think the correct/complete version of this code won't look at the constraint type of the target at all.

Note, here's an example of a function that is currently rejected, and should continue to be rejected after this change. It doesn't have a extends clause on T, but that's not the reason for it:

function shouldReject<T, K extends keyof T>(key: K, v: T[K]): {[k in keyof T]?: T[k]} {
	return { [key]: v }
    // Type '{ [x: string]: T[K]; }' is not assignable to type '{ [k in keyof T]?: T[k] | undefined; }'.(2322)
}

in this case, we don't know whether K is just one value, or a union of possible values. For example, if we called shouldReject<{foo: string, bar: number, qux: boolean}, "foo" | "bar"> then T[K] is string | number, which means that we could pass in "(foo", 123) and produce a value that's clearly not a Partial<T>.

I actually proposed #41363 a while ago to deal with this; the correct typing for the function would be T[assign K] instead of T[K] in the argument, but that's clearly beyond the scope of this PR, it's just vaguely relevant.

Copy link
Contributor Author

@jonhue jonhue Jan 30, 2021

Choose a reason for hiding this comment

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

I agree with the more general point that it is rarely useful to look at the constraints of types when determining whether one is an instance of the other. I do think, however, that this is one of the rare cases where it actually is useful.
I completely agree with all the other points you're making 🙂
I also added your example to the test, although I don' think it will be immediately impacted by the change.

Copy link
Member

@weswigham weswigham Mar 4, 2022

Choose a reason for hiding this comment

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

I'd rather just use the objectType directly and let relationship checking break down the type to its constraints over the course of it's usual decomposition paths.

if (objectTypeConstraint && isTypeAssignableTo(source, objectTypeConstraint) && target.declaration.questionToken?.kind === SyntaxKind.QuestionToken) return Ternary.True;
Copy link
Contributor Author

@jonhue jonhue Jan 29, 2021

Choose a reason for hiding this comment

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

I'm sure there is a nicer (and safer!) way of determining whether the properties of a mapped type are optional. Right now I'm looking at the syntax node. But this approach does not work with +? for example.

Copy link
Member

@weswigham weswigham Mar 4, 2022

Choose a reason for hiding this comment

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

getMappedTypeModifiers as called just above here. The modifiers variable already in-scope should have them.

}
if (!isGenericMappedType(source)) {
const targetConstraint = getConstraintTypeFromMappedType(target);
Expand Down
72 changes: 72 additions & 0 deletions tests/baselines/reference/genericMappedTypeAcceptsInferredObjectType.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
tests/cases/compiler/genericMappedTypeAcceptsInferredObjectType.ts(22,14): error TS2345: Argument of type '{ foo: T["foo"]; }' is not assignable to parameter of type 'GenericMap<T>'.
tests/cases/compiler/genericMappedTypeAcceptsInferredObjectType.ts(28,31): error TS2345: Argument of type '{ foo: T["foo"]; }' is not assignable to parameter of type 'GenericMap<ExtendedConstraint>'.
Property 'bar' is missing in type '{ foo: T["foo"]; }' but required in type 'GenericMap<ExtendedConstraint>'.
tests/cases/compiler/genericMappedTypeAcceptsInferredObjectType.ts(29,31): error TS2345: Argument of type '{ bar: T["bar"]; }' is not assignable to parameter of type 'GenericMap<ExtendedConstraint>'.
Property 'foo' is missing in type '{ bar: T["bar"]; }' but required in type 'GenericMap<ExtendedConstraint>'.
tests/cases/compiler/genericMappedTypeAcceptsInferredObjectType.ts(31,14): error TS2345: Argument of type '{ foo: T["foo"]; bar: T["bar"]; }' is not assignable to parameter of type 'GenericMap<T>'.
tests/cases/compiler/genericMappedTypeAcceptsInferredObjectType.ts(33,14): error TS2345: Argument of type '{ foo: T["foo"]; }' is not assignable to parameter of type 'OptionalGenericMap<T>'.
tests/cases/compiler/genericMappedTypeAcceptsInferredObjectType.ts(34,14): error TS2345: Argument of type '{ bar: T["bar"]; }' is not assignable to parameter of type 'OptionalGenericMap<T>'.
tests/cases/compiler/genericMappedTypeAcceptsInferredObjectType.ts(39,2): error TS2322: Type '{ [x: string]: T[K]; }' is not assignable to type '{ [k in keyof T]?: T[k]; }'.


==== tests/cases/compiler/genericMappedTypeAcceptsInferredObjectType.ts (7 errors) ====
interface Constraint {
foo: string
}

interface ExtendedConstraint extends Constraint {
bar: string
}

type GenericMap<T> = {
[P in keyof T]: T[P]
}

type OptionalGenericMap<T> = {
[P in keyof T]?: T[P]
}

const required = <T>(x: GenericMap<T>) => x
const optional = <T>(x: OptionalGenericMap<T>) => x

const withinConstraint = <T extends Constraint>(foo: T['foo']) => {
required<Constraint>({ foo }) // no error as { foo: T['foo'] } <: GenericMap<Constraint>
required<T>({ foo }) // error as { foo: T['foo'] } /<: GenericMap<T> because other properties may be missing
~~~~~~~
!!! error TS2345: Argument of type '{ foo: T["foo"]; }' is not assignable to parameter of type 'GenericMap<T>'.
optional<T>({}) // no error as {} <: OptionalGenericMap<Constraint>
optional<T>({ foo }) // no error as { foo: T['foo'] } <: OptionalGenericMap<T>
}

const withinExtendedConstraint = <T extends ExtendedConstraint>(foo: T['foo'], bar: T['bar']) => {
required<ExtendedConstraint>({ foo }) // error as { foo: T['foo'] } /<: GenericMap<ExtendedConstraint> because bar is missing
~~~~~~~
!!! error TS2345: Argument of type '{ foo: T["foo"]; }' is not assignable to parameter of type 'GenericMap<ExtendedConstraint>'.
!!! error TS2345: Property 'bar' is missing in type '{ foo: T["foo"]; }' but required in type 'GenericMap<ExtendedConstraint>'.
!!! related TS2728 tests/cases/compiler/genericMappedTypeAcceptsInferredObjectType.ts:6:2: 'bar' is declared here.
required<ExtendedConstraint>({ bar }) // error as { bar: T['bar'] } /<: GenericMap<ExtendedConstraint> because foo is missing
~~~~~~~
!!! error TS2345: Argument of type '{ bar: T["bar"]; }' is not assignable to parameter of type 'GenericMap<ExtendedConstraint>'.
!!! error TS2345: Property 'foo' is missing in type '{ bar: T["bar"]; }' but required in type 'GenericMap<ExtendedConstraint>'.
!!! related TS2728 tests/cases/compiler/genericMappedTypeAcceptsInferredObjectType.ts:2:2: 'foo' is declared here.
required<ExtendedConstraint>({ foo, bar }) // no error as { foo: T['foo'], bar: T['bar'] } <: GenericMap<ExtendedConstraint>
required<T>({ foo, bar }) // error as { foo: T['foo'], bar: T['bar'] } /<: GenericMap<T> because other properties may be missing
~~~~~~~~~~~~
!!! error TS2345: Argument of type '{ foo: T["foo"]; bar: T["bar"]; }' is not assignable to parameter of type 'GenericMap<T>'.
optional<T>({}) // no error as {} <: OptionalGenericMap<T>
optional<T>({ foo }) // no error as { foo: T['foo'] } <: OptionalGenericMap<T>
Copy link
Contributor Author

@jonhue jonhue Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe another hint for the reviewers because the threads might look a little convoluted: I would consider this line not erroring a nice to have but not a necessity to fix the referenced issue. So it is probably best to leave this unaddressed here and improve this behavior further in a follow-up. 🙂

Here are my thoughts on this matter up to this point: #42382 (comment)

~~~~~~~
Copy link
Contributor

@Nathan-Fenner Nathan-Fenner Jan 29, 2021

Choose a reason for hiding this comment

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

Looks like this case is not handled yet, even though it should be. I think it's because you're currently relying on the "constraint" type to do too much work; you should not need to look at the constraint for the target at all, it's rarely useful for assignability to a generic type.

Copy link
Contributor Author

@jonhue jonhue Jan 30, 2021

Choose a reason for hiding this comment

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

Yeah, that's what I mentioned in #42382 (comment).

!!! error TS2345: Argument of type '{ foo: T["foo"]; }' is not assignable to parameter of type 'OptionalGenericMap<T>'.
optional<T>({ bar }) // no error as { bar: T['bar'] } <: OptionalGenericMap<T>
~~~~~~~
!!! error TS2345: Argument of type '{ bar: T["bar"]; }' is not assignable to parameter of type 'OptionalGenericMap<T>'.
optional<T>({ foo, bar }) // no error as { foo: T['foo'], bar: T['bar'] } <: OptionalGenericMap<T>
}

function shouldReject<T, K extends keyof T>(key: K, v: T[K]): {[k in keyof T]?: T[k]} {
return { [key]: v }
~~~~~~~~~~~~~~~~~~~
!!! error TS2322: Type '{ [x: string]: T[K]; }' is not assignable to type '{ [k in keyof T]?: T[k]; }'.
// Type '{ [x: string]: T[K]; }' is not assignable to type '{ [k in keyof T]?: T[k] | undefined; }'.(2322)
}

68 changes: 68 additions & 0 deletions tests/baselines/reference/genericMappedTypeAcceptsInferredObjectType.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
//// [genericMappedTypeAcceptsInferredObjectType.ts]
interface Constraint {
foo: string
}

interface ExtendedConstraint extends Constraint {
bar: string
}

type GenericMap<T> = {
[P in keyof T]: T[P]
}

type OptionalGenericMap<T> = {
[P in keyof T]?: T[P]
}

const required = <T>(x: GenericMap<T>) => x
const optional = <T>(x: OptionalGenericMap<T>) => x

const withinConstraint = <T extends Constraint>(foo: T['foo']) => {
required<Constraint>({ foo }) // no error as { foo: T['foo'] } <: GenericMap<Constraint>
required<T>({ foo }) // error as { foo: T['foo'] } /<: GenericMap<T> because other properties may be missing
optional<T>({}) // no error as {} <: OptionalGenericMap<Constraint>
optional<T>({ foo }) // no error as { foo: T['foo'] } <: OptionalGenericMap<T>
}

const withinExtendedConstraint = <T extends ExtendedConstraint>(foo: T['foo'], bar: T['bar']) => {
required<ExtendedConstraint>({ foo }) // error as { foo: T['foo'] } /<: GenericMap<ExtendedConstraint> because bar is missing
required<ExtendedConstraint>({ bar }) // error as { bar: T['bar'] } /<: GenericMap<ExtendedConstraint> because foo is missing
required<ExtendedConstraint>({ foo, bar }) // no error as { foo: T['foo'], bar: T['bar'] } <: GenericMap<ExtendedConstraint>
required<T>({ foo, bar }) // error as { foo: T['foo'], bar: T['bar'] } /<: GenericMap<T> because other properties may be missing
optional<T>({}) // no error as {} <: OptionalGenericMap<T>
optional<T>({ foo }) // no error as { foo: T['foo'] } <: OptionalGenericMap<T>
optional<T>({ bar }) // no error as { bar: T['bar'] } <: OptionalGenericMap<T>
optional<T>({ foo, bar }) // no error as { foo: T['foo'], bar: T['bar'] } <: OptionalGenericMap<T>
}

function shouldReject<T, K extends keyof T>(key: K, v: T[K]): {[k in keyof T]?: T[k]} {
return { [key]: v }
// Type '{ [x: string]: T[K]; }' is not assignable to type '{ [k in keyof T]?: T[k] | undefined; }'.(2322)
}


//// [genericMappedTypeAcceptsInferredObjectType.js]
var required = function (x) { return x; };
var optional = function (x) { return x; };
var withinConstraint = function (foo) {
required({ foo: foo }); // no error as { foo: T['foo'] } <: GenericMap<Constraint>
required({ foo: foo }); // error as { foo: T['foo'] } /<: GenericMap<T> because other properties may be missing
optional({}); // no error as {} <: OptionalGenericMap<Constraint>
optional({ foo: foo }); // no error as { foo: T['foo'] } <: OptionalGenericMap<T>
};
var withinExtendedConstraint = function (foo, bar) {
required({ foo: foo }); // error as { foo: T['foo'] } /<: GenericMap<ExtendedConstraint> because bar is missing
required({ bar: bar }); // error as { bar: T['bar'] } /<: GenericMap<ExtendedConstraint> because foo is missing
required({ foo: foo, bar: bar }); // no error as { foo: T['foo'], bar: T['bar'] } <: GenericMap<ExtendedConstraint>
required({ foo: foo, bar: bar }); // error as { foo: T['foo'], bar: T['bar'] } /<: GenericMap<T> because other properties may be missing
optional({}); // no error as {} <: OptionalGenericMap<T>
optional({ foo: foo }); // no error as { foo: T['foo'] } <: OptionalGenericMap<T>
optional({ bar: bar }); // no error as { bar: T['bar'] } <: OptionalGenericMap<T>
optional({ foo: foo, bar: bar }); // no error as { foo: T['foo'], bar: T['bar'] } <: OptionalGenericMap<T>
};
function shouldReject(key, v) {
var _a;
return _a = {}, _a[key] = v, _a;
// Type '{ [x: string]: T[K]; }' is not assignable to type '{ [k in keyof T]?: T[k] | undefined; }'.(2322)
}
Loading