Don’t mix in base constraint completions on indexed access types with type parameter index types

[andrewbranch]

Recall #30507, trying to get completions at a position like:

function f<T extends { x?: string }>(p: T) {}

f({ /**/ });

Since an empty object satisfies the constraint { x?: string }, T is inferred as an empty object, and therefore getting members of the contextual type at that position yields an empty list. So in #32100, #33937, and #34855, we started instantiating f’s call signature with its constraint, getting the type of the argument in that signature, and mixing in properties from that type into the completions list.

Unfortunately, that had some unintended side effects when the signature’s parameter types have a non-linear relationship with a type parameter included in the signature, like conditional types and indexed access types that reference one of the type parameters. In real-world complaints from users, every case I’ve seen has been an indexed access type where the index type is a type parameter:

type Colors = {
  rgb: { r: number, g: number, b: number };
  hsl: { h: number, s: number, l: number }
};

function createColor<T extends keyof Colors>(kind: T, values: Colors[T]) {}

createColor('rgb', {
  /* oh nooo, I get completions for r, g, b, h, s, and l! */
});

In these cases, having more options is wrong and annoying.

This PR special-cases indexed access types where the index type is a type parameter of the contextual-type-providing signature (with some wiggle room to look past unions and intersections and conditional types).

It’s a band-aid fix, but a) I have spent several days trying to think of something better that’s not wildly expensive, and b) it covers the only failure case that people have actually complained about. That said, I don’t love this and am open to suggestions.

Fixes #35702, #36025

[DanielRosenwasser]

@typescript-bot pack this

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 83ce8a4. You can monitor the build here. It should now contribute to this PR's status checks.

[DanielRosenwasser]

It’s a band-aid fix

Not convinced that it is "wrong" to do 😄

[andrewbranch]

Nor am I; I just suspect that a being with 5x my intelligence and 10x my time could probably find a better way.

[weswigham]

Hrmmm, so hear me out here - our primary consideration is contextually typed object literal completions; specifically f({/**/}) when f<T extends {x?: number}>(x: T), while not mucking up f('a', {/**/}) when f<T extends 'a' | 'b'>(x: T, y: { a: {a}, b: {b} }[T]). What if we had a mode where if the instantiated type is a subtype of the uninstantiated apparent type (so in the second case, we'd check if {} is a subtype of {a} | {b}, which it isn't.), we pick the uninstantiated apparent type, otherwise we take the instantiated type? Inference is going to select either an assignable parameter or the base constraint, so if the parameter is assignable, but not a subtype of the base constraint, then it seems reasonable to use the base constraint for completions instead (since the assignable parameter type is likely missing potential members).

[andrewbranch]

@weswigham I looked into either an assignability- or subtype-based approach several months ago, and IIRC, it didn’t work because of how either optional properties or excess properties are treated... but it’s been too long for me to remember specifically why it didn’t pan out.

[weswigham]

Ofc, there's still the possibility of something even more complicated like

f<T extends 'a' | 'b', U extends {a?: string}, V extends {b?: string}>(x: T, y: { a: U, b: V }[T]);

where the desired completions for f('a', {/**/}) is a, from {a?: string} which is neither the inferred type at that position, nor the base constraint of the parameter at that position.

So, thinking out loud here, maybe the problem is attempting to do this with instantiated vs base constraint - with the example above, it seems more like we want to do inference to produce an instantiated signature, but ignoring the parameter we're editing...

[andrewbranch]

I also think that by the nature of completions, you might get too many false negatives to use type relations—e.g., inference picks the best candidate, but you haven’t yet typed some remaining required members, so the object as it stands now is not assignable/subtype of anything relevant.

[andrewbranch]

The best way I’ve been able to think about when you want base constraint completions on an object literal is when the object literal affects the inference of its own type. But, that reasoning may still not be quite right, and moreover I couldn’t find a way to know that.

[andrewbranch]

@typescript-bot pack this

[typescript-bot]

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 28cfb4c. You can monitor the build here. It should now contribute to this PR's status checks.

[weswigham]

@andrewbranch try out my patch/branch here. It seems to do what we want and isn't too jank about it.

[andrewbranch]

@weswigham it doesn’t work when the type parameter has already been inferred because of a different property on the same object argument:

[weswigham]

Hm, yeah, eliding the whole argument while looking for the properties of a nested literal would have that effect. I think the targeted thing we want is eliding just the {} we're looking for the contextual type of from the inference process.

[andrewbranch]

I’m not sure that’s sufficient either. My example is trivially changed to

type X = { a: { a }, b: { b } }

function f<T extends 'a' | 'b'>(p: { kind: T } & X[T]) {}

f({
  kind: "a",
  /**/
})

[weswigham]

@andrewbranch Or rather, we want to block making inferences to the targeted {} as a whole, not to individual members of it.

[weswigham]

@andrewbranch I have a prototype of the idea here. It needs to cache a little bit more safely (probably by cloning node trees like my prior attempt did), but directly forbidding inference from occurring on exactly the target expression seems to do the trick.

[andrewbranch]

@weswigham I tried out that commit. There are a couple failing tests, and I’m a little lost as to what’s happening in inferTypes. The simplest of the failures is this:

interface MyOptions {
    hello?: boolean;
    world?: boolean;
}
declare function bar<T extends MyOptions>(options?: Partial<T>): void;
bar({ hello: true, /*1*/ });

When you have an empty object at the call site bar({ /**/ }), both hello and world are offered 👍. But once the object has either hello or world, no more completions are shown 👎. I debugged these cases, and neither one hits inferTypes when isFromInferenceBlockedSource returns true, so I’m a little confused why one works and the other doesn’t.

There’s a more complicated one with some really strange behavior:

interface Test {
  keyPath?: string;
  autoIncrement?: boolean;
}

function test<T extends Record<string, Test>>(opt: T) { }

test({
  a: {
    keyPath: 'x.y',
    autoIncrement: true
  },
  b: {
    /**/
  }
})

As written, you get no completions inside b. If you comment out a’s contents or the property declaration of a entirely, you still get no completions. But, if you make a’s type an error, by e.g. changing keyPath to a number, completions appear in b. 🤔

[weswigham]

The simplest of the failures is this

I looked over it; it's because we don't do a direct inference from the object literal type to Readonly<T>, since it's a mapped type - we construct a reverse mapped type around the object literal and the infer that to T. We just need to carry the flag from the literal into the reverse-mapped type we construct (or just check for the flag before we even bother making the reverse mapping).

There’s a more complicated one with some really strange behavior:

Ahh, neat; looks like we need to block inference to all object (and array) literals up the spine between the expression under inspection and the surrounding call like expression, too. (Since if we lock in one of their types, we'll lock in the type of the child expression we want to ignore)

I tried out that commit. There are a couple failing tests

I hadn't looked at any yet, since I was just sketching how we might be able to solve the problem, but with the latest revision on that branch, I have, and they now all pass (seems like they just needed the fixes for the two things you brought up, I guess). I've taken the liberty of adding fourslash tests to that branch, too (since debugging a fourslash test is way easier than an editor); which is visible in the branch diff here.

[andrewbranch]

Superceded by #36556