Repository: amaranth-lang/amaranth
Pull request #1153 (2024-02-23 05:44:31)

lib.wiring.connect: diagnostic when no connection made.
Commit message:

> If a `connect()` call results in no connections being made, and it's because there were no outputs specified at all, issue an error.

---

(I've read the RFCs and git logs reasonably carefully but I'm basically assuming I've missed something/a decision made since. Apologies if so!)

[RFC 2 § Interface connection](https://amaranth-lang.org/rfcs/0002-interfaces.html#interface-connection) states `lib.wiring.connect`'s requirements:

> This function connects interface objects that satisfy the following requirements:
>
> [...]
>
> * For each given path where all members are port members, exactly one member has an `Out` flow.

The actual implementation diverges; the docstring listing these requirements omits the last point, and [notes later on](https://github.com/amaranth-lang/amaranth/blob/09029cdd914d550e924cecfb9aa1692a536b4161/amaranth/lib/wiring.py#L1393):

> (If no interface object has an output for a given path, \*\*no connection at all\*\* is made.)

I can appreciate this probably comes in handy when connecting larger/nested signatures, but while orienting my head around "which way is `In` / which way is `Out`" and taking advantage of the new `Memory`'s port signatures, I ended up lost for a while wondering why nothing was propagating when I was connecting a `WritePort` to a submodule.

tl;dr I annotated the destination member as `Out(WritePort.Signature(…))`, since it had another `Out(…)` next to it and I was on autopilot while adding it. Write ports consist entirely of `In` members, meaning `connect(m, mem.write_port(), thing.write_port)` in such circumstances will quietly do nothing.

(Read ports do issue errors under such circumstances, since they have one `Out` member which will then clash. I was doing _type-guided programming_ (aka "just keep adding/removing `flipped` on various members until it doesn't throw an error") y'know.)

I've added a diagnostic here which raises if we never encounter a path with a corresponding output port member. Having a constant output (with its matching input) is sufficient.

I don't have visibility on larger designs so I'm not sure if this is going too far; no sweat if it's inappropriate.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/amaranth-lang/amaranth/pull/1153?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:
> Project coverage is 85.86%. Comparing base [(`09029cd`)](https://app.codecov.io/gh/amaranth-lang/amaranth/commit/09029cdd914d550e924cecfb9aa1692a536b4161?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang) to head [(`d9b18aa`)](https://app.codecov.io/gh/amaranth-lang/amaranth/pull/1153?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=amaranth-lang).


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


```diff
@@            Coverage Diff             @@
##             main    #1153      +/-   ##
==========================================
- Coverage   85.88%   85.86%   -0.02%     
==========================================
  Files          43       43              
  Lines        8679     8683       +4     
  Branches     2065     2066       +1     
==========================================
+ Hits         7454     7456       +2     
- Misses       1024     1027       +3     
+ Partials      201      200       -1     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/amaranth-lang/amaranth/pull/1153?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).

whitequark: > > * For each given path where all members are port members, exactly one member has an `Out` flow.
> 
> The actual implementation diverges; the docstring listing these requirements omits the last point, and [notes later on](https://github.com/amaranth-lang/amaranth/blob/09029cdd914d550e924cecfb9aa1692a536b4161/amaranth/lib/wiring.py#L1393):
> 
> > (If no interface object has an output for a given path, **no connection at all** is made.)

Thanks for bringing this to our attention. I don't actually recall what happened there; I think this is a simple case of a miscommunication between the spec author and the implementer (made somewhat more awkward by the fact that both of those are, theoretically, me.)

I cannot provide a justification for allowing `In` to `In` connections (I feel like I had one at some point, but that might just be a false memory), but I'm also hesitant to forbid it outright. There is a number of features that `connect` provides that may look a little strange, e.g. the ability to connect three interfaces neither of which is a mirror image of another. I think enabling users of a programming language to creatively experiment with some of the weirder features is a great outcome when it doesn't create a hazard for the majority of uses, and this is a principle I've tried to consistently apply in Amaranth.

> I was doing _type-guided programming_ (aka "just keep adding/removing `flipped` on various members until it doesn't throw an error") y'know.)

Enabling such type-guided programming is an explicit goal, and the current situation does create a hazard. Adding a diagnostic here is clearly appropriate; I find it unlikely that anyone will strongly desire to have a `connect` call with non-empty interfaces that does nothing. I'm happy to add a diagnostic for this case and I'm glad that it covers a usability issue that I assumed we had covered but in fact didn't.

The larger question is whether to forbid `In` to `In` connections outright. Practically, we will have to deprecate them first; even if it contradicts the RFC, the ultimate source of truth for the language semantics is the documentaiton, not the RFCs (which are a process for making progress and achieving consensus rather than documentation). I don't know whether we should do that and I'm open to discussion here. It doesn't seem like we need to urgently address this, though.
whitequark: > I cannot provide a justification for allowing `In` to `In` connections

Thinking about it more, I think a part of the motivations at least was the case of `connect(m, *sigs)` where `len(sigs) == 1`. Clearly, if `sigs[0]` has any `In` members, no connection to them will be made (and this diagnostic would fire, I think).

So to break down the possible cases here:
1. `connect(m, Signature({})...)`: zero or more empty signatures. Clearly should be allowed; no potential to be a mistake and obvious usefulness for generic code.
2. `connect(m, any_signature)`: exactly one signature. Probably should be allowed; some potential for being a mistake (forgotten an argument?), but stands out quite well in source code, and obvious usefulness for generic code.
3. `connect(m, Signature({...: In(...)}), Signature({...: In(...)}))`: two or more signatures with matching `In` members and nothing else. Clearly should be forbidden; extremely likely to be a mistake, and very little usefulness.
4. `connect(m, Signature({"x": In(...), "y": In(...)}), Signature({"x": In(...), "y": Out(...)}))`: two or more signatures with matching `In` members but also matching `In`-`Out` pairs. Neutral; relatively unlikely to be a mistake, unclear usefulness.

I think we should (if we do not already) have tests for cases 1-4, and I think turning case 3 into a hard error (while leaving cases 1 and 2 alone) without a deprecation cycle is clearly justified. I think there's no clear justification for doing anything about case 4 but I'm happy to reconsider if others justify why it should or shouldn't be allowed.
kivikakk: > simple case of a miscommunication between the spec author and the implementer (made somewhat more awkward by the fact that both of those are, theoretically, me.)

Relatable!

> I think connecting any number of empty signatures is important to allow, for the same reason we allow zero-bit signals; it is useful in generic code to stub out unused interfaces.

Yep, agreed, and I'm a fan of using "stands out well in source code" as a point of reference when determining how likely things are to be a mistake/source of surprising danger.

> I think we should (if we do not already) have tests for cases 1-4, and I think turning case 3 into a hard error (while leaving cases 1 and 2 alone) without a deprecation cycle is clearly justified.


Scanned briefly, appears; 1: not yet; 2: not yet; 3: only the one added here; 4: not yet. Will add {1,2,4} and some commentary.

Patch:
diff --git a/amaranth/lib/wiring.py b/amaranth/lib/wiring.py
index 6b82cb4..4d89139 100644
--- a/amaranth/lib/wiring.py
+++ b/amaranth/lib/wiring.py
@@ -1371,6 +1371,7 @@ def connect(m, *args, **kwargs):
     * For a given path, if any of the interface objects has an input port member corresponding
       to a constant value, then the rest of the interface objects must have output port members
       corresponding to the same constant value.
+    * When connecting multiple interface objects, at least one connection must be made.
 
     For example, if :py:`obj1` is being connected to :py:`obj2` and :py:`obj3`, and :py:`obj1.a.b`
     is an output, then :py:`obj2.a.b` and :py:`obj2.a.b` must exist and be inputs. If :py:`obj2.c`
@@ -1420,10 +1421,15 @@ def connect(m, *args, **kwargs):
                                   reasons_as_string)
         signatures[handle] = obj.signature
 
-    # Collate signatures and build connections.
+    # Connecting zero or one signatures is OK.
+    if len(signatures) <= 1:
+        return
+
+    # Collate signatures, build connections, track whether we see any input or output.
     flattens = {handle: iter(sorted(signature.members.flatten()))
                 for handle, signature in signatures.items()}
     connections = []
+    any_in, any_out = False, False
     # Each iteration of the outer loop is intended to connect several (usually a pair) members
     # to each other, e.g. an out member `[0].a` to an in member `[1].a`. However, because we
     # do not just check signatures for equality (in order to improve diagnostics), it is possible
@@ -1437,6 +1443,7 @@ def connect(m, *args, **kwargs):
         # implied in the flow of each port member, so the signature members are only classified
         # here to ensure they are not connected to port members.
         is_first = True
+        first_path = None
         sig_kind, out_kind, in_kind = [], [], []
         for handle, flattened_members in flattens.items():
             path_for_handle, member = next(flattened_members, (None, None))
@@ -1499,6 +1506,8 @@ def connect(m, *args, **kwargs):
             # There are no port members at this point; we're done with this path.
             continue
         # There are only port members after this point.
+        any_in = any_in or bool(in_kind)
+        any_out = any_out or bool(out_kind)
         is_first = True
         for (path, member) in in_kind + out_kind:
             member_shape = member.shape
@@ -1574,6 +1583,14 @@ def connect(m, *args, **kwargs):
                                        out_path=(*out_path, index), in_path=(*in_path, index))
             assert out_member.dimensions == in_member.dimensions
             connect_dimensions(out_member.dimensions, out_path=out_path, in_path=in_path)
+
+    # If no connections were made, and there were inputs but no outputs in the
+    # signatures, issue a diagnostic as this is most likely in error.
+    if len(connections) == 0 and any_in and not any_out:
+        raise ConnectionError(f"Only input to input connections have been made between several "
+                              f"interfaces; should one of them have been flipped?")
+
+
     # Now that we know all of the connections are legal, add them to the module. This is done
     # instead of returning them because adding them to a non-comb domain would subtly violate
     # assumptions that `connect()` is intended to provide.


Test patch:
diff --git a/tests/test_lib_wiring.py b/tests/test_lib_wiring.py
index 35570ab..4bdfabe 100644
--- a/tests/test_lib_wiring.py
+++ b/tests/test_lib_wiring.py
@@ -980,6 +980,65 @@ class ConnectTestCase(unittest.TestCase):
             '(eq (sig q__a__0__0) (sig p__a__0__0))',
         ])
 
+    def test_connect_none(self):
+        # Connecting zero or more empty signatures is permitted as (a) it's not
+        # something you can write mistakenly out by hand, and (b) it permits
+        # generic code to expand to nothing without errors around edges.
+        m = Module()
+        connect(m)
+
+    def test_connect_empty(self):
+        m = Module()
+        connect(m, p=NS(signature=Signature({})))
+
+    def test_connect_empty_multi(self):
+        m = Module()
+        connect(m, p=NS(signature=Signature({})),
+                   q=NS(signature=Signature({})))
+
+    def test_connect_one(self):
+        # Connecting just one signature should be allowed for the same reasons
+        # as above. (It's possible to forget an argument, but that stands out.)
+        m = Module()
+        connect(m, p=NS(signature=Signature({"a": Out(1), "b": In(1)}),
+                        a=Signal(),
+                        b=Signal()))
+
+    def test_connect_one_in_only(self):
+        # As above, even if there's only inputs.
+        m = Module()
+        connect(m, p=NS(signature=Signature({"a": In(1)}),
+                        a=Signal()))
+
+    def test_connect_multi_in_only_fails(self):
+        # If we're only attempting to connect multiple inputs, we're not
+        # actually doing anything and it's most likely a mistake.
+        m = Module()
+        with self.assertRaisesRegex(ConnectionError,
+                r"^Only input to input connections have been made between several interfaces; "
+                r"should one of them have been flipped\?$"):
+            connect(m,
+                    p=NS(signature=Signature({"a": In(1), "b": In(8)}),
+                         a=Signal(),
+                         b=Signal(8)),
+                    q=NS(signature=Signature({"a": In(1), "b": In(8)}),
+                         a=Signal(),
+                         b=Signal(8)))
+
+    def test_connect_multi_some_in_pairs(self):
+        # Connecting matching inputs is an allowed no-op if there are also
+        # actual input-output connections to be made. See
+        # https://github.com/amaranth-lang/amaranth/pull/1153#issuecomment-1962810678
+        # for more discussion.
+        m = Module()
+        connect(m,
+                p=NS(signature=Signature({"a": In(1), "b": In(1)}),
+                     a=Signal(),
+                     b=Signal()),
+                q=NS(signature=Signature({"a": Out(1), "b": In(1)}),
+                     a=Signal(),
+                     b=Signal()))
+
 
 class ComponentTestCase(unittest.TestCase):
     def test_basic(self):