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

bpo-44975: [typing] Support issubclass for ClassVar data members #27883

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

@Fidget-Spinner
Copy link
Contributor

@Fidget-Spinner Fidget-Spinner commented Aug 22, 2021

https://bugs.python.org/issue44975

@Fidget-Spinner
Copy link
Contributor Author

@Fidget-Spinner Fidget-Spinner commented Aug 22, 2021

CC @uriyyo please take a look. Thank you.

Copy link
Member

@uriyyo uriyyo left a comment

LGTM💫

I have few minor questions

@@ -1396,8 +1396,15 @@ def _get_protocol_attrs(cls):


def _is_callable_members_only(cls):
attr_names = _get_protocol_attrs(cls)
annotations = getattr(cls, '__annotations__', {})
Copy link
Member

@uriyyo uriyyo Aug 22, 2021

What about using typing.get_type_hints to resolve annotations for a class?

It will allow having ClassVar annotated as str:

@runtime_checkable
class P(Protocol):
    x: 'ClassVar[int]' = 1
Suggested change
annotations = getattr(cls, '__annotations__', {})
annotations = get_type_hints(cls)

Copy link
Contributor Author

@Fidget-Spinner Fidget-Spinner Aug 23, 2021

Good question! I'd considered that too but decided against it for two main reasons:

  1. get_type_hints is slow
  2. get_type_hints won't work with forward references/ PEP 563 if ClassVar is not defined in the caller's namespace

Consider the following:

# foo.py
from __future__ import annotations
from typing import *

@runtime_checkable
class X(Protocol):
 x: ClassVar[int] = 1
 y: SomeUndeclaredType = None
# bar.py
from .foo import X
class Y: ...

# Error! get_type_hints cannot resolve 'ClassVar' and 'SomeUndeclaredType'
issubclass(X, Y)

Nonetheless, your suggestion has reminded me of a basic workaround for string annotations. Thanks.

Lib/typing.py Outdated Show resolved Hide resolved
uriyyo
uriyyo approved these changes Aug 23, 2021
Copy link
Member

@uriyyo uriyyo left a comment

LGTM💫

Lib/typing.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
Copy link
Contributor

@sobolevn sobolevn left a comment

👍


class C: pass

class D:
Copy link
Contributor

@sobolevn sobolevn Sep 23, 2021

Let's also add a case with the same class prop, but different value. Example:

class E:
    x = 2

Because right now all names / values always match. It is not clear whether value is a part of the protocol or not.

class D:
x = 1
self.assertNotIsSubclass(C, P)
self.assertIsSubclass(D, P)
Copy link
Contributor

@sobolevn sobolevn Sep 23, 2021

One more case that is not covered: same name, different type.

class F:
    x = 'a'

It should not pass, but maybe ClassVar wrapper might break / change that.

x = 1
self.assertNotIsSubclass(C, P)
self.assertIsSubclass(D, P)

Copy link
Contributor

@sobolevn sobolevn Sep 23, 2021

Let's also test these classes:

class G:
   x: ClassVar[int] = 1
class H:
   x: 'ClassVar[int]' = 1

@@ -1394,10 +1394,24 @@ def _get_protocol_attrs(cls):
attrs.add(attr)
return attrs

_classvar_prefixes = ("typing.ClassVar[", "t.ClassVar[", "ClassVar[")
Copy link
Contributor

@sobolevn sobolevn Sep 23, 2021

What about raw ClassVar annotation with generic argument?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants