Fixed const reverse mapped types themselves to be treated as const #55794
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Andarist
commented
Sep 20, 2023
| // @strict: true | ||
| // @noEmit: true | ||
|
|
||
| declare function test1<const T>(obj: { |
There was a problem hiding this comment.
The first 3 test cases are testing objects and how readonly, -readonly, and lack of them impact the T and the fresh object that is created by mapping over T.
Those objects today would be constified for the most part but their properties wouldn't always be marked as readonly when they should be. When readonly modifier is used on the mapped type then that result (typeof obj) has readonly properties but the const type parameter itself doesn't have them. This is consistent with #12589 . You might also want to recheck when/how readonly is preserved/stripped on members of the reverse mapped type here
Andarist
commented
Sep 20, 2023
| [K in keyof T]: T[K]; | ||
| }): T; | ||
|
|
||
| const result4 = test4(["1", 2]); |
There was a problem hiding this comment.
This is an especially important part of the fix. Currently T is inferred as (2 | "1")[] but it really should be readonly ["1", 2]
Andarist
force-pushed
the
fix/const-reverse-mapped-types
branch
from
1dbd5f3 to
d6d1604
Compare
Andarist
commented
Sep 21, 2023
There was a problem hiding this comment.
There are also some other test cases that I'd like to add here but I'm not entirely sure what should be the expected results for them
const T, P extends keyof T- this might iterate overPwhich might be the subset ofkeyof T. Should we still assign constness toTin such a case? See the playground TS playground. Note that it's not really possible to gatherPacross from different mapped types and somehow different mapped type to build up a "combined" reverse mapped type: TS playgroundK in keyof T | "extra"- what should happen with a non-homomorphic case like this? TS playground
There was a problem hiding this comment.
- Probably? I'm not a huge fan of overuse of
consttype variables, but if they're in-use, there a strong signal that const-ness is desired somewhere related to that type parameter. The workflow is usually "does this do what I want? No? Add aconst.", so theconstnormally only gets added by people if it's meant to be meaningful to inference. - Eh, I'd leave that as whatever it falls out as from being not homomorphic. It'll get unified if and when we ever adjust the logic to consider something like that as homomorphic-enough to get the homomorphic type variable treatment.
This was referenced Sep 21, 2023
weswigham
approved these changes
Oct 4, 2023
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the diff-based top-repos suite on this PR at d6d1604. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at d6d1604. 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 d6d1604. 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. |
sandersn
added
the
For Uncommitted Bug
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.