Repository navigation
fix dunder all rename #4344 - #5625
asukaminato0721 wants to merge 11 commits into
Conversation
|
✅ This PR meets all requirements from our pull request checklist. Optional recommendations:
Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour. |
|
This pull request has been imported. If you are a Meta employee, you can view this in D131486069. (Because this pull request was imported automatically, there will not be any future comments.) |
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| /// Return the content range of a single string literal with the given expression range. | ||
| fn string_literal_content_range(ast: &ModModule, range: TextRange) -> Option<TextRange> { |
There was a problem hiding this comment.
This searches the AST again for a string literal whose content range #5588 already computes (Bindings::dunder_all_entries, or DunderAllEntry if the range moves into the exports step as suggested there). Can prepare_rename read the stored content range instead, so this helper is not needed?
| handle, | ||
| position, | ||
| FindPreference { | ||
| import_behavior: if dunder_all_entry.is_some() { |
There was a problem hiding this comment.
Switching import_behavior only for __all__ entries adds a special case to prepare_rename. If go-to-definition and references for __all__ entries both go through dunder_all_binding + find_export_for_key (my comment on #5588), can this use one preference for all targets?
| .map(Vec1::into_vec) | ||
| .unwrap_or_default(); | ||
|
|
||
| if dunder_all_entry.is_some() |
There was a problem hiding this comment.
This condition is hard to follow: it rejects the entry if any definition is bundled, or if a definition lands on another __all__ string. Can it be expressed directly from the binding instead? For example: reject when dunder_all_binding finds no binding, or when the resolved export is bundled. Please also add a short comment that states the invariant.
| &source_handle, | ||
| TextSize::try_from(position).unwrap(), | ||
| FindPreference { | ||
| import_behavior: ImportBehavior::StopAtRenamedImports, |
There was a problem hiding this comment.
This changes the shared replace_paired_references helper for every paired-rename test, not only the __all__ ones. Is that intended? If yes, please explain in the summary why StopAtRenamedImports is right for the existing tests too.
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.
Use string content ranges to preserve quotes and prefixes. Keep local source definitions during reference operations so rename updates both source files and their stubs. Validate the target before offering rename.
8f47c09 to
0fd832e
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
support rename for
__all__The shared paired-rename test helper uses
StopAtRenamedImportsso local aliases keep their own identity, consistent with rename.stack on #5588
Test Plan
add test