Repository navigation
fix dunder slots references #4344 - #5629
asukaminato0721 wants to merge 19 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 D131487820. (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 for the coverage here (Unicode names, class identity with Inner/Other/a local __slots__, unopened indexed files), and the is_attribute refactor is clean. Requesting changes because the approach repeats the pattern from the earlier review on #5587/#5588: reference identity is reconstructed through navigation (find_definition) over strings found by an AST walk, and the global path fully solves unrelated modules. Details inline. These comments cover only the increment over #5628.
Suggested direction, shared with my comment on #5627: one slots-entry record (name, string content range, owning class) built from the KeyClassField(class, "__slots__") binding. Local references check the entry's class and name against the definition. Global references index the entries, like #5588 does for __all__ occurrences, or at least filter by name at the binding level before solving. Definition, hover, references, and rename (#5631) can then all consume the same data.
| if is_attribute | ||
| && self | ||
| .get_bindings_metadata(handle) | ||
| .is_some_and(|metadata| metadata.has_class_field(&dunder::SLOTS)) |
There was a problem hiding this comment.
For every attribute/variable reference query in a module that has any slotted class, this walks the whole AST, collects every string equal to the name, and runs Ast::locate_node + dunder_slots_entry + find_definition on each one. The cost scales with the module, not with the number of slots entries. With a retained slots-entry record (name, content range, owning class), this becomes a lookup of the entries whose name matches.
| let matches_definition = definition_matcher(module, definition_range); | ||
| for range in literals { | ||
| if dunder_slots_entry(&Ast::locate_node(&ast, range.start())).is_some() | ||
| && let Ok(definitions) = self.find_definition( |
There was a problem hiding this comment.
This uses go-to-definition results as reference identity, which is the pattern the earlier review on #5587/#5588 asked to avoid: navigation can pick a stub, another file copy, or one declaration among several. Please resolve the entry through its owning class (from the __slots__ class-field binding) and the existing attribute semantics, then compare against the definition.
| definition_kind, | ||
| DefinitionMetadata::Attribute | DefinitionMetadata::VariableOrAttribute(_) | ||
| ) { | ||
| // Slot strings require the candidate's class bindings for attribute resolution. |
There was a problem hiding this comment.
This branch also matches VariableOrAttribute, which covers plain module-level variables. So a references query on an ordinary exported variable now runs Require::Everything on every candidate module that has any class with __slots__, before checking whether those slots name this symbol or whether the class is related. Please index the slots entries (as #5588 does for __all__), or at least filter candidates by slot name at the binding level before promoting them.
| references.sort(); | ||
| assert_eq!( | ||
| references, | ||
| vec![("base".to_owned(), 1), ("child".to_owned(), 1)] |
There was a problem hiding this comment.
Here Child(Base) redeclares foo in its own __slots__, and the test counts it as a reference to Base.foo. At runtime Python creates a separate slot descriptor on Child. Folding it into the base attribute may be the intended behavior for rename, but please make that a deliberate decision and state it in the summary.
| "__slots__ = [r'foo']", | ||
| "__slots__ = {\"foo\": \"Field documentation.\"}", | ||
| "__slots__: tuple[str, ...] = (\"\"\"foo\"\"\",)", | ||
| "names = __slots__ = (\"foo\",)", |
There was a problem hiding this comment.
Same note as on #5627: if slots entries come from the class-field binding, I believe the chained form binds its value as ExprOrBinding::Binding, and the checker's explicit_slots treats it as Unknown. I suggest dropping this case so the IDE matches the checker.
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.
Resolve each slot string to its attribute before adding its content range. Load class bindings only for candidate modules with __slots__ fields so indexed files also return these references.
af82df7 to
4f9121e
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
A subclass slot string uses the checker's existing inherited attribute identity when the subclass has no separate field declaration. This is intentional for references and rename, although Python can create a separate runtime descriptor.
stack on #5627
Test Plan
add test