Repository navigation
fix dunder slots support hover #4344 - #5628
asukaminato0721 wants to merge 16 commits into
Conversation
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 D131487564. (Because this pull request was imported automatically, there will not be any future comments.) |
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, this fixes the misleading (attribute) bar: Literal['bar'] hover from #5627, and the tests cover type parameters and nested classes. Requesting changes for two reasons: (1) as noted on #5627, please fold this fix into #5627, so the intermediate hover never ships; (2) it adds another AST-matching path through dunder_slots_entry, which the reviews on #5627/#5629 ask to replace with one slots-entry record built from the KeyClassField(class, "__slots__") binding. These comments cover only the increment over #5627.
| } | ||
|
|
||
| /// Return the instance attribute type named by a literal `__slots__` entry. | ||
| pub(crate) fn dunder_slots_entry_type( |
There was a problem hiding this comment.
| }; | ||
| self.ad_hoc_solve(handle, "dunder_slots_entry_type", |solver| { | ||
| solver | ||
| .completions(solver.instantiate(&class), Some(&name), true, false) |
There was a problem hiding this comment.
completions lists every member of the class to find one attribute. Can this use the normal attribute lookup (for example type_of_attr_get on the instance type) instead? That says what is meant, and it avoids building the full completion list on hover.
| let mut type_ = transaction | ||
| .subscript_operator_type_at(handle, position) | ||
| let mut type_ = ast | ||
| .and_then(|ast| transaction.dunder_slots_entry_type(handle, ast, position)) |
There was a problem hiding this comment.
This runs before every other hover type lookup, so every hover now pays for an AST location query. With a slots-entry record from the bindings, the check becomes a cheap range lookup. Or, at minimum, run it only when the cursor is in a string literal.
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.
Read the owning class and literal slot name from the existing AST nodes. Use attribute definition lookup to navigate to the field declaration.
Resolve slot names against a generic instance of their owning class. This displays the field type while preserving its type parameters.
ec79306 to
cbb8633
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
Folded into #5627 as requested in review.
stack on #5627
Test Plan
add test