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

Does the metadata default dict have to be a mutable 'singleton'? #278

Closed
altendky opened this issue Oct 30, 2017 · 4 comments
Closed

Does the metadata default dict have to be a mutable 'singleton'? #278

altendky opened this issue Oct 30, 2017 · 4 comments

Comments

@altendky
Copy link
Contributor

altendky commented Oct 30, 2017

attr.ib() creates a mutable 'singleton' {} as the default for metadata.

convert=None, metadata={}, type=None):

All empty metadata attributes share the same empty metadata dict.

#96 (comment)

I just ran into this while working on a couple of libraries that leverage the structure provided by attrs classes. I want them to be able to easily add their own data to the attribute metadata and preferred the mutate_metadata() below over the create_metadata() option. I have a couple of different things doing similar to this already so stacking them in a more decorator-like way seems cleaner.

@attr.s
class C:
    c = attr.ib()
    mutate_metadata(attribute=c)


@attr.s
class D:
    d = attr.ib(metadata={**create_metadata()})

Naive mutation resulted in sharing the modified 'singleton' dictionary with other attributes that didn't specify the metadata on creation.

Perhaps immutable metadata makes sense but then it should at least be immutable instead of just global? I would obviously like it to be mutable at least until @attr.s for my usage, though I haven't considered the implications of that.

https://repl.it/NZ22/4

import attr


def create_metadata():
    return {'create': 37}


def mutate_metadata(attribute):
    if len(attribute.metadata) == 0:
        attribute.metadata = {}

    attribute.metadata['mutate'] = 42


def naively_mutate_metadata(attribute):
    attribute.metadata['naive_mutate'] = 13


@attr.s
class C:
    c = attr.ib()
    mutate_metadata(attribute=c)

    d = attr.ib(metadata={**create_metadata()})

    e = attr.ib()
    naively_mutate_metadata(e)

    f = attr.ib()

print(attr.fields(C).c.metadata)
assert len(attr.fields(C).c.metadata) == 1

print(attr.fields(C).d.metadata)
assert len(attr.fields(C).d.metadata) == 1

print(attr.fields(C).e.metadata)
assert len(attr.fields(C).e.metadata) == 1

print(attr.fields(C).f.metadata)
assert len(attr.fields(C).f.metadata) == 0
{'mutate': 42}
{'create': 37}
{'naive_mutate': 13}
{'naive_mutate': 13}
Traceback (most recent call last):
  File "python", line 41, in <module>
AssertionError
@altendky
Copy link
Contributor Author

Even with this patch all tests pass for both py2 and 3. This includes test_empty_metadata_singleton() which I would expect should fail.

diff --git a/src/attr/_make.py b/src/attr/_make.py
index eaac084..70e612d 100644
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -61,7 +61,7 @@ Sentinel to indicate the lack of a value when ``None`` is ambiguous.
 
 def attrib(default=NOTHING, validator=None,
            repr=True, cmp=True, hash=None, init=True,
-           convert=None, metadata={}, type=None):
+           convert=None, metadata=None, type=None):
     """
     Create a new attribute on a class.
 
@@ -140,6 +140,8 @@ def attrib(default=NOTHING, validator=None,
         raise TypeError(
             "Invalid value for hash.  Must be True, False, or None."
         )
+    if metadata is None:
+        metadata = {}
     return _CountingAttr(
         default=default,
         validator=validator,

@Tinche
Copy link
Member

Tinche commented Oct 30, 2017

Hi,

a couple of remarks.

Once the class is "baked", if an attribute has no metadata, attr.Attribute instances should share the same empty singleton. This is an optimization to avoid hundreds of unnecessary empty dictionaries that I'd like to keep. I'd also like to keep the type of attr.Attribute.metadata as Dict[str, Any] instead of Optional[Dict[str, Any]] (and we have to because of backwards comp).

Also, if you want to apply your changes before the attr.s decorator, you're dealing with _CountingAttrs, which are internal as the name suggests, so extra care is needed. :)

That being said, I don't see a problem with making _CountingAttr instances have non-singleton metadata dictionaries, as long as final Attribute instances acquire logic to potentially reuse the singleton when CountingAttrs get converted into Attributes. I think this is the first use case of this sort we've seen, and I don't see a reason to not support it?

@altendky
Copy link
Contributor Author

The single-empty-dict optimization only applies to the number of attr.ib() defined and is not related to the number of objects instantiated from the attrs classes, correct? I'm just surprised to find this optimization. I'm sure 'you' already know this but for the record sys.getsizeof({}) is ~250-300 bytes depending on Python version.

Anyways, reapplying the single-empty-dict optimization at the time of @attr.s should cover my use case.

It's really about hiding the implementation (not terribly important but seems kind) and providing more isolation for each additional lib I apply to the attribute (improved legibility). In my two present cases, I already updated the metadata modifier to check for an empty dict and replace it with a fresh one to avoid this issue. The concern remains that this optimization is exposed to the user without using a read-only proxy (like _compat.metadata_proxy() creates) and could cause confusion for others. Though, as mentioned, this seems to be a highly uncommon activity.

I looked at Attribute.__init__() and found that the actual _empty_metadata_singleton is applied there. This means that test_empty_metadata_singleton() is passing appropriately. As such, it seems to me that the use of multiple empty dicts before that should not be an issue.

I'll be heading out soon but will try to come up with a valid test that also fits in. The following ends up with several object(): 42 mappings and fails. Looks like I've got some learning to do about Hypothesis-provided values.

    @given(lists(simple_attrs_without_metadata, min_size=2, max_size=5))
    def test_empty_countingattr_metadata_independent(self, list_of_attrs):
        """
        All empty metadata attributes are independent until ``@attr.s``.
        """
        list_of_attrs[0].metadata[object()] = 42
        C = make_class("C", dict(zip(gen_attr_names(), list_of_attrs)))
        for a in fields(C)[1:]:
            assert len(a.metadata) == 0

altendky added a commit to altendky/attrs that referenced this issue Oct 30, 2017
hynek pushed a commit that referenced this issue Dec 19, 2017
* Make _CountingAttr empty metadata unique

Issue #278

* Correct st.none().map() to st.builds()

* Add 'real' and 'force coverage' tests for not None metadata

* Add changelog for pr 280

* Correct import order in tests/test_make.py

* Add back coverage force test

* Remove debug print from test/utils.py
@hynek
Copy link
Member

hynek commented Dec 30, 2017

This should be fixed.

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

No branches or pull requests

3 participants