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

Fix #440: Incorrect pickles for subclasses of generic classes #448

Merged
merged 3 commits into from Feb 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.md
Expand Up @@ -5,6 +5,9 @@
and `abc.abstractstaticmethod`.
([PR #450](https://github.com/cloudpipe/cloudpickle/pull/450))

- Support for pickling subclasses of generic classes.
([PR #448](https://github.com/cloudpipe/cloudpickle/pull/448))

2.0.0
=====

Expand Down
6 changes: 5 additions & 1 deletion cloudpickle/cloudpickle.py
Expand Up @@ -910,8 +910,12 @@ def _typevar_reduce(obj):


def _get_bases(typ):
if hasattr(typ, '__orig_bases__'):
if '__orig_bases__' in getattr(typ, '__dict__', {}):
# For generic types (see PEP 560)
# Note that simply checking `hasattr(typ, '__orig_bases__')` is not
# correct. Subclasses of a fully-parameterized generic class does not
# have `__orig_bases__` defined, but `hasattr(typ, '__orig_bases__')`
# will return True because it's defined in the base class.
bases_attr = '__orig_bases__'
else:
# For regular class objects
Expand Down
41 changes: 41 additions & 0 deletions tests/cloudpickle_test.py
Expand Up @@ -2335,6 +2335,47 @@ def check_generic(generic, origin, type_value, use_args):
assert check_generic(C[int], C, int, use_args) == "ok"
assert worker.run(check_generic, C[int], C, int, use_args) == "ok"

def test_generic_subclass(self):
T = typing.TypeVar('T')

class Base(typing.Generic[T]):
pass

class DerivedAny(Base):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is deriving from Base (which is a user-defined generic type) without specifying the type T a realistic typing use case? Otherwise, I think we should just keep the DerivedInt/LeafInt classes in the test, which, from a quick glance seem to be regular typing/subclassing pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that this is a realistic use case. Consider this scenario: a library author wrote a generic base class, but users of this library may not be that proficient with typing (or they chose to not type annotate stuff at all), so they inherit from the generic base class without specifying the type. I've seen this quite a lot actually.

PEP 484 also states:

Subclassing a generic class without specifying type parameters assumes Any for each position.

So I think it's important that we support this use case in cloudpickle as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification, that's convincing.

pass

class LeafAny(DerivedAny):
pass

class DerivedInt(Base[int]):
pass

class LeafInt(DerivedInt):
pass

class DerivedT(Base[T]):
pass

class LeafT(DerivedT[T]):
pass

klasses = [
Base, DerivedAny, LeafAny, DerivedInt, LeafInt, DerivedT, LeafT
]
for klass in klasses:
assert pickle_depickle(klass, protocol=self.protocol) is klass

with subprocess_worker(protocol=self.protocol) as worker:

def check_mro(klass, expected_mro):
assert klass.mro() == expected_mro
return "ok"

for klass in klasses:
mro = klass.mro()
assert check_mro(klass, mro)
assert worker.run(check_mro, klass, mro) == "ok"

def test_locally_defined_class_with_type_hints(self):
with subprocess_worker(protocol=self.protocol) as worker:
for type_ in _all_types_to_test():
Expand Down