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.

Sign up for GitHub

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

Jump to bottom

Make generic arrays indexable by numericStringType #56878

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Make generic arrays indexable by numericStringType #56878

wants to merge 1 commit into from

Conversation

Andarist
Copy link
Contributor

fixes #56823

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Dec 26, 2023
@DanielRosenwasser
Copy link
Member

@typescript-bot pack this
@typescript-bot test this
@typescript-bot test top200
@typescript-bot user test this
@typescript-bot run dt
@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 4, 2024 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 4, 2024 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 4, 2024 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 4, 2024 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 012fa84. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 4, 2024 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 4, 2024 •
edited
Loading

Hey @DanielRosenwasser, 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/159269/artifacts?artifactName=tgz&fileId=869C7D363C6CE07F1ABC6FCDDA7B85C8BA51480C313A8D34201EBE6DF87BD99C02&fileName=/typescript-5.4.0-insiders.20240104.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.4.0-pr-56878-6".;

@typescript-bot
Copy link
Collaborator

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

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

puppeteer

packages/browsers/test/src/tsconfig.json

@typescript-bot
Copy link
Collaborator

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

@typescript-bot
Copy link
Collaborator

@DanielRosenwasser
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,461k (± 0.01%) 295,490k (± 0.01%) ~ 295,446k 295,527k p=0.128 n=6
Parse Time 2.65s (± 0.15%) 2.65s (± 0.41%) ~ 2.64s 2.67s p=1.000 n=6
Bind Time 0.83s (± 1.01%) 0.83s (± 1.48%) ~ 0.81s 0.84s p=0.928 n=6
Check Time 8.15s (± 0.58%) 8.14s (± 0.21%) ~ 8.11s 8.16s p=0.807 n=6
Emit Time 7.12s (± 0.29%) 7.09s (± 0.25%) -0.03s (- 0.42%) 7.08s 7.12s p=0.034 n=6
Total Time 18.75s (± 0.32%) 18.71s (± 0.25%) ~ 18.65s 18.78s p=0.226 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 193,509k (± 1.59%) 191,935k (± 0.56%) ~ 191,449k 194,143k p=0.229 n=6
Parse Time 1.36s (± 1.20%) 1.35s (± 1.85%) ~ 1.32s 1.38s p=0.869 n=6
Bind Time 0.72s (± 0.00%) 0.72s (± 0.00%) ~ 0.72s 0.72s p=1.000 n=6
Check Time 9.25s (± 0.38%) 9.25s (± 0.31%) ~ 9.22s 9.30s p=0.629 n=6
Emit Time 2.62s (± 1.04%) 2.61s (± 0.71%) ~ 2.60s 2.65s p=1.000 n=6
Total Time 13.94s (± 0.43%) 13.94s (± 0.35%) ~ 13.88s 14.01s p=1.000 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,413k (± 0.00%) 347,408k (± 0.01%) ~ 347,372k 347,439k p=0.575 n=6
Parse Time 2.45s (± 0.48%) 2.45s (± 0.54%) ~ 2.43s 2.46s p=0.796 n=6
Bind Time 0.93s (± 0.44%) 0.93s (± 0.56%) ~ 0.92s 0.93s p=0.595 n=6
Check Time 6.86s (± 0.40%) 6.88s (± 0.50%) ~ 6.84s 6.92s p=0.368 n=6
Emit Time 4.05s (± 0.24%) 4.05s (± 0.33%) ~ 4.03s 4.06s p=0.928 n=6
Total Time 14.29s (± 0.21%) 14.30s (± 0.33%) ~ 14.23s 14.37s p=0.571 n=6
TFS - node (v18.15.0, x64)
Memory used 302,730k (± 0.01%) 302,725k (± 0.01%) ~ 302,702k 302,749k p=0.572 n=6
Parse Time 2.00s (± 0.82%) 1.99s (± 1.30%) ~ 1.95s 2.02s p=0.367 n=6
Bind Time 1.00s (± 1.55%) 1.01s (± 0.75%) ~ 1.00s 1.02s p=0.281 n=6
Check Time 6.30s (± 0.54%) 6.29s (± 0.45%) ~ 6.25s 6.31s p=0.685 n=6
Emit Time 3.59s (± 0.33%) 3.58s (± 0.34%) ~ 3.56s 3.59s p=0.183 n=6
Total Time 12.88s (± 0.38%) 12.85s (± 0.34%) ~ 12.81s 12.91s p=0.261 n=6
material-ui - node (v18.15.0, x64)
Memory used 506,809k (± 0.00%) 506,803k (± 0.00%) ~ 506,788k 506,830k p=0.297 n=6
Parse Time 2.59s (± 0.57%) 2.58s (± 0.35%) ~ 2.57s 2.59s p=0.363 n=6
Bind Time 0.98s (± 1.00%) 0.99s (± 1.64%) ~ 0.97s 1.02s p=0.168 n=6
Check Time 16.91s (± 0.34%) 16.96s (± 0.35%) ~ 16.88s 17.04s p=0.230 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.48s (± 0.31%) 20.53s (± 0.34%) ~ 20.45s 20.62s p=0.228 n=6
xstate - node (v18.15.0, x64)
Memory used 512,894k (± 0.00%) 512,867k (± 0.01%) ~ 512,831k 512,898k p=0.093 n=6
Parse Time 3.27s (± 0.26%) 3.28s (± 0.26%) ~ 3.26s 3.28s p=1.000 n=6
Bind Time 1.54s (± 0.53%) 1.54s (± 0.27%) ~ 1.53s 1.54s p=1.000 n=6
Check Time 2.82s (± 0.71%) 2.82s (± 0.74%) ~ 2.80s 2.85s p=1.000 n=6
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) ~ 0.07s 0.07s p=1.000 n=6
Total Time 7.70s (± 0.35%) 7.70s (± 0.26%) ~ 7.69s 7.73s p=0.935 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,346ms (± 0.92%) 2,339ms (± 0.47%) ~ 2,328ms 2,359ms p=0.872 n=6
Req 2 - geterr 5,453ms (± 1.21%) 5,491ms (± 1.73%) ~ 5,364ms 5,580ms p=0.297 n=6
Req 3 - references 325ms (± 0.71%) 325ms (± 0.74%) ~ 322ms 328ms p=1.000 n=6
Req 4 - navto 274ms (± 1.29%) 273ms (± 1.32%) ~ 271ms 278ms p=0.438 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 92ms (± 4.79%) 90ms (± 5.35%) ~ 83ms 93ms p=0.199 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,476ms (± 1.22%) 2,494ms (± 0.93%) ~ 2,457ms 2,528ms p=0.298 n=6
Req 2 - geterr 4,135ms (± 1.87%) 4,120ms (± 1.41%) ~ 4,081ms 4,237ms p=0.630 n=6
Req 3 - references 336ms (± 1.34%) 343ms (± 1.53%) ~ 332ms 346ms p=0.053 n=6
Req 4 - navto 285ms (± 1.24%) 286ms (± 0.79%) ~ 284ms 290ms p=0.118 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 83ms (± 7.09%) 88ms (± 5.66%) ~ 78ms 91ms p=0.143 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,596ms (± 0.74%) 2,597ms (± 0.68%) ~ 2,566ms 2,616ms p=0.810 n=6
Req 2 - geterr 1,745ms (± 1.66%) 1,693ms (± 1.57%) 🟩-53ms (- 3.02%) 1,671ms 1,733ms p=0.013 n=6
Req 3 - references 112ms (±10.09%) 109ms (± 9.86%) ~ 101ms 123ms p=0.510 n=6
Req 4 - navto 362ms (± 1.01%) 365ms (± 0.27%) +4ms (+ 0.97%) 364ms 367ms p=0.009 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 311ms (± 1.06%) 305ms (± 2.67%) ~ 295ms 313ms p=0.226 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 153.40ms (± 0.22%) 153.28ms (± 0.22%) -0.12ms (- 0.08%) 152.14ms 158.34ms p=0.000 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time 228.67ms (± 0.15%) 228.57ms (± 0.15%) -0.09ms (- 0.04%) 227.12ms 232.83ms p=0.002 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time 230.67ms (± 0.20%) 230.69ms (± 0.19%) ~ 228.76ms 237.07ms p=0.509 n=600
typescript-startup - node (v18.15.0, x64)
Execution time 230.39ms (± 0.19%) 230.37ms (± 0.21%) ~ 228.48ms 237.58ms p=0.514 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

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

Everything looks good!

@Andarist Andarist mentioned this pull request Jan 22, 2024
Closed
@Andarist Andarist force-pushed the fix/generic-arr-indexable-by-numericstringtype branch from ab72d39 to 012fa84 Compare January 23, 2024 14:29
@rotu
Copy link

rotu commented Feb 6, 2024

FYI, this is overly permissive. Yes, x['10'] should be an allowable index, but 'ob0', '0xf', '001', '+1', '1.000' are all values of ${number} which are not admissible as array indices and probably shouldn't be in ${number}. #57296

Additionally, '0.5', '3e-12', '-0', '-5', are in ${number} legitimately, but are not valid array indices. We should probably check that a string literal satisfies ToIndex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielRosenwasser DanielRosenwasser Awaiting requested review from DanielRosenwasser

@rbuckton rbuckton Awaiting requested review from rbuckton

At least 1 approving review is required to merge this pull request.

Assignees

@DanielRosenwasser DanielRosenwasser

Labels
For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
Status: Waiting on reviewers
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Type parameter constrained to an array is not indexable by ${number}
4 participants
@Andarist @DanielRosenwasser @typescript-bot @rotu