Skip to content

Fix the problem with copy and deepcopy; also improve pickling a bit #311

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.

Sign up for GitHub

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

Merged
gvanrossum merged 2 commits into from
Oct 29, 2016
Merged

Fix the problem with copy and deepcopy; also improve pickling a bit #311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
baa8b77
Fix the problem with copy and deepcopy; also improve pickling a bit
ilevkivskyi Oct 29, 2016
8893a0b
Fix the problem on 3.2
ilevkivskyi Oct 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions python2/test_typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import re
import sys
from unittest import TestCase, main, SkipTest
from copy import copy, deepcopy

from typing import Any
from typing import TypeVar, AnyStr
Expand Down Expand Up @@ -803,6 +804,24 @@ class C(B[int]):
self.assertEqual(x.foo, 42)
self.assertEqual(x.bar, 'abc')
self.assertEqual(x.__dict__, {'foo': 42, 'bar': 'abc'})
simples = [Any, Union, Tuple, Callable, ClassVar, List, typing.Iterable]
for s in simples:
for proto in range(pickle.HIGHEST_PROTOCOL + 1):
z = pickle.dumps(s, proto)
x = pickle.loads(z)
self.assertEqual(s, x)

def test_copy_and_deepcopy(self):
T = TypeVar('T')
class Node(Generic[T]): pass
things = [Any, Union[T, int], Tuple[T, int], Callable[..., T], Callable[[int], int],
Tuple[Any, Any], Node[T], Node[int], Node[Any], typing.Iterable[T],
typing.Iterable[Any], typing.Iterable[int], typing.Dict[int, str],
typing.Dict[T, Any], ClassVar[int], ClassVar[List[T]], Tuple['T', 'T'],
Union['T', int], List['T'], typing.Mapping['T', int]]
for t in things:
self.assertEqual(t, deepcopy(t))
self.assertEqual(t, copy(t))

def test_errors(self):
with self.assertRaises(TypeError):
Expand Down
8 changes: 8 additions & 0 deletions python2/typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,9 @@ def __new__(cls, *args, **kwds):
return self
raise TypeError("Cannot instantiate %r" % cls)

def __reduce__(self):
return _trim_name(type(self).__name__)


class _ForwardRef(_TypingBase):
"""Wrapper to hold a forward reference."""
Expand Down Expand Up @@ -1140,6 +1143,11 @@ def __instancecheck__(self, instance):
return issubclass(instance.__class__, self)
return False

def __copy__(self):
return self.__class__(self.__name__, self.__bases__, dict(self.__dict__),
self.__parameters__, self.__args__, self.__origin__,
self.__extra__, self.__orig_bases__)


# Prevent checks for Generic to crash when defining Generic.
Generic = None
Expand Down
19 changes: 19 additions & 0 deletions src/test_typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import re
import sys
from unittest import TestCase, main, skipUnless, SkipTest
from copy import copy, deepcopy

from typing import Any
from typing import TypeVar, AnyStr
Expand Down Expand Up @@ -845,6 +846,24 @@ class C(B[int]):
self.assertEqual(x.foo, 42)
self.assertEqual(x.bar, 'abc')
self.assertEqual(x.__dict__, {'foo': 42, 'bar': 'abc'})
simples = [Any, Union, Tuple, Callable, ClassVar, List, typing.Iterable]
for s in simples:
for proto in range(pickle.HIGHEST_PROTOCOL + 1):
z = pickle.dumps(s, proto)
x = pickle.loads(z)
self.assertEqual(s, x)

def test_copy_and_deepcopy(self):
T = TypeVar('T')
class Node(Generic[T]): ...
things = [Union[T, int], Tuple[T, int], Callable[..., T], Callable[[int], int],
Tuple[Any, Any], Node[T], Node[int], Node[Any], typing.Iterable[T],
typing.Iterable[Any], typing.Iterable[int], typing.Dict[int, str],
typing.Dict[T, Any], ClassVar[int], ClassVar[List[T]], Tuple['T', 'T'],
Union['T', int], List['T'], typing.Mapping['T', int]]
for t in things + [Any]:
self.assertEqual(t, copy(t))

@mitar mitar Aug 14, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ilevkivskyi: Are you sure this works correctly? From documentation:

It does “copy” functions and classes (shallow and deeply), by returning the original object unchanged; this is compatible with the way these are treated by the pickle module.

Test:

class Metaclass(type):
  def __copy__(self):
    return 1

class Foo(metaclass=Metaclass):
  pass

>>> copy.copy(Foo)
<class '__main__.Foo'>
>>> Foo.__copy__()
1
>>> copy.copy(Foo) is Foo
True

I do not think your test is really testing this correctly. Nor copying is really working correctly.

@ilevkivskyi ilevkivskyi Aug 14, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, we exactly follow the docs for copy module (classes are treated as immutable, except in Python 3.2 and 2.7, this is why we have __copy__ defined), what is the problem?

@mitar mitar Aug 14, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, so idea is that copy.copy should return same reference, and __copy__ is here just for compatibility with 3.2 and 2.7 to return something sane? But why is then doing self.__class__(...) and not just returning self? So why is really making a copy? Or you want in 3.2 and 2.7 to be a copy?

Should a test include that t is copy(t) on versions which are not 2.7 and 3.2?

@ilevkivskyi ilevkivskyi Aug 14, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So why is really making a copy?

IIRC, to match the behaviour of 3.2 for other classes. Or maybe it is something to do with picke in 3.2, I am not sure now TBH.

Should a test include that t is copy(t) on versions which are not 2.7 and 3.2?

This is not something super-important, but if you want to make a PR (with a comment quoting copy docs), then we will add it.

@mitar mitar Aug 14, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Done: #460

self.assertEqual(t, deepcopy(t))

def test_errors(self):
with self.assertRaises(TypeError):
Expand Down
8 changes: 8 additions & 0 deletions src/typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,9 @@ def __new__(cls, *args, _root=False, **kwds):
return self
raise TypeError("Cannot instantiate %r" % cls)

def __reduce__(self):
return _trim_name(type(self).__name__)


class _ForwardRef(_TypingBase, _root=True):
"""Wrapper to hold a forward reference."""
Expand Down Expand Up @@ -1051,6 +1054,11 @@ def __instancecheck__(self, instance):
# classes are supposed to be rare anyways.
return issubclass(instance.__class__, self)

def __copy__(self):
return self.__class__(self.__name__, self.__bases__, dict(self.__dict__),
self.__parameters__, self.__args__, self.__origin__,
self.__extra__, self.__orig_bases__)


# Prevent checks for Generic to crash when defining Generic.
Generic = None
Expand Down