-
Notifications
You must be signed in to change notification settings - Fork 12.9k
[WIP] Symbol properties on objects are imported by type and not by value when auto-completed #61902
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 <[email protected]>
…rts exist Co-authored-by: RyanCavanaugh <[email protected]>
…nly imports Co-authored-by: RyanCavanaugh <[email protected]>
Co-authored-by: RyanCavanaugh <[email protected]>
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 you're almost there! Keep going
Thanks for the encouragement! I'm working on refining the fix. The logic is in the right place but I need to debug why value imports are still being added to type-only imports. I'll continue iterating on the solution. |
Co-authored-by: RyanCavanaugh <[email protected]>
Co-authored-by: RyanCavanaugh <[email protected]>
Co-authored-by: RyanCavanaugh <[email protected]>
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 keep going
I'm debugging why the value import is still being added to the type-only import. The fix should prevent this but VALUE is still ending up in the type-only import instead of creating a mixed import. Investigating the control flow. |
Co-authored-by: RyanCavanaugh <[email protected]>
The ideal fix looks like // From
import type { Foo } from "./bar";
// to
import { Baz, type Foo } from "./bar"; but it kept coming back to a fix that produces // From
import type { Foo } from "./bar";
// to
import type { Foo } from "./bar";
import { Baz } from "./bar"; I think it's confused because of how synthesized completion members interact with bracket vs dot notation. The likely fix is probably in completions.ts, not importFixes. |
Root Cause: When there's an existing type-only import, the auto-import system incorrectly adds value imports to it instead of creating mixed imports or separate imports.
Fix: Modified
tryAddToExistingImport
inimportFixes.ts
to:addAsTypeOnly
isAllowed
but should prefer value importsTesting: Verified that completely disabling existing import addition fixes the issue (creates separate imports). The targeted fix is in the right location but may need refinement.
Test Case: Simple case that reproduces the issue:
Fixes #61894.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.