fix!: Normalize input/output value/static/other ports in OpType

[aborgna-q]

Closes #777.

Now OpTrait clearly separates other_port from static_port, and OpType has a uniform interface for value ports (from the signature), static ports (contstants), and other ports.

Fixes #779. The bug was caused by the encoder ignoring the constant input port offset, so the decoder reconnected the edge to the StateOrder port instead. Now we use the proper OpType methods.

Fixes #778.

[codecov]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (968c8b0) 83.99% compared to head (76ea4e6) 84.02%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
src/hugr/serialize.rs	71.42%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #783      +/-   ##
==========================================
+ Coverage   83.99%   84.02%   +0.03%     
==========================================
  Files          72       72              
  Lines       13736    13782      +46     
  Branches    13736    13782      +46     
==========================================
+ Hits        11537    11581      +44     
- Misses       1383     1389       +6     
+ Partials      816      812       -4     

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

[ss2165]

I personally think static_input and static_output don't belong in OpTrait. They only ever apply to a few specific ops, and therefore only need to be implemented there. Perhaps two new traits StaticInput (implemented by LoadConst and Call) and StaticOutput (implemented by FuncDefn, FuncDecl and Const)?

This will induce some more pattern matching on these operations (or just their tags) in top-level op interfaces and I actually think that adds clarity (but probably more code admittedly). As an alternative, the PR #782 introduces such a trait then uses a blanket implementation at the top level.

[aborgna-q]

Pattern matching based on a tag wouldn't work if the trait is only implemented on some ops, we'd still have to hard-code a pattern match on the operations in ops.rs (that's something I was trying to avoid).

#782 could be an alternative, though since it needs to be implemented everywhere I'm not sure what we gain vs the default OpTrait method?

[ss2165]

we'd still have to hard-code a pattern match on the operations in ops.rs (that's something I was trying to avoid

if that's the only place that's done, is that such a bad thing?
In general I'd like to move towards a pattern where we cast ops to what we expect them to be (as_X, is_X) and then use concrete methods on those that always return (rather than option) as it makes what the code is doing a lot more obvious, rather than chase a trail of trait implementations.

[ss2165]

no dataflow operations have a static output right, so this method doesn't need to exist

[aborgna-q]

The problem is that this kind of operations are also needed to generically list all nodes of a node. Otherwise the static ports are hidden, not in the dataflow_signature nor in the other_port.

Listing the different ports of a node then requires the user to be aware of this, and maybe pattern matching on the specific ops...

Explicitly saying "these are the three varieties of ports you may encounter, here are the accessors" is a lot more straightforward.

[ss2165]

Explicitly saying "these are the three varieties of ports you may encounter, here are the accessors" is a lot more straightforward.

I'm convinced!

[ss2165]

very nice, thanks 👍