Skip to content

Remove CheckMode.IsForStringLiteralArgumentCompletions #55867

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

Conversation

Andarist
Copy link
Contributor

It's just an internal refactor.

I initially removed this special check mode almost entirely in #53996 . It was brought up in #54121 . I managed to remove it while reviewing this PR (see here) but I concluded then that this change should be proposed as a follow up - so here it is :)

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Sep 25, 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.

@@ -28,4 +28,5 @@
//// const shallowExpression = parse("foo|/*ts*/")
//// const nestedExpression = parse({ prop: "foo|/*ts2*/" })

verify.completions({ marker: ["ts", "ts2"], exact: ["foo|foo", "foo|bar", "foo|baz"] });
verify.completions({ marker: ["ts"], exact: ["foo", "bar", "baz", "foo|foo", "foo|bar", "foo|baz"] });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This matches Ron's initial expectations from #54121 .

The other case, ideally, should provide the same completions but currently, the ones based on the contextual types and the ones based on getCandidateSignaturesForStringLiteralCompletions work quite differently (and even more so with changes from #54121 ). I plan to close the gap between both a little bit but I'm leaving that for future PRs.

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Seems good to me as a cleanup; I the fewer check modes the better 😄

@DanielRosenwasser
Copy link
Member

@typescript-bot pack this
@typescript-bot test top100
@typescript-bot user test tsserver
@typescript-bot test tsserver top200
@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 29, 2023

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 29, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 29, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 29, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 29, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 29, 2023

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/158016/artifacts?artifactName=tgz&fileId=702CF64F8B3FD4165BC45D1E0BFDFF9180E3E10721483ECE39D2EE338AB3500102&fileName=/typescript-5.3.0-insiders.20230929.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/[email protected]".;

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@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,080k (± 0.01%) 295,082k (± 0.01%) ~ 295,043k 295,158k p=0.810 n=6
Parse Time 2.64s (± 0.52%) 2.63s (± 0.62%) ~ 2.61s 2.65s p=0.324 n=6
Bind Time 0.84s (± 1.06%) 0.84s (± 0.97%) ~ 0.83s 0.85s p=0.550 n=6
Check Time 8.08s (± 0.13%) 8.05s (± 0.29%) ~ 8.03s 8.08s p=0.120 n=6
Emit Time 7.04s (± 0.22%) 7.05s (± 0.33%) ~ 7.02s 7.08s p=0.223 n=6
Total Time 18.59s (± 0.12%) 18.58s (± 0.22%) ~ 18.53s 18.64s p=0.421 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 193,638k (± 1.67%) 191,651k (± 1.24%) ~ 190,628k 196,506k p=0.336 n=6
Parse Time 1.35s (± 1.40%) 1.35s (± 1.09%) ~ 1.34s 1.38s p=1.000 n=6
Bind Time 0.73s (± 0.00%) 0.73s (± 0.00%) ~ 0.73s 0.73s p=1.000 n=6
Check Time 9.19s (± 0.84%) 9.15s (± 0.27%) ~ 9.12s 9.19s p=0.295 n=6
Emit Time 2.64s (± 0.81%) 2.63s (± 0.51%) ~ 2.61s 2.65s p=0.279 n=6
Total Time 13.92s (± 0.51%) 13.86s (± 0.23%) ~ 13.82s 13.90s p=0.147 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,306k (± 0.00%) 347,296k (± 0.01%) ~ 347,259k 347,312k p=0.296 n=6
Parse Time 2.46s (± 0.70%) 2.46s (± 0.66%) ~ 2.44s 2.48s p=0.625 n=6
Bind Time 0.94s (± 0.43%) 0.94s (± 0.00%) ~ 0.94s 0.94s p=0.405 n=6
Check Time 6.92s (± 0.51%) 6.90s (± 0.42%) ~ 6.86s 6.94s p=0.626 n=6
Emit Time 4.02s (± 0.30%) 4.02s (± 0.49%) ~ 3.98s 4.03s p=0.737 n=6
Total Time 14.33s (± 0.29%) 14.32s (± 0.26%) ~ 14.29s 14.39s p=0.743 n=6
TFS - node (v18.15.0, x64)
Memory used 302,587k (± 0.02%) 302,538k (± 0.01%) ~ 302,513k 302,560k p=0.092 n=6
Parse Time 1.98s (± 0.26%) 2.00s (± 0.69%) +0.02s (+ 1.09%) 1.99s 2.02s p=0.010 n=6
Bind Time 1.01s (± 1.15%) 1.00s (± 1.03%) ~ 0.99s 1.02s p=0.241 n=6
Check Time 6.24s (± 0.30%) 6.25s (± 0.38%) ~ 6.21s 6.28s p=0.418 n=6
Emit Time 3.57s (± 0.35%) 3.56s (± 0.58%) ~ 3.54s 3.60s p=0.622 n=6
Total Time 12.81s (± 0.20%) 12.83s (± 0.30%) ~ 12.80s 12.90s p=1.000 n=6
material-ui - node (v18.15.0, x64)
Memory used 470,495k (± 0.00%) 470,491k (± 0.01%) ~ 470,458k 470,524k p=0.378 n=6
Parse Time 2.58s (± 0.47%) 2.57s (± 0.20%) ~ 2.57s 2.58s p=0.923 n=6
Bind Time 1.00s (± 1.04%) 1.00s (± 1.38%) ~ 0.98s 1.02s p=0.618 n=6
Check Time 16.67s (± 0.52%) 16.61s (± 0.70%) ~ 16.43s 16.76s p=0.470 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.25s (± 0.49%) 20.18s (± 0.59%) ~ 20.00s 20.32s p=0.378 n=6
xstate - node (v18.15.0, x64)
Memory used 512,610k (± 0.01%) 512,601k (± 0.01%) ~ 512,539k 512,645k p=0.873 n=6
Parse Time 3.27s (± 0.42%) 3.27s (± 0.36%) ~ 3.25s 3.28s p=0.383 n=6
Bind Time 1.55s (± 0.26%) 1.55s (± 0.53%) ~ 1.55s 1.57s p=0.218 n=6
Check Time 2.87s (± 1.09%) 2.86s (± 0.58%) ~ 2.84s 2.88s p=0.332 n=6
Emit Time 0.08s (± 0.00%) 0.08s (± 5.21%) ~ 0.07s 0.08s p=0.405 n=6
Total Time 7.76s (± 0.37%) 7.76s (± 0.30%) ~ 7.72s 7.78s p=1.000 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,383ms (± 1.10%) 2,386ms (± 1.43%) ~ 2,342ms 2,419ms p=0.575 n=6
Req 2 - geterr 5,330ms (± 1.39%) 5,351ms (± 1.62%) ~ 5,273ms 5,466ms p=1.000 n=6
Req 3 - references 330ms (± 1.62%) 328ms (± 0.71%) ~ 326ms 332ms p=0.627 n=6
Req 4 - navto 277ms (± 0.62%) 277ms (± 0.63%) ~ 274ms 278ms p=0.934 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 77ms (± 8.16%) 80ms (± 9.33%) ~ 75ms 90ms p=0.111 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,473ms (± 1.23%) 2,456ms (± 1.03%) ~ 2,423ms 2,487ms p=0.298 n=6
Req 2 - geterr 4,112ms (± 1.64%) 4,190ms (± 1.47%) +78ms (+ 1.89%) 4,065ms 4,222ms p=0.031 n=6
Req 3 - references 336ms (± 1.23%) 334ms (± 0.51%) ~ 332ms 337ms p=0.511 n=6
Req 4 - navto 283ms (± 0.18%) 284ms (± 0.29%) ~ 283ms 285ms p=0.523 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 78ms (± 5.90%) 76ms (± 0.68%) ~ 75ms 76ms p=0.386 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,590ms (± 0.60%) 2,592ms (± 0.74%) ~ 2,561ms 2,614ms p=0.810 n=6
Req 2 - geterr 1,704ms (± 2.65%) 1,693ms (± 1.95%) ~ 1,660ms 1,737ms p=0.630 n=6
Req 3 - references 113ms (± 9.00%) 110ms (± 6.94%) ~ 105ms 124ms p=0.870 n=6
Req 4 - navto 360ms (± 0.23%) 359ms (± 0.14%) -2ms (- 0.46%) 358ms 359ms p=0.009 n=6
Req 5 - completionInfo count 2,071 (± 0.00%) 2,071 (± 0.00%) ~ 2,071 2,071 p=1.000 n=6
Req 5 - completionInfo 304ms (± 1.86%) 306ms (± 2.00%) ~ 295ms 314ms p=0.685 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.42ms (± 0.19%) 152.28ms (± 0.18%) -0.14ms (- 0.09%) 151.13ms 155.03ms p=0.000 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time 227.30ms (± 0.14%) 227.24ms (± 0.16%) -0.06ms (- 0.02%) 225.93ms 232.82ms p=0.013 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time 228.85ms (± 0.16%) 228.99ms (± 0.17%) +0.14ms (+ 0.06%) 227.03ms 234.74ms p=0.002 n=600
typescript-startup - node (v18.15.0, x64)
Execution time 228.53ms (± 0.17%) 228.50ms (± 0.17%) ~ 227.02ms 235.86ms p=0.407 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/55867/merge:

Everything looks good!

1 similar comment
@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@DanielRosenwasser DanielRosenwasser merged commit efc343a into microsoft:main Sep 29, 2023
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.

4 participants