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.

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

keyof NonNullable<T> cannot be used to index type T #23368

Closed
ghost opened this issue Apr 12, 2018 · 5 comments · Fixed by #49119
Closed

keyof NonNullable<T> cannot be used to index type T #23368

ghost opened this issue Apr 12, 2018 · 5 comments · Fixed by #49119
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Milestone

Comments

@ghost
Copy link

ghost commented Apr 12, 2018

TypeScript Version: 2.9.0-dev.20180412

Code

function assign<T extends object>(to: T, from: T | undefined) {
	for (const p in from!)
		to[p] = from![p];
	return to;
}

Expected behavior:

No error.

Actual behavior:

src/a.ts(3,9): error TS2536: Type 'keyof NonNullable<T>' cannot be used to index type 'T'.

Related issues: #19461

@mhegazy mhegazy added the Bug A bug in TypeScript label Apr 12, 2018
@mhegazy mhegazy added this to the TypeScript 2.9 milestone Apr 12, 2018
@weswigham
Copy link
Member

Partial dupe of #22934 ? In any case, also should also be "fixed" by #22971

The root cause is still that NonNullable<T> and T are (rightly) unrelated in this direction, but when T extends object, one expects NonNullable<T> to just be T.

@ghost
Copy link
Author

ghost commented Apr 12, 2018

Possibly related:

function f<T extends string | undefined>(t: T): NonNullable<T> {
    if (t === undefined) throw "bad";
    return t;
}

@weswigham
Copy link
Member

@andy-ms I think that's just because narrowing doesn't apply higher order types right now.

@weswigham weswigham added this to Not started in Rolling Work Tracking May 4, 2018
@weswigham weswigham moved this from Not started to In Review in Rolling Work Tracking May 4, 2018
@mhegazy mhegazy modified the milestones: TypeScript 3.0, TypeScript 3.1 Jul 2, 2018
@RyanCavanaugh RyanCavanaugh modified the milestones: TypeScript 3.1, Future Aug 23, 2018
@dhruvrajvanshi
Copy link
Contributor

dhruvrajvanshi commented Apr 30, 2019

Hey everyone. I was trying to figure this issue out and I realized that keyof <Higher order type> reduces to string | number | symbol which is causing this problem.

If this issue is worth fixing, I suggest adding the following rule to the isTypeRelated relation.

keyof X extends keyof Y
when
    Y extends X

This rule should be applied before the rule that reduces keyof Y to string | number | symbol because that loses the type relationship between X and Y

Example of the first rule being applied:

function f<X, Y extends X>(keyofX: keyof X, keyofY: keyof Y) {
    keyofY = keyofX; // fine because keyof X is a subset of keyof Y
    keyofX = keyofY; // error because Y may have more keys than X
}

Right now, an error is raised, but for the wrong reasons.

It tries to assign string | number | symbol to keyofX after reducing keyof Y which is fine, but would miss out on edge cases like this.

The original issue doesn't occur anymore but this does

function f1<T>(keyofT: keyof T, keyofNonNullT: keyof NonNullable<T>) {
	keyofT = keyofNonNullT // should not raise error but does on the latest master
}

EDIT:

Okay, I implemented the new rule but turns out this isn't quite enough to fix the second example because
NonNullable<T> extends T is false. I think this would require another rule for conditional types.

@dhruvrajvanshi
Copy link
Contributor

Update:

I implemented a special case for checks like
(keyof isRelatedTo keyof ( extends ? ... : ...))
right before the checker gives up and reduces the source type to string | number | symbol which works as expected.

Is that kind of logic too much of a special case for the type checker? NonNullable has become kinda pervasive so I don't know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
5 participants