-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[red-knot] Infer the members of a protocol class #17556
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
9255038 to
0d68ffd
Compare
a08a191 to
afb9ea0
Compare
0d68ffd to
4ded3fb
Compare
afb9ea0 to
8906bc9
Compare
4ded3fb to
ece0c86
Compare
8906bc9 to
b4e8e97
Compare
ece0c86 to
b3a1538
Compare
b4e8e97 to
1809727
Compare
b3a1538 to
e34384c
Compare
1809727 to
6aa7491
Compare
| pub(super) struct ProtocolClassLiteral<'db>(ClassLiteralType<'db>); | ||
|
|
||
| impl<'db> ProtocolClassLiteral<'db> { | ||
| pub(super) fn members(self, db: &'db dyn Db) -> &'db ordermap::set::Slice<Name> { |
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.
Keep in mind (both in terms of method naming, and where you put what functionality) that autocomplete is definitely going to require the ability for ClassLiteralType itself to have a "list all members" method. And if ProtocolClassLiteral will be used inside a future Type::ProtocolInstance, it will probably also need to expose that list-all-members method, without all the Protocol-specific filtering. That could suggest some future refactoring to share some aspects of list-members functionality, but more concretely I think it suggests that we should reserve the method name members for the general "list all known members" that all types will need, and use something like protocol_members to distinguish this protocol-specific method.
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.
autocomplete
for some reason it took me a moment there to realise that you mean "the autocompletion feature we intend to build into the red-knot LSP for IDE users" rather than "autocompletion in the IDE now for red-knot developers" 😆
That could suggest some future refactoring to share some aspects of list-members functionality, but more concretely I think it suggests that we should reserve the method name
membersfor the general "list all known members" that all types will need, and use something likeprotocol_membersto distinguish this protocol-specific method.
Interesting -- I think they will end up doing nearly the same thing. I suppose the only difference between them really is that the members method I'm adding here (which I'll rename to protocol_members) should not include implicit instance attributes (these are illegal in protocol classes, and do not constitute protocol members). But the autocomplete feature probably should include those members:
class Foo(Protocol):
x: int
def __init__(self):
self.y = 42 # we should emit an error here,
# and not infer `Foo` as having a protocol member `y`
# (it should only have a single member, `x`)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.
Right, makes sense. It seems like also the filtering-out of some "special" names is something we might not want to do for autocomplete?
e34384c to
0c975b4
Compare
6aa7491 to
a8fba56
Compare
| members.extend( | ||
| use_def_map | ||
| .all_public_declarations() | ||
| .flat_map(|(symbol_id, declarations)| { | ||
| symbol_from_declarations(db, declarations) | ||
| .map(|symbol| (symbol_id, symbol)) | ||
| }) | ||
| .filter_map(|(symbol_id, symbol)| { | ||
| symbol.symbol.ignore_possibly_unbound().map(|_| symbol_id) | ||
| }) | ||
| .chain(use_def_map.all_public_bindings().filter_map( | ||
| |(symbol_id, bindings)| { | ||
| symbol_from_bindings(db, bindings) | ||
| .ignore_possibly_unbound() | ||
| .map(|_| symbol_id) | ||
| }, | ||
| )) | ||
| .map(|symbol_id| symbol_table.symbol(symbol_id).name()) | ||
| .filter(|name| !excluded_from_proto_members(name)) | ||
| .cloned(), | ||
| ); | ||
| } |
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.
in due course this will need to be converted to a HashMap of some kind where the keys are the names of the protocol members and the values are the types of the protocol members. But I don't want to do that yet because I'm not yet sure whether e.g. an attribute member annotated with Callable should be treated the same as a method member (I need to write some more tests...)
carljm
left a comment
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.
Looks great!
e96e63f to
7ced9ed
Compare
Pull request was closed
* main: [red-knot] fix collapsing literal and its negation to object (#17605) [red-knot] Add more tests for protocols (#17603) [red-knot] Ban direct instantiations of `Protocol` classes (#17597) [`pyupgrade`] Preserve parenthesis when fixing native literals containing newlines (`UP018`) (#17220) [`airflow`] fix typos (`AIR302`, `AIR312`) (#17574) [red-knot] Special case `@abstractmethod` for function type (#17591) [red-knot] Emit diagnostics for isinstance() and issubclass() calls where a non-runtime-checkable protocol is the second argument (#17561) [red-knot] Infer the members of a protocol class (#17556) [red-knot] Add `FunctionType::to_overloaded` (#17585) [red-knot] Add mdtests for `global` statement (#17563) [syntax-errors] Make duplicate parameter names a semantic error (#17131)
Summary
This PR adds the necessary machinery to infer the members of a protocol class, and uses that machinery to infer a precise return type for the
get_protocol_membersintrospection function.What this PR doesn't do is infer what the type of each protocol member is. We'll obviously have to do that in due course to properly implement protocol subtyping and assignability; when we add that functionality, I'm envisaging that
ProtocolClassLiteral::members()would return aHashMapmapping from the member to its type rather than a boxed slice ofNames. I'm deliberately not doing that right now, however, as implementing all the subtyping rules around protocols is a somewhat large task, which I intend to approach incrementally over several PRs. The first stage will possibly not even consider the types of the protocol members at all.Test Plan
Existing mdtests updated.