Implement RFC 37: Make `Signature` immutable.

Fixes #995.

amaranth/lib/wiring.py

@@ -151,8 +151,12 @@ class SignatureError(Exception):
 class SignatureMembers(Mapping):
     def __init__(self, members=()):
         self._dict = dict()
-        self._frozen = False
+        for name, member in dict(members).items():
-        self += members
+            self._check_name(name)
+            if type(member) is not Member:
+                raise TypeError(f"Value {member!r} must be a member; "
+                                f"did you mean In({member!r}) or Out({member!r})?")
+            self._dict[name] = member
 
     def flip(self):
         return FlippedSignatureMembers(self)
@@ -179,16 +183,7 @@ class SignatureMembers(Mapping):
         return self._dict[name]
 
     def __setitem__(self, name, member):
-        self._check_name(name)
+        raise SignatureError("Members cannot be added to a signature once constructed")
-        if name in self._dict:
-            raise SignatureError(f"Member '{name}' already exists in the signature and cannot "
-                                 f"be replaced")
-        if type(member) is not Member:
-            raise TypeError(f"Assigned value {member!r} must be a member; "
-                            f"did you mean In({member!r}) or Out({member!r})?")
-        if self._frozen:
-            raise SignatureError("Cannot add members to a frozen signature")
-        self._dict[name] = member
 
     def __delitem__(self, name):
         raise SignatureError("Members cannot be removed from a signature")
@@ -199,21 +194,6 @@ class SignatureMembers(Mapping):
     def __len__(self):
         return len(self._dict)
 
-    def __iadd__(self, members):
-        for name, member in dict(members).items():
-            self[name] = member
-        return self
-
-    @property
-    def frozen(self):
-        return self._frozen
-
-    def freeze(self):
-        self._frozen = True
-        for member in self.values():
-            if member.is_signature:
-                member.signature.freeze()
-
     def flatten(self, *, path=()):
         for name, member in self.items():
             yield ((*path, name), member)
@@ -244,8 +224,7 @@ class SignatureMembers(Mapping):
         return attrs
 
     def __repr__(self):
-        frozen_repr = ".freeze()" if self._frozen else ""
+        return f"SignatureMembers({self._dict})"
-        return f"SignatureMembers({self._dict}){frozen_repr}"
 
 
 @final
@@ -277,17 +256,6 @@ class FlippedSignatureMembers(Mapping):
     def __len__(self):
         return self.__unflipped.__len__()
 
-    def __iadd__(self, members):
-        self.__unflipped.__iadd__({name: member.flip() for name, member in members.items()})
-        return self
-
-    @property
-    def frozen(self):
-        return self.__unflipped.frozen
-
-    def freeze(self):
-        self.__unflipped.freeze()
-
     # These methods do not access instance variables and so their implementation can be shared
     # between the normal and the flipped member collections.
     flatten = SignatureMembers.flatten
@@ -358,12 +326,6 @@ class Signature(metaclass=SignatureMeta):
     def members(self):
         return self.__members
 
-    @members.setter
-    def members(self, new_members):
-        # The setter is called when `sig.members += ...` is used.
-        if new_members is not self.__members:
-            raise AttributeError("property 'members' of 'Signature' object cannot be set")
-
     def __eq__(self, other):
         other_unflipped = other.flip() if type(other) is FlippedSignature else other
         if type(self) is type(other_unflipped) is Signature:
@@ -374,14 +336,6 @@ class Signature(metaclass=SignatureMeta):
             # usually be overridden in a derived class.
             return self is other
 
-    @property
-    def frozen(self):
-        return self.members.frozen
-
-    def freeze(self):
-        self.members.freeze()
-        return self
-
     def flatten(self, obj):
         for name, member in self.members.items():
             path = (name,)
@@ -544,11 +498,6 @@ class FlippedSignature:
     def members(self):
         return FlippedSignatureMembers(self.__unflipped.members)
 
-    @members.setter
-    def members(self, new_members):
-        if new_members.flip() is not self.__unflipped.members:
-            raise AttributeError("property 'members' of 'FlippedSignature' object cannot be set")
-
     def __eq__(self, other):
         if type(other) is FlippedSignature:
             # Trivial case.
@@ -561,10 +510,8 @@ class FlippedSignature:
             # in infinite recursion.
             return NotImplemented
 
-    # These methods do not access instance variables and so their implementation can be shared
+    # This method does not access instance variables and so its implementation can be shared
     # between the normal and the flipped member collections.
-    frozen = Signature.frozen
-    freeze = Signature.freeze
     is_compliant = Signature.is_compliant
 
     # FIXME: document this logic
@@ -581,16 +528,10 @@ class FlippedSignature:
             return getattr(self.__unflipped, name)
 
     def __setattr__(self, name, value):
-        if name == "members":
+        try: # descriptor first
-            # Although `sig.flip().members` does not call `__getattr__` but directly invokes
+            _gettypeattr(self.__unflipped, name).__set__(self, value)
-            # the descriptor of the `FlippedSignature.members` property, `sig.flip().members +=`
+        except AttributeError:
-            # does call `__setattr__`, and this must be special-cased for the setter to work.
+            setattr(self.__unflipped, name, value)
-            FlippedSignature.members.__set__(self, value)
-        else:
-            try: # descriptor first
-                _gettypeattr(self.__unflipped, name).__set__(self, value)
-            except AttributeError:
-                setattr(self.__unflipped, name, value)
 
     def __delattr__(self, name):
         try: # descriptor first
@@ -703,7 +644,7 @@ def connect(m, *args, **kwargs):
             reasons_as_string = "".join("\n- " + reason for reason in reasons)
             raise ConnectionError(f"Argument {handle!r} does not match its signature:" +
                                   reasons_as_string)
-        signatures[handle] = obj.signature.freeze()
+        signatures[handle] = obj.signature
 
     # Collate signatures and build connections.
     flattens = {handle: signature.members.flatten()
@@ -875,8 +816,8 @@ class Component(Elaboratable):
     @property
     def signature(self):
         cls = type(self)
-        signature = Signature({})
+        members = {}
-        for base in cls.mro()[:cls.mro().index(Component)]:
+        for base in reversed(cls.mro()[:cls.mro().index(Component)]):
             for name, annot in base.__dict__.get("__annotations__", {}).items():
                 if name.startswith("_"):
                     continue
@@ -897,9 +838,11 @@ class Component(Elaboratable):
                         category=SyntaxWarning,
                         stacklevel=2)
                 elif type(annot) is Member:
-                    signature.members[name] = annot
+                    if name in members:
-        if not signature.members:
+                        raise SignatureError(f"Member '{name}' is redefined in {base.__module__}.{base.__qualname__}")
+                    members[name] = annot
+        if not members:
             raise NotImplementedError(
                 f"Component '{cls.__module__}.{cls.__qualname__}' does not have signature member "
                 f"annotations")
-        return signature
+        return Signature(members)

docs/changes.rst

@@ -63,6 +63,7 @@ Implemented RFCs
 .. _RFC 31: https://amaranth-lang.org/rfcs/0031-enumeration-type-safety.html
 .. _RFC 34: https://amaranth-lang.org/rfcs/0034-interface-rename.html
 .. _RFC 35: https://amaranth-lang.org/rfcs/0035-shapelike-valuelike.html
+.. _RFC 37: https://amaranth-lang.org/rfcs/0037-make-signature-immutable.html
 
 
 * `RFC 1`_: Aggregate data structure library
@@ -83,6 +84,7 @@ Implemented RFCs
 * `RFC 31`_: Enumeration type safety
 * `RFC 34`_: Rename ``amaranth.lib.wiring.Interface`` to ``PureInterface``
 * `RFC 35`_: Add ``ShapeLike``, ``ValueLike``
+* `RFC 37`_: Make ``Signature`` immutable
 
 
 Language changes

tests/test_lib_wiring.py

@@ -182,29 +182,9 @@ class SignatureMembersTestCase(unittest.TestCase):
 
     def test_setitem(self):
         members = SignatureMembers()
-        members["a"] = In(1)
-        self.assertEqual(members["a"], In(1))
-
-    def test_setitem_existing(self):
-        members = SignatureMembers({"a": In(1)})
         with self.assertRaisesRegex(SignatureError,
-                r"^Member 'a' already exists in the signature and cannot be replaced$"):
+                r"^Members cannot be added to a signature once constructed$"):
-            members["a"] = Out(2)
+            members["a"] = In(1)
-
-    def test_setitem_wrong(self):
-        members = SignatureMembers()
-        with self.assertRaisesRegex(TypeError,
-                r"^Member name must be a string, not 1$"):
-            members[1] = Out(1)
-        with self.assertRaisesRegex(TypeError,
-                r"^Assigned value 1 must be a member; did you mean In\(1\) or Out\(1\)\?$"):
-            members["a"] = 1
-        with self.assertRaisesRegex(NameError,
-                r"^Member name '_a' must be a valid, public Python attribute name$"):
-            members["_a"] = Out(1)
-        with self.assertRaisesRegex(NameError,
-                r"^Member name cannot be 'signature'$"):
-            members["signature"] = Out(1)
 
     def test_delitem(self):
         members = SignatureMembers()
@@ -216,7 +196,7 @@ class SignatureMembersTestCase(unittest.TestCase):
         members = SignatureMembers()
         self.assertEqual(list(iter(members)), [])
         self.assertEqual(len(members), 0)
-        members["a"] = In(1)
+        members = SignatureMembers({"a": In(1)})
         self.assertEqual(list(iter(members)), ["a"])
         self.assertEqual(len(members), 1)
 
@@ -226,35 +206,6 @@ class SignatureMembersTestCase(unittest.TestCase):
         self.assertEqual(list(iter(SignatureMembers({"b": In(1), "a": Out(1)}))),
                          ["a", "b"])
 
-    def test_iadd(self):
-        members = SignatureMembers()
-        members += {"a": In(1)}
-        members += [("b", Out(1))]
-        self.assertEqual(members, SignatureMembers({"a": In(1), "b": Out(1)}))
-
-    def test_freeze(self):
-        members = SignatureMembers({"a": In(1)})
-        self.assertEqual(members.frozen, False)
-        members.freeze()
-        self.assertEqual(members.frozen, True)
-        with self.assertRaisesRegex(SignatureError,
-                r"^Cannot add members to a frozen signature$"):
-            members += {"b": Out(1)}
-
-    def test_freeze_rec(self):
-        sig = Signature({})
-        members = SignatureMembers({
-            "a": In(1),
-            "s": Out(sig)
-        })
-        self.assertEqual(members.frozen, False)
-        self.assertEqual(sig.frozen, False)
-        self.assertEqual(sig.members.frozen, False)
-        members.freeze()
-        self.assertEqual(members.frozen, True)
-        self.assertEqual(sig.frozen, True)
-        self.assertEqual(sig.members.frozen, True)
-
     def test_flatten(self):
         sig = Signature({
             "b": Out(1),
@@ -307,15 +258,19 @@ class SignatureMembersTestCase(unittest.TestCase):
                 self.assertIsInstance(y, Signal)
         self.assertEqual(members["a"][1][2].name, "members__a__1__2")
 
+    def test_create_wrong(self):
+        with self.assertRaisesRegex(TypeError,
+                r"^Value 1 must be a member; did you mean In\(1\) or Out\(1\)\?$"):
+            SignatureMembers({"a": 1})
+
     def test_repr(self):
         self.assertEqual(repr(SignatureMembers({})),
                          "SignatureMembers({})")
         self.assertEqual(repr(SignatureMembers({"a": In(1)})),
                          "SignatureMembers({'a': In(1)})")
         members = SignatureMembers({"b": Out(2)})
-        members.freeze()
         self.assertEqual(repr(members),
-                         "SignatureMembers({'b': Out(2)}).freeze()")
+                         "SignatureMembers({'b': Out(2)})")
 
 
 class FlippedSignatureMembersTestCase(unittest.TestCase):
@@ -325,12 +280,14 @@ class FlippedSignatureMembersTestCase(unittest.TestCase):
         self.assertIsInstance(fmembers, FlippedSignatureMembers)
         self.assertIn("a", fmembers)
         self.assertEqual(fmembers["a"], Out(1))
-        fmembers["b"] = Out(2)
+        members = SignatureMembers({"a": In(1), "b": In(2)})
+        fmembers = members.flip()
         self.assertEqual(len(fmembers), 2)
-        self.assertEqual(members["b"], In(2))
+        self.assertEqual(fmembers["b"], Out(2))
         self.assertEqual(list(fmembers), ["a", "b"])
-        fmembers += {"c": In(2)}
+        members = SignatureMembers({"a": In(1), "b": In(2), "c": Out(2)})
-        self.assertEqual(members["c"], Out(2))
+        fmembers = members.flip()
+        self.assertEqual(fmembers["c"], In(2))
         self.assertIs(fmembers.flip(), members)
 
     def test_eq(self):
@@ -345,14 +302,6 @@ class FlippedSignatureMembersTestCase(unittest.TestCase):
                 r"^Members cannot be removed from a signature$"):
             del fmembers["a"]
 
-    def test_freeze(self):
-        members = SignatureMembers({"a": In(1)})
-        fmembers = members.flip()
-        self.assertEqual(fmembers.frozen, False)
-        fmembers.freeze()
-        self.assertEqual(members.frozen, True)
-        self.assertEqual(fmembers.frozen, True)
-
     def test_repr(self):
         fmembers = SignatureMembers({"a": In(1)}).flip()
         self.assertEqual(repr(fmembers), "SignatureMembers({'a': In(1)}).flip()")
@@ -369,25 +318,9 @@ class SignatureTestCase(unittest.TestCase):
         self.assertNotEqual(Signature({"a": In(1)}),
                             Signature({"a": Out(1)}))
 
-    def test_freeze(self):
-        sig = Signature({"a": In(1)})
-        self.assertEqual(sig.frozen, False)
-        sig.freeze()
-        self.assertEqual(sig.frozen, True)
-        with self.assertRaisesRegex(SignatureError,
-                r"^Cannot add members to a frozen signature$"):
-            sig.members += {"b": Out(1)}
-
-    def test_members_plus_equal(self):
-        sig = Signature({})
-        # This invokes the setter of Signature.members.
-        sig.members += {"a": In(1)}
-        self.assertIn("a", sig.members)
-
     def test_members_equal_wrong(self):
         sig = Signature({})
-        with self.assertRaisesRegex(AttributeError,
+        with self.assertRaises(AttributeError):
-                r"property 'members' of 'Signature' object cannot be set"):
             sig.members = SignatureMembers({})
 
     def assertFlattenedSignature(self, actual, expected):
@@ -643,16 +576,9 @@ class FlippedSignatureTestCase(unittest.TestCase):
         fsig.x()
         self.assertEqual(x_type, S)
 
-    def test_members_plus_equal(self):
-        sig = Signature({})
-        # This invokes the setter of FlippedSignature.members.
-        sig.flip().members += {"a": In(1)}
-        self.assertIn("a", sig.members)
-
     def test_members_equal_wrong(self):
         sig = Signature({})
-        with self.assertRaisesRegex(AttributeError,
+        with self.assertRaises(AttributeError):
-                r"property 'members' of 'FlippedSignature' object cannot be set"):
             sig.flip().members = SignatureMembers({})
 
 
@@ -843,12 +769,6 @@ class ConnectTestCase(unittest.TestCase):
                 r"- 'arg0' does not have an attribute 'a'$"):
             connect(m, NS(signature=Signature({"a": In(1)})))
 
-    def test_signature_freeze(self):
-        m = Module()
-        intf = NS(signature=Signature({}))
-        connect(m, intf)
-        self.assertTrue(intf.signature.frozen)
-
     def test_member_missing(self):
         m = Module()
         with self.assertRaisesRegex(ConnectionError,
@@ -1123,3 +1043,14 @@ class ComponentTestCase(unittest.TestCase):
 
         c = C()
         self.assertEqual(c.signature, Signature({"clk": In(1), "rst": In(1)}))
+
+    def test_inherit_wrong(self):
+        class A(Component):
+            a: In(1)
+
+        class B(A):
+            a: Out(1)
+
+        with self.assertRaisesRegex(SignatureError,
+                r"^Member 'a' is redefined in .*<locals>.B$"):
+            B()