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

jonhue · 2021-01-17T14:27:24Z

First of all, no program whose type checks were successful before should be affected by this change.
Secondly, an argument as to why soundness is preserved by this change (borrowing types from the original issue):

to check whether {id: T['id'] } <: Query<T> it is enough if {id: T['id']} <: T rather than requiring {id: T['id']} = T
in above step we may need to ensure that T is not a union, I'm interested in your thoughts on that.

jonhue · 2021-01-17T14:32:21Z

tests/cases/compiler/genericMappedTypeAcceptsInferredObjectType.ts

+	optional<T>({ foo }) // no error as { foo: T['foo'] } <: OptionalGenericMap<T>
+	optional<T>({ bar }) // no error as { bar: T['bar'] } <: OptionalGenericMap<T>


Strictly speaking, these two cases aren't in the scope of the original issue. As you're able to see from the baselines, the current behavior here is to throw errors. Intuitively, however, the behavior I described in the comments makes more sense to me.

I would be interested in your thoughts on this matter 🙂
Regarding implementation, I wasn't able to figure out how to check whether a mapped type is optional (includes the ?). Do we have a property that tracks that or should we check whether the value type includes undefined?

The change to allow assignability for the optional case should be okay, but the other changes are not (see other comments).

I think the information about optional mapped types may be attached to the SymbolFlags for symbol of the key, but I'm not sure since I have not looked at that part of the typechecker before.

For these two cases not to result in an error, we would have to something like

isTypeAssignableTo(source, partial(objectTypeConstraint))

instead of isTypeAssignableTo(source, objectTypeConstraint).

In other words, we'd have to check that each property of source is assignable to a corresponding property in objectTypeConstraint, but we do not want to check that each property in objectTypeConstraint also appears in source.

Are we already doing something similar somewhere in the type checker?

I think if you stop overeagerly jumping to the constraint, you find you have to partialize the object type we're comparing against. (To propagate forward the partial modifiers found on the target mapped type itself). Now... actually doing that may turn out to be counterproductive, since then you'd be unwrapping a partial mapped type.... into a partial mapped type; which is just a circular definition, and thus not a useful simplification. So!

Instead of the constraint stuff, I think you'd find it's more useful to break down the source - if every Source["prop"] is related to ObjectType["prop"], then the relation is good. And not just the concrete keys either - each element returned by keyof Source needs to be individually indexed-access'd up and compared, since we have arbitrarily many index signatures nowadays.

Nathan-Fenner · 2021-01-28T18:38:12Z

tests/baselines/reference/genericMappedTypeAcceptsInferredObjectType.errors.txt

+    const optional = <T>(x: OptionalGenericMap<T>) => x
+
+    const withinConstraint = <T extends Constraint>(foo: T['foo']) => {
+    	required<T>({ foo }) // no error as { foo: T['foo'] } <: GenericMap<T>


This is incorrect, tsc should reject this call. T could be {foo: string, bar: number}. Then {foo: string} should definitely not be assignable to {foo: string, bar: number}.

It's not the case that {foo: T['foo'] } is definitely a subtype of GenericMap<T>. It's only definitely a subtype of OptionalGenericMap<T> (that that in TS's type system today; it is possible that this could change in the future).

This was the meaning of my original comment on #37670 ; tsc should reject this code, that's not a defect in tsc.

Only optional calls below are fine.

Yes, you're absolutely right! I did overlook that.

Nathan-Fenner · 2021-01-28T19:04:32Z

tests/baselines/reference/mappedTypeRelationships.errors.txt

@@ -250,8 +241,6 @@ tests/cases/conformance/types/mapped/mappedTypeRelationships.ts(168,5): error TS
    function f41<T extends Thing>(x: Readonly<Thing>, y: Readonly<T>) {
        x = y;
        y = x;  // Error
-        ~
-!!! error TS2322: Type 'Readonly<Thing>' is not assignable to type 'Readonly<T>'.


This error should definitely still be here! We have

type Thing = { a: string, b: string };

so for example, T = { a: "x" | "y" | "z", b: "hello" | "goodbye" } is a subtype of Thing, but you can't allow y = x, since x could be e.g. { a: "foo", b: "bar" } which is definitely not a Readonly<T>.

I think all of the other errors in this file should also stay, but have not looked carefully at them; none of them are using Partial so the same reasoning should apply in all cases: the errors are correct.

yup, thanks for explaining this!

jonhue · 2021-01-29T09:05:42Z

src/compiler/checker.ts

+                            const objectType = (<IndexedAccessType>template).objectType;
+                            if (objectType === source) return Ternary.True;
+                            const objectTypeConstraint = getConstraintOfType(objectType);
+                            if (objectTypeConstraint && isTypeAssignableTo(source, objectTypeConstraint) && target.declaration.questionToken?.kind === SyntaxKind.QuestionToken) return Ternary.True;


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.

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

Nathan-Fenner · 2021-01-29T19:25:50Z

src/compiler/checker.ts

+                        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);


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.

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.

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.

Nathan-Fenner · 2021-01-29T19:27:13Z

tests/baselines/reference/genericMappedTypeAcceptsInferredObjectType.errors.txt

+!!! 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>
+    	            ~~~~~~~


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.

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

jonhue · 2021-02-15T17:00:05Z

@sandersn I think this has to be moved back to needs review :)

jonhue · 2021-03-11T10:34:05Z

tests/baselines/reference/genericMappedTypeAcceptsInferredObjectType.errors.txt

+    	            ~~~~~~~~~~~~
+!!! 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>


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)

dead-claudia · 2021-03-17T03:26:39Z

There is an obvious semantics for keyof T with unions: demote them from primitives to their related record types as applicable and then intersect them. In the world of concrete types, this is how it works, and I would think it to be a bug if generic types are inconsistent with that.

jonhue · 2021-03-17T08:58:59Z

@isiahmeadows I agree with your statement. But does it apply to this specific case where we have keyof T where T is a generic type? Maybe I am missing something though.

dead-claudia · 2021-03-17T20:51:44Z

@jonhue I would also expect the usual application/abstraction identity to hold, too, just like x and (a => a)(x) being equivalent.

// The `U` types below should be equivalent for all `T`
type U = {[P in keyof T]: T[P]}

type F<X> = {[P in keyof X]: X[P]}
type U = F<T>

Of course, when checking type F<X> in isolation, you don't know it's being used that way. But I'd expect it to be validated in potential expectation it could be used that way, for consistency if nothing else.

jonhue · 2021-03-18T11:37:49Z

@isiahmeadows I agree with that too 😄 I do think though that it might be better to track this in a separate pr to keep each individual change as small as possible.

weswigham

I like the idea of fixing this, but this method seems a little incomplete to me. I think something that relies less on the constraint and more on comparing generated indexed accesses derived from keyof the source would be more consistent.

weswigham · 2022-03-04T23:08:32Z

src/compiler/checker.ts

+                            const objectType = (<IndexedAccessType>template).objectType;
+                            if (objectType === source) return Ternary.True;
+                            const objectTypeConstraint = getConstraintOfType(objectType);
+                            if (objectTypeConstraint && isTypeAssignableTo(source, objectTypeConstraint) && target.declaration.questionToken?.kind === SyntaxKind.QuestionToken) return Ternary.True;


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

weswigham · 2022-03-04T23:08:32Z

src/compiler/checker.ts

+                        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);


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.

weswigham · 2022-03-04T23:08:32Z

tests/cases/compiler/genericMappedTypeAcceptsInferredObjectType.ts

+	optional<T>({ foo }) // no error as { foo: T['foo'] } <: OptionalGenericMap<T>
+	optional<T>({ bar }) // no error as { bar: T['bar'] } <: OptionalGenericMap<T>


I think if you stop overeagerly jumping to the constraint, you find you have to partialize the object type we're comparing against. (To propagate forward the partial modifiers found on the target mapped type itself). Now... actually doing that may turn out to be counterproductive, since then you'd be unwrapping a partial mapped type.... into a partial mapped type; which is just a circular definition, and thus not a useful simplification. So!

Instead of the constraint stuff, I think you'd find it's more useful to break down the source - if every Source["prop"] is related to ObjectType["prop"], then the relation is good. And not just the concrete keys either - each element returned by keyof Source needs to be individually indexed-access'd up and compared, since we have arbitrarily many index signatures nowadays.

sandersn · 2022-03-31T23:33:27Z

Do you want to keep working on this? Otherwise I'd like to close it to reduce the number of open PRs.

jonhue · 2022-04-01T05:45:35Z

I don't habe the time at the moment.

jonhue added 3 commits January 16, 2021 22:53

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

b510419

accept baselines

03a33c5

accept baselines of failing tests

5b1570e

typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Jan 17, 2021

jonhue commented Jan 17, 2021

View reviewed changes

sandersn added this to Not started in PR Backlog Jan 25, 2021

jonhue mentioned this pull request Jan 28, 2021

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

Open

Nathan-Fenner suggested changes Jan 28, 2021

View reviewed changes

PR Backlog automation moved this from Not started to Waiting on author Jan 28, 2021

Nathan-Fenner reviewed Jan 28, 2021

View reviewed changes

review

bdf3096

jonhue commented Jan 29, 2021

View reviewed changes

Nathan-Fenner reviewed Jan 29, 2021

View reviewed changes

add negative test case

4977339

sandersn assigned weswigham Mar 10, 2021

sandersn requested review from weswigham and ahejlsberg March 10, 2021 01:11

sandersn moved this from Waiting on author to Needs review in PR Backlog Mar 10, 2021

jonhue commented Mar 11, 2021

View reviewed changes

weswigham requested changes Mar 4, 2022

View reviewed changes

PR Backlog automation moved this from Waiting on reviewers to Waiting on author Mar 4, 2022

jonhue closed this Apr 1, 2022

PR Backlog automation moved this from Waiting on author to Done Apr 1, 2022

jamesrweb mentioned this pull request Nov 24, 2022

Known typescript sub-typing issues P5-wrapper/react#207

Open

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

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

jonhue commented Jan 17, 2021 •

edited

jonhue Jan 17, 2021 •

edited

Nathan-Fenner Jan 28, 2021

jonhue Jan 29, 2021 •

edited

weswigham Mar 4, 2022

Nathan-Fenner Jan 28, 2021

jonhue Jan 29, 2021

Nathan-Fenner Jan 28, 2021

jonhue Jan 29, 2021

jonhue Jan 29, 2021

weswigham Mar 4, 2022

Nathan-Fenner Jan 29, 2021 •

edited

jonhue Jan 30, 2021

weswigham Mar 4, 2022

Nathan-Fenner Jan 29, 2021

jonhue Jan 30, 2021

jonhue commented Feb 15, 2021

jonhue Mar 11, 2021 •

edited

dead-claudia commented Mar 17, 2021

jonhue commented Mar 17, 2021

dead-claudia commented Mar 17, 2021

jonhue commented Mar 18, 2021

weswigham left a comment

weswigham Mar 4, 2022

weswigham Mar 4, 2022

weswigham Mar 4, 2022

sandersn commented Mar 31, 2022

jonhue commented Apr 1, 2022

		optional<T>({ foo }) // no error as { foo: T['foo'] } <: OptionalGenericMap<T>
		optional<T>({ bar }) // no error as { bar: T['bar'] } <: OptionalGenericMap<T>

Mar	APR	May
	02
2022	2023	2024

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

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

Conversation

jonhue commented Jan 17, 2021 • edited

jonhue Jan 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonhue Jan 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nathan-Fenner Jan 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonhue commented Feb 15, 2021

jonhue Mar 11, 2021 • edited

Choose a reason for hiding this comment

dead-claudia commented Mar 17, 2021

jonhue commented Mar 17, 2021

dead-claudia commented Mar 17, 2021

jonhue commented Mar 18, 2021

weswigham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Mar 31, 2022

jonhue commented Apr 1, 2022

jonhue commented Jan 17, 2021 •

edited

jonhue Jan 17, 2021 •

edited

jonhue Jan 29, 2021 •

edited

Nathan-Fenner Jan 29, 2021 •

edited

jonhue Mar 11, 2021 •

edited