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

isolatedModules error on global shadowed by imported type #56732

Merged
merged 3 commits into from
Dec 13, 2023

Conversation

frigus02
Copy link
Contributor

Fixes #56521

I took inspiration from #56354, which seems very similar.

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Dec 11, 2023
Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Thanks! I think this is pretty close.

src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
@frigus02
Copy link
Contributor Author

Thanks for the quick review! I think I addressed your comments.

(Not sure about the comment resolving etiquette here. I hope me resolving them is fine?)

@frigus02
Copy link
Contributor Author

The new error currently incorrectly fires for the following Node.js code:

import { Console } from "node:console";
const c: Console = new Console();

That's because console.Console is indeed a global. But it's re-exported in the module node:console. Importantly, though, the global is console.Console, not Console, so the import is necessary.

Node.js typings for reference: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/420d2ceb50f728aa715da1250a2a9f27760a640c/types/node/console.d.ts#L412

Weirdly there is no error for this code:

import { Console } from "node:console";
new Console();

I'm going to look into that tomorrow. If you have any ideas, I'd definitely appreciate your help, @andrewbranch.

@frigus02
Copy link
Contributor Author

Aha. I think SymbolFlags.Value is not a single bit. I just copied the check from a few lines up. But I think I need to use (meaning & SymbolFlags.Value) === SymbolFlags.Value here. I'll test some more tomorrow and update the code and tests.

@andrewbranch
Copy link
Member

Oh, good catch. Yeah, I think something like that should work, or alternatively a lookup with SymbolFlags.Type | SymbolFlags.NamespaceModule

@frigus02
Copy link
Contributor Author

As far as I can tell this looks good now. I did not use SymbolFlags.Type | SymbolFlags.NamespaceModule, yet. But happy to switch to that if you prefer.

@andrewbranch
Copy link
Member

@typescript-bot perf test this
@typescript-bot test top200

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 13, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 13, 2023

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

Update: The results are in!

@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,410k (± 0.01%) 295,395k (± 0.01%) ~ 295,380k 295,420k p=0.261 n=6
Parse Time 2.64s (± 0.46%) 2.64s (± 0.41%) ~ 2.62s 2.65s p=0.432 n=6
Bind Time 0.82s (± 0.00%) 0.82s (± 0.50%) ~ 0.82s 0.83s p=0.405 n=6
Check Time 8.14s (± 0.39%) 8.14s (± 0.29%) ~ 8.11s 8.18s p=0.936 n=6
Emit Time 7.08s (± 0.15%) 7.08s (± 0.36%) ~ 7.05s 7.12s p=0.745 n=6
Total Time 18.69s (± 0.18%) 18.69s (± 0.25%) ~ 18.61s 18.75s p=0.936 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 195,329k (± 1.54%) 194,823k (± 1.49%) ~ 191,383k 197,488k p=1.000 n=6
Parse Time 1.35s (± 1.21%) 1.36s (± 1.08%) ~ 1.34s 1.38s p=0.677 n=6
Bind Time 0.72s (± 0.00%) 0.72s (± 0.00%) ~ 0.72s 0.72s p=1.000 n=6
Check Time 9.26s (± 0.44%) 9.26s (± 0.35%) ~ 9.20s 9.29s p=1.000 n=6
Emit Time 2.63s (± 0.46%) 2.64s (± 0.65%) ~ 2.62s 2.67s p=0.154 n=6
Total Time 13.96s (± 0.38%) 13.97s (± 0.34%) ~ 13.89s 14.02s p=0.747 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,379k (± 0.01%) 347,356k (± 0.01%) ~ 347,333k 347,386k p=0.078 n=6
Parse Time 2.45s (± 0.40%) 2.45s (± 0.40%) ~ 2.44s 2.47s p=1.000 n=6
Bind Time 0.93s (± 0.56%) 0.92s (± 0.56%) ~ 0.92s 0.93s p=0.311 n=6
Check Time 6.90s (± 0.20%) 6.89s (± 0.31%) ~ 6.87s 6.92s p=0.566 n=6
Emit Time 4.05s (± 0.48%) 4.05s (± 0.44%) ~ 4.02s 4.07s p=0.808 n=6
Total Time 14.32s (± 0.22%) 14.32s (± 0.24%) ~ 14.29s 14.37s p=0.936 n=6
TFS - node (v18.15.0, x64)
Memory used 302,640k (± 0.00%) 302,650k (± 0.00%) ~ 302,640k 302,661k p=0.230 n=6
Parse Time 2.00s (± 0.74%) 1.99s (± 0.49%) ~ 1.98s 2.01s p=0.440 n=6
Bind Time 1.00s (± 0.98%) 1.01s (± 1.25%) ~ 0.99s 1.02s p=0.270 n=6
Check Time 6.28s (± 0.29%) 6.27s (± 0.46%) ~ 6.23s 6.30s p=0.746 n=6
Emit Time 3.59s (± 0.89%) 3.59s (± 0.66%) ~ 3.56s 3.63s p=0.683 n=6
Total Time 12.86s (± 0.31%) 12.86s (± 0.13%) ~ 12.84s 12.88s p=0.687 n=6
material-ui - node (v18.15.0, x64)
Memory used 506,788k (± 0.00%) 506,806k (± 0.01%) ~ 506,751k 506,841k p=0.173 n=6
Parse Time 2.58s (± 0.47%) 2.58s (± 0.67%) ~ 2.56s 2.61s p=0.558 n=6
Bind Time 0.99s (± 1.28%) 0.98s (± 0.77%) ~ 0.97s 0.99s p=0.301 n=6
Check Time 16.87s (± 0.37%) 16.90s (± 0.39%) ~ 16.83s 17.00s p=0.518 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.43s (± 0.27%) 20.46s (± 0.36%) ~ 20.40s 20.59s p=0.520 n=6
xstate - node (v18.15.0, x64)
Memory used 512,813k (± 0.01%) 512,847k (± 0.01%) ~ 512,780k 512,933k p=0.471 n=6
Parse Time 3.27s (± 0.19%) 3.27s (± 0.19%) ~ 3.26s 3.28s p=1.000 n=6
Bind Time 1.54s (± 0.36%) 1.54s (± 0.49%) ~ 1.53s 1.55s p=0.137 n=6
Check Time 2.81s (± 0.43%) 2.82s (± 0.73%) ~ 2.80s 2.86s p=0.367 n=6
Emit Time 0.07s (± 0.00%) 0.07s (± 5.69%) ~ 0.07s 0.08s p=0.405 n=6
Total Time 7.69s (± 0.19%) 7.71s (± 0.26%) ~ 7.69s 7.74s p=0.252 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,340ms (± 0.36%) 2,338ms (± 0.45%) ~ 2,327ms 2,352ms p=0.687 n=6
Req 2 - geterr 5,414ms (± 1.22%) 5,457ms (± 1.58%) ~ 5,359ms 5,551ms p=0.199 n=6
Req 3 - references 323ms (± 0.34%) 324ms (± 0.87%) ~ 320ms 326ms p=0.516 n=6
Req 4 - navto 278ms (± 1.27%) 277ms (± 1.39%) ~ 273ms 280ms p=0.337 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 86ms (± 6.36%) 87ms (± 5.86%) ~ 82ms 92ms p=0.738 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,487ms (± 0.64%) 2,482ms (± 1.35%) ~ 2,435ms 2,533ms p=0.810 n=6
Req 2 - geterr 4,086ms (± 1.54%) 4,096ms (± 1.46%) ~ 4,048ms 4,215ms p=0.376 n=6
Req 3 - references 344ms (± 1.16%) 341ms (± 1.40%) ~ 337ms 348ms p=0.627 n=6
Req 4 - navto 285ms (± 0.26%) 285ms (± 0.29%) ~ 284ms 286ms p=0.306 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 87ms (± 5.20%) 87ms (± 5.73%) ~ 77ms 90ms p=0.607 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,593ms (± 0.72%) 2,600ms (± 0.85%) ~ 2,571ms 2,632ms p=0.471 n=6
Req 2 - geterr 1,698ms (± 1.88%) 1,699ms (± 1.33%) ~ 1,672ms 1,729ms p=1.000 n=6
Req 3 - references 112ms (± 9.92%) 122ms (± 2.29%) ~ 117ms 125ms p=0.060 n=6
Req 4 - navto 365ms (± 0.56%) 366ms (± 0.21%) ~ 365ms 367ms p=0.319 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 307ms (± 2.21%) 306ms (± 2.16%) ~ 295ms 314ms p=0.872 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.78ms (± 0.20%) 152.73ms (± 0.18%) ~ 151.66ms 157.07ms p=0.304 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time 228.54ms (± 0.19%) 228.35ms (± 0.17%) -0.19ms (- 0.08%) 227.09ms 235.35ms p=0.000 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time 229.62ms (± 0.20%) 229.64ms (± 0.20%) ~ 227.79ms 236.45ms p=1.000 n=600
typescript-startup - node (v18.15.0, x64)
Execution time 229.48ms (± 0.19%) 229.35ms (± 0.16%) -0.12ms (- 0.05%) 228.07ms 232.75ms p=0.020 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

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

Everything looks good!

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Thanks @frigus02!

@andrewbranch andrewbranch merged commit ba9eddb into microsoft:main Dec 13, 2023
19 checks passed
@frigus02 frigus02 deleted the shadow-type branch December 13, 2023 18:35
c0sta pushed a commit to c0sta/TypeScript that referenced this pull request Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect ts.transpileModule() emit when imported type shadows type but not constructor of global symbol
3 participants