fix: allow add_input/add_output on all DFGs

[qartik]

Closes: #1819

[codecov]

Codecov Report

Attention: Patch coverage is 95.55556% with 2 lines in your changes missing coverage. Please review.

Project coverage is 86.55%. Comparing base (74ce446) to head (577971c).

Files with missing lines	Patch %	Lines
hugr-core/src/builder/dataflow.rs	95.55%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1824   +/-   ##
=======================================
  Coverage   86.55%   86.55%           
=======================================
  Files         194      194           
  Lines       35156    35158    +2     
  Branches    31969    31971    +2     
=======================================
+ Hits        30430    30432    +2     
  Misses       2968     2968           
  Partials     1758     1758           

Flag	Coverage Δ
python	92.31% <ø> (ø)
rust	85.98% <95.55%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[qartik]

update_fn_signature still needs to be updated or it will panic for anything other than FuncDefn.

[acl-cqc]

Yeah, so you've moved add_input and add_output from FunctionBuilder<Hugr> into DFGBuilder<Hugr> and a proxy for that in FunctionBuilder<Hugr>. Instead you could define

trait MutableIO : Dataflow {
  fn update_signature(...); // no default impl
  fn add_input(...) { .... } // code you've moved becomes default impl delegating to self.update_signature
  fn add_output(...) { ....} // similarly
}

then you impl MutableIO for FunctionBuilder<Hugr> and impl MutableIO for DFGBuilder<Hugr> and just have to provide the two update_signature methods, i.e. then you don't need the delegate! { to self.0 { fn add_input(...); fn add_output(...); } }. Does that work??

[doug-q]

I do not like the trait MutableIO idea, because I think update_signature is too powerful to be exposed like that. It's one thing to add an input or output to a FuncDefn or DFG, and quite another to arbitrarily change the signature. It can remove ports and change the types of ports.

I have posted a PR with my attempt (#1866). which exposes add_input and add_output on both FunctionBuilder and DFGBuilder, with code shared via add_input_impl and add_output_impl on DFGBuilder.

I am not convinced that we should do this, as mutating the signature of a DFG does adding ports to the parent DFG, which is not required in the FuncDefn case.

@aborgna-q what do you think?

[aborgna-q]

I agree that MutableIO is too much of a footgun. It wouldn't hurt to keep it with just the limited add_input / add_output methods, and have the rest of the implementation private to each type similar to @doug-q's patch.

I made a couple comments in #1866 too.

[doug-q]

It wouldn't hurt to keep it

Does it make sense for TailLoop,Case, orDataflowBlock? I would argue not because of the complexity, and the random other ports you'd have to deal with.

For TailLoop: adding an input or output would always have to add an output or input as well! The user would have to wire these up.
For Case: adding and input or output would also require mutating the parent conditional and all siblings

For DataflowBlock: Adding an input or output would mean mutating all transitively connected DataFlowBlocks, and maybe the parent CFG

I would argue that a trait doesn't help us here, because in each case we should return the new ports that need to be connected. In fact I should add this to DFGBuilder::add_input