-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Reverse mapped types with intersection constraint #55811
Reverse mapped types with intersection constraint #55811
Conversation
This PR doesn"t have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
@microsoft-github-policy-service agree |
@jfet97 can you open a new bug with your proposal where we can discuss it? For complex issues like this the PR itself is usually pretty short, but if it sits around for long the code gets stale. |
This looks pretty good, but since it"s definitely pretty high on the break risk scale (since it"s intentionally adding new errors), let me run the extended tests and say that we"ll probably want to merge this after we cut the 5.3 rc, so it can be in the nightly for awhile. @typescript-bot test this |
Heya @weswigham, I"ve started to run the parallelized Definitely Typed test suite on this PR at d3d7a45. You can monitor the build here. Update: The results are in! |
Heya @weswigham, I"ve started to run the regular perf test suite on this PR at d3d7a45. You can monitor the build here. Update: The results are in! |
Heya @weswigham, I"ve started to run the diff-based top-repos suite on this PR at d3d7a45. You can monitor the build here. Update: The results are in! |
@weswigham Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@weswigham Here are the results of running the top-repos suite comparing Everything looks good! |
Hey @weswigham, the results of running the DT tests are ready. |
@weswigham thanks for your review, I"m glad you liked it! To be honest I didn"t expect any particular problems because:
I believe the tests not finding any errors supports my opinion, but there is no rush to add this feature of course. I"m just sharing my thoughts. |
src/compiler/checker.ts
Outdated
@@ -13523,14 +13523,34 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
return instantiateType(instantiable, createTypeMapper([type.indexType, type.objectType], [getNumberLiteralType(0), createTupleType([replacement])])); | |||
} | |||
|
|||
function getLimitedConstraint(type: ReverseMappedType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I"d really like a comment explaining a "limited constraint" is, maybe with an example or two. Otherwise LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments have been added. I hope it"s clearer now.
@jakebailey would you be so kind to create a TS playground for this one? :) |
@typescript-bot pack this |
Heya @jakebailey, I"ve started to run the tarball bundle task on this PR at 21fae9e. You can monitor the build here. |
Hey @jakebailey, I"ve packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
Another example can be found on this playground by @Andarist , to be compared with the 5.3.2 one. The latter fails to infer completely in the first scenario. |
TLDR
This PR enhances reverse mapped types capabilities providing users with the ability to enable EPC (Excess Property Checking) on type parameters inference sites.
Background
This PR has come up trying to find a way to solve some problems discussed in #55645 and #55644, which are a simplification of what"s going on in this PR from XState. In those cases the usual type parameters inference behaviour is not enough, mainly because it disables EPC (Excess Property Checking).
FIxes #55927
Idea
Reverse mapped types are a powerful tool. Currently TypeScript is able to reverse three kind of mapped types:
{ [P in keyof T]: X }
{ [P in K]: X }
, whereK
is a type parameterThis PR adds a fourth case: a mapped type with an intersection constraint, as long as the intersection contains one of the constraints of the previous cases.
While the union constraint could be seen as a way to force the presence of some properties, the intersection one could be seen as a way to prevent the presence of extra properties.
Examples
Example N. 1
Here the type parameter
T
is now inferred, by reversing the mapped type, as{ bar: 1; baz: "a"; record: { a: number; b: number } }
, instead of falling back to its constraintFoo
. The properties of the parameters
are constrained to be exactly the ones already declared inFoo
, so EPC comes into play if the argument contains extra properties:In this way you get both inference and EPC check!
Note that the
extra
property is stripped away fromT
because it wouldn"t survive the action of the mapped type anyway.Example N. 2
In the following example, inspired from #12936, is the type parameter
U
the one that gets inferred by reversing the mapped type. It is inferred as{ x: 1; y: "y"; record: { a: number; b: number } }
instead of falling back to its constraint. As before, no extra properties are allowed so we get an EPC error onz
that will always be stripped away from the inferred type:Tests
Two tests files have been added:
reverseMappedTypeIntersectionConstraint
, in which the main capabilities brought by this PR have been tested, although I must admit I don"t think there are enough test casesreverseMappedTypeLimitedConstraint
, in which the stripping behaviour on the inferred type parameters has been written down, but it is somehow subsumed by the previous test and for this reason it might be deleted altogetherA note on EPC
What"s good about this PR is the fact that it lets the user enable EPC on type parameters without fundamentally change how TS treats type parameters inference. It"s just a nice side effect of enhancing reverse mapped types" capabilities.
Remarks
This PR may help with some cases presented in #12936 as well.
#55794 could help improving what happens when a
const
type parameter is used, because there are some inconsistencies withreadonly
modifiers.Acknowledgements
Huge thanks to @Andarist for always having my back 😄.