Skip to content

error on duplicate symbols in classes #55438

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

Merged

Conversation

Zzzen
Copy link
Contributor

@Zzzen Zzzen commented Aug 20, 2023

Fixes #55163
This is a follow-up of #55193. I was testing whether getEffectivePropertyNameForPropertyNameNode should be used more, and it turns out that the answer is yes. It helps identify more duplicate symbol properties.

playground

const uniqueSymbol0 = Symbol.for("");
const uniqueSymbol1 = Symbol.for("");

class Cls1 {
  [uniqueSymbol0] = "first";
  [uniqueSymbol0] = "last";  // should error
  [uniqueSymbol1] = "first";
  [uniqueSymbol1] = "last";  // should error
}

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Aug 20, 2023
@sandersn sandersn requested review from rbuckton and gabritto August 28, 2023 23:06
@gabritto
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 29, 2023

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 29, 2023

Hey @gabritto, 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/157125/artifacts?artifactName=tgz&fileId=B1BBDF4A245CCE8174AFF7CAE6999C7D3EBA58ABABDA5D2DAEBC32AB951523AE02&fileName=/typescript-5.3.0-insiders.20230829.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]".;

@gabritto

This comment was marked as duplicate.

@gabritto

This comment was marked as duplicate.

1 similar comment
@gabritto
Copy link
Member

@typescript-bot run DT
@typescript-bot user test this
@typescript-bot test top100
@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 29, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 29, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 29, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 29, 2023

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

Update: The results are in!

@typescript-bot

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@typescript-bot
Copy link
Collaborator

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

There were infrastructure failures potentially unrelated to your change:

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

Otherwise...

Something interesting changed - please have a look.

Details

rxjs-src

/mnt/ts_downloads/rxjs-src/build.sh

  • [NEW] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
    • /home/vsts/work/1/s/typescript-55438/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55438/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55438/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55438/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55438/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55438/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55438/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55438/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55438/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55438/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55438/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55438/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55438/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55438/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55438/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
  • [MISSING] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)

@typescript-bot
Copy link
Collaborator

@gabritto
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 (v16.17.1, x64)
Memory used 300,256k (± 0.01%) 300,257k (± 0.01%) ~ 300,245k 300,289k p=0.468 n=6
Parse Time 3.00s (± 0.14%) 3.01s (± 0.17%) ~ 3.00s 3.01s p=0.112 n=6
Bind Time 0.93s (± 0.00%) 0.93s (± 0.00%) ~ 0.93s 0.93s p=1.000 n=6
Check Time 9.30s (± 0.28%) 9.31s (± 0.32%) ~ 9.27s 9.35s p=0.624 n=6
Emit Time 7.63s (± 0.23%) 7.62s (± 0.51%) ~ 7.58s 7.69s p=0.213 n=6
Total Time 20.86s (± 0.10%) 20.86s (± 0.29%) ~ 20.81s 20.97s p=0.568 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used 194,967k (± 0.85%) 193,928k (± 0.03%) ~ 193,844k 194,002k p=0.810 n=6
Parse Time 1.58s (± 0.00%) 1.58s (± 0.00%) ~ 1.58s 1.58s p=1.000 n=6
Bind Time 0.80s (± 0.51%) 0.79s (± 0.65%) ~ 0.79s 0.80s p=0.112 n=6
Check Time 9.92s (± 0.46%) 9.93s (± 0.44%) ~ 9.89s 10.01s p=0.573 n=6
Emit Time 2.74s (± 0.23%) 2.74s (± 0.27%) ~ 2.73s 2.75s p=0.718 n=6
Total Time 15.03s (± 0.29%) 15.05s (± 0.34%) ~ 15.00s 15.14s p=0.629 n=6
Monaco - node (v16.17.1, x64)
Memory used 347,175k (± 0.01%) 347,183k (± 0.01%) ~ 347,140k 347,206k p=0.936 n=6
Parse Time 2.69s (± 0.28%) 2.69s (± 0.30%) ~ 2.67s 2.69s p=0.340 n=6
Bind Time 0.99s (± 0.00%) 0.99s (± 0.41%) ~ 0.98s 0.99s p=0.405 n=6
Check Time 7.93s (± 0.51%) 7.92s (± 0.26%) ~ 7.90s 7.95s p=0.747 n=6
Emit Time 4.27s (± 0.60%) 4.26s (± 0.45%) ~ 4.23s 4.28s p=1.000 n=6
Total Time 15.88s (± 0.26%) 15.86s (± 0.26%) ~ 15.79s 15.91s p=0.570 n=6
TFS - node (v16.17.1, x64)
Memory used 301,165k (± 0.00%) 301,180k (± 0.00%) +16k (+ 0.01%) 301,173k 301,190k p=0.037 n=6
Parse Time 2.18s (± 0.68%) 2.19s (± 0.69%) ~ 2.16s 2.20s p=0.451 n=6
Bind Time 1.12s (± 0.73%) 1.11s (± 1.68%) ~ 1.07s 1.12s p=0.383 n=6
Check Time 7.25s (± 0.30%) 7.23s (± 0.39%) ~ 7.18s 7.26s p=0.329 n=6
Emit Time 3.97s (± 0.65%) 3.99s (± 0.52%) ~ 3.96s 4.01s p=0.331 n=6
Total Time 14.52s (± 0.26%) 14.51s (± 0.18%) ~ 14.47s 14.54s p=0.809 n=6
material-ui - node (v16.17.1, x64)
Memory used 479,471k (± 0.00%) 479,473k (± 0.00%) ~ 479,456k 479,486k p=0.936 n=6
Parse Time 3.15s (± 0.13%) 3.15s (± 0.20%) ~ 3.14s 3.16s p=0.673 n=6
Bind Time 0.91s (± 0.00%) 0.91s (± 0.00%) ~ 0.91s 0.91s p=1.000 n=6
Check Time 17.84s (± 0.37%) 17.80s (± 0.29%) ~ 17.72s 17.87s p=0.378 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 21.90s (± 0.31%) 21.86s (± 0.25%) ~ 21.78s 21.93s p=0.378 n=6
xstate - node (v16.17.1, x64)
Memory used 542,855k (± 0.01%) 542,803k (± 0.01%) ~ 542,737k 542,869k p=0.173 n=6
Parse Time 3.70s (± 0.23%) 3.70s (± 0.15%) ~ 3.69s 3.70s p=0.855 n=6
Bind Time 1.40s (± 4.45%) 1.41s (± 4.02%) ~ 1.34s 1.45s p=0.929 n=6
Check Time 3.26s (± 3.07%) 3.21s (± 2.66%) ~ 3.13s 3.32s p=0.127 n=6
Emit Time 0.08s (± 6.19%) 0.08s (± 0.00%) ~ 0.08s 0.08s p=0.174 n=6
Total Time 8.44s (± 0.50%) 8.40s (± 0.41%) ~ 8.35s 8.44s p=0.107 n=6
System info unknown
Hosts
  • node (v16.17.1, x64)
Scenarios
  • Angular - node (v16.17.1, x64)
  • Compiler-Unions - node (v16.17.1, x64)
  • Monaco - node (v16.17.1, x64)
  • TFS - node (v16.17.1, x64)
  • material-ui - node (v16.17.1, x64)
  • xstate - node (v16.17.1, 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 (v16.17.1, x64)
Req 1 - updateOpen 2,490ms (± 0.16%) 2,491ms (± 0.14%) ~ 2,486ms 2,496ms p=1.000 n=6
Req 2 - geterr 5,927ms (± 0.26%) 5,924ms (± 0.13%) ~ 5,915ms 5,933ms p=0.423 n=6
Req 3 - references 343ms (± 0.44%) 343ms (± 0.31%) ~ 341ms 344ms p=0.406 n=6
Req 4 - navto 278ms (± 0.19%) 278ms (± 0.20%) ~ 277ms 278ms p=0.640 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 81ms (± 8.20%) 83ms (±10.36%) ~ 76ms 94ms p=0.731 n=6
CompilerTSServer - node (v16.17.1, x64)
Req 1 - updateOpen 2,624ms (± 0.73%) 2,628ms (± 0.69%) ~ 2,612ms 2,656ms p=0.688 n=6
Req 2 - geterr 4,767ms (± 0.23%) 4,771ms (± 0.21%) ~ 4,760ms 4,786ms p=0.808 n=6
Req 3 - references 350ms (± 0.21%) 350ms (± 0.12%) ~ 350ms 351ms p=0.389 n=6
Req 4 - navto 270ms (± 0.30%) 270ms (± 0.20%) ~ 269ms 270ms p=0.859 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 79ms (± 0.52%) 79ms (± 0.52%) ~ 78ms 79ms p=1.000 n=6
xstateTSServer - node (v16.17.1, x64)
Req 1 - updateOpen 2,707ms (± 0.26%) 2,714ms (± 0.17%) ~ 2,707ms 2,720ms p=0.091 n=6
Req 2 - geterr 1,967ms (± 0.70%) 1,951ms (± 2.00%) ~ 1,877ms 1,992ms p=0.378 n=6
Req 3 - references 141ms (± 0.58%) 135ms (± 8.33%) ~ 112ms 141ms p=0.079 n=6
Req 4 - navto 352ms (± 0.00%) 352ms (± 0.39%) ~ 351ms 354ms p=1.000 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 322ms (± 0.53%) 321ms (± 2.10%) ~ 312ms 327ms p=0.514 n=6
System info unknown
Hosts
  • node (v16.17.1, x64)
Scenarios
  • CompilerTSServer - node (v16.17.1, x64)
  • Compiler-UnionsTSServer - node (v16.17.1, x64)
  • xstateTSServer - node (v16.17.1, 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 (v16.17.1, x64)
Execution time 156.08ms (± 0.15%) 156.17ms (± 0.16%) +0.08ms (+ 0.05%) 154.21ms 160.34ms p=0.000 n=600
tsserver-startup - node (v16.17.1, x64)
Execution time 230.17ms (± 0.13%) 231.01ms (± 0.16%) +0.84ms (+ 0.37%) 229.37ms 237.69ms p=0.000 n=600
tsserverlibrary-startup - node (v16.17.1, x64)
Execution time 235.07ms (± 0.17%) 235.92ms (± 0.11%) +0.85ms (+ 0.36%) 234.51ms 238.70ms p=0.000 n=600
typescript-startup - node (v16.17.1, x64)
Execution time 235.78ms (± 0.14%) 235.68ms (± 0.14%) -0.10ms (- 0.04%) 234.26ms 240.93ms p=0.001 n=600
System info unknown
Hosts
  • node (v16.17.1, x64)
Scenarios
  • tsc-startup - node (v16.17.1, x64)
  • tsserver-startup - node (v16.17.1, x64)
  • tsserverlibrary-startup - node (v16.17.1, x64)
  • typescript-startup - node (v16.17.1, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@typescript-bot
Copy link
Collaborator

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

module M {
interface I { }
export class C {
[Symbol.iterator]: I;
[Symbol.toPrimitive](x: I) { }
~~~~~~~~~~~~~~~~~~~~
Copy link
Member

Choose a reason for hiding this comment

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

I'd say we'd ideally keep the elaboration "X was also declared here" in the errors, is that possible?

@@ -65,11 +69,19 @@ mappedTypeProperties.ts(40,17): error TS2363: The right-hand side of an arithmet
[P in PlaceType]: any
~~~~~~~~~~~~~~~~
!!! error TS7061: A mapped type may not declare properties or methods.
~
!!! error TS2304: Cannot find name 'P'.
Copy link
Member

Choose a reason for hiding this comment

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

Those seem to be just noise but maybe it's ok to have them as a side effect of the change.

[uniqueSymbol0] = "last";
~~~~~~~~~~~~~~~
!!! error TS2300: Duplicate identifier '[uniqueSymbol0]'.
Copy link
Member

Choose a reason for hiding this comment

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

Not related to the PR changes, but the error here is way less specific/informative than the comparable one you get in object literals.

@gabritto gabritto merged commit c68d1dd into microsoft:main Sep 1, 2023
snovader pushed a commit to EG-A-S/TypeScript that referenced this pull request Sep 23, 2023
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
Archived in project
Development

Successfully merging this pull request may close these issues.

TS1117 doesn't catch duplicate Symbols
3 participants