impl Completions: Auto import with common alias("import numpy as np") #2207#2312
impl Completions: Auto import with common alias("import numpy as np") #2207#2312asukaminato0721 wants to merge 5 commits intofacebook:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements auto-import completions with common module aliases (e.g., import numpy as np) as part of addressing issue #2207. When users type common aliases like "np" or "pd", the IDE will now suggest the full aliased import and use the alias at the cursor position.
Changes:
- Added a common alias mapping for popular Python libraries (numpy, pandas, tensorflow, matplotlib, scipy, etc.)
- Modified auto-import completion logic to suggest aliased imports when users type known aliases
- Updated quick fix code actions to offer aliased imports for unknown names that match common aliases
- Added deduplication logic to prevent showing both aliased and non-aliased import suggestions for the same module
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| pyrefly/lib/state/ide.rs | Adds COMMON_MODULE_ALIASES map and helper function common_alias_target_module; extends import_regular_import_edit to support optional alias parameter |
| pyrefly/lib/lsp/wasm/completion.rs | Implements alias-aware auto-import completions with deduplication; allows aliases to bypass MIN_CHARACTERS_TYPED_AUTOIMPORT threshold |
| pyrefly/lib/state/lsp.rs | Implements alias-aware quick fixes for unknown names with deduplication logic |
| pyrefly/lib/test/lsp/completion.rs | Adds test for scipy.io/spio alias import completion behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[test] | ||
| fn autoimport_common_alias_for_module() { | ||
| let code = r#" | ||
| T = spio | ||
| # ^ | ||
| "#; | ||
| let files = [("main", code), ("scipy.io", "data = 1\n")]; | ||
| let (handles, state) = mk_multi_file_state(&files, Require::Exports, false); | ||
| let handle = handles.get("main").unwrap(); | ||
| let position = extract_cursors_for_test(code)[0]; | ||
| let completions = | ||
| state | ||
| .transaction() | ||
| .completion(handle, position, ImportFormat::Absolute, true); | ||
| let autoimport = completions | ||
| .into_iter() | ||
| .find(|item| item.label == "spio") | ||
| .expect("expected spio to be in completions"); | ||
| assert!( | ||
| autoimport | ||
| .detail | ||
| .as_ref() | ||
| .is_some_and(|detail| detail.contains("import scipy.io as spio")), | ||
| "expected alias import detail for spio, got {:?}", | ||
| autoimport.detail | ||
| ); | ||
| assert_eq!(autoimport.insert_text.as_deref(), Some("spio")); | ||
| let label_details = autoimport | ||
| .label_details | ||
| .expect("auto import completion should include label details"); | ||
| assert_eq!( | ||
| label_details.detail.as_deref(), | ||
| Some(" (import scipy.io as spio)") | ||
| ); | ||
| assert_eq!(label_details.description.as_deref(), Some("scipy.io")); | ||
| } |
There was a problem hiding this comment.
The test doesn't verify that duplicate suggestions are prevented. Since one of the key features mentioned in the PR description is "avoiding duplicate non-aliased suggestions", consider adding an assertion to check that there's no duplicate "scipy.io" entry without an alias in the completions list. This would ensure the deduplication logic in aliased_modules is working correctly.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
connernilsen
left a comment
connernilsen
left a comment
There was a problem hiding this comment.
Looks good, just a few small things I'd like to see addressed before pulling this in. Let me know if you disagree with anything, down to discuss or change my mind.
| }; | ||
| for (handle_to_import_from, name, export) in | ||
| self.search_exports_fuzzy(identifier.as_str()) | ||
| let identifier_text = identifier.as_str(); |
There was a problem hiding this comment.
nit: can we pull the common alias part out into a separate function for readability?
| ); | ||
| assert_eq!(label_details.description.as_deref(), Some("numpy")); | ||
| } | ||
|
|
There was a problem hiding this comment.
If we end up adding checks to make sure the suggested alias imports aren't in user-space code, then we should add a test for that too.
|
Also adding @kinto0, since he may have more comments than I do. |
This comment has been minimized.
This comment has been minimized.
|
I'll pull it in and see what people think |
connernilsen
left a comment
connernilsen
left a comment
There was a problem hiding this comment.
Okay, import seems to be failing, let me try approving first
|
Looks like GH is down ;( |
|
Hey @asukaminato0721, looks like there are a few merge conflicts. Would you be able to resolve them when you get a chance? It looks like GH is working again, so I'll pull the changes in afterward. |
asukaminato0721
force-pushed
the
2207-4
branch
from
4e2efe3 to
17efbf5
Compare
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
@connernilsen has imported this pull request. If you are a Meta employee, you can view this in D93129678. |
kinto0
left a comment
kinto0
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
|
@connernilsen merged this pull request in ce4e6d6. |
Summary
part of #2207
Added a common-alias map and helper lookups for auto-imports.
Auto-import completions and unknown-name quick fixes now honor common aliases (inserting
import <module> as <alias>and using the alias at the cursor) while avoiding duplicate non-aliased suggestions.Test Plan
Added a completion test covering alias import behavior for
spio -> scipy.io