| msg398742 - (view) |
Author: Yurii Karabas (uriyyo) *  |
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) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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__ |
|
|