diff --git a/amaranth/lib/wiring.py b/amaranth/lib/wiring.py index e171fb7..a9ae082 100644 --- a/amaranth/lib/wiring.py +++ b/amaranth/lib/wiring.py @@ -151,8 +151,12 @@ class SignatureError(Exception): class SignatureMembers(Mapping): def __init__(self, members=()): self._dict = dict() - self._frozen = False - self += members + for name, member in dict(members).items(): + 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) - 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 + raise SignatureError("Members cannot be added to a signature once constructed") 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}){frozen_repr}" + return f"SignatureMembers({self._dict})" @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": - # Although `sig.flip().members` does not call `__getattr__` but directly invokes - # the descriptor of the `FlippedSignature.members` property, `sig.flip().members +=` - # does call `__setattr__`, and this must be special-cased for the setter to work. - FlippedSignature.members.__set__(self, value) - else: - try: # descriptor first - _gettypeattr(self.__unflipped, name).__set__(self, value) - except AttributeError: - setattr(self.__unflipped, name, value) + 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({}) - for base in cls.mro()[:cls.mro().index(Component)]: + members = {} + 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 not signature.members: + if name in 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) diff --git a/docs/changes.rst b/docs/changes.rst index 45e71c8..34cad61 100644 --- a/docs/changes.rst +++ b/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 diff --git a/tests/test_lib_wiring.py b/tests/test_lib_wiring.py index a7ba0e5..7aeef86 100644 --- a/tests/test_lib_wiring.py +++ b/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$"): - members["a"] = Out(2) - - 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) + r"^Members cannot be added to a signature once constructed$"): + members["a"] = In(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)} - self.assertEqual(members["c"], Out(2)) + members = SignatureMembers({"a": In(1), "b": In(2), "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, - r"property 'members' of 'Signature' object cannot be set"): + with self.assertRaises(AttributeError): 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, - r"property 'members' of 'FlippedSignature' object cannot be set"): + with self.assertRaises(AttributeError): 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 .*.B$"): + B()