Fixed an issue with contextual type for intersection properties (take 2) #52095
Conversation
From experience with the larger TS community, I can say with some confidence that Also aren't |
One is |
|
@phryneas Hmm, if they behave differently then that makes this rather misleading: |
|
That indeed seems to be a bit misleading. The definition of type Record<K extends keyof any, T> = {
[P in K]: T;
}; |
|
Yeah, I was aware that was the implementation. I just thought that @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. |
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 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 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. |
…ections-here-we-go-again
|
Does this PR create an observable difference between |
|
@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 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:
|
|
For the record we already only support TS 4.2+ as of RTK 1.9. |
|
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 |
…ections-here-we-go-again
…ections-here-we-go-again
|
@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. |
…m concrete props or from applicable index infos
|
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. |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
The break above seems to be related to how contextual
|
typescript-bot
added
the
For Milestone Bug
|
I think I fixed the above in the last commit. The fix is somewhat implicit but at the same time, it feels like there might be other bugs with @jakebailey would you be so kind to rerun the tests? :) |
|
@typescript-bot test it |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
|
@typescript-bot test it |
|
Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! |
|
@ahejlsberg Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
fixes #48812
fixes #55150
fixes #59473
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:
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 uses, 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.
Usually, concrete properties take precedence over index signatures. Through experimentation (and thanks to user/topXXX tests 😉 ), I found out that generic mapped type substitutions are neither. They can resolve to both and while they are generic we can't tell how "important" they are (if they should take precedence over index signatures or not, etc).
With coherent types, concrete properties should always be compatible with index signatures. So if we think about it, even sourcing the contextual property type from all intersection constituents and intersecting those results should be OK. And from that PoV, intersecting those mapped type substitutions with either concrete properties or applicable index infos is still OK (since we should be able to just intersect all 3 categories together).
If we intersect all of the categories we fail this test case with this error. This happens because the index signature there (
{ [k: string]: any; }) spoils the result -anyis contagious. A simple solution to that is to... discardany. it doesn't provide any new information for the contextual type and any type that results from other "hints" will be compatible with it.This particular issue is even manifested in much simpler scenarios, like the ones reported in #59473:
There is an extra complication with the generic mapped type substitutions. At times, a property might be filtered out by a generic
Omitor similar. This isn't a new problem and we can see it at play without intersections here. This example isn't wrong per se since the resulting type is still compatible with the constraint and all. It's just moderately surprising that the contextual parameter type was assigned to this parameter.Previously, this wasn't a problem as
substituteIndexedMappedTypewas not called within intersections. It becomes a bigger problem now since for some properties we shouldn't use indexed mapped type substitutions - they could spoil the overall result if we intersect something that shouldn't be there with some other type that should be there and that was meant to "shadow" over the the one provided by anOmitting mapped type.Negated types aren't a thing in TS, but we can still prove that some properties are filtered out by mapped type constraints that are conditional types with
nevertrue type and properties assignable to theirextendsType.And last but not least, we need to exclude index info types when a concrete property type is found in any constituent. The previous version of the code used resolved structured members so intersections members were resolved and concrete properties were correctly "shadowing over" index infos. Without it, we'd break this:
That's why all of this requires custom intersection handling and a two-pass iteration through those constituents.
TLDR requirements:
substituteIndexedMappedTypeanyhas to be remapped tounknownas otherwise it spoils results givenT & any // anyOmit-like generic mapped types should be skipped when it's provable that a certain property is meant to be omitted by themcc @andrewbranch @RyanCavanaugh @phryneas @markerikson