fix Auto import suggestions for collections.abc should take precedence over typing #1685 #1949
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1685 by making auto-import suggestions prefer public module re-exports (like collections.abc) over their private implementation modules (like _collections_abc). The fix implements two key changes: (1) extending the should_include_reexport function to recognize dotted-name private shims that encode dots as underscores, and (2) deprioritizing private imports in completion sorting when a public alternative exists.
Key Changes:
- Enhanced private module shimming detection to handle stdlib patterns like
_collections_abc→collections.abc - Modified autoimport completion logic to track and deprioritize private imports when public alternatives exist
- Updated sort_text assignment to ensure consistent lexicographic ordering of completion items
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pyrefly/lib/state/lsp.rs |
Implements two-pass autoimport candidate collection: first pass collects all candidates and tracks which names have public exports, second pass assigns lower priority sort_text to private imports with public alternatives. Also extends should_include_reexport to handle dotted module name patterns. |
pyrefly/lib/test/lsp/completion.rs |
Adds test validating that public re-exports (foo.bar) are suggested before their private implementations (_foo_bar) in autoimport completions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Some stdlib shims encode dotted modules with underscores (e.g. _collections_abc). | ||
| if canonical_module.as_str().starts_with('_') && original_module.as_str().contains('.') { | ||
| let canonical_trim = canonical_module.as_str().trim_start_matches('_'); | ||
| if canonical_trim == original_module.as_str().replace('.', "_") { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for matching dotted-name private shims may produce false positives. It checks if the canonical module starts with '_' and the original contains '.', then compares by replacing dots with underscores. However, this could incorrectly match unrelated modules. For example, _foo_bar would match both foo.bar and foo_bar (a non-dotted module).
Consider adding more stringent checks, such as verifying that the canonical module is a top-level module (no dots in canonical_module), or using a more precise pattern matching approach to avoid false positives.
| #[test] | ||
| fn autoimport_prefers_public_reexport_for_dotted_private_module() { | ||
| let code = r#" | ||
| T = Thing | ||
| # ^ | ||
| "#; | ||
| let report = get_batched_lsp_operations_report_allow_error( | ||
| &[ | ||
| ("main", code), | ||
| ("_foo_bar", "Thing = 1\n"), | ||
| ("foo.bar", "from _foo_bar import Thing\n"), | ||
| ], | ||
| get_test_report(Default::default(), ImportFormat::Absolute), | ||
| ); | ||
| assert_eq!( | ||
| r#" | ||
| # main.py | ||
| 2 | T = Thing | ||
| ^ | ||
| Completion Results: | ||
| - (Variable) Thing: from foo.bar import Thing | ||
| - (Variable) Thing: from _foo_bar import Thing | ||
| # _foo_bar.py | ||
| # foo.bar.py | ||
| "# | ||
| .trim(), | ||
| report.trim(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
The test validates the general pattern of preferring public re-exports over private modules for dotted module names (e.g., foo.bar vs _foo_bar), but it doesn't test the actual use case mentioned in issue #1685: collections.abc.Iterable vs _collections_abc.Iterable. Consider adding a test case that directly validates the stdlib pattern mentioned in the issue to ensure the fix works for the actual reported problem.
This comment has been minimized.
This comment has been minimized.
|
This might conflict with some in-flight changes from @javabster, so heads up that this might need to be rebased at some point in the next week or so |
asukaminato0721
force-pushed
the
1685
branch
from
84635d1 to
9a9fa38
Compare
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
4 participants
Summary
Fixes #1685
Addressed issue 1685 by making autoimport prefer public shim modules (like collections.abc) over their private implementations and ensuring private entries are deprioritized when a public alternative exists.
Updated to recognize dotted-name private shims (e.g., _collections_abc -> collections.abc) and to sort autoimport candidates so private imports fall behind public ones when both exist.
Test Plan
test autoimport_prefers_public_reexport_for_dotted_private_module