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

[typing] Runtime protocols with ClassVar data members should support issubclass #89138

Closed
Fidget-Spinner opened this issue Aug 22, 2021 · 7 comments
Assignees
Labels
3.11 only security fixes stdlib Python modules in the Lib dir topic-typing type-feature A feature request or enhancement

Comments

@Fidget-Spinner
Copy link
Member

BPO 44975
Nosy @gvanrossum, @JelleZijlstra, @Fidget-Spinner
PRs
  • bpo-44975: [typing] Support issubclass for ClassVar data members #27883
  • 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 = 'https://github.com/Fidget-Spinner'
    closed_at = None
    created_at = <Date 2021-08-22.08:56:49.329>
    labels = ['type-feature', 'library', '3.11']
    title = '[typing] Runtime protocols with ClassVar data members should support issubclass'
    updated_at = <Date 2022-01-21.21:12:18.991>
    user = 'https://github.com/Fidget-Spinner'

    bugs.python.org fields:

    activity = <Date 2022-01-21.21:12:18.991>
    actor = 'gvanrossum'
    assignee = 'kj'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2021-08-22.08:56:49.329>
    creator = 'kj'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 44975
    keywords = ['patch']
    message_count = 2.0
    messages = ['400059', '411184']
    nosy_count = 3.0
    nosy_names = ['gvanrossum', 'JelleZijlstra', 'kj']
    pr_nums = ['27883']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue44975'
    versions = ['Python 3.11']

    @Fidget-Spinner
    Copy link
    Member Author

    This is a feature request by a user at python/typing#822.

    A copy of their request:
    Currently issubclass cannot be used for runtime_checkable protocols with data members, because those attributes could be set in __init__. I propose to relax this restriction to allow protocols with ClassVar members, as those should be present in the class definition.

    I'm unsure if I need to update PEP-544 too. I don't remember if PEPs can be updated when 'Accepted', or was it 'Final' PEPs that can't be updated anymore?

    @Fidget-Spinner Fidget-Spinner added the 3.11 only security fixes label Aug 22, 2021
    @Fidget-Spinner Fidget-Spinner self-assigned this Aug 22, 2021
    @Fidget-Spinner Fidget-Spinner added stdlib Python modules in the Lib dir type-feature A feature request or enhancement 3.11 only security fixes labels Aug 22, 2021
    @Fidget-Spinner Fidget-Spinner self-assigned this Aug 22, 2021
    @Fidget-Spinner Fidget-Spinner added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Aug 22, 2021
    @gvanrossum
    Copy link
    Member

    IMO we shouldn't update PEP-544. PEPs reflect the historical proposal, they are not documentation for the current state of the interpreter.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @hauntsaninja
    Copy link
    Contributor

    I'm -1 on implementing this feature.

    In general, I believe stdlib should try to get out of the business of runtime type checking. Even with isinstance on runtime checkable protocols I've seen complaints about performance and unsoundness and interactions with descriptors. I don't think it's particularly intuitive for a runtime type check to only check presence of a ClassVar and then not check the type (and we definitely don't want to check the type).

    @gvanrossum
    Copy link
    Member

    So should we just deprecate @runtime_checkable?

    @hauntsaninja
    Copy link
    Contributor

    hauntsaninja commented Oct 11, 2022

    That's a good question!

    Arguments in favour of keeping runtime_checkable:

    • The "protocols" people are most familiar with are the ones in collections.abc. These support isinstance, so perhaps it'd be surprising if custom protocols didn't have an analogue available to them (that said, the abcs also have virtual subclassing, which type checkers and protocols do not support)
    • Without isinstance for protocols, it becomes more important that type checkers understand hasattr checks (mypy only gained support for this recently)
    • Empirically, unsoundness related to method signatures hasn't proven to be a big problem. Aka if the method exists and it's callable, it seems it's usually got a signature that's compatible enough
    • I don't like deprecating functionality when there isn't a clear alternative

    Arguments in favour of deprecation:

    • Runtime type checking is heavily contentious. It's mostly impossible to make things sound or performant, let alone both
    • By pushing users to look outside of the standard library, it's easier for users to opt-in to the exact behaviour they want, and for the ecosystem to experiment with different tradeoffs and avoid backwards compatibility concerns
    • isinstance is one of the hottest functions in most codebases I've worked on, so it's nice not to encourage adding potential performance pitfalls to it

    Overall, one way forward would be to keep runtime_checkable, but alias it to something that more clearly communicates the narrow scope. Maybe @isinstance_checks_methods_exist :-)

    @hauntsaninja
    Copy link
    Contributor

    hauntsaninja commented Oct 11, 2022

    For this specific feature request, as Jelle says in https://github.com/python/cpython/pull/27883/files#r789976652 , we definitely shouldn't attempt to do type checking of the value. At most, we should validate attribute existence.

    A claim that would make me neutral on checking for existence is if we believed that unsoundness related to the type being different is unlikely to be a problem. (With methods, I believe we've been mostly fine in practice because we check whether its a callable, which really narrows things down, plus callables aren't invariant)

    @gvanrossum
    Copy link
    Member

    Okay, let's close this one as won'tfix, and if someone feels strongly about the rest please open a new issue!

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes stdlib Python modules in the Lib dir topic-typing type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants