Skip to content

Improve apparent type of mapped types with mapped type modifiers #58091

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

Closed

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Apr 5, 2024

fixes #58060

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Apr 5, 2024
const target = (type.target ?? type) as MappedType;
const typeVariable = getHomomorphicTypeVariable(target);
if (typeVariable && !target.declaration.nameType) {
const constraint = getConstraintTypeFromMappedType(type);
if (constraint.flags & TypeFlags.Index) {
const baseConstraint = getBaseConstraintOfType((constraint as IndexType).type);
if (baseConstraint && everyType(baseConstraint, t => isArrayOrTupleType(t) || isArrayOrTupleOrIntersection(t))) {
return instantiateType(target, prependTypeMapping(typeVariable, baseConstraint, type.mapper));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type.mapper here looks something like:

m1: T_id -> Box<string>[]
m2: m1: K -> K
    m2: m1: T_id -> UnboxArray<T_outer>
        m2: T_id -> UnboxArray<T_outer>

Since typeVariable is T_id TS ended up "skipping" over the inner mapping of UnboxArray. The constraint (of the type) is keyof T_outer with a baseConstraint of Box<string>[].

In a sense, I'd like to map T_outer to baseConstraint (Box<string>[]), process that through type.mapper and then map T_id to that result (or smth along those lines). I couldn't find a clear way to do that given that we need to account for non-homomorphic instantiations of homomorphic mapped types here. Using the modifiers type does seem to do the trick but I'm not overly confident in this solution and I won't mind being proven that this is wrong 😉

@jakebailey
Copy link
Member

@typescript-bot test it

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 5, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
test top400 ✅ Started ✅ Results
user test this ✅ Started ✅ Results
run dt ✅ Started ✅ Results
perf test this faster ✅ Started 👀 Results

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user tests comparing main and refs/pull/58091/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

@jakebailey
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Angular - node (v18.15.0, x64)
Memory used 295,703k (± 0.01%) 295,694k (± 0.01%) ~ 295,661k 295,714k p=0.298 n=6
Parse Time 3.92s (± 0.63%) 3.92s (± 0.49%) ~ 3.90s 3.95s p=0.808 n=6
Bind Time 1.20s (± 0.63%) 1.20s (± 1.14%) ~ 1.18s 1.22s p=0.672 n=6
Check Time 11.87s (± 0.38%) 11.89s (± 0.51%) ~ 11.77s 11.93s p=0.295 n=6
Emit Time 10.46s (± 0.29%) 10.47s (± 0.40%) ~ 10.43s 10.54s p=0.747 n=6
Total Time 27.45s (± 0.25%) 27.48s (± 0.35%) ~ 27.33s 27.59s p=0.375 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 193,505k (± 0.99%) 192,255k (± 0.75%) ~ 191,629k 195,194k p=0.128 n=6
Parse Time 1.36s (± 1.20%) 1.36s (± 0.93%) ~ 1.35s 1.38s p=1.000 n=6
Bind Time 0.72s (± 0.00%) 0.72s (± 0.57%) ~ 0.71s 0.72s p=0.405 n=6
Check Time 9.38s (± 0.38%) 9.40s (± 0.26%) ~ 9.37s 9.43s p=0.145 n=6
Emit Time 2.62s (± 0.71%) 2.61s (± 0.51%) ~ 2.59s 2.63s p=0.806 n=6
Total Time 14.06s (± 0.26%) 14.10s (± 0.25%) ~ 14.04s 14.13s p=0.146 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,684k (± 0.00%) 347,670k (± 0.00%) -14k (- 0.00%) 347,656k 347,683k p=0.013 n=6
Parse Time 2.48s (± 0.54%) 2.49s (± 0.33%) ~ 2.48s 2.50s p=0.546 n=6
Bind Time 0.89s (± 0.00%) 0.89s (± 0.71%) ~ 0.88s 0.90s p=1.000 n=6
Check Time 6.94s (± 0.42%) 6.95s (± 0.40%) ~ 6.92s 6.99s p=0.871 n=6
Emit Time 4.07s (± 0.25%) 4.06s (± 0.35%) ~ 4.04s 4.08s p=0.410 n=6
Total Time 14.38s (± 0.24%) 14.39s (± 0.16%) ~ 14.37s 14.42s p=0.462 n=6
TFS - node (v18.15.0, x64)
Memory used 302,547k (± 0.01%) 302,541k (± 0.01%) ~ 302,497k 302,579k p=0.936 n=6
Parse Time 2.95s (± 0.61%) 2.96s (± 0.48%) ~ 2.94s 2.98s p=0.289 n=6
Bind Time 1.43s (± 0.84%) 1.43s (± 0.85%) ~ 1.42s 1.45s p=0.865 n=6
Check Time 9.14s (± 0.46%) 9.15s (± 0.32%) ~ 9.12s 9.20s p=0.808 n=6
Emit Time 5.33s (± 1.08%) 5.31s (± 0.68%) ~ 5.27s 5.37s p=0.573 n=6
Total Time 18.86s (± 0.52%) 18.85s (± 0.30%) ~ 18.79s 18.95s p=0.810 n=6
material-ui - node (v18.15.0, x64)
Memory used 510,533k (± 0.00%) 510,509k (± 0.00%) ~ 510,472k 510,528k p=0.149 n=6
Parse Time 2.65s (± 0.70%) 2.67s (± 0.31%) ~ 2.66s 2.68s p=0.271 n=6
Bind Time 0.99s (± 1.05%) 0.98s (± 0.83%) ~ 0.97s 0.99s p=0.796 n=6
Check Time 17.21s (± 0.43%) 17.23s (± 0.50%) ~ 17.10s 17.37s p=1.000 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.85s (± 0.40%) 20.88s (± 0.38%) ~ 20.76s 21.01s p=0.936 n=6
mui-docs - node (v18.15.0, x64)
Memory used 1,743,908k (± 0.00%) 1,743,921k (± 0.00%) ~ 1,743,893k 1,743,946k p=0.936 n=6
Parse Time 6.59s (± 0.61%) 6.61s (± 0.83%) ~ 6.53s 6.69s p=0.469 n=6
Bind Time 2.33s (± 0.36%) 2.32s (± 0.80%) ~ 2.28s 2.33s p=0.383 n=6
Check Time 56.19s (± 0.37%) 56.25s (± 0.35%) ~ 56.07s 56.52s p=0.872 n=6
Emit Time 0.13s (± 3.87%) 0.13s (± 3.10%) ~ 0.13s 0.14s p=0.595 n=6
Total Time 65.25s (± 0.34%) 65.31s (± 0.31%) ~ 65.08s 65.61s p=0.748 n=6
self-build-src - node (v18.15.0, x64)
Memory used 2,299,388k (± 0.03%) 2,299,281k (± 0.04%) ~ 2,298,256k 2,300,963k p=0.689 n=6
Parse Time 7.42s (± 0.49%) 7.44s (± 0.93%) ~ 7.34s 7.51s p=0.378 n=6
Bind Time 2.70s (± 0.78%) 2.72s (± 0.73%) ~ 2.70s 2.75s p=0.225 n=6
Check Time 48.86s (± 0.51%) 48.73s (± 0.52%) ~ 48.50s 49.18s p=0.378 n=6
Emit Time 3.81s (± 2.73%) 3.84s (± 4.03%) ~ 3.71s 4.13s p=0.936 n=6
Total Time 62.83s (± 0.50%) 62.75s (± 0.40%) ~ 62.47s 63.20s p=0.689 n=6
self-build-src-public-api - node (v18.15.0, x64)
Memory used 2,396,983k (± 2.47%) 2,373,565k (± 0.01%) ~ 2,373,175k 2,373,858k p=0.378 n=6
Parse Time 7.69s (± 0.81%) 7.66s (± 1.84%) ~ 7.50s 7.88s p=0.810 n=6
Bind Time 2.45s (± 1.09%) 2.48s (± 0.71%) ~ 2.45s 2.50s p=0.062 n=6
Check Time 49.29s (± 0.53%) 49.12s (± 0.38%) ~ 48.87s 49.40s p=0.230 n=6
Emit Time 3.96s (± 2.01%) 3.99s (± 2.68%) ~ 3.78s 4.08s p=0.423 n=6
Total Time 63.40s (± 0.39%) 63.27s (± 0.29%) ~ 62.95s 63.51s p=0.575 n=6
self-compiler - node (v18.15.0, x64)
Memory used 418,063k (± 0.01%) 418,073k (± 0.01%) ~ 417,981k 418,139k p=0.575 n=6
Parse Time 3.37s (± 0.95%) 3.38s (± 0.81%) ~ 3.34s 3.42s p=0.808 n=6
Bind Time 1.29s (± 1.20%) 1.31s (± 1.85%) ~ 1.27s 1.34s p=0.254 n=6
Check Time 17.83s (± 0.48%) 17.79s (± 0.33%) ~ 17.72s 17.88s p=0.470 n=6
Emit Time 1.37s (± 0.60%) 1.37s (± 1.07%) ~ 1.34s 1.38s p=0.448 n=6
Total Time 23.86s (± 0.49%) 23.84s (± 0.29%) ~ 23.78s 23.94s p=0.810 n=6
vscode - node (v18.15.0, x64)
Memory used 2,904,541k (± 0.00%) 2,904,525k (± 0.00%) ~ 2,904,457k 2,904,583k p=0.688 n=6
Parse Time 15.94s (± 0.63%) 15.92s (± 0.48%) ~ 15.79s 16.00s p=0.748 n=6
Bind Time 4.98s (± 2.15%) 4.93s (± 0.50%) ~ 4.91s 4.96s p=0.560 n=6
Check Time 86.53s (± 0.45%) 86.72s (± 0.43%) ~ 86.37s 87.36s p=0.336 n=6
Emit Time 23.75s (± 0.44%) 23.75s (± 0.50%) ~ 23.63s 23.95s p=0.688 n=6
Total Time 131.20s (± 0.27%) 131.31s (± 0.26%) ~ 130.90s 131.87s p=0.575 n=6
webpack - node (v18.15.0, x64)
Memory used 408,738k (± 0.02%) 408,697k (± 0.01%) ~ 408,648k 408,786k p=0.298 n=6
Parse Time 4.79s (± 0.89%) 4.83s (± 0.47%) ~ 4.79s 4.85s p=0.063 n=6
Bind Time 2.02s (± 0.49%) 2.02s (± 1.31%) ~ 1.99s 2.06s p=1.000 n=6
Check Time 20.69s (± 0.27%) 20.69s (± 0.42%) ~ 20.60s 20.85s p=0.689 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 27.50s (± 0.20%) 27.55s (± 0.30%) ~ 27.43s 27.65s p=0.378 n=6
xstate - node (v18.15.0, x64)
Memory used 514,380k (± 0.01%) 514,422k (± 0.02%) ~ 514,329k 514,544k p=0.810 n=6
Parse Time 4.92s (± 0.68%) 4.89s (± 0.70%) ~ 4.84s 4.93s p=0.169 n=6
Bind Time 2.30s (± 0.51%) 2.31s (± 0.45%) ~ 2.29s 2.32s p=0.241 n=6
Check Time 4.23s (± 0.57%) 4.22s (± 0.50%) ~ 4.18s 4.24s p=0.624 n=6
Emit Time 0.11s (± 4.76%) 0.12s (± 3.45%) ~ 0.11s 0.12s p=0.282 n=6
Total Time 11.56s (± 0.38%) 11.54s (± 0.42%) ~ 11.48s 11.59s p=0.689 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • mui-docs - node (v18.15.0, x64)
  • self-build-src - node (v18.15.0, x64)
  • self-build-src-public-api - node (v18.15.0, x64)
  • self-compiler - node (v18.15.0, x64)
  • vscode - node (v18.15.0, x64)
  • webpack - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top 400 repos comparing main and refs/pull/58091/merge:

Everything looks good!

@ahejlsberg
Copy link
Member

@Andarist Definitely the right idea to use getModifiersTypeFromMappedType, but the remaining logic can also be simplified more. I've put up #58098 with what I think is the correct fix.

@Andarist
Copy link
Contributor Author

Andarist commented Apr 7, 2024

Damn, I was close! :P It was worth a shot :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mapped type reported as incompatible when passed through another mapped type (5.4 regression)
4 participants