From b64397f6ef1dd9f585c118868c9cc3661b682714 Mon Sep 17 00:00:00 2001 From: Amelia Cuss Date: Fri, 23 Feb 2024 16:26:46 +1100 Subject: [PATCH] lib.wiring.connect: diagnostic when no connection made. If a connect() call results in no connections being made, and it's because there were no outputs specified at all, issue an error. Tests enumerate cases per https://github.com/amaranth-lang/amaranth/pull/1153#issuecomment-1962810678. Co-authored-by: Catherine --- amaranth/lib/wiring.py | 19 ++++++++++++- tests/test_lib_wiring.py | 59 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/amaranth/lib/wiring.py b/amaranth/lib/wiring.py index 6b82cb420..4d891392f 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_dimensions(dimensions, *, out_path, in_path): 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. diff --git a/tests/test_lib_wiring.py b/tests/test_lib_wiring.py index 35570ab13..4bdfabec2 100644 --- a/tests/test_lib_wiring.py +++ b/tests/test_lib_wiring.py @@ -980,6 +980,65 @@ def test_dimension_multi(self): '(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):