python-attrs / attrs

Python Classes Without Boilerplate
https://www.attrs.org/
MIT License
5.3k stars 374 forks source link

make_class(): Populate the __annotations__ dict #1271

Closed sscherfke closed 4 months ago

sscherfke commented 8 months ago

make_class() currently doesn't touch the __annotations__ dict when it creates a new class.

This can become a problem when a class is created dynamically with unresolved types. And when __annotations__ is empty, resolve_type() on the generated class will do nothing.

Here is a use case: https://gitlab.com/sscherfke/typed-settings/-/issues/54

There's not much required to fix this:

diff --git a/src/attr/_make.py b/src/attr/_make.py
index 0114582..bbdfc15 100644
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -3058,7 +3058,12 @@ def make_class(
         True,
     )

-    return _attrs(these=cls_dict, **attributes_arguments)(type_)
+    cls = _attrs(these=cls_dict, **attributes_arguments)(type_)
+    # Only add type annotations now or "_attrs()" will complain:
+    cls.__annotations__ = {
+        k: v.type for k, v in cls_dict.items() if v.type is not None
+    }
+    return cls

 # These are required by within this module so we define them here and merely
diff --git a/tests/test_make.py b/tests/test_make.py
index 736ab8c..e89ee87 100644
--- a/tests/test_make.py
+++ b/tests/test_make.py
@@ -1164,6 +1164,32 @@ class TestMakeClass:

         attr.make_class("test", {"id": attr.ib(type=str)}, (MyParent[int],))

+    def test_annotations(self):
+        """
+        make_class fills the __annotations__ dict for attributes with a known
+        type.
+        """
+        a = attr.ib(type=bool)
+        b = attr.ib(type=None)  # Won't be added to ann. b/c of unfavorable default
+        c = attr.ib()
+
+        C = attr.make_class("C", {"a": a, "b": b, "c": c})
+        C = attr.resolve_types(C)
+
+        assert C.__annotations__ == {"a": bool}
+
+    def test_annotations_resolve(self):
+        """
+        resolve_types() resolves the annotations added by make_class().
+        """
+        a = attr.ib(type="bool")
+
+        C = attr.make_class("C", {"a": a})
+        C = attr.resolve_types(C)
+
+        assert attr.fields(C).a.type is bool
+        assert C.__annotations__ == {"a": "bool"}
+

 class TestFields:
     """

The only problem with this is the unfortunate default of field(type=None), b/c we cannot differentiate between an explicit None type an no type. I’m afraid though, that changing the default of type= to a sentinel object would be a braking change.

sscherfke commented 8 months ago

Solved it in Typed Settings, so fixing this is not super important. The __annotations__ dict being available for dynamic classes would be nice nonetheless, though.

hynek commented 6 months ago

Any reason you dumped a diff instead of a PR? 😅

sscherfke commented 6 months ago

It's just a draft for helping you decide whether you like this change or not. If you like it, I will prepare a PR. :-)