-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
base: main
Are you sure you want to change the base?
Conversation
…erty key equivalence Fixes microsoft#61606. Need PR results to see if this is worth it or not.
@typescript-bot test it |
There was a problem hiding this 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)) {
Hey @RyanCavanaugh, the results of running the DT tests are ready. Everything looks the same! |
@RyanCavanaugh Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
@RyanCavanaugh Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@RyanCavanaugh Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Fixes #61606. Need PR results to see if this is worth it or not.
Update: Seems good?