lib.wiring.connect: diagnostic when no connection made.

[kivikakk]

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 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:

(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]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.89%. Comparing base (09029cd) to head (b64397f).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1153      +/-   ##
==========================================
+ Coverage   85.88%   85.89%   +0.01%     
==========================================
  Files          43       43              
  Lines        8679     8687       +8     
  Branches     2065     2067       +2     
==========================================
+ Hits         7454     7462       +8     
- Misses       1024     1025       +1     
+ Partials      201      200       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[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:

(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 motivation 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 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.

[whitequark]

LGTM