-
-
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
Updates to PEP 544: Protocols #255
Conversation
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 everything here, I just have some nits and a question about subclassing.
* All structural subtyping checks will be performed by static type checkers, | ||
such as ``mypy`` [mypy]_. No additional support for protocol validation will | ||
be provided at runtime. | ||
* Classes ``Mapping``, ``MutableMapping``, ``Sequence``, and | ||
``MutableSequence`` in ``collections.abc`` module will support structural | ||
instance and subclass checks (like e.g. ``collections.abc.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.
This is important to call out! Many people don't necessarily care about typing, but do care about collections.abc. It also makes things that depend on this harder to backport -- we can't change collections.abc in 3.6 or 3.5, since this isn't a bugfix and it isn't covered by typing's provisional status. (Let alone in 2.7.)
It's the right thing to do but probably shouldn't be used yet by code that is okay only supporting 3.6+.
What would happen if you didn't do this?
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 actually an optional change. I have a small branch in my CPython fork for this. I propose this for two reasons:
- This makes
typing.Mapping
andcollections.abc.Mapping
interchangeable/consistent. It would be strange/unexpected if all protocols intyping
andcollections.abc
behave the same way except for these four. - This makes implementation in
typing
cleaner.
A possible strategy would be to use if sys.version_info >= ...
in typing
implementation, so that the behaviour of typing
protocols is the same in all versions, and we can safely make the collections.abc
change only in 3.7.
T_contra = TypeVar('T_contra', contravariant=True) | ||
|
||
class Box(Protocol[T_co]): | ||
def content(self) -> T_co: |
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.
Somehow this example made me think of something where you have a covariant base protocol (like Box) with an invariant subprotocol, e.g. by adding a set_content(T) method. This subject regularly makes my head explode -- would everything work out okay in such a 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 think everything should be OK in this case (at least I can't invent any problematic example). Invariant subclass of a covariant class, and invariant subclass of a contravriant class are both OK (for nominal classes and for protocols).
At the same time going from covariant to contravariant or vice-versa is not safe, as well as covariant and contravariant subclasses of an invariant class are not safe.
Actually @pkch proposed a documentation PR about this some time ago python/mypy#3179, but we decided to close it since mypy does not yet support these checks (variance overrides) for nominal classes. Instead it was proposed to put this discussion in PEP 483. I suppose @pkch would like to make a PR to the peps repo.
pep-0544.txt
Outdated
Declaring variance is not necessary for protocol classes, since it can be | ||
inferred from a protocol definition. Examples:: | ||
User-defined generic protocols support explicitly declared variance. | ||
Type checkers will warn if an inferred variance is different from |
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->the
pep-0544.txt
Outdated
@@ -533,6 +538,16 @@ inferred from a protocol definition. Examples:: | |||
another_var: Proto[int] | |||
var = another_var # Error! 'Proto[float]' is incompatible with 'Proto[int]'. | |||
|
|||
Note that unlike nominal classes, de-facto covariant protocols cannot be | |||
declared as invariant, since this can break transitivity of subtyping, | |||
see `rejected`_ ideas for details. For 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.
Make this a (parenthetical) or use a semicolon instead of comma.
pep-0544.txt
Outdated
@@ -1118,6 +1137,72 @@ This was rejected for the following reasons: | |||
it has an unsafe override. | |||
|
|||
|
|||
Allow covariant subtyping of mutable attributes |
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 remove the word "Allow" (or similar) from the section headings under "rejected proposals", since the conclusion is that we will not allow it.
pep-0544.txt
Outdated
Allow covariant subtyping of mutable attributes | ||
----------------------------------------------- | ||
|
||
Covariant subtyping of mutable attributes is not safe. Consider 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.
Add "Rejected because ..."
pep-0544.txt
Outdated
x: int | ||
|
||
c = C() | ||
f(c) # Typechecks if covariant subtyping of mutable attributes is 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.
"Would typecheck if ... were allowed"
@gvanrossum Should anything else be done here? If not, then maybe it makes sense to merge this and make a second round on python-dev? |
Go for it! |
This adds two items to rejected ideas that appeared in recent discussions on mypy tracker, and updates notes on runtime implementation.
It looks like all questions/uncertainties are now addressed, so that the PEP is ready for a next round on python-dev.
Attn: @JukkaL @ambv @gvanrossum