Fixed an issue with contextual type for intersection properties (take 2)

[Andarist]

fixes #48812
fixes #55150

This PR is another take on what already has been approved and merged: #48668 . It later got reverted to give time for further investigation of #49307 since that PR broke Redux Toolkit's types.

This PR adds adjusted test cases that were discovered in #49307. The minimal repro case posted by @andrewbranch was this:

type Validate<T> = T & { [K in keyof T]: object }
declare function f<S, T extends Record<string, (state: S) => any>>(s: S, x: Validate<T>): void;

f(0, {
  foo: s => s + 1, // contextual type for `s` provided, no implicit any reported
})

I decided to treat indexed mapped type substitutions within intersections differently. Now they are intersected with either concrete property types or with applicable index info types.

They are not treated as "concrete" - so they don't take precedence over applicable index info types. They are always used though, if we find any concrete property types then we intersect them with those. If we don't have any concrete property types then we find applicable index info types and we intersect with those.

cc @andrewbranch @RyanCavanaugh @phryneas @markerikson

[fatcerberus]

At the moment RTK uses an index signature for their SliceCaseReducers type (here) and this PR breaks that. @andrewbranch has reasonable doubts (starting from #49307 (comment)) if that should ever work.

From experience with the larger TS community, I can say with some confidence that IndexSignature & SpecificProps is a very common workaround in the wild for #17867 (as a matter of fact, it's the very first reply in that issue!). I have to grit my teeth every time I encounter it and have actively tried to steer people away from it, but most people tend to insist they need it anyway despite the footguns. So any change to the behavior of that pattern is likely to break people, unfortunately.

Also aren't Record<string, T> and { [x: string]: T } the exact same type?

[phryneas]

Also aren't Record<string, T> and { [x: string]: T } the exact same type?

One is { [K in string]: T } and the other is { [x: string]: T } - while they describe the same thing (in the case of the full string type, but not in any other case), they have always behaved differently.

[fatcerberus]

@phryneas Hmm, if they behave differently then that makes this rather misleading:

[phryneas]

That indeed seems to be a bit misleading. The definition of Record definitely uses a mapped type though.

type Record<K extends keyof any, T> = {
    [P in K]: T;
};

[fatcerberus]

Yeah, I was aware that was the implementation. I just thought that { [K in string]: T } automatically reduced to an index signature, thanks mostly to the above tooltip.

@matheusiacono I'm... not sure why you 👎'd my comment? That the hover tip displays an index signature when the actual type behaves differently from what's displayed is factual and not just my opinion.

[Andarist]

From experience with the larger TS community, I can say with some confidence that IndexSignature & SpecificProps is a very common workaround in the wild for #17867 (as a matter of fact, it's the very first reply in that issue!). I have to grit my teeth every time I encounter it and have actively tried to steer people away from it, but most people tend to insist they need it anyway despite the footguns. So any change to the behavior of that pattern is likely to break people, unfortunately.

Note that this PR doesn't necessarily have to break patterns like this. It's hard to tell though - I would have to examine a concrete example, with a concrete use case in mind (contextual typing, completions, etc).

As to the difference between Record<string, T> and { [x: string]: T }. Those are 2 different types, type displays can sometimes be confusing like that - but TS at times tracks how a given type originated and might utilize this information. For example PlainFormFields and MappedFormFields display exactly the same here but yet they behave differently.

It's helpful to check out the TS AST Viewer and the flags contained on those types (link here). Also note that their types have different IDs - which is the ultimate no answer to the question if they are the same.

// flags: Object (2 ^ 19) | DefinitelyNonNullable | StructuredType | StructuredOrInstantiable | ObjectFlagsType | Narrowable | IncludesMask | NotPrimitiveUnion
// objectFlags: Anonymous (2 ^ 4) | ObjectTypeKindMask
type A = { [key: string]: number }
// flags: Object (2 ^ 19) | DefinitelyNonNullable | StructuredType | StructuredOrInstantiable | ObjectFlagsType | Narrowable | IncludesMask | NotPrimitiveUnion
// objectFlags: Mapped (2 ^ 5) | Instantiated (2 ^ 6) | CouldContainTypeVariablesComputed (2 ^ 19) | CouldContainTypeVariables (2 ^ 20) | ObjectTypeKindMask
type B = Record<string, number>
// flags: Object (2 ^ 19) | DefinitelyNonNullable | StructuredType | StructuredOrInstantiable | ObjectFlagsType | Narrowable | IncludesMask | NotPrimitiveUnion
// objectFlags: Mapped (2 ^ 5) | ObjectTypeKindMask
type C = { [K in string]: number }

We might notice here that all of them have the same flags but they are all different when it comes to objectFlags. B and C are roughly the same though, B just have some extra objectFlags since it's coming from an instantiated type alias.

That being said - I can't be sure if the proposed change is how this whole fix should look like. It's a conversation starter, maybe I will have to tweak this based on the TS team feedback.

[andrewbranch]

Does this PR create an observable difference between Record<string, T> and { [x: string]: T } or did you just discover an existing one that lets you work around the other effects of the PR?

[Andarist]

@andrewbranch it creates an observable difference in getting the type of the property of a contextual type. Some of the the new test cases wouldn't infer correctly with the generic constraint of { [x: string]: T } but they infer OK with Record<string, T>. You could change the definition of this SliceCaseReducers back and forth to observe the difference

However, I'm not saying - by any means - that this is what I want to land. This whole PR is meant to restart the conversation around this since the original PR landed and got reverted later.

When discussing the regression in #49307, both you and @RyanCavanaugh said that the original PR had merit and that contextual properties coming from index signatures are dubious (when a concrete property is available) and that cases like this were handled inconsistently by TS. The original PR was reverted to give you more time to think about this - it has been half a year and I just hope to revive this conversation.

There are some important things to note down here - we don't need to create any observable difference between both of those. This is not required to fix what this PR is fixing.

So to move this PR forward we can either:

1. ignore index signatures in presence of concrete properties. This would likely reintroduce the regression from Nightly bug(?) with infering function generic while being part of intersection #49307. It's even possible that wouldn't necessarily be a bad thing (I'm not super clear on that though) but it's unknown to me how RTK could rewrite their types today to fix their types. I proposed a solution here but it doesn't work in their repo - so there is more to it (the failed PR can be found here). It also doesn't necessarily mean that their current types shouldn't work - they are certainly unusual but maybe they don't abuse the implementation and are aligned with TS design. That's something that the TS team would have to decide on.
2. never ignore those index signatures. This PR would still fix what it is fixing. I just didn't do it because you had doubts about index signatures behavior in Nightly bug(?) with infering function generic while being part of intersection #49307
3. use a mixed approach - but I'm not sure what exact heuristics should be used. Perhaps we should check if the contextual type is part of the generic constraint? If it is - then it would make sense to use its "index signature", when the contextual type would be concrete then the index signature could be ignored. I'm not sure what the exact condition for such a check would be preferred but I could experiment with something if you decide that this is what your team wants to have implemented.

[markerikson]

For the record we already only support TS 4.2+ as of RTK 1.9.

[Andarist]

Your CI job just reported that the proposed solution doesn't work for you 🤣 it seems that there is more to this there than what was captured by the slimmed-down repro

[Andarist]

@andrewbranch @RyanCavanaugh would there be a chance to put this on 5.2 agenda since the work on that has just started? I would gladly tweak the content of this PR however you see fit - but right now it's not entirely obvious how this should behave when index signatures are involved. This probably warrants a design meeting or something.

[Andarist]

To get the ball rolling I made some changes to this PR. I decided to treat indexed mapped type substitutions within intersections differently. Now they are intersected with either concrete property types or with applicable index info types.

They are not treated as "concrete" - so they don't take precedence over applicable index info types. They are always used though, if we find any concrete property types then we intersect them with those. If we don't have any concrete property types then we find applicable index info types and we intersect with those.

It is somewhat strange... but it maintains backward compatibility.

[Andarist]

@ahejlsberg @gabritto friendly 🏓

[Andarist]

@jakebailey would u mind creating a playground for this one?

[jakebailey]

@typescript-bot pack this

[typescript-bot]

Heya @jakebailey, I've started to run the tarball bundle task on this PR at 6d24ca8. You can monitor the build here.

[typescript-bot]

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/156012/artifacts?artifactName=tgz&fileId=0273E254677DDA1973EF2BA249B11A93323D44EF57789F29D2E3B03C76C9E48C02&fileName=/typescript-5.2.0-insiders.20230725.tgz"
    }
}

and then running npm install.

[jakebailey]

@typescript-bot pack this

[typescript-bot]

Heya @jakebailey, I've started to run the tarball bundle task on this PR at ea31c96. You can monitor the build here.

[typescript-bot]

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/156035/artifacts?artifactName=tgz&fileId=034BB1BC0058CA3DCAE8F8E246E1A078C04F7BB57850FA2237319F51889C9BB002&fileName=/typescript-5.2.0-insiders.20230726.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.2.0-pr-52095-19".;

[Andarist]

@ahejlsberg @gabritto friendly 🏓

[Andarist]

@ahejlsberg @gabritto @RyanCavanaugh any chance that this could be considered for 5.5? It fixes one of the "Help wanted" issues - let me help you! 😉