This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: typing.Protocol silently overrides __init__ method of delivered class
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.11
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: JelleZijlstra, adriangb, gvanrossum, kj, lukasz.langa, uriyyo
Priority: normal Keywords: patch

Created on 2021-08-02 09:53 by uriyyo, last changed 2022-04-11 14:59 by admin.

Pull Requests
URL Status Linked Edit
PR 27543 closed uriyyo, 2021-08-02 09:59
PR 31628 open adriangb, 2022-03-01 05:55
Messages (16)
msg398742 - (view) Author: Yurii Karabas (uriyyo) * (Python triager) Date: 2021-08-02 09:53
typing.Protocol silently overrides __init__ method of delivered class.

I think it should be forbidden to define __init__ method at Protocol delivered class in case if cls is not Protocol.
msg402333 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-09-21 17:33
I'm not sure whether we should downright reject `__init__()` methods. Sadly, they aren't usable at the moment anyway, but they could. Hear me out.

There seems to be a gap in the PEP 544 definition of protocols, i.e. whether `__init__` should be definable as part of a protocol. Currently this isn't specified and it looks like Mypy is ignoring `__init__` in Protocol definitions.

Consider this example: https://gist.github.com/ambv/0ed9c9ec5b8469878f47074bdcd37b0c

Mypy doesn't consider `__init__()` to be part of a protocol, even though it's an instance method like any other. It is valid (albeit weird) to call it again on an already initialized instance:

>>> class C:
...   def __init__(self) -> None:
...     print('init!')
...
>>> c = C()
init!
>>> c.__init__()
init!
>>> c.__init__()
init!

In the linked gist example, Mypy currently ignores `__init__()` but doesn't ignore `get_ball()`, thus incorrectly judging BallContainer and TwoBallContainer to be equivalent.
In fact, it only rejects the third ifmain call with `object` because `object` lacks a `get_ball()` method.

So... would that be useful? Do we ever want to define protocols on the basis of how their initializers look like? My uninformed intuition is that this might be potentially useful, however I fail to come up with a realistic example here.
msg402348 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-09-21 19:10
I wonder if part of the problem here isn't that protocols are primarily focused on instances, where the __init__ method is explicitly *not* considered part of the type.

IIRC supporting protocols for classes was an afterthought.

That said you have made a good case for seeing it as part of the protocol  when the Type[] operator is applied to the protocol.

So yes, I think we should be careful ruling that out too soon. Do we have any evidence that users are confused and define __init__ methods on protocols that are being ignored by their type checkers? The OP didn't say.

When in doubt, let the status quo win.
msg402355 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-09-21 20:11
Yurri, is the issue you reported your original finding? If you haven't found this issue in the wild, we might want to let this be as-is until we specify exactly whether `__init__()` should be considered part of a protocol.
msg402361 - (view) Author: Yurii Karabas (uriyyo) * (Python triager) Date: 2021-09-21 20:35
Łukasz this issue is my original finding.

I have found this case when I was working on another issue that was related to `typing.Protocol`.

I agree that we can leave it as it is for now because I didn't find any information from python community about this case.
msg402381 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-09-21 22:04
Marking issue as "pending" until we figure out how PEP 544 should be amended.
msg414169 - (view) Author: Adrian Garcia Badaracco (adriangb) * Date: 2022-02-28 00:18
While this is figured out, would it be possible to remove the silent overriding? This seems like something type checkers should be doing, not silent runtime modification of classes. Pyright already (correctly) checks this, so I think it would just need to be added to MyPy.

>>> class C(Protocol):
...   def __init__(self) -> None:
...     print('init!')
...
>>> c = C()  # Pyright error, MyPy says it's okay

I came across this while trying to create a class that requires concrete subclasses to define a specific field, and it seems like Protocol is the only thing that can pull this off because of how type checkers special case it: https://gist.github.com/adriangb/6c1a001ee7035bad5bd56df70e0cf5e6

I don't particularly like this use of Protocol, but it works (aside from the overriding issue).

I don't know if this usage of implementing `__init__` on a protocol class should be valid or not, but I do think it's interesting that `__init__` is never called on the protocol class itself, even if the protocol class is the one defining it. It is only called on `MyAPIKey`, which is a concrete class that happens to inherit the implementation from a protocol class. So maybe that should be valid? I'm not sure.

But I do know that making type checkers enforce this instead runtime would allow this use case to work while still prohibiting the (in my opinion invalid) use case of calling `__init__` on the protocol class itself.
msg414172 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2022-02-28 04:48
If the problem is in mypy, it should be filed in the mypy tracker, not here.
msg414173 - (view) Author: Adrian Garcia Badaracco (adriangb) * Date: 2022-02-28 05:04
Apologies if that was noise, I filed an issue on the MyPy issue tracker: https://github.com/python/mypy/issues/12261
msg414236 - (view) Author: Jelle Zijlstra (JelleZijlstra) * (Python committer) Date: 2022-03-01 03:20
Regardless of mypy's behavior (which isn't impacted by what typing.py does), there's a legitimate complaint here about the runtime behavior: any `__init__` method defined in a Protocol gets silently replaced.

>>> from typing import Protocol
>>> class X(Protocol):
...     def __init__(self, x, y): pass
... 
>>> X.__init__
<function _no_init_or_replace_init at 0x10de4e5c0>

Fixing that won't be easy though, unless we give up on making it impossible to instantiate a Protocol.
msg414238 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2022-03-01 05:46
Oops.

So this is an intentional feature -- Protocol replaces __init__ so that you can't (accidentally) instantiate a protocol. And the code to do this has changed a couple of times recently to deal with some edge cases. At least one of the PRs was by Yurii, who created this issue. I didn't read through all that when I closed the issue, so I'm reopening it.

Maybe Yurii can devise a solution? (Although apparently their first attempt, https://github.com/python/cpython/pull/27543 was closed without merging.) Yurii and Lukasz should probably figure this out.
msg414240 - (view) Author: Adrian Garcia Badaracco (adriangb) * Date: 2022-03-01 06:00
Agreed.

What if we allow protocols that implement `__init__` but still disallow instantiating a protocol that does not? It's a 1 line change, all existing tests pass and it would still catch what I think was the original intention (trying to instantiate a Protocol class with no __init__): https://github.com/python/cpython/pull/31628/files#diff-ddb987fca5f5df0c9a2f5521ed687919d70bb3d64eaeb8021f98833a2a716887
msg414241 - (view) Author: Adrian Garcia Badaracco (adriangb) * Date: 2022-03-01 06:06
Guido, it looks like you replied while I was typing my reply out.

Yurii can correct me here but I believe PR #27543 was an attempt to disallow defining `__init__` on a Protocol completely. What I proposed above is the opposite behavior, while still fixing the issue of `__init__` getting silently overridden (which is the crux / title of this issue).

I'm not sure which approach is right.
msg414242 - (view) Author: Jelle Zijlstra (JelleZijlstra) * (Python committer) Date: 2022-03-01 06:11
It doesn't make logical sense to instantiate any Protocol, whether it has __init__ or not, because a Protocol is inherently an abstract class. But we can just leave enforcement of that rule to static type checkers, so Adrian's proposed change makes sense to me.
msg414243 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2022-03-01 06:22
Would it make sense to enforce the no-instantiation rule in __new__ instead of __init__?
msg414244 - (view) Author: Adrian Garcia Badaracco (adriangb) * Date: 2022-03-01 06:52
I am not sure if that solves anything (other than the fact that __new__ is much less common to implement than __init__), but I may just be slow to pick up the implications of moving the check to __new__
History
Date User Action Args
2022-04-11 14:59:48adminsetgithub: 88970
2022-03-01 06:52:43adriangbsetmessages: + msg414244
2022-03-01 06:22:56gvanrossumsetmessages: + msg414243
2022-03-01 06:11:10JelleZijlstrasetmessages: + msg414242
2022-03-01 06:06:57adriangbsetmessages: + msg414241
2022-03-01 06:00:41adriangbsetmessages: + msg414240
2022-03-01 05:55:08adriangbsetstage: test needed -> patch review
pull_requests: + pull_request29750
2022-03-01 05:46:58gvanrossumsetstatus: closed -> open
resolution: third party ->
messages: + msg414238

stage: resolved -> test needed
2022-03-01 03:20:11JelleZijlstrasetmessages: + msg414236
2022-02-28 16:41:36gvanrossumsetstatus: open -> closed
resolution: third party
stage: patch review -> resolved
2022-02-28 05:04:06adriangbsetmessages: + msg414173
2022-02-28 04:48:21gvanrossumsetmessages: + msg414172
2022-02-28 02:06:42JelleZijlstrasetnosy: + JelleZijlstra
2022-02-28 00:18:49adriangbsetstatus: pending -> open
nosy: + adriangb
messages: + msg414169

2021-09-21 22:04:01lukasz.langasetstatus: open -> pending

messages: + msg402381
2021-09-21 20:35:52uriyyosetmessages: + msg402361
2021-09-21 20:11:39lukasz.langasetmessages: + msg402355
2021-09-21 19:10:23gvanrossumsetnosy: + gvanrossum
messages: + msg402348
2021-09-21 17:33:41lukasz.langasetnosy: + lukasz.langa
messages: + msg402333
2021-08-02 09:59:22uriyyosetkeywords: + patch
stage: patch review
pull_requests: + pull_request26052
2021-08-02 09:53:46uriyyocreate