[ty] Fix protocol interface inference for stub protocols and subprotocols #19950
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ use std::{collections::BTreeMap, ops::Deref}; | |
| use itertools::Itertools; | ||
|
|
||
| use ruff_python_ast::name::Name; | ||
| use rustc_hash::FxHashMap; | ||
|
|
||
| use super::TypeVarVariance; | ||
| use crate::semantic_index::place_table; | ||
|
|
@@ -286,6 +287,7 @@ impl<'db> ProtocolMemberData<'db> { | |
| struct ProtocolMemberDataDisplay<'db> { | ||
| db: &'db dyn Db, | ||
| data: ProtocolMemberKind<'db>, | ||
| qualifiers: TypeQualifiers, | ||
| } | ||
|
|
||
| impl std::fmt::Display for ProtocolMemberDataDisplay<'_> { | ||
|
|
@@ -305,7 +307,12 @@ impl<'db> ProtocolMemberData<'db> { | |
| d.finish() | ||
| } | ||
| ProtocolMemberKind::Other(ty) => { | ||
| f.write_str("AttributeMember(")?; | ||
| write!(f, "`{}`", ty.display(self.db))?; | ||
| if self.qualifiers.contains(TypeQualifiers::CLASS_VAR) { | ||
| f.write_str("; ClassVar")?; | ||
| } | ||
| f.write_char(')') | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -314,6 +321,7 @@ impl<'db> ProtocolMemberData<'db> { | |
| ProtocolMemberDataDisplay { | ||
| db, | ||
| data: self.kind, | ||
| qualifiers: self.qualifiers, | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -517,6 +525,12 @@ enum BoundOnClass { | |
| No, | ||
| } | ||
|
|
||
| impl BoundOnClass { | ||
| const fn is_yes(self) -> bool { | ||
| matches!(self, BoundOnClass::Yes) | ||
| } | ||
| } | ||
|
|
||
| /// Inner Salsa query for [`ProtocolClassLiteral::interface`]. | ||
| #[salsa::tracked(cycle_fn=proto_interface_cycle_recover, cycle_initial=proto_interface_cycle_initial, heap_size=ruff_memory_usage::heap_size)] | ||
| fn cached_protocol_interface<'db>( | ||
|
|
@@ -533,69 +547,69 @@ fn cached_protocol_interface<'db>( | |
| let parent_scope = parent_protocol.body_scope(db); | ||
| let use_def_map = use_def_map(db, parent_scope); | ||
| let place_table = place_table(db, parent_scope); | ||
| let mut direct_members = FxHashMap::default(); | ||
|
|
||
| // Bindings in the class body that are not declared in the class body | ||
| // are not valid protocol members, and we plan to emit diagnostics for them | ||
| // elsewhere. Invalid or not, however, it's important that we still consider | ||
| // them to be protocol members. The implementation of `issubclass()` and | ||
| // `isinstance()` for runtime-checkable protocols considers them to be protocol | ||
| // members at runtime, and it's important that we accurately understand | ||
| // type narrowing that uses `isinstance()` or `issubclass()` with | ||
| // runtime-checkable protocols. | ||
| for (symbol_id, bindings) in use_def_map.all_end_of_scope_symbol_bindings() { | ||
| let Some(ty) = place_from_bindings(db, bindings).ignore_possibly_unbound() else { | ||
| continue; | ||
| }; | ||
| direct_members.insert( | ||
| symbol_id, | ||
| (ty, TypeQualifiers::default(), BoundOnClass::Yes), | ||
| ); | ||
| } | ||
|
|
||
| for (symbol_id, declarations) in use_def_map.all_end_of_scope_symbol_declarations() { | ||
|
Contributor
There was a problem hiding this comment. We do this sort of thing a lot in ty's code base: iterating over both bindings and declarations separately, hoping that there is a 1:1 relationship. This is not always the case. For example: class D: # maybe a protocol, maybe a dataclass with fields, …
if condition:
attr: int
else:
attr = ""This is unlikely to be a problem for protocols in particular, I think. And it's certainly not something specific to this PR. But I think it would be very beneficial in a lot of places if we could somehow iterate over all definitions and then get the declared type (for Here, it would eliminate the mutation of the entry. And the bug would probably not have happened in the first place if the iterator would yield either a |
||
| let place = place_from_declarations(db, declarations).ignore_conflicting_declarations(); | ||
| if let Some(new_type) = place.place.ignore_possibly_unbound() { | ||
| direct_members | ||
| .entry(symbol_id) | ||
| .and_modify(|(ty, quals, _)| { | ||
| *ty = new_type; | ||
| *quals = place.qualifiers; | ||
| }) | ||
| .or_insert((new_type, place.qualifiers, BoundOnClass::No)); | ||
| } | ||
| } | ||
|
|
||
| for (symbol_id, (ty, qualifiers, bound_on_class)) in direct_members { | ||
| let name = place_table.symbol(symbol_id).name(); | ||
| if excluded_from_proto_members(name) { | ||
| continue; | ||
| } | ||
| if members.contains_key(name) { | ||
| continue; | ||
| } | ||
|
|
||
| let member = match ty { | ||
| Type::PropertyInstance(property) => ProtocolMemberKind::Property(property), | ||
| Type::Callable(callable) | ||
| if bound_on_class.is_yes() && callable.is_function_like(db) => | ||
| { | ||
| ProtocolMemberKind::Method(callable) | ||
| } | ||
| Type::FunctionLiteral(function) if bound_on_class.is_yes() => { | ||
| ProtocolMemberKind::Method(function.into_callable_type(db)) | ||
| } | ||
| _ => ProtocolMemberKind::Other(ty), | ||
| }; | ||
|
|
||
| members.insert( | ||
| name.clone(), | ||
| ProtocolMemberData { | ||
| kind: member, | ||
| qualifiers, | ||
| }, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| ProtocolInterface::new(db, members) | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The polarity of these tests has changed. They previously asserted that
Foowas not a a subtype ofHasXWithDefault. Indeed,Foodoes not have anxattribute with a default, so if the current tests reflect the intended behavior, can we add a comment why this is correct (is the fact that the protocol member has a default value irrelevant?)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I keep on changing my mind about this, and need to make a decision 😆 thanks for calling it out.
As the conformance-suite diff shows, the conformance suite tests include an example where a class with implicit instance attributes is considered a subtype of a protocol where the member is bound in the class body. I think you can make that sound, but only if you say that the instance attribute is not present on the meta-protocol (
type[HasXWithDefault]), which would obviously imply that we'd do attribute lookup differently fortype[]Protocol types than we would fortype[]nominal types.I was actually confused about why the conformance suite diff changed, though, and you made me look again and realise that this PR was fixing another bug. We were inferring this protocol as having an
x: Literal[0]member rather than anx: intmember:I'll add a comment by this test, and add another test that explicitly asserts that we infer the correct interface for members that have default values.