-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
PEP 544: Protocols #224
PEP 544: Protocols #224
Conversation
I'm excited by this proposal but couldn't help but make some minor changes while reading through it. I hope they'll help make it more readable.
Copyediting changes
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.
Left some comments based on a quick read-through.
pep-0544.txt
Outdated
provide sophisticated runtime instance and class checks against protocol | ||
classes. This would be difficult and error-prone and will contradict the logic | ||
of PEP 484. As well, following PEP 484 and PEP 526 we state that protocols are | ||
**completely optional** and there is no intent to make them required. |
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.
It's a little unclear what 'completely optional' means here and whether the following sentences are related to this. This could perhaps mean some of these things:
- They are optional like everything defined in PEP 484: They don't affect Python runtime semantics and they are merely a feature in
typing
. Programmers are free to not use them. - The standard library doesn't use protocols outside
typing
. - Programmers are free to not use them even if they use type annotations.
- Python implementations don't need to support them.
This could rephrased like this to avoid the ambiguity:
... are completely optional:
- No runtime semantics will be imposed for variables or parameters annotated with a protocol class.
- Any checks will be performed only by third-party type checkers and other tools.
- ... maybe more
There is no intent to make protocols non-optional in the future.
a compatible type signature. | ||
|
||
|
||
Protocol members |
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.
This seems to imply that sequences no longer support iteration, since Sequence.__iter__
is not abstract and thus is not a protocol member. Is this intentional?
My view is that protocols should not have non-protocol attributes or methods. For example, none of the ABCs in collections.abc
seem to have what I'd consider non-protocol members. Protocols should describe an interface, not an implementation. Default implementations are okay, but they should probably only use other members defined in the protocol.
If we want to support non-protocol members, I think we should restrict them to names with a double underscore prefix to make this super explicit. However, even this brings in some arguably unneeded complexity: self will now have a special type, since accessing non-protocol members is only safe through self
. Example:
class C(Protocol):
...
def f(self, other: 'C') -> None:
self.__g() # ok
other.__g() # not ok
def __g(self) -> None:
<something>
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.
This seems to imply that sequences no longer support iteration, since
Sequence.__iter__
is not abstract and thus is not a protocol member. Is this intentional?
Sequence
implements __iter__
therefore it will be accepted wherever Iterable
is expected. But asking for __iter__
on Sequence
indeed will fail.
This indeed looks problematic. Here is a solution that I could propose: we do not have such thing as non-protocol members. However, there will be two kinds of members, abstract and non-abstract. All of them should be implemented in order to consider a class as structural (implicit) subtype.
The reason to have non-abstract methods is that one can get them "for free" using explicit subclassing provided abstract methods are implemented in subclass. So that for larger protocols like Sequence
or Mapping
one doesn't need to do
def method(self):
return super().method()
for every method.
This coincides with how ABCs work at runtime, so that this will be intuitive for someone who already worked with ABCs.
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.
Protocols should describe an interface, not an implementation.
👍
Default implementations are okay, but they should probably only use other members defined in the protocol.
👍
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.
Here is a solution that I could propose: we do not have such thing as non-protocol members.
That's now specified below in the PEP right?
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.
That's now specified below in the PEP right?
Yes.
pep-0544.txt
Outdated
Generic protocol types follow the same rules of variance as non-protocol | ||
types. Protocols can be used in all special type constructors provided | ||
by the ``typing`` module and follow the corresponding subtyping rules. | ||
For example, protocol ``PInt`` is a subtype of ``Union[PInt, int]``. |
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.
This is unclear. What is PInt
?
pep-0544.txt
Outdated
|
||
Generic protocol types follow the same rules of variance as non-protocol | ||
types. Protocols can be used in all special type constructors provided | ||
by the ``typing`` module and follow the corresponding subtyping rules. |
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.
Could this be rephrased as something like "Protocol types can be used in all contexts where any other types can used, such as in (examples). Generic protocols follow the rules for generic abstract classes, except for using structural compatibility instead of compatibility defined by inheritance relationships."
pep-0544.txt
Outdated
finish(GoodJob()) # OK | ||
|
||
In addition, we propose to add another special type construct | ||
``All`` that represents intersection types. Although for normal types |
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.
I'd consider leaving out All
from this proposal, at least initially. This is something we can add later if there is a need, but currently I don't see evidence for this being important. Effectively the same behavior can be achieved through multiple inheritance. Besides, it would be a bit arbitrary if All
only worked with protocols, and making All
work with arbitrary types would be a much more complex feature, and not as directly related to this PEP.
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.
I think we could include this for three reasons:
- I could easily imagine a function parameter that requires more than one protocol, especially if the preferred style is to have several simple protocols, rather than few complex. By the way, some classes in
collections.abc
are simple intersections, for exampleCollection == All[Sized, Iterable, Container]
. - It could be easily implemented for protocols. I also propose to use
All
instead ofIntersection
to emphasize it only works for protocols (mnemonics: variable implements all these protocols). On the contrary, I think that intersections of nominal classes would be very rarely used. - If we decide to include this later, then people might simply be not aware of this feature (this is a minor reason, but still we need to take this into account).
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.
I agree with Jukka -- I think it's confusing for All
to work only with protocols, extending All
to work with all types would add considerable complexity, and I'm not convinced there's a pressing need.
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.
I was pushing for Intersection
for the original PEP because I saw the need for being able to intersect ABCs. Given that protocols themselves solve this, and we can use multiple inheritance as Jukka is suggesting, I agree that it's wiser to leave this out.
I would still like to have it, but that can be a separate PEP. I agree it's complex to implement for concrete types, and better yet, for combinations of both.
pep-0544.txt
Outdated
(``Iterable`` is statically equivalent to ``Iterable[Any]`). | ||
The metaclass of ``typing.Protocol`` will automatically add | ||
a ``__subclasshook__()`` that provides the same semantics | ||
for class and instance checks as for ``collections.abc`` classes:: |
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.
I don't love the automatic isinstance
check support, even though that's how the mypy Protocol
class used to work. I don't think that they can be made to work reliably for arbitrary ABCs. Consider this example:
class P(Protocol):
@abstractmethod
def common_method_name(self, x: int) -> int: ...
class X:
<a bunch of methods>
def common_method_name(self) -> None: ...
def do_stuff(o: Union[P, X]) -> int:
if isinstance(o, P):
return o.common_method_name(1) # oops what if it's an X instance instead?
else:
return (do something with an X)
Maybe a type checker can warn about this example, but more generally the isinstance
check could be unreliable, except for things where there is a common signature convention such as Iterable
.
Another potentially problematic case:
class P(Protocol):
x: int
class C:
def initialize(self) -> None:
self.x = 0
c1 = C()
c2 = C()
c2.initialize()
isinstance(c1, P) # False
isinstance(c2, P) # True
def f(x: Union[P, int]) -> None:
if isinstance(x, P):
# type of x is P here
...
else:
# type of x is int here?
print(x + 1)
f(C()) # oops
Again, maybe we can live with this, but this could be pretty surprising. I'd argue that requiring an explicit class decorator would be better, since we can then attach warnings about problems like this in the documentation. The programmer would be able to evaluate whether the benefits outweigh the potential for confusion for each protocol and explicitly opt in -- but the default behavior would be safer.
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.
I'd argue that requiring an explicit class decorator would be better, since we can then attach warnings about problems like this in the documentation.
This is how I initially wrote this, since I intuitively felt that this could not be 100% reliable. Guido wanted to make it default, by now we have good examples to make this opt-in, so that I will revert this to class decorator.
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.
I'm not sure I understand how a class decorator would solve the problem. Wouldn't that mean that some classes that implement the protocol (i.e. the decorated ones) will work properly with isinstance
at runtime but others will not?
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.
classes that implement the protocol (i.e. the decorated ones)
The decorator is applied to protocol, not to implementation. This was in a previous draft. At runtime the decorator will add same __subclasshook__()
to a protocol, that collections.abc
classes have now. As Jukka pointed out, there are pros and cons of such approach, and a user could decide.
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.
How would the __subclasshook__
work?
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.
Basically, if a class has all necessary attributes, then it is a subclass. This is why I mention below that "type checkers will narrow types after such checks by the type erased Proto
(i.e. with all variables having type Any
and all methods having type Callable[..., Any]
)" because we cannot reliably check the types of attributes at runtime.
This is how it is done in collections.abc
see https://github.com/python/cpython/blob/master/Lib/_collections_abc.py#L72 and e.g. https://github.com/python/cpython/blob/master/Lib/_collections_abc.py#L93
pep-0544.txt
Outdated
# Error! Can't use 'isinstance()' with subscripted protocols | ||
|
||
|
||
Using Protocols in Python 3.5 and Earlier |
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.
What about Python 2.7?
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.
I think this could be used the same way for 2.7 - 3.5, modulo the fact that function type comments should be used in 2.7.
pep-0544.txt
Outdated
reasons: | ||
|
||
* Protocols don't have some properties of regular classes. In particular, | ||
``isinstance()`` is not always well-defined for protocols, whereas it is |
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.
A better argument would be something like this: isinstance
, as defined for classes that don't explicitly extend Protocol
, is based on the nominal hierarchy. In order to make everything a protocol by default and have isinstance
work would require changing the semantics of isinstance
, which won't happen.
mainly because their interfaces are too large, complex or | ||
implementation-oriented (for example, they may include de facto | ||
private attributes and methods without a ``__`` prefix). | ||
* Most actually useful protocols in existing Python code seem to be implicit. |
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.
Additional reason: Many built-in functions only accept concrete instances of int
(and subclass instances), and similarly for other built-in classes. Making int
a structural type wouldn't be safe without major changes to the Python runtime, which won't happen.
References | ||
========== | ||
|
||
.. [typing] |
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.
Is it customary to include references to discussions that have happened about the PEP, such as in the typing issue tracker?
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.
Yes, this is a good idea.
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.
I wonder if we could do more deep linking to specific comments or subthreads? (No big deal if you think it's too much effort, but it might save some readers some time finding the right part of the long discussion in e.g. python/typing#11 -- including myself :-).
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.
OK, will do this.
@JukkaL Thank you for helpful comments! I agree with almost all things. I will make corresponding changes in the text soon. The only thing where we do not agree is Also we probably need to find a way how to indicate whether an abstract method has a default implementation or not in stubs. |
As a general comment, I don't think it's a good idea to standardize this before having an implementation. An implementation would allow us to understand what's difficult to implement, what sorts of unexpected interactions it has with other type-system features, what parts are confusing, and what parts are practically useful. I think it will be very difficult to get an understanding of those things otherwise. |
pep-0544.txt
Outdated
----------------------------------- | ||
|
||
To explicitly declare that a certain class implements the given protocols, they | ||
can be used as regular base classes. In this case: |
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.
I'm worried this could be pretty confusing: you could have a class that explicitly implements some protocols and implicitly implements others. I don't know of any other language that allows this.
pep-0544.txt
Outdated
finish(GoodJob()) # OK | ||
|
||
In addition, we propose to add another special type construct | ||
``All`` that represents intersection types. Although for normal types |
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.
I agree with Jukka -- I think it's confusing for All
to work only with protocols, extending All
to work with all types would add considerable complexity, and I'm not convinced there's a pressing need.
|
||
Protocols are essentially anonymous. To emphasize this point, static type | ||
checkers might refuse protocol classes inside ``NewType()`` to avoid an | ||
illusion that a distinct type is provided:: |
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.
I think this restriction makes sense, actually -- trying to construct a NewType
from a protocol isn't cogent. You probably want an alias instead. Having NewType
create a non-protocol subtype is surprising behavior and is not consistent with how NewType
works in general, in my opinion. We shouldn't support this without a clear need.
pep-0544.txt
Outdated
(``Iterable`` is statically equivalent to ``Iterable[Any]`). | ||
The metaclass of ``typing.Protocol`` will automatically add | ||
a ``__subclasshook__()`` that provides the same semantics | ||
for class and instance checks as for ``collections.abc`` classes:: |
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.
I'm not sure I understand how a class decorator would solve the problem. Wouldn't that mean that some classes that implement the protocol (i.e. the decorated ones) will work properly with isinstance
at runtime but others will not?
be provided at runtime. | ||
|
||
|
||
Changes in the typing module |
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.
How do you propose to handle the backwards-incompatibility problem for all currently released versions of Python?
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 idea is that there should be no backward incompatibility. The classes in typing
module (as well as in collections.abc
) already behave like protocols at runtime:
class Whatever:
def __iter__(self): return []
assert isinstance(Whatever(), Iterable)
as I mention below "Practically, few changes will be needed in typing
since some of these classes already behave the necessary way at runtime", most of these will be changed only in typeshed.
Also, after I implement the changes proposed by Jukka, all code that passes mypy now, will still pass (unless I don't see some subtleties).
@ddfisher Of course I am not going to wait until this PEP is accepted :-) |
@ilevkivskyi when you're ready for the first version to go in just let us know and we will commit it. Then you can have discussions go to your personal fork and just send PRs for updates as needed. |
@brettcannon I implemented most comments that appeared so far, but I would prefer to wait for input from @ambv |
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.
This is very well written. Good job! See comments, @ilevkivskyi.
Let's commit, @brettcannon.
pep-0544.txt
Outdated
subclassed or registered. This is particularly difficult to do with library | ||
types as the type objects may be hidden deep in the implementation | ||
of the library. Moreover, extensive use of ABCs might impose additional | ||
runtime costs. |
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.
Moreover, extensive use of ABCs might impose additional runtime costs.
Do we have any numbers? We know ABCs aren't free but I don't know if this is worth mentioning unless it's a major factor. The rationale for protocols to be the pythonic, dynamic and idiomatic version of ABCs is strong enough.
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.
class C(collections.abc.Iterable):
def __iter__(self):
return []
is 2x slower than
class C:
def __iter__(self):
return []
The numbers for typing.Iterable
are even worse, but I have some ideas on how to improve those. The 2x slowdown is still not much, so I use "extensive" and "might". If you think it is not necessary we could remove this altogether.
specified below. The above code with a protocol class matches common Python | ||
conventions much better. It is also automatically extensible and works | ||
with additional, unrelated classes that happen to implement | ||
the required protocol. |
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.
It is also automatically extensible and works with additional, unrelated classes that happen to implement the required protocol.
Yes, this is the killer feature.
pep-0544.txt
Outdated
Before describing the actual specification, we review and comment on existing | ||
approaches related to structural subtyping in Python and other languages: | ||
|
||
* Zope interfaces [zope-interfaces]_ was one of the first widely used |
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.
as a project name, use zope.interface
(so I'd change it here).
When describing behavior (like on line 133), keep "Zope interfaces".
pep-0544.txt
Outdated
|
||
Such behavior seems to be a perfect fit for both runtime and static behavior | ||
of protocols. The main goal of this proposal is to support such behavior | ||
statically. In addition, to allow users achieving such runtime behavior |
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 main goal of this proposal is to support such behavior statically.
I'd put this point right in the rationale when you're already discussing ABCs.
to allow users achieving
Shouldn't it be "to allow users achieve"?
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.
http://ell.stackexchange.com/questions/11193/allow-to-infinitive-substantive-verb-ing says it probably should be "to achieve" here.
a compatible type signature. | ||
|
||
|
||
Protocol members |
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.
Protocols should describe an interface, not an implementation.
👍
Default implementations are okay, but they should probably only use other members defined in the protocol.
👍
pep-0544.txt
Outdated
The protocols as described here are basically a minimal extension to | ||
the existing concept of ABCs. We argue that this is the way they should | ||
be understood, instead of as something that replaces Zope interfaces, | ||
for example. Attempting such interoperabilities will significantly |
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.
Zope interfaces are a superset of Protocols defined here, just using an incompatible syntax to define them. They necessarily use Attributes because pre-PEP 526 there was no other way to annotate this. Arguably in the 3.6+ world zope.interface
could (optionally?) adopt the Protocol syntax.
In which case to achieve interoperability we'd only need to figure out a way to inform the type checker that "Interface" (or rather "InterfaceClass") is essentially "Protocol". This way we could type check successfully (pretty much ignoring implements()
and treating those classes as implicit implementations. I think this is perfectly fine as the goal is gradual typing: if the type checker stays silent in some cases, that's OK. It just never should raise false positives.
Adapters, invariants, and runtime implements/provides checkers are obviously left out. This is a non-goal.
pep-0544.txt
Outdated
|
||
def f(x: Union[P, int]) -> None: | ||
if isinstance(x, P): | ||
# type of x is P here |
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.
type of x is P here
or rather a concrete type that implements P ;-)
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 intention probably was that the static type of x
is P
.
------------------------------------------ | ||
|
||
The problem with this is instance checks could be unreliable, except for | ||
situations where there is a common signature convention such as ``Iterable``. |
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.
Can you give an example of this unreliability? This sounds like a much stronger argument than the attribute assignment presented below (read comment about it).
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.
I added a shorter version of Jukka's example here.
c = C() | ||
isinstance(c1, P) # False | ||
c.initialize() | ||
isinstance(c, P) # True |
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.
This is not surprising to me and at runtime I can see this as being a useful feature. You can imagine:
class Client(Protocol):
...
class Connected(Protocol):
connection: socket
... # later in the code
isinstance(someClient, Connected)
Maybe we don't want to encourage this programming style but it definitely descibes some real world code, sigh.
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 problem here is that the runtime and static view of the type of someClient
differ, potentially resulting in runtime type errors uncaught by type checkers, as shown by the code below this line. Since protocols are mostly about static type checking, I think it's reasonable to make restrictions that are biased towards letting static checkers do a better job.
|
||
f(C()) # oops | ||
|
||
We argue that requiring an explicit class decorator would be better, since |
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.
That's the one part I'm not sure about. Having every Protocol behave consistently at runtime would make it easier to use.
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.
As discussed in the examples I gave, it would also make protocols more fragile and potentially surprising. My argument is that explicit here is better than implicit, even if it adds a bit of work for the programmer. Also, currently my working hypothesis is that protocols won't be very common in typical statically typed code -- they will be used every now and then, but not very frequently. If correct, the extra effort needed for adding the decorator would very little and basically insignificant. If this proves to be an incorrect hypothesis, it's much easier to add the missing implicit runtime check in the future than to remove an existing implicit runtime check, since the latter would break existing code. Basically, my reasoning is that if in doubt, leave it out from the first version until we have evidence that it would be useful.
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.
OK, I am actually convinced by @JukkaL's argument here. (But we should probably apply a similar reasoning to cut out some other esoteric ideas, like variables.)
@brettcannon I think this could be merged now, unless there are some objections. I hope I addressed all points that were discussed. There may be some things that could be ironed out, but we can do this is additional PRs later. |
Let's merge it. That does not mean I am accepting the PEP but is ready for
pyrhon-dev discussion.
…--Guido (mobile)
On Mar 17, 2017 6:49 PM, "Ivan Levkivskyi" ***@***.***> wrote:
@brettcannon <https://github.com/brettcannon> I think this could be
merged now, unless there are some objections. I hope I addressed all points
that were discussed. There may be some things that could be ironed out, but
we can do this is additional PRs later.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#224 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACwrMpSk7CC9zp0bIc8O1Nyi3PafVb3kks5rmzhAgaJpZM4MaIqF>
.
|
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.
This is just the first half of my review. I skipped the background stuff and jumped right into the specification. I'll try to find time for the rest later this week.
of a protocol, it is said to implement the protocol and to be compatible | ||
with a protocol. If a class is compatible with a protocol but the protocol | ||
is not included in the MRO, the class is an *implicit* subtype | ||
of the protocol. |
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.
A subtlety here: you could explicitly subclass a protocol and still not implement it. At least, that's how I would interpret the behavior of Hashable
(and a few others that have similar behavior) where you can set __hash__ = None
in a subclass in order to not implement the protocol. This is useful for immutable subclasses of mutable classes.
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.
Yes. I will add a note that type checkers should understand setting attributes to None
in the sense of data model docs: "Setting a special method to None
indicates that the corresponding operation is not available."
------------------- | ||
|
||
Protocols are defined by including a special new class ``typing.Protocol`` | ||
(an instance of ``abc.ABCMeta``) in the base classes list, preferably |
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.
preferably -> typically
---------------- | ||
|
||
All methods defined in the protocol class body are protocol members, both | ||
normal and decorated with ``@abstractmethod``. If some or all parameters of |
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.
/some or all parameters of/ -> /any parameters of a/
Note that although formally the implicit return type of a method with | ||
a trivial body is ``None``, type checker will not warn about above example, | ||
such convention is similar to how methods are defined in stub files. | ||
Static methods, class methods, and properties are equally allowed |
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.
New paragraph to start here.
|
||
Note that although formally the implicit return type of a method with | ||
a trivial body is ``None``, type checker will not warn about above example, | ||
such convention is similar to how methods are defined in stub files. |
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.
Hmm, I don't really like the idea of "trivial bodies". Remember that when explicitly subclassing an ABC, abstract methods can still be called, through super()
. An abstract method that shouldn't be called ought to raise NotImplementedError
.
In stubs, mypy already doesn't type-check trivial bodies (though I don't recall its exact definition of "trivial" -- it probably doesn't exactly match what you have here). If we want to specify that behavior for all checkers reading stubs (which would seem to make sense) it should go in PEP 484, not here.
Or perhaps I am missing a use case for these that you've found in real code?
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.
An abstract method that shouldn't be called ought to raise
NotImplementedError
.
OK, I think this is a reasonable requirement, I will update this part.
|
||
Now the protocol ``SizedAndCloseable`` is a protocol with two methods, | ||
``__len__`` and ``close``. If one omits ``Protocol`` in the base class list, | ||
this would be a regular (non-protocol) class that must implement ``Sized``. |
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.
Hm, this example makes this requirement feel like a (slight) nuisance -- in this particular example it's pretty clear that it's meant to define a protocol?
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.
It may be clear for a human (taking into account the name of subclass). But for a type checker there are two equally possible options: a user actually wanted a protocol, or just forgot to implement a protocol method. As you said above:
we don't want to accidentally have some class act as a protocol just because one of its base classes happens to be one
I think explicit is better than implicit applies here.
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.
Fair enough. Objection withdrawn.
``__len__`` and ``close``. If one omits ``Protocol`` in the base class list, | ||
this would be a regular (non-protocol) class that must implement ``Sized``. | ||
If ``Protocol`` is included in the base class list, all the other base classes | ||
must be protocols. A protocol can't extend a regular class. |
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.
This seems to make sense, but does it really? There's no such rule for ABCs. What harm would there be in a protocol inheriting from a regular class? Hm, I guess there's ambiguity over whether the methods in that regular class all become protocol methods with default implementations. Hm... I guess it does make sense, but maybe an explicit example showing the rationale would help.
|
||
from typing import Sized | ||
|
||
class SupportsClose(...): ... # Like above |
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.
I would just write this out rather than letting the reader guess to where exactly this is referring.
|
||
Recursive protocols are also supported. Forward references to the protocol | ||
class names can be given as strings as specified by PEP 484. Recursive | ||
protocols will be useful for representing self-referential data structures |
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.
will be -> are
Subtyping relationships with other types | ||
---------------------------------------- | ||
|
||
Protocols cannot be instantiated, so there are no values with |
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.
That sounds a little weird -- certainly an instance of a class that implements a protocol is a value of that protocol type? Maybe all you need is to add "exactly" somewhere to the sentence.
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.
OK, this is the rest of my review. Hopefully I didn't repeat myself too much or ignore your responses to the earlier review (honestly I'm too tired to look it up).
* A protocol is never a subtype of a concrete type. | ||
* A concrete type or a protocol ``X`` is a subtype of another protocol ``P`` | ||
if and only if ``X`` implements all protocol members of ``P``. In other | ||
words, subtyping with respect to a protocol is always structural. |
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.
If X
is a protocol itself, "implements" gets a different meaning, right? I think in that case it just means that it must define all of P's methods right? Maybe it would be less ambiguous if you separated the two cases into separate bullets.
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.
Maybe it would be less ambiguous if you separated the two cases into separate bullets.
I agree.
def __iter__(self) -> Iterator[T]: | ||
... | ||
|
||
Note that ``Protocol[T, S, ...]`` is allowed as a shorthand for |
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.
I think this deserves calling out, rather than just "Note".
def walk(graph: Traversable) -> None: | ||
... | ||
tree: Tree[float] = Tree(0, []) | ||
walk(tree) # OK, 'Tree[float]' is a subtype of 'Traversable' |
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.
Hm, I'm not sure I like this example. Isn't the problem more that Traversible
ought to be generic but isn't in this example?
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.
Isn't the problem more that
Traversible
ought to be generic but isn't in this example?
No, this "problem" (I would rather say "rule") applies to all recursive protocols. For example:
class Linked(Protocol):
child: 'Linked'
class C:
child: 'C'
x: Linked
x = C() # This should be OK, according to the "rule"
I think we just need a simpler example here.
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.
Agreed. The generics just distract. (Maybe we need a simple example first and then one showing how to combine this with generics?)
class Exitable(Protocol): | ||
def exit(self) -> int: | ||
... | ||
class Quitable(Protocol): |
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.
I'd write "Quittable" -- just like it's "quitting" vs. "exiting". (English is never regular. :-)
|
||
def finish(task: Union[Exitable, Quitable]) -> int: | ||
... | ||
class GoodJob: |
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.
Can you find a different name for this? "Good job" is such an incredibly boring cliché...
def d(self) -> int: # ... or it can be abstract | ||
return 0 | ||
|
||
In Python 2.7 the function type comments should be used as per PEP 484. |
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.
I think they should also be allowed for Python 3 (even 3.6+) and the text earlier that introduces these should mention this. (It's fine of course to recommend PEP 526 syntax and to use it in all examples except one showing the fallback syntax.)
Use assignments to check explicitly that a class implements a protocol | ||
---------------------------------------------------------------------- | ||
|
||
In Go language the explicit checks for implementation are performed |
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 Go language" -> either "In Go" or "In the Go language"
|
||
def do_stuff(o: Union[P, X]) -> int: | ||
if isinstance(o, P): | ||
return o.common_method_name(1) # oops, what if it's an X instance? |
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.
So this more or less assumes that the runtime isinstance check doesn't compare the arguments. But what if it did?
Or are you saying that in this case if P
was decorated with @runtime
the isinstance check would still be broken?
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.
Or are you saying that in this case if
P
was decorated with@runtime
the isinstance check would still be broken?
I think yes, for example now isinstance(obj, Iterable)
returns True
for a class with __iter__ = 42
. And we can't make @runtime
much better because not all information is available statically. As Jukka said, it is not a big problem since static type checker will warn about this, but big enough to make support for isinstance()
with protocols opt-in.
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.
OK, I'm convinced (also this review UI is very confusing, it seems we are having discussion on different subcommits of the same PR).
I still don't like the name @runtime
though -- it's too generic. Can we brainstorm about a better name?
|
||
f(C()) # oops | ||
|
||
We argue that requiring an explicit class decorator would be better, since |
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.
OK, I am actually convinced by @JukkaL's argument here. (But we should probably apply a similar reasoning to cut out some other esoteric ideas, like variables.)
References | ||
========== | ||
|
||
.. [typing] |
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.
I wonder if we could do more deep linking to specific comments or subthreads? (No big deal if you think it's too much effort, but it might save some readers some time finding the right part of the long discussion in e.g. python/typing#11 -- including myself :-).
There's a couple more things that I should bring up. It looks like the mypy core team (all Dropboxers) is going to be very busy with other features, to the point where we probably won't have time to even review an implementation of this PEP for the foreseeable future. Now, this is all subject to various priorities that may still change, and perhaps the implementation is super simple, but if it's anything like I expect, even a complete, finished implementation (and one that allows importing There's also the absolute requirement to send this PEP to python-dev for discussion. Since I'm not an author we don't need a BDFL-delegate -- we just need to be careful to listen to feedback. And while I am aware that my own time is a particularly scarce resource, I still don't want to delegate this particular topic. |
OK, I will do this now. |
This sounds a bit frustrating, but what do you mean by "foreseeable future", few years or few days? |
A few months at least, is my best estimate based on discussions we had in our project planning (not all [arts of the project are open source). I agree it's frustrating -- it is for us too, but we need to acknowledge that we have limited velocity.
I believe David is more skeptical of this PEP than Jukka and I. But even so, the extra frustrating thing is that, indeed, the PEP process (by convention at least) prefers to have a reference implementation to support a proposal, without any guarantees that the reference implementation will ever be deployed. This is similar to the RFC-based IETF standardization process for internet protocols -- and the similarity is not coincidental, we modeled Python's PEP process after the RFC process (with which several core Python devs were highly familiar between 1995-2000).
I'm truly sorry. It is frustrating for all of us (as are my continued health issues that reduce my productivity dramatically). |
What would it take for the project to nominate a few non-Dropbox core contributors?
This is not a huge obstacle for inclusion in Python 3.7, as long as we get to it sometime this year. Also, a reference implementation for the purpose of this PEP can be on a "branch" of mypy. In other words, for the PEP reviewers to consider it, it doesn't have to be merged. This was pretty common in the past. @ilevkivskyi, I will provide preliminary review and use it internally before it gets merged (we are living on master anyway). |
OK, what about this way: I will have a branch in my fork of mypy, where I will build a reference implementation. People will be able to play with it (I will take care to merge mypy master frequently, so that the branch will be fully functional). And it is fine with me if it will be not merged for few months. To avoid a potential huge (and impossible to review) PR if/when the PEP is accepted, I will cherry-pick few smaller branches and make few sequential PRs. @gvanrossum @ambv @JukkaL does this sound reasonable? |
It sounds reasonable (though I cannot make any hard promises). CC @gnprice. |
Fixes #1843 (It was also necessary to fix few minor things to make this work correctly) The rules are simple, assuming we have: ```python class A: @AbstractMethod def m(self) -> None: pass class C(A): def m(self) -> None: ... ``` then ```python def fun(cls: Type[A]): cls() # OK fun(A) # Error fun(C) # OK ``` The same applies to variables: ```python var: Type[A] var() # OK var = A # Error var = C # OK ``` Also there is an option for people who want to pass abstract classes around: type aliases, they work as before. For non-abstract ``A``, ``Type[A]`` also works as before. My intuition why you opened #1843 is when someone writes annotation ``Type[A]`` with an abstract ``A``, then most probably one wants a class object that _implements_ a certain protocol, not just inherits from ``A``. NOTE: As discussed in python/peps#224 this behaviour is good for both protocols and usual ABCs.
This adds static support for structural subtyping. Previous discussion is here python/typing#11
Fixes #222