Improve generic keyof relationship checking for generic conditionals with keyless branch types

[weswigham]

Originally from #49091. This essentially applies keyof to the constraint of a conditional in a deferred way, ignoring branches which don't contribute to the final result set of keys. This allows, eg, keyof (T extends null ? never : T) to be related to keyof T. In addition, intersection members for whom keyof returns never are omitted from the relationship check, allowing a deferred effective keyof (T & object) to be related to keyof T (which already works when not indirected via a conditional via construction).

So in total, given a type like

type FilterNull<T> = T extends null ? never : T;

then keyof FilterNull<(T & object) | null> is now able to be related to keyof T.

Practically, I initially looked into this to support using the conditional-based NonNullable in more places, but now that it's just an intersection, this is moreso just for users using keyof over their own filtering conditionals (as is common in react libraries nowadays).

[sandersn]

Can you give an example of how a particular react libraries would benefit from this? A concrete example of usage would help me understand how widely applicable it might be.

[ahejlsberg]

Hmm, I'm not sure these changes are desirable. See the discussion here. We effectively rely on the fact that keyof T resolves to never for any T that includes null or undefined.

[weswigham]

I dunno, all the cases in the test in this PR seem pretty desirable...

[weswigham]

(also that's unchanged by this PR?)

[ahejlsberg]

With this PR there is no error in the example below where previously there was. The error is desirable because it protects us from accessing through a possibly-null object without requiring local proof that the object is non-null. The thing that saves us is that keyof T for a T that includes null or undefined is never, but it isn't with this PR.

type NonNull<T> = T extends null ? never : T;

function foo<T>(t: T, k: keyof NonNull<T>) {
    t[k];  // Was error, now isn't
}

foo(null, 'foo');

[weswigham]

If that should be invalid, it should be because T is possibly undefined, not because of anything else - keyof NonNull<T> is legitimately always a safe key to index T with, if T isn't undefined. If we want to catch stuff like that, it should be a simple check to add - we just need to add an error if the thing we're indexing is possibly undefined.

[ahejlsberg]

If we want to catch stuff like that, it should be a simple check to add - we just need to add an error if the thing we're indexing is possibly undefined.

I'm quite certain that would be a severe breaking change, and it would force checks that are unnecessary because we're already protecting you by having keyof T be never when T is possibly null or undefined.

[weswigham]

we're already protecting you by having keyof T be never when T is possibly null or undefined.

Except it's not never is it? It's an unresolved generic. Certainly when I write keyof T it doesn't become never. It's related like the useful keyof T it is everywhere, except, maybe, in some specific instance where it's falling back to the constraint incorrectly? (Because our constraint for generic keyof is a least bound, not a constraint in the same sense as other types.) Certainly if it was just never then two unrelated keyof T and keyof U would be equivalent, which is patently false.

Point being we can clearly do better here. Using one problem to excuse another isn't a great place to be - both probably need fixing.

[sandersn]

@weswigham do you want to keep working on this? Is there a reasonable path to fixing the keyof T-where-T-contains-undefined problem? I believe Anders' recent PR drops the undefined in more locations, but I'm not sure it gets dropped in all of them.

[sandersn]

It's been almost 3 years since the last change to this PR. I'm going to close it for now and we can re-open it if it's worth working on before the Strada code is decommissioned.