Skip to content

Check expression operand equivalence in isMatchingReference before computing property key equivalence #61608

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

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

Conversation

RyanCavanaugh
Copy link
Member

@RyanCavanaugh RyanCavanaugh commented Apr 22, 2025

Fixes #61606. Need PR results to see if this is worth it or not.

Update: Seems good?

…erty key equivalence

Fixes microsoft#61606. Need PR results to see if this is worth it or not.
@Copilot Copilot AI review requested due to automatic review settings April 22, 2025 22:06
@github-project-automation github-project-automation bot moved this to Not started in PR Backlog Apr 22, 2025
@RyanCavanaugh
Copy link
Member Author

@typescript-bot test it

@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug labels Apr 22, 2025
@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 22, 2025

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
test top400 ✅ Started ✅ Results
user test this ✅ Started ✅ Results
run dt ✅ Started ✅ Results
perf test this faster ✅ Started 👀 Results

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses bug #61606 by updating the check for element access expressions in the type checker. The key changes include:

  • Adding a new test case (tests/cases/compiler/circularRefFromClassMember.ts) to verify correct behavior.
  • Updating the corresponding baseline (tests/baselines/reference/circularRefFromClassMember.js).
  • Modifying the logic in src/compiler/checker.ts to first ensure that the operand expressions are equivalent before comparing property keys.

Reviewed Changes

Copilot reviewed 3 out of 5 changed files in this pull request and generated no comments.

File Description
tests/cases/compiler/circularRefFromClassMember.ts New test case to validate the fix for bug #61606.
tests/baselines/reference/circularRefFromClassMember.js Updated baseline output consistent with the new test.
src/compiler/checker.ts Revised element access check to verify operand equivalence prior to property key comparison.
Files not reviewed (2)
  • tests/baselines/reference/circularRefFromClassMember.symbols: Language not supported
  • tests/baselines/reference/circularRefFromClassMember.types: Language not supported
Comments suppressed due to low confidence (1)

src/compiler/checker.ts:27187

  • The new check ensures that the operands of the element accesses are equivalent before comparing property keys. Please verify that this ordering correctly handles all edge cases and add an inline comment explaining the rationale if necessary.
if (isAccessExpression(target) && isMatchingReference((source as AccessExpression).expression, target.expression)) {

@typescript-bot
Copy link
Collaborator

Hey @RyanCavanaugh, the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

@typescript-bot
Copy link
Collaborator

@RyanCavanaugh Here are the results of running the user tests with tsc comparing main and refs/pull/61608/merge:

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Timeout"

Otherwise...

Everything looks good!

@typescript-bot
Copy link
Collaborator

@RyanCavanaugh
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-Unions - node (v18.15.0, x64)
Errors 34 34 ~ ~ ~ p=1.000 n=6
Symbols 62,390 62,390 ~ ~ ~ p=1.000 n=6
Types 50,395 50,395 ~ ~ ~ p=1.000 n=6
Memory used 194,785k (± 1.02%) 194,279k (± 0.91%) ~ 192,897k 196,620k p=0.810 n=6
Parse Time 1.31s (± 0.57%) 1.30s (± 0.58%) -0.01s (- 1.02%) 1.29s 1.31s p=0.024 n=6
Bind Time 0.73s 0.73s ~ ~ ~ p=1.000 n=6
Check Time 9.72s (± 0.18%) 9.75s (± 0.46%) ~ 9.71s 9.83s p=0.244 n=6
Emit Time 2.74s (± 0.83%) 2.73s (± 0.75%) ~ 2.70s 2.75s p=0.328 n=6
Total Time 14.50s (± 0.09%) 14.51s (± 0.37%) ~ 14.46s 14.60s p=0.809 n=6
angular-1 - node (v18.15.0, x64)
Errors 56 56 ~ ~ ~ p=1.000 n=6
Symbols 948,670 948,670 ~ ~ ~ p=1.000 n=6
Types 410,947 410,947 ~ ~ ~ p=1.000 n=6
Memory used 1,224,348k (± 0.01%) 1,224,320k (± 0.00%) ~ 1,224,259k 1,224,396k p=0.689 n=6
Parse Time 6.66s (± 0.35%) 6.63s (± 0.88%) ~ 6.54s 6.68s p=0.466 n=6
Bind Time 1.88s (± 0.64%) 1.88s (± 0.55%) ~ 1.87s 1.89s p=0.337 n=6
Check Time 31.94s (± 0.33%) 31.92s (± 0.49%) ~ 31.68s 32.11s p=1.000 n=6
Emit Time 15.24s (± 0.38%) 15.30s (± 0.30%) ~ 15.24s 15.37s p=0.127 n=6
Total Time 55.72s (± 0.25%) 55.73s (± 0.28%) ~ 55.53s 55.98s p=0.936 n=6
mui-docs - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 2,361,522 2,361,522 ~ ~ ~ p=1.000 n=6
Types 868,648 868,648 ~ ~ ~ p=1.000 n=6
Memory used 2,316,650k (± 0.00%) 2,316,624k (± 0.00%) ~ 2,316,549k 2,316,722k p=0.688 n=6
Parse Time 8.73s (± 0.32%) 8.75s (± 0.23%) ~ 8.72s 8.78s p=0.373 n=6
Bind Time 2.19s (± 0.63%) 2.19s (± 0.45%) ~ 2.18s 2.20s p=0.933 n=6
Check Time 69.69s (± 0.45%) 70.00s (± 0.46%) ~ 69.45s 70.38s p=0.109 n=6
Emit Time 0.30s (± 2.49%) 0.30s (± 2.75%) ~ 0.29s 0.31s p=0.306 n=6
Total Time 80.92s (± 0.40%) 81.23s (± 0.40%) ~ 80.67s 81.63s p=0.127 n=6
self-build-src - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,228,025 1,228,025 ~ ~ ~ p=1.000 n=6
Types 267,258 267,258 ~ ~ ~ p=1.000 n=6
Memory used 2,360,471k (± 0.03%) 2,360,469k (± 0.02%) ~ 2,360,020k 2,361,061k p=1.000 n=6
Parse Time 5.20s (± 0.85%) 5.21s (± 0.54%) ~ 5.16s 5.24s p=0.574 n=6
Bind Time 1.80s (± 0.45%) 1.81s (± 0.76%) ~ 1.79s 1.83s p=0.865 n=6
Check Time 35.30s (± 0.27%) 35.33s (± 0.24%) ~ 35.26s 35.44s p=0.810 n=6
Emit Time 3.00s (± 0.77%) 3.01s (± 0.85%) ~ 2.99s 3.06s p=0.257 n=6
Total Time 45.30s (± 0.21%) 45.37s (± 0.17%) ~ 45.27s 45.46s p=0.128 n=6
self-build-src-public-api - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,228,025 1,228,025 ~ ~ ~ p=1.000 n=6
Types 267,258 267,258 ~ ~ ~ p=1.000 n=6
Memory used 3,038,076k (± 9.78%) 3,038,438k (± 9.78%) ~ 2,431,413k 3,160,699k p=0.575 n=6
Parse Time 6.93s (± 1.22%) 7.02s (± 1.32%) +0.09s (+ 1.37%) 6.87s 7.16s p=0.045 n=6
Bind Time 2.16s (± 1.42%) 2.15s (± 1.80%) ~ 2.09s 2.19s p=0.574 n=6
Check Time 42.86s (± 0.59%) 42.80s (± 0.36%) ~ 42.60s 42.95s p=0.575 n=6
Emit Time 3.55s (± 4.63%) 3.48s (± 2.46%) ~ 3.38s 3.59s p=0.575 n=6
Total Time 55.51s (± 0.33%) 55.46s (± 0.37%) ~ 55.07s 55.67s p=1.000 n=6
self-compiler - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 263,403 263,403 ~ ~ ~ p=1.000 n=6
Types 107,094 107,094 ~ ~ ~ p=1.000 n=6
Memory used 441,709k (± 0.02%) 441,744k (± 0.01%) ~ 441,665k 441,786k p=0.230 n=6
Parse Time 3.55s (± 0.49%) 3.56s (± 0.95%) ~ 3.52s 3.61s p=0.568 n=6
Bind Time 1.33s (± 0.88%) 1.32s (± 1.01%) ~ 1.30s 1.34s p=0.117 n=6
Check Time 19.00s (± 0.40%) 19.01s (± 0.37%) ~ 18.91s 19.09s p=0.748 n=6
Emit Time 1.53s (± 0.69%) 1.54s (± 0.89%) +0.02s (+ 1.31%) 1.53s 1.56s p=0.033 n=6
Total Time 25.41s (± 0.20%) 25.45s (± 0.22%) ~ 25.37s 25.53s p=0.336 n=6
ts-pre-modules - node (v18.15.0, x64)
Errors 71 71 ~ ~ ~ p=1.000 n=6
Symbols 225,981 225,981 ~ ~ ~ p=1.000 n=6
Types 94,356 94,356 ~ ~ ~ p=1.000 n=6
Memory used 371,296k (± 0.01%) 371,363k (± 0.03%) ~ 371,252k 371,600k p=0.378 n=6
Parse Time 2.90s (± 0.22%) 2.91s (± 0.99%) ~ 2.86s 2.94s p=0.411 n=6
Bind Time 1.61s (± 1.46%) 1.61s (± 0.88%) ~ 1.59s 1.63s p=0.807 n=6
Check Time 16.52s (± 0.42%) 16.49s (± 0.28%) ~ 16.43s 16.57s p=0.627 n=6
Emit Time 0.00s (±244.70%) 0.00s ~ ~ ~ p=0.405 n=6
Total Time 21.03s (± 0.33%) 21.01s (± 0.16%) ~ 20.97s 21.05s p=0.936 n=6
vscode - node (v18.15.0, x64)
Errors 4 4 ~ ~ ~ p=1.000 n=6
Symbols 3,367,726 3,367,726 ~ ~ ~ p=1.000 n=6
Types 1,141,078 1,141,078 ~ ~ ~ p=1.000 n=6
Memory used 3,425,118k (± 0.01%) 3,425,154k (± 0.01%) ~ 3,424,836k 3,425,355k p=0.575 n=6
Parse Time 14.58s (± 0.28%) 14.59s (± 0.57%) ~ 14.53s 14.75s p=1.000 n=6
Bind Time 4.68s (± 0.53%) 4.67s (± 0.30%) ~ 4.65s 4.68s p=0.291 n=6
Check Time 93.06s (± 3.71%) 91.20s (± 0.16%) ~ 90.99s 91.34s p=0.378 n=6
Emit Time 29.69s (± 2.25%) 29.44s (± 0.52%) ~ 29.32s 29.73s p=1.000 n=6
Total Time 142.00s (± 2.88%) 139.90s (± 0.13%) ~ 139.60s 140.15s p=0.575 n=6
webpack - node (v18.15.0, x64)
Errors 2 2 ~ ~ ~ p=1.000 n=6
Symbols 316,147 316,147 ~ ~ ~ p=1.000 n=6
Types 140,126 140,126 ~ ~ ~ p=1.000 n=6
Memory used 470,923k (± 0.02%) 470,836k (± 0.04%) ~ 470,538k 471,017k p=0.298 n=6
Parse Time 5.17s (± 0.56%) 5.16s (± 0.82%) ~ 5.11s 5.21s p=0.572 n=6
Bind Time 2.24s (± 0.87%) 2.26s (± 1.17%) ~ 2.21s 2.28s p=0.124 n=6
Check Time 25.81s (± 0.38%) 26.06s (± 2.07%) ~ 25.70s 27.14s p=0.378 n=6
Emit Time 0.00s (±244.70%) 0.00s ~ ~ ~ p=0.405 n=6
Total Time 33.22s (± 0.30%) 33.48s (± 1.61%) ~ 33.08s 34.55s p=0.378 n=6
xstate-main - node (v18.15.0, x64)
Errors 5 5 ~ ~ ~ p=1.000 n=6
Symbols 570,517 570,517 ~ ~ ~ p=1.000 n=6
Types 191,417 191,417 ~ ~ ~ p=1.000 n=6
Memory used 500,781k (± 0.05%) 500,818k (± 0.03%) ~ 500,602k 501,005k p=1.000 n=6
Parse Time 4.32s (± 0.60%) 4.32s (± 0.32%) ~ 4.30s 4.33s p=0.625 n=6
Bind Time 1.53s (± 0.79%) 1.54s (± 0.68%) ~ 1.52s 1.55s p=0.868 n=6
Check Time 25.38s (± 2.91%) 25.17s (± 1.97%) ~ 24.73s 26.04s p=0.630 n=6
Emit Time 0.00s 0.00s (±244.70%) ~ 0.00s 0.01s p=0.405 n=6
Total Time 31.24s (± 2.31%) 31.02s (± 1.57%) ~ 30.56s 31.87s p=0.630 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Compiler-Unions - node (v18.15.0, x64)
  • angular-1 - node (v18.15.0, x64)
  • mui-docs - node (v18.15.0, x64)
  • self-build-src - node (v18.15.0, x64)
  • self-build-src-public-api - node (v18.15.0, x64)
  • self-compiler - node (v18.15.0, x64)
  • ts-pre-modules - node (v18.15.0, x64)
  • vscode - node (v18.15.0, x64)
  • webpack - node (v18.15.0, x64)
  • xstate-main - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

@RyanCavanaugh Here are the results of running the top 400 repos with tsc comparing main and refs/pull/61608/merge:

Everything looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Backlog Bug PRs that fix a backlog bug
Projects
Status: Not started
Development

Successfully merging this pull request may close these issues.

Unexpected circularity error when variable is initialized from a class member
2 participants