-
Notifications
You must be signed in to change notification settings - Fork 13k
Add test cases for symbol auto-import bug with existing type-only imports #61897
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
Conversation
Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
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.
This is a good start and you're almost there. Keep thinking. It's not like you have somewhere else to be.
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.
Keep going, I believe in you
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.
@copilot let's try again with the progress you've made so far!
Add explicit handling for ElementAccessExpression in isInExpressionContext to ensure symbols used as property keys (e.g., thing[SYM_FOO_BAR]) are correctly identified as being in expression context, causing them to be imported as values rather than types. Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
I've implemented the fix! The issue was in the |
Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
src/compiler/utilities.ts
Outdated
case SyntaxKind.SatisfiesExpression: | ||
return node === (parent as SatisfiesExpression).expression; | ||
case SyntaxKind.ElementAccessExpression: | ||
return node === (parent as ElementAccessExpression).argumentExpression; |
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.
This is not a correct fix. In the element access expression foo[bar]
, both foo
and bar
are expression contexts.
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.
You're correct that my initial fix was incomplete. I've updated the ElementAccessExpression case to handle both the expression and argumentExpression parts. However, the test is still failing, so I need to investigate further. The core issue seems to be that isValidTypeOnlyAliasUseSite isn't properly accounting for expression context when determining import behavior. Let me continue debugging this. Commit: 7b29d31
Updated the fix to handle both expression and argumentExpression in ElementAccessExpression, and also modified isValidTypeOnlyAliasUseSite to check isInExpressionContext. However, the test is still failing and needs further investigation. Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
This PR adds comprehensive test cases that reproduce the bug described in #61894, where symbol properties are incorrectly imported as types instead of values when there's an existing type-only import.
Issue Description
When auto-completing symbol properties in element access expressions (e.g.,
thing[SYM_FOO_BAR]
) with an existing type-only import, TypeScript incorrectly adds the symbol to the type-only import instead of importing it as a value.Test Cases Added
completionsSymbolAutoimportFromTypeOnlyImport.ts
- Reproduces the exact bug:completionsSymbolAutoimportFromTypeOnlyImportControl.ts
- Control test showing correct behavior without existing type-only import:Root Cause Identified
The investigation reveals that
isValidTypeOnlyAliasUseSite(location)
returnstrue
in the dot completion context for symbol properties, when it should returnfalse
since symbols are used as values in property access expressions.Testing
These test cases provide a foundation for future investigation and validation of fixes to the auto-import logic.
Addressing #61894.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.