Skip to content
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

Troubles with @runtime_checkable protocols #83089

Closed
ilevkivskyi opened this issue Nov 24, 2019 · 12 comments
Closed

Troubles with @runtime_checkable protocols #83089

ilevkivskyi opened this issue Nov 24, 2019 · 12 comments
Labels
3.9 3.10 3.11 stdlib type-bug

Comments

@ilevkivskyi
Copy link
Contributor

@ilevkivskyi ilevkivskyi commented Nov 24, 2019

BPO 38908
Nosy @gvanrossum, @ilevkivskyi, @miss-islington, @uriyyo, @Fidget-Spinner
PRs
  • #26067
  • #26073
  • #26075
  • #26077
  • #26096
  • #26337
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-05-12.17:29:07.094>
    created_at = <Date 2019-11-24.17:39:56.423>
    labels = ['type-bug', 'library', '3.9', '3.10', '3.11']
    title = 'Troubles with @runtime_checkable protocols'
    updated_at = <Date 2021-05-27.13:51:05.584>
    user = 'https://github.com/ilevkivskyi'

    bugs.python.org fields:

    activity = <Date 2021-05-27.13:51:05.584>
    actor = 'miss-islington'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-05-12.17:29:07.094>
    closer = 'kj'
    components = ['Library (Lib)']
    creation = <Date 2019-11-24.17:39:56.423>
    creator = 'levkivskyi'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 38908
    keywords = ['patch']
    message_count = 12.0
    messages = ['357400', '357555', '391292', '391318', '393511', '393516', '393517', '393528', '393532', '393536', '393537', '394540']
    nosy_count = 6.0
    nosy_names = ['gvanrossum', 'levkivskyi', 'Kevin Shweh', 'miss-islington', 'uriyyo', 'kj']
    pr_nums = ['26067', '26073', '26075', '26077', '26096', '26337']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue38908'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Nov 24, 2019

    The PEP-544 specifies that:

    A protocol can be used as a second argument in isinstance() and issubclass() only if it is explicitly opt-in by @runtime_checkable decorator.

    It is not specified exactly whether this should be enforced by static type checkers or at runtime. Currently it is enforced in both cases: mypy flags this as error, and a TypeError is raised at runtime.

    There is however a problem with current runtime implementation: abc and functools modules may call issubclass() on non-runtime checkable protocols if they appear as explicit superclasses. This is currently solved by a sys._getframe() hack in typing module.

    The problem is that current solution is incomplete. For example, the TypeError is not raised in the case of non-method protocols:

    >>> class P(Protocol):
    ...     x: int
    ... 
    >>> 
    >>> class C: ...
    ... 
    >>> isinstance(C(), P)
    False  # should be TypeError

    I tried to fix it this morning but after an hour of attempts to tweak the existing hack I gave up. It looks like there are only two reasonable solutions:

    • Don't enforce @typing.runtime_checkable at runtime, make it a type-checker-only feature (like @typing.final).
    • Add a little helper to abc module that would skip classes in MRO for which C._is_protocol is True but C._is_runtime_protocol is False.

    Any thoughts/preferences?

    @ilevkivskyi ilevkivskyi added 3.8 3.9 stdlib type-bug labels Nov 24, 2019
    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Nov 27, 2019

    If the "little helper in abc" solves it, let's do that. The sys._getframe() hack has always been a bit smelly -- I assume we can get rid of that then?

    @KevinShweh
    Copy link
    Mannequin

    @KevinShweh KevinShweh mannequin commented Apr 17, 2021

    It seems like the straightforward, minimal fix would be to just add

    if (getattr(cls, '_is_protocol', False) and
    not getattr(cls, '_is_runtime_protocol', False) and
    not _allow_reckless_class_cheks()):
    raise TypeError(...)

    to _ProtocolMeta.__instancecheck__. Does that fail on some edge case (that the current implementation works on)? It's a little weird that _ProtocolMeta.__instancecheck__ doesn't explicitly check that the protocol is runtime-checkable.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Apr 18, 2021

    Hi Kevin,

    If you want to work on this, could you come up with some test cases that
    try to explore edge cases for this behavior? Ideally some that already
    pass, and some that currently don't pass (because of the limitation
    mentioned by Ivan). It's possible that the tests for typing.py already
    contain some suitable passing tests. You could then investigate whether
    your proposal seems to work. If it does, you can submit a PR and we can lay
    this issue to rest!

    --Guido

    @uriyyo
    Copy link
    Member

    @uriyyo uriyyo commented May 12, 2021

    Hi Guido,

    I decided to help with this issue as far as Kevin did not respond.

    @uriyyo
    Copy link
    Member

    @uriyyo uriyyo commented May 12, 2021

    I checked and this bug is not present at 3.11 version. So I simply added test to cover case mentioned by Ivan.

    @uriyyo
    Copy link
    Member

    @uriyyo uriyyo commented May 12, 2021

    Please forget my previous msg, this bug still present at 3.11 version. PR contains fix proposed by Kevin.

    Sorry for making to much noise.

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented May 12, 2021

    New changeset a2d94a0 by Miss Islington (bot) in branch '3.10':
    bpo-38908: Fix issue when non runtime_protocol failed to raise TypeError (GH-26067)
    a2d94a0

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented May 12, 2021

    New changeset 88136bb by Ken Jin in branch '3.9':
    [3.9] bpo-38908: Fix issue when non runtime_protocol does not raise TypeError (GH-26067) (GH-26075)
    88136bb

    @Fidget-Spinner
    Copy link
    Member

    @Fidget-Spinner Fidget-Spinner commented May 12, 2021

    Yurii and Kevin, thanks for pushing the patch forward! Thanks for the review too Guido.

    I'm closing this issue as all bugfix PRs have landed to bugfix branches. This will make its way into the next versions of Python for those respective branches.

    One point of contention is whether to introduce this in 3.9.6. This will cause code previously working in 3.9.5 to throw an error if people were using it incorrectly. So it might be better to only enforce this in 3.10 onwards.

    I created #70265 just in case the decision is made to revert. Sorry for any inconvenience caused everyone!

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented May 12, 2021

    New changeset 9b90ce6 by Ken Jin in branch '3.9':
    [3.9] Revert "[3.9] bpo-38908: Fix issue when non runtime_protocol does not raise TypeError (GH-26067)" (GH-26077)
    9b90ce6

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented May 27, 2021

    New changeset 09696a3 by Miss Islington (bot) in branch '3.10':
    [3.10] bpo-38908: [docs] Add changes to 3.10 whatsnew and fix some minor inaccuracies in news (GH-26096) (GH-26337)
    09696a3

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 3.10 3.11 stdlib type-bug
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants