Skip to content
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

Fixed types of properties of contextual filtering mapped types #56201

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Oct 24, 2023

This is based on the contextualPropertyOfGenericMappedType test case - just with the added filtering .nameType

fixes #56881

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Oct 24, 2023
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@RyanCavanaugh
Copy link
Member

@typescript-bot test this
@typescript-bot test top100
@typescript-bot user test this
@typescript-bot user test tsserver
@typescript-bot test tsserver top100
@typescript-bot run dt
@typescript-bot perf test this
@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 24, 2023

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 24, 2023

Heya @RyanCavanaugh, I've started to run the parallelized Definitely Typed test suite on this PR at 546f5d7. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 24, 2023

Heya @RyanCavanaugh, I've started to run the diff-based user code test suite (tsserver) on this PR at 546f5d7. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 24, 2023

Heya @RyanCavanaugh, I've started to run the diff-based top-repos suite on this PR at 546f5d7. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 24, 2023

Heya @RyanCavanaugh, I've started to run the diff-based top-repos suite (tsserver) on this PR at 546f5d7. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 24, 2023

Heya @RyanCavanaugh, I've started to run the regular perf test suite on this PR at 546f5d7. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 24, 2023

Heya @RyanCavanaugh, I've started to run the diff-based user code test suite on this PR at 546f5d7. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 24, 2023

Hey @RyanCavanaugh, 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/158327/artifacts?artifactName=tgz&fileId=027BC9B658787EB39EEF320DDC8C76612CCC5A7F7458CEA8DAA28ED4E9C7C0BC02&fileName=/typescript-5.3.0-insiders.20231024.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.3.0-pr-56201-9".;

@typescript-bot
Copy link
Collaborator

@RyanCavanaugh Here are the results of running the user test suite comparing main and refs/pull/56201/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

@RyanCavanaugh Here are the results of running the user test suite comparing main and refs/pull/56201/merge:

There were infrastructure failures potentially unrelated to your change:

  • 3 instances of "Package install failed"
  • 1 instance of "Unknown failure"

Otherwise...

Everything looks good!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Compiler

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Angular - node (v18.15.0, x64)
Memory used 295,114k (± 0.01%) 295,125k (± 0.01%) ~ 295,086k 295,174k p=0.872 n=6
Parse Time 2.63s (± 0.31%) 2.63s (± 0.64%) ~ 2.60s 2.65s p=1.000 n=6
Bind Time 0.83s (± 0.66%) 0.84s (± 0.97%) ~ 0.83s 0.85s p=0.859 n=6
Check Time 8.05s (± 0.28%) 8.05s (± 0.19%) ~ 8.03s 8.07s p=0.565 n=6
Emit Time 7.09s (± 0.68%) 7.07s (± 0.24%) ~ 7.05s 7.10s p=0.413 n=6
Total Time 18.62s (± 0.29%) 18.58s (± 0.10%) ~ 18.57s 18.62s p=0.222 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 193,601k (± 1.65%) 191,632k (± 1.22%) ~ 190,652k 196,417k p=0.261 n=6
Parse Time 1.34s (± 1.12%) 1.34s (± 0.90%) ~ 1.32s 1.35s p=0.675 n=6
Bind Time 0.73s (± 0.00%) 0.73s (± 0.00%) ~ 0.73s 0.73s p=1.000 n=6
Check Time 9.17s (± 0.35%) 9.18s (± 0.56%) ~ 9.11s 9.25s p=0.686 n=6
Emit Time 2.64s (± 0.54%) 2.64s (± 0.44%) ~ 2.62s 2.65s p=0.934 n=6
Total Time 13.88s (± 0.30%) 13.89s (± 0.37%) ~ 13.82s 13.95s p=0.809 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,362k (± 0.01%) 347,368k (± 0.00%) ~ 347,360k 347,386k p=0.630 n=6
Parse Time 2.46s (± 0.42%) 2.46s (± 0.17%) ~ 2.46s 2.47s p=0.390 n=6
Bind Time 0.94s (± 0.00%) 0.94s (± 0.43%) ~ 0.93s 0.94s p=0.405 n=6
Check Time 6.92s (± 0.55%) 6.92s (± 0.38%) ~ 6.89s 6.96s p=0.747 n=6
Emit Time 4.05s (± 0.50%) 4.05s (± 0.30%) ~ 4.04s 4.07s p=0.622 n=6
Total Time 14.37s (± 0.32%) 14.38s (± 0.16%) ~ 14.35s 14.41s p=0.520 n=6
TFS - node (v18.15.0, x64)
Memory used 302,599k (± 0.01%) 302,606k (± 0.01%) ~ 302,580k 302,648k p=0.748 n=6
Parse Time 1.98s (± 0.90%) 2.00s (± 1.03%) ~ 1.98s 2.04s p=0.062 n=6
Bind Time 1.00s (± 0.81%) 1.01s (± 1.40%) ~ 0.99s 1.03s p=0.402 n=6
Check Time 6.28s (± 0.42%) 6.26s (± 0.37%) ~ 6.23s 6.29s p=0.147 n=6
Emit Time 3.57s (± 0.55%) 3.57s (± 0.29%) ~ 3.56s 3.59s p=1.000 n=6
Total Time 12.84s (± 0.16%) 12.85s (± 0.19%) ~ 12.82s 12.88s p=0.744 n=6
material-ui - node (v18.15.0, x64)
Memory used 470,516k (± 0.00%) 470,517k (± 0.00%) ~ 470,490k 470,546k p=1.000 n=6
Parse Time 2.57s (± 0.75%) 2.56s (± 0.48%) ~ 2.55s 2.58s p=0.503 n=6
Bind Time 0.99s (± 0.64%) 0.99s (± 0.64%) ~ 0.98s 1.00s p=1.000 n=6
Check Time 16.62s (± 0.44%) 16.59s (± 0.49%) ~ 16.48s 16.71s p=0.630 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.18s (± 0.38%) 20.14s (± 0.42%) ~ 20.03s 20.27s p=0.573 n=6
xstate - node (v18.15.0, x64)
Memory used 512,641k (± 0.01%) 512,589k (± 0.01%) ~ 512,536k 512,711k p=0.128 n=6
Parse Time 3.26s (± 0.42%) 3.28s (± 0.32%) ~ 3.26s 3.29s p=0.160 n=6
Bind Time 1.55s (± 0.26%) 1.55s (± 0.41%) ~ 1.54s 1.56s p=0.673 n=6
Check Time 2.86s (± 0.60%) 2.84s (± 0.70%) ~ 2.81s 2.87s p=0.063 n=6
Emit Time 0.08s (± 0.00%) 0.08s (± 0.00%) ~ 0.08s 0.08s p=1.000 n=6
Total Time 7.74s (± 0.28%) 7.73s (± 0.32%) ~ 7.70s 7.77s p=0.625 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)
  • xstate - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

tsserver

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-UnionsTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,389ms (± 1.11%) 2,346ms (± 1.48%) ~ 2,320ms 2,412ms p=0.066 n=6
Req 2 - geterr 5,341ms (± 1.58%) 5,410ms (± 1.57%) ~ 5,319ms 5,507ms p=0.093 n=6
Req 3 - references 328ms (± 1.65%) 328ms (± 0.96%) ~ 326ms 334ms p=0.561 n=6
Req 4 - navto 276ms (± 1.38%) 275ms (± 1.60%) ~ 271ms 280ms p=0.935 n=6
Req 5 - completionInfo count 1,356 (± 0.00%) 1,356 (± 0.00%) ~ 1,356 1,356 p=1.000 n=6
Req 5 - completionInfo 82ms (± 8.33%) 82ms (± 9.25%) ~ 75ms 91ms p=1.000 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,477ms (± 0.94%) 2,462ms (± 1.51%) ~ 2,425ms 2,513ms p=0.518 n=6
Req 2 - geterr 4,082ms (± 1.59%) 4,133ms (± 2.00%) ~ 4,049ms 4,218ms p=0.378 n=6
Req 3 - references 341ms (± 1.06%) 338ms (± 1.49%) ~ 332ms 343ms p=0.112 n=6
Req 4 - navto 285ms (± 0.29%) 284ms (± 0.35%) ~ 283ms 285ms p=0.256 n=6
Req 5 - completionInfo count 1,518 (± 0.00%) 1,518 (± 0.00%) ~ 1,518 1,518 p=1.000 n=6
Req 5 - completionInfo 84ms (± 4.77%) 82ms (± 8.04%) ~ 75ms 88ms p=0.737 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,587ms (± 0.70%) 2,597ms (± 0.44%) ~ 2,578ms 2,608ms p=0.298 n=6
Req 2 - geterr 1,698ms (± 2.40%) 1,688ms (± 2.64%) ~ 1,642ms 1,744ms p=0.748 n=6
Req 3 - references 117ms (± 7.53%) 115ms (± 8.69%) ~ 106ms 126ms p=1.000 n=6
Req 4 - navto 361ms (± 1.35%) 360ms (± 0.29%) ~ 358ms 361ms p=0.867 n=6
Req 5 - completionInfo count 2,073 (± 0.00%) 2,073 (± 0.00%) ~ 2,073 2,073 p=1.000 n=6
Req 5 - completionInfo 312ms (± 1.54%) 306ms (± 2.49%) ~ 297ms 315ms p=0.126 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • CompilerTSServer - node (v18.15.0, x64)
  • Compiler-UnionsTSServer - node (v18.15.0, x64)
  • xstateTSServer - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Startup

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
tsc-startup - node (v18.15.0, x64)
Execution time 152.57ms (± 0.18%) 152.66ms (± 0.22%) +0.09ms (+ 0.06%) 151.45ms 157.63ms p=0.013 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time 227.68ms (± 0.17%) 227.54ms (± 0.15%) -0.14ms (- 0.06%) 226.02ms 231.06ms p=0.000 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time 228.93ms (± 0.17%) 228.97ms (± 0.23%) ~ 227.35ms 245.58ms p=0.691 n=600
typescript-startup - node (v18.15.0, x64)
Execution time 228.89ms (± 0.16%) 228.76ms (± 0.16%) -0.12ms (- 0.05%) 227.41ms 234.78ms p=0.003 n=600
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • tsc-startup - node (v18.15.0, x64)
  • tsserver-startup - node (v18.15.0, x64)
  • tsserverlibrary-startup - node (v18.15.0, x64)
  • typescript-startup - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

Hey @RyanCavanaugh, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@typescript-bot
Copy link
Collaborator

@RyanCavanaugh Here are the results of running the top-repos suite comparing main and refs/pull/56201/merge:

Everything looks good!

1 similar comment
@typescript-bot
Copy link
Collaborator

@RyanCavanaugh Here are the results of running the top-repos suite comparing main and refs/pull/56201/merge:

Everything looks good!


declare function f2<T extends object>(
data: T,
handlers: { [P in keyof T as T[P] extends string ? P : never]: (value: T[P], prop: P) => void },
Copy link

@julienandreu julienandreu Dec 27, 2023

Choose a reason for hiding this comment

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

Question: Is there any reason you're using the extends string here instead of extends PropertyKey ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's meant to actually filter something, so I intentionally filter T[P] that match some subset of potential types. Note that I'm checking against T[P] and not just P - but even if the check would be against P, I could still want to filter the keys so PropertyKey isn't what I'd use here

@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Aug 7, 2024
@andrewbranch andrewbranch merged commit 3cf708e into microsoft:main Aug 8, 2024
32 checks passed
@jakebailey
Copy link
Member

This also makes me wonder if this is also a bug:

- const keysRemapped = !!target.declaration.nameType;
+ const keysRemapped = getMappedTypeNameTypeKind(target) === MappedTypeNameTypeKind.Remapping;

(Or maybe other places where nameType is used)

@Andarist Andarist deleted the fix/type-of-prop-of-contextual-filtering-mapped-type branch August 8, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Contextual parameter types failed to be provided based on filtering mapped types
7 participants