bpo-38293: Allow shallow and deep copying of property objects by GudniNathan · Pull Request #16438 · python/cpython

GudniNathan · 2019-09-27T19:13:25Z

Copying property objects results in a TypeError. Steps to reproduce:

>>> import copy
>>> obj = property()
>>> copy.copy(obj)

This affects both shallow and deep copying.
My idea for a fix is to add property objects to the list of "atomic" objects in the copy module.
These already include types like functions and type objects.

I also added property objects to the unit tests test_copy_atomic and test_deepcopy_atomic. This is my first PR, and it's highly likely I've made some mistake, so please be kind :)

https://bugs.python.org/issue38293

Automerge-Triggered-By: @csabella

eamanu

Good catch @GudniNatan i am not copy expert but I test it and run successfully.

I don't know if will need necessary a NEWS entry, IMO yes, but I would like hear some
core dev opinion.

brandtbucher

Thanks @GudniNatan, and welcome to CPython. This looks good!

I think this should have a NEWS entry. It's a new feature (and there's precedent for it when copy/deepcopy support was added for other objects in the past). Creating one is super quick!

Maybe just something like:

Add :func:`copy.copy` and :func:`copy.deepcopy` support to :func:`property` objects.

Just out of curiosity, what was your use case for wanting to copy these?

GudniNathan · 2019-09-29T10:26:04Z

I was experimenting with using property objects to validate data in dataclasses. Here is some code that should help illustrate:

from dataclasses import field, dataclass, asdict, fields


def string_check(x):
    if type(x) != str:
        raise ValueError("Should be a string.")
    return x


def validator_factory(field_name, validator):
    def fget(self):
        return self.__dict__[field_name]

    def fset(self, value):
        self.__dict__[field_name] = validator(value)

    return property(fget, fset)


@dataclass
class MyClass():
    my_value: str = validator_factory("my_value", string_check)


@dataclass
class ClassSchema():
    field_defaults: list


my_class_schema = ClassSchema(
    field_defaults=[f.default for f in fields(MyClass)]
)

print(my_class_schema)
print(asdict(my_class_schema))

The MyClass dataclass has an attribute my_value, and the idea is that if you set it to be anything other than a string, it will throw a ValueError.

We were also using a library (dataclasses-jsonschema) that has some similar functionality to ClassSchema, in that it stores the default values of our dataclasses. It will then try to convert those to dictionaries with the dataclasses asdict function. However, asdict will try to give you a dictionary that is independent of the instance you feed it. To do that it uses the deepcopy, and then you get the TypeError: can't pickle property objects

So basically calling asdict() on certain dataclass instances results in this weird error.

gvanrossum

I have a nagging doubt. You can create a property from any callable, e.g. property(lambda self: None). The argument is not type-checked, so you can also create a property from a non-callable, e.g. property([1, 2, 3]). When we deep-copy such a property, shouldn't we deep-copy the list (i.e. the fget attribute)? After all, when we deep-copy a tuple containing a list, the list gets copied.

@GudniNatan For your use case, is deep-copyability required?

arigo · 2019-10-23T04:41:31Z

@gvanrossum following that argument, copy.copy(func) should not return its original object either, because func is mutable (i.e. a following func.foo = 42 should not appear on its copy). Of course I'm not suggesting that this should be changed; this is only a reminder that copy() and deepcopy() are best-effort hacks in the first place.

gvanrossum · 2019-10-23T05:27:45Z

@arigo That's fair. Tuples are special, because they are meant to be robust data structures, and a tuple containing a list is a well-known (and used) corner case. I'll approve.

miss-islington · 2020-01-12T17:41:52Z

Thanks @GudniNatan for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

miss-islington · 2020-01-12T17:41:57Z

Sorry @GudniNatan, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 9f3fc6c5b4993f2b362263b494f84793a21aa073 3.8

…GH-16438) Copying property objects results in a TypeError. Steps to reproduce: ``` >>> import copy >>> obj = property() >>> copy.copy(obj) ```` This affects both shallow and deep copying. My idea for a fix is to add property objects to the list of "atomic" objects in the copy module. These already include types like functions and type objects. I also added property objects to the unit tests test_copy_atomic and test_deepcopy_atomic. This is my first PR, and it's highly likely I've made some mistake, so please be kind :) https://bugs.python.org/issue38293 (cherry picked from commit 9f3fc6c) Co-authored-by: Guðni Natan Gunnarsson <1493259+GudniNatan@users.noreply.github.com>

bedevere-bot · 2020-01-12T17:42:13Z

GH-17968 is a backport of this pull request to the 3.7 branch.

miss-islington · 2020-01-12T17:45:28Z

Thanks @GudniNatan for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

bedevere-bot · 2020-01-12T17:45:37Z

GH-17969 is a backport of this pull request to the 3.8 branch.

…GH-16438) Copying property objects results in a TypeError. Steps to reproduce: ``` >>> import copy >>> obj = property() >>> copy.copy(obj) ```` This affects both shallow and deep copying. My idea for a fix is to add property objects to the list of "atomic" objects in the copy module. These already include types like functions and type objects. I also added property objects to the unit tests test_copy_atomic and test_deepcopy_atomic. This is my first PR, and it's highly likely I've made some mistake, so please be kind :) https://bugs.python.org/issue38293 (cherry picked from commit 9f3fc6c) Co-authored-by: Guðni Natan Gunnarsson <1493259+GudniNatan@users.noreply.github.com>

Copying property objects results in a TypeError. Steps to reproduce: ``` >>> import copy >>> obj = property() >>> copy.copy(obj) ```` This affects both shallow and deep copying. My idea for a fix is to add property objects to the list of "atomic" objects in the copy module. These already include types like functions and type objects. I also added property objects to the unit tests test_copy_atomic and test_deepcopy_atomic. This is my first PR, and it's highly likely I've made some mistake, so please be kind :) https://bugs.python.org/issue38293 (cherry picked from commit 9f3fc6c) Co-authored-by: Guðni Natan Gunnarsson <1493259+GudniNatan@users.noreply.github.com>

bpo-38293: Allow shallow and deep copying of property objects (pythonGH-16438)

Copying property objects results in a TypeError. Steps to reproduce: ``` >>> import copy >>> obj = property() >>> copy.copy(obj) ```` This affects both shallow and deep copying. My idea for a fix is to add property objects to the list of "atomic" objects in the copy module. These already include types like functions and type objects. I also added property objects to the unit tests test_copy_atomic and test_deepcopy_atomic. This is my first PR, and it's highly likely I've made some mistake, so please be kind :) https://bugs.python.org/issue38293 (cherry picked from commit 9f3fc6c) Co-authored-by: Guðni Natan Gunnarsson <1493259+GudniNatan@users.noreply.github.com>

brandtbucher · 2020-01-12T18:13:56Z

Congrats on your first CPython contribution @GudniNatan! 🍾

Looking forward to seeing more from you in the future.

…GH-16438) Copying property objects results in a TypeError. Steps to reproduce: ``` >>> import copy >>> obj = property() >>> copy.copy(obj) ```` This affects both shallow and deep copying. My idea for a fix is to add property objects to the list of "atomic" objects in the copy module. These already include types like functions and type objects. I also added property objects to the unit tests test_copy_atomic and test_deepcopy_atomic. This is my first PR, and it's highly likely I've made some mistake, so please be kind :) https://bugs.python.org/issue38293

Allow shallow and deep copying of property objects + unit tests

c766497

the-knights-who-say-ni added the CLA signed label Sep 27, 2019

bedevere-bot added the awaiting review label Sep 27, 2019

eamanu reviewed Sep 28, 2019

View reviewed changes

brandtbucher added the type-feature A feature request or enhancement label Sep 29, 2019

brandtbucher requested changes Sep 29, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Sep 29, 2019

📜🤖 Added by blurb_it.

26fe8e7

brandtbucher approved these changes Sep 29, 2019

View reviewed changes

gvanrossum reviewed Oct 23, 2019

View reviewed changes

gvanrossum approved these changes Oct 23, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Oct 23, 2019

csabella added needs backport to 3.7 labels Jan 12, 2020

miss-islington merged commit 9f3fc6c into python:master Jan 12, 2020

bedevere-bot removed the awaiting merge label Jan 12, 2020

miss-islington self-assigned this Jan 12, 2020

bedevere-bot removed the needs backport to 3.7 label Jan 12, 2020

csabella added needs backport to 3.8 and removed needs backport to 3.8 labels Jan 12, 2020

bedevere-bot removed the needs backport to 3.8 label Jan 12, 2020

sthagen added a commit to sthagen/python-cpython that referenced this pull request Jan 12, 2020

Merge pull request #166 from python/master

948f650

bpo-38293: Allow shallow and deep copying of property objects (pythonGH-16438)

Uh oh!

Uh oh!

Conversation

GudniNathan commented Sep 27, 2019 • edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eamanu left a comment

Choose a reason for hiding this comment

Uh oh!

brandtbucher left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GudniNathan commented Sep 29, 2019

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

arigo commented Oct 23, 2019

Uh oh!

gvanrossum commented Oct 23, 2019

Uh oh!

miss-islington commented Jan 12, 2020

Uh oh!

miss-islington commented Jan 12, 2020

Uh oh!

bedevere-bot commented Jan 12, 2020

Uh oh!

miss-islington commented Jan 12, 2020

Uh oh!

bedevere-bot commented Jan 12, 2020

Uh oh!

brandtbucher commented Jan 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

GudniNathan commented Sep 27, 2019 •

edited by miss-islington

Loading

brandtbucher left a comment •

edited

Loading