Repository navigation
fix dunder all references #4344 - #5588
asukaminato0721 wants to merge 8 commits into
Conversation
|
This pull request has been imported. If you are a Meta employee, you can view this in D130539715. (Because this pull request was imported automatically, there will not be any future comments.) |
This comment has been minimized.
This comment has been minimized.
stroxler
left a comment
stroxler
left a comment
There was a problem hiding this comment.
This feedback applies to both #5588 and #5587, they both seem to have the same structure where the LSP is doing more work than it probably should:
Could we rework this so __all__ entries resolve through the checked module’s semantic information, rather than treating go-to-definition locations as symbol types or identities?
Navigation can select a filesystem copy instead of the editor buffer, a source file instead of a stub, or one declaration instead of the final exported binding. Those choices are appropriate for navigation, but not for determining a symbol’s type or whether two occurrences refer to it.
The reconstruction to replace is:
In both PRs, get_hover_with_verbosity substitutes a type by calling get_type_at at the navigation target’s source position.
In 5588, the added local_references_from_definition block resolves each entry through navigation, then uses exact module-path/range equality as reference identity.
The replacement should live in the semantic query/reference layer in state/lsp.rs:
For exported-symbol hover, use the current checked handle’s solved export type: get_solutions(handle) and the retained KeyExport(name) answer. This represents the end-of-module export, rather than a particular declaration’s type.
For references, reuse the existing binding/import resolution behind find_export_for_key, key_to_intermediate_definition, and the normal reference collectors/indexes. Preserve local alias identity and existing buffer/source normalization.
There is no bound-name entry for an __all__ string today, so this is not just replacing one function call. Add a small shared occurrence bridge that connects the recognized name/content range to the existing local binding or import. KeyExport supplies the type; it does not itself supply reference identity.
Keep the useful entry extraction and alias handling. No binder overhaul is needed. Add native-LSP tests with open buffers, source/stub pairs and unopened dependencies—the warm memory-backed helper tests mask these failures.
05028e3 to
e585ab2
Compare
e585ab2 to
ea59784
Compare
This comment has been minimized.
This comment has been minimized.
kinto0
left a comment
kinto0
left a comment
There was a problem hiding this comment.
Thanks, the reference identity now goes through KeyExport -> local binding -> find_export_for_key, which is the direction from the earlier review, and alias identity is preserved. The native-LSP coverage (open buffer, stub + alias, unopened re-exports) is good. Inline comments cover only the references commits; the hover commits shared with #5587 are reviewed there.
Two general notes:
- The summary says "fix go to definition for
__all__items", but this PR fixes references. Please update it. - mypy_primer reports +10/-17 errors in home-assistant
core(status_codeonNoneType,explicit-any-use) on this PR, while #5587 (same base, run at the same time) reports no change. Nothing here should change check results, so this looks like primer dependency noise. Can you rerun it, and explain the diff if it reproduces?
| infer_param_types_from_docstring: bool, | ||
| ) -> Self { | ||
| let module_range = x.range; | ||
| let dunder_all_entries = exports |
There was a problem hiding this comment.
This runs Ast::locate_node again for every __all__ entry in every module, including in CLI check mode, only to recover the content range of a string literal that the exports step already visited. Can the exports step store the content range in DunderAllEntry instead? Then Bindings does not need to search the AST, and the semantic information is extracted once, as early as possible.
| use crate::state::errors::ModuleRanges; | ||
| use crate::state::ide::IntermediateDefinition; | ||
| use crate::state::ide::key_to_intermediate_definition; | ||
| use crate::state::lsp::dunder_all_binding; |
There was a problem hiding this comment.
This makes the solver (alt) depend on state::lsp. Since dunder_all_binding only needs Bindings, can it be a method on Bindings instead of a free function in lsp.rs?
| if let Some((definition_handle, _, export)) = | ||
| self.resolve_named_import(handle, module_info.name(), name.clone(), preference) | ||
| { | ||
| let definition = match preference.import_behavior { |
There was a problem hiding this comment.
This keeps two resolution paths for the same concept: go-to-definition still uses navigation (resolve_named_import), and references use the checked binding. Can both use dunder_all_binding + find_export_for_key(handle, key, preference) and let preference decide how far to follow imports? That removes the branch and keeps go-to-definition consistent with references.
| } | ||
|
|
||
| /// Return the ranges and names of string entries in a user-specified `__all__`. | ||
| pub fn dunder_all_names(&self) -> Option<impl Iterator<Item = (TextRange, &Name)>> { |
There was a problem hiding this comment.
Nit: the Option wrapper makes callers write .into_iter().flatten(). Returning a plain iterator that is empty when __all__ is not user-specified would be simpler.
| } | ||
|
|
||
| #[test] | ||
| fn dunder_all_global_references_respect_definition_identity() { |
There was a problem hiding this comment.
Nit: this is a global-references test with real files in a temp dir, but it is in local_find_refs.rs. It also covers the same case as references_dunder_all_in_unopened_reexports in lsp_interaction/references.rs. Can you drop one of them, or move this one next to the other global-reference tests?
Use the existing definition lookup to read the exported symbol type. Keep string literal types for strings outside __all__.
Read the current checked module’s final export answer so buffers, stubs, and unopened dependencies supply the correct type.
Resolve __all__ entries only after identifier lookup fails. Cover go-to-type-definition and remove unrelated hover changes.
Resolve each entry to its definition before adding its string content range. Respect import aliases so references to independent names remain separate.
Preserve local alias identity and retain export occurrences in the normal reference index so unopened modules need no navigation queries.
Keep string content ranges from export extraction and resolve navigation and references through the checked binding. Preserve interface documentation without changing symbol identity.
ea59784 to
e36ec6d
Compare
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Summary
Fixes part of #4344
fix references for
__all__items through the checked local binding or import.stack on #5587
Test Plan
add test