Repository: amaranth-lang/amaranth
Pull request #994 (2023-12-11 16:53:09)

lib.wiring: preserve insertion order in `SignatureMembers.__iter__`.
The order in which members of a `Signature` are defined can communicate developer intent, which is useful to preserve when generating documentation.

whitequark: I am pretty sure this only does not break `connect` by chance. You need to verify that the logic in `connect`, which relies on iteration orders, is still valid.
jfng: Updated to fix `connect` breakage and test for unordered signature members.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/amaranth-lang/amaranth/pull/994?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`8e6ae9e`)](https://app.codecov.io/gh/amaranth-lang/amaranth/commit/8e6ae9e6e0bc5ab1413f99179f4b76920e8fdc39?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang) 83.94% compared to head [(`5edd301`)](https://app.codecov.io/gh/amaranth-lang/amaranth/pull/994?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang) 83.89%.


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #994      +/-   ##
==========================================
- Coverage   83.94%   83.89%   -0.05%     
==========================================
  Files          55       55              
  Lines        8061     8061              
  Branches     1966     1966              
==========================================
- Hits         6767     6763       -4     
- Misses       1082     1085       +3     
- Partials      212      213       +1     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/amaranth-lang/amaranth/pull/994?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang).


Patch:
diff --git a/amaranth/lib/wiring.py b/amaranth/lib/wiring.py
index 53bfafd..a7bff3b 100644
--- a/amaranth/lib/wiring.py
+++ b/amaranth/lib/wiring.py
@@ -163,7 +163,7 @@ class SignatureMembers(Mapping):
 
     def __eq__(self, other):
         return (isinstance(other, (SignatureMembers, FlippedSignatureMembers)) and
-                list(self.flatten()) == list(other.flatten()))
+                list(sorted(self.flatten())) == list(sorted(other.flatten())))
 
     def __contains__(self, name):
         return name in self._dict
@@ -189,7 +189,7 @@ class SignatureMembers(Mapping):
         raise SignatureError("Members cannot be removed from a signature")
 
     def __iter__(self):
-        return iter(sorted(self._dict))
+        return iter(self._dict)
 
     def __len__(self):
         return len(self._dict)
@@ -647,7 +647,7 @@ def connect(m, *args, **kwargs):
         signatures[handle] = obj.signature
 
     # Collate signatures and build connections.
-    flattens = {handle: signature.members.flatten()
+    flattens = {handle: iter(sorted(signature.members.flatten()))
                 for handle, signature in signatures.items()}
     connections = []
     # Each iteration of the outer loop is intended to connect several (usually a pair) members


Test patch:
diff --git a/tests/test_lib_wiring.py b/tests/test_lib_wiring.py
index b09664c..d72d7bd 100644
--- a/tests/test_lib_wiring.py
+++ b/tests/test_lib_wiring.py
@@ -200,11 +200,11 @@ class SignatureMembersTestCase(unittest.TestCase):
         self.assertEqual(list(iter(members)), ["a"])
         self.assertEqual(len(members), 1)
 
-    def test_iter_sorted(self):
+    def test_iter_insertion_order(self):
         self.assertEqual(list(iter(SignatureMembers({"a": In(1), "b": Out(1)}))),
                          ["a", "b"])
         self.assertEqual(list(iter(SignatureMembers({"b": In(1), "a": Out(1)}))),
-                         ["a", "b"])
+                         ["b", "a"])
 
     def test_flatten(self):
         sig = Signature({
@@ -915,6 +915,27 @@ class ConnectTestCase(unittest.TestCase):
             '(eq (sig q__a) (sig p__a))'
         ])
 
+    def test_unordered(self):
+        m = Module()
+        connect(m,
+                p=NS(signature=Signature({"a": Out(1),
+                                          "b": Out(Signature({"f": Out(1), "g": Out(1)}))}),
+                     a=Signal(name="p__a"),
+                     b=NS(signature=Signature({"f": Out(1), "g": Out(1)}),
+                          f=Signal(name="p__b__f"),
+                          g=Signal(name="p__b__g"))),
+                q=NS(signature=Signature({"b": In(Signature({"g": Out(1), "f": Out(1)})),
+                                          "a": In(1)}),
+                     b=NS(signature=Signature({"g": Out(1), "f": Out(1)}).flip(),
+                          g=Signal(name="q__b__g"),
+                          f=Signal(name="q__b__f")),
+                     a=Signal(name="q__a")))
+        self.assertEqual([repr(stmt) for stmt in m._statements], [
+            '(eq (sig q__a) (sig p__a))',
+            '(eq (sig q__b__f) (sig p__b__f))',
+            '(eq (sig q__b__g) (sig p__b__g))',
+        ])
+
     def test_dimension(self):
         sig = Signature({"a": Out(1).array(2)})