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

isolatedModules error on alias merging with local value #56354

Merged
merged 2 commits into from
Nov 15, 2023
Merged

isolatedModules error on alias merging with local value #56354

merged 2 commits into from
Nov 15, 2023

Conversation

andrewbranch
Copy link
Member

Fixes #55584

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Nov 9, 2023
@andrewbranch andrewbranch added Breaking Change Would introduce errors in existing code and removed Author: Team labels Nov 9, 2023
@andrewbranch
Copy link
Member Author

@typescript-bot test top200
@typescript-bot user test
@typescript-bot perf test

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 9, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 9, 2023 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

@andrewbranch
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,174k (± 0.02%) 295,186k (± 0.01%) ~ 295,170k 295,230k p=0.296 n=6
Parse Time 2.63s (± 0.15%) 2.64s (± 0.52%) ~ 2.62s 2.65s p=0.450 n=6
Bind Time 0.84s (± 1.43%) 0.83s (± 0.00%) -0.01s (- 1.58%) 0.83s 0.83s p=0.028 n=6
Check Time 8.04s (± 0.10%) 8.05s (± 0.19%) ~ 8.03s 8.07s p=0.236 n=6
Emit Time 7.07s (± 0.36%) 7.10s (± 0.46%) ~ 7.05s 7.14s p=0.243 n=6
Total Time 18.59s (± 0.11%) 18.61s (± 0.24%) ~ 18.56s 18.68s p=0.326 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 192,102k (± 1.23%) 191,674k (± 1.24%) ~ 190,688k 196,520k p=0.575 n=6
Parse Time 1.35s (± 0.77%) 1.36s (± 1.29%) ~ 1.34s 1.39s p=0.391 n=6
Bind Time 0.73s (± 0.00%) 0.73s (± 0.00%) ~ 0.73s 0.73s p=1.000 n=6
Check Time 9.15s (± 0.32%) 9.14s (± 0.45%) ~ 9.08s 9.19s p=1.000 n=6
Emit Time 2.62s (± 0.34%) 2.63s (± 0.46%) ~ 2.61s 2.64s p=0.362 n=6
Total Time 13.86s (± 0.20%) 13.86s (± 0.24%) ~ 13.81s 13.89s p=0.809 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,349k (± 0.01%) 347,356k (± 0.00%) ~ 347,339k 347,366k p=0.575 n=6
Parse Time 2.46s (± 0.70%) 2.45s (± 0.48%) ~ 2.44s 2.47s p=0.557 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.20%) 6.92s (± 0.28%) ~ 6.89s 6.94s p=0.618 n=6
Emit Time 4.05s (± 0.59%) 4.06s (± 0.46%) ~ 4.03s 4.08s p=0.681 n=6
Total Time 14.38s (± 0.23%) 14.37s (± 0.31%) ~ 14.31s 14.41s p=1.000 n=6
TFS - node (v18.15.0, x64)
Memory used 302,587k (± 0.00%) 302,603k (± 0.01%) ~ 302,569k 302,659k p=0.378 n=6
Parse Time 2.00s (± 1.09%) 2.00s (± 0.93%) ~ 1.98s 2.03s p=0.418 n=6
Bind Time 1.01s (± 1.25%) 1.00s (± 1.63%) ~ 0.99s 1.03s p=0.413 n=6
Check Time 6.25s (± 0.40%) 6.29s (± 0.53%) ~ 6.25s 6.33s p=0.075 n=6
Emit Time 3.57s (± 0.33%) 3.60s (± 0.39%) +0.03s (+ 0.89%) 3.58s 3.62s p=0.009 n=6
Total Time 12.83s (± 0.27%) 12.90s (± 0.25%) +0.07s (+ 0.51%) 12.85s 12.95s p=0.015 n=6
material-ui - node (v18.15.0, x64)
Memory used 470,565k (± 0.01%) 470,562k (± 0.01%) ~ 470,521k 470,600k p=0.810 n=6
Parse Time 2.56s (± 0.59%) 2.57s (± 0.53%) ~ 2.56s 2.60s p=0.557 n=6
Bind Time 1.00s (± 0.52%) 0.99s (± 0.52%) ~ 0.99s 1.00s p=0.311 n=6
Check Time 16.61s (± 0.46%) 16.65s (± 0.40%) ~ 16.54s 16.71s p=0.423 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.36%) 20.21s (± 0.34%) ~ 20.10s 20.28s p=0.375 n=6
xstate - node (v18.15.0, x64)
Memory used 512,795k (± 0.01%) 512,860k (± 0.02%) ~ 512,748k 512,985k p=0.173 n=6
Parse Time 3.27s (± 0.30%) 3.28s (± 0.26%) ~ 3.26s 3.28s p=0.588 n=6
Bind Time 1.54s (± 0.35%) 1.54s (± 0.67%) ~ 1.53s 1.56s p=0.663 n=6
Check Time 2.86s (± 0.65%) 2.85s (± 0.65%) ~ 2.83s 2.88s p=0.746 n=6
Emit Time 0.08s (± 0.00%) 0.08s (± 0.00%) ~ 0.08s 0.08s p=1.000 n=6
Total Time 7.75s (± 0.30%) 7.75s (± 0.25%) ~ 7.72s 7.78s p=0.571 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,376ms (± 0.76%) 2,384ms (± 0.49%) ~ 2,365ms 2,398ms p=0.423 n=6
Req 2 - geterr 5,376ms (± 1.49%) 5,370ms (± 1.32%) ~ 5,316ms 5,477ms p=0.575 n=6
Req 3 - references 327ms (± 1.53%) 326ms (± 0.79%) ~ 323ms 329ms p=0.871 n=6
Req 4 - navto 277ms (± 0.98%) 278ms (± 1.26%) ~ 273ms 280ms p=0.620 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 83ms (± 6.89%) 83ms (± 7.21%) ~ 75ms 90ms p=0.869 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,502ms (± 0.58%) 2,509ms (± 0.92%) ~ 2,473ms 2,540ms p=0.471 n=6
Req 2 - geterr 4,060ms (± 1.49%) 4,090ms (± 1.61%) ~ 4,039ms 4,177ms p=0.128 n=6
Req 3 - references 351ms (± 6.06%) 341ms (± 1.69%) ~ 333ms 347ms p=0.370 n=6
Req 4 - navto 282ms (± 0.72%) 282ms (± 0.62%) ~ 279ms 284ms p=0.655 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 (± 8.11%) 86ms (± 6.89%) ~ 78ms 90ms p=0.391 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,586ms (± 0.25%) 2,598ms (± 0.31%) +12ms (+ 0.47%) 2,586ms 2,610ms p=0.020 n=6
Req 2 - geterr 1,733ms (± 2.72%) 1,750ms (± 1.80%) ~ 1,692ms 1,777ms p=0.630 n=6
Req 3 - references 111ms (± 8.82%) 110ms (± 9.23%) ~ 100ms 122ms p=0.629 n=6
Req 4 - navto 368ms (± 1.18%) 366ms (± 0.53%) ~ 364ms 369ms p=0.570 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 310ms (± 1.97%) 310ms (± 2.35%) ~ 299ms 317ms p=0.936 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.68ms (± 0.17%) 152.54ms (± 0.15%) -0.14ms (- 0.09%) 151.53ms 154.28ms p=0.000 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time 228.15ms (± 0.15%) 227.93ms (± 0.15%) -0.21ms (- 0.09%) 226.69ms 232.68ms p=0.000 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time 229.23ms (± 0.16%) 229.19ms (± 0.16%) ~ 227.61ms 235.21ms p=0.347 n=600
typescript-startup - node (v18.15.0, x64)
Execution time 229.10ms (± 0.15%) 229.17ms (± 0.16%) ~ 227.77ms 234.18ms p=0.063 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

@andrewbranch
Copy link
Member Author

@typescript-bot test top200

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 13, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

Something interesting changed - please have a look.

Details

heyxyz/hey

apps/prerender/tsconfig.json

apps/web/tsconfig.json

prisma/prisma

75 of 101 projects failed to build with the old tsc and were ignored

packages/debug/tsconfig.build.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/debug/tsconfig.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/get-platform/tsconfig.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/engines/tsconfig.build.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/engines/tsconfig.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/generator-helper/tsconfig.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/internals/tsconfig.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/instrumentation/tsconfig.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/client/tsconfig.build.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/cli/tsconfig.build.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

@andrewbranch
Copy link
Member Author

I’m kind of ambivalent on this. Should isolatedModules be slightly stricter than necessary and break these projects to help swc and eslint, or should these examples be used as evidence that transpilers can handle this pattern and close the issue as Working as Intended?

@jakebailey
Copy link
Member

Thinking about this from the isolatedDeclarations angle (which IIRC will require isolatedModules), this too would be a pattern we'd have to ban, I think, since it's the same problem but for the same symbol in type space?

@andrewbranch
Copy link
Member Author

I don’t think this applies to isolatedDeclarations, unless I’m missing something. The problematic question for single-file JS transpilers here is whether an import needs to be dropped. An isolatedDeclarations emitter need not ever drop an import.

@jakebailey
Copy link
Member

Ah, true; I guess I was thinking about it in terms of "can a d.ts emit reasonably understand what to export", but I guess they'd just leave things as-is.

I'm personally all for doing this break. It's only 2 out of the top 200 and the fix seems to be straightforward, right? We could rerun top500 or something to really see. That'd be fun...

@typescript-bot test top500

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 14, 2023 •
edited
Loading

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

Update: The results are in!

@andrewbranch
Copy link
Member Author

Yeah, the fix is just to make the imports type-only.

@typescript-bot
Copy link
Collaborator

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

Something interesting changed - please have a look.

Details

heyxyz/hey

apps/prerender/tsconfig.json

apps/web/tsconfig.json

prisma/prisma

75 of 101 projects failed to build with the old tsc and were ignored

packages/debug/tsconfig.build.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/debug/tsconfig.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/get-platform/tsconfig.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/engines/tsconfig.build.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/engines/tsconfig.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/generator-helper/tsconfig.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/internals/tsconfig.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/instrumentation/tsconfig.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/client/tsconfig.build.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

packages/cli/tsconfig.build.json

  • error TS2865: Import 'Debug' conflicts with local value, so must be declared with a type-only import when 'isolatedModules' is enabled.

vercel/commerce

tsconfig.json

@jakebailey
Copy link
Member

Yeah, given the above run I think this is okay.

jakebailey
jakebailey approved these changes Nov 14, 2023
View reviewed changes
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.

I'm going to approve it, but maybe this warrants a design meeting quick topic?

@andrewbranch andrewbranch merged commit a636797 into microsoft:main Nov 15, 2023
19 checks passed
@andrewbranch andrewbranch deleted the bug/55584 branch November 15, 2023 22:11
@DanielRosenwasser DanielRosenwasser mentioned this pull request Nov 16, 2023
Closed
@frigus02 frigus02 mentioned this pull request Dec 11, 2023
Merged
@Conaclos Conaclos mentioned this pull request Feb 15, 2024
Closed
@nazarhussain nazarhussain mentioned this pull request Mar 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakebailey jakebailey jakebailey approved these changes

Assignees

@andrewbranch andrewbranch

Labels
Author: Team Breaking Change Would introduce errors in existing code For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

imported symbol that is shadowed with a local symbol does not error
3 participants
@andrewbranch @typescript-bot @jakebailey