feat: Helper functions for requesting inference, use with builder in tests

[acl-cqc]

• Add builder::ft2 that takes 2 Into<Typerow>s, and builds a FunctionType with extension delta TO_BE_INFERRED
• And builder::ft1 that takes a single Into<TypeRow> and makes an endomorphic FunctionType similarly
• Use these to update a bunch of tests made worse by earlier PRs (i.e. these now use inference rather than manually specifying deltas)
• Correct some doc comments....
• ....and add a dfg_builder_endo method that was hinted at by one of the incorrect doc comments, and that infers the delta.

[acl-cqc]

On #1195, @ss2165 wrote:

Thoughts on builder interface:

• Builders that infer extensions should be default, therefore they should have the short names (probably just the set of names we currently use with explicit deltas).
• Explicit delta versions, if necessary, can have some standard naming convention (e.g. dfg_builder_delta)
• Could a potential simplification be just to require FunctionType inputs wherever possible (rather than in_types + out_types) and then make the default extension set in FunctionType TO_BE_INFERRED. Then with_extension_delta on FunctionType would overwrite the default set? Are there uses of FunctionType where having the default extension set be empty is important?

[acl-cqc]

On #1195, @ss2165 wrote:

Thoughts on builder interface:

• Could a potential simplification be just to require FunctionType inputs wherever possible (rather than in_types + out_types) and then make the default extension set in FunctionType TO_BE_INFERRED. Then with_extension_delta on FunctionType would overwrite the default set? Are there uses of FunctionType where having the default extension set be empty is important?

On the latter - yes: every time a FunctionType is used inside a Type::new_function, say, or for a FuncDefn (which we don't infer, but I suppose could - just requires a more complex algorithm that builds a call graph and looks for cycles), or a FuncDecl (which we definitely can't infer). Also not for OpDef signatures...

Moreover whilst I agree that the builder should default to inference I'm less sure about Hugr's built directly without the builder. One thing we could do though would be

• Rename FunctionType::new (i.e. empty-delta) to ::new_pure
• Add FunctionType::new taking 2*TypeRow + ExtensionSet
• Same for new_endo / new_pure_endo
• Add FunctionType::new_inferred (or maybe a shorter name) taking 2*TypeRow (and setting TO_BE_INFERRED)
• And ::new_inferred_endo

However whilst fine for directly-constructed Hugrs (inference is available at reduced cost but still slightly more than being explicit, which feels right) this still leaves the builder wanting.

I also thought about making the builder methods take not a FunctionType but an impl Into<FTSpec> (for some new type FTSpec) where we impl From<FunctionType> for FTSpec (providing all the existing interfaces from before this PR), but then we can also have impl From<(TypeRow, TypeRow)> for FTSpec (with extensions TO_BE_INFERRED) and impl From<TypeRow> for FTSpec (endomorphic TO_BE_INFERRED). However the problem with this is that we rely heavily (e.g. in FunctionType::new) on impl Into<TypeRow> with all sorts of handy conversions there (e.g. from a single Type or Vec<Type>) which we'd not be able to use. (I think the Rust foreign-trait impl rule prevents us from defining transitive conversions - we can't have impl <T: Into<TypeRow>> From<T> for FTSpec as we might like.)

Hence I think specific builder functions are the right way to go...

[acl-cqc]

@ss2165 sending this your way because you looked at the original (#1195) from which this was split. Feel free to reassign....

[codecov]

Codecov Report

Attention: Patch coverage is 78.37838% with 8 lines in your changes missing coverage. Please review.

Project coverage is 86.98%. Comparing base (c05edd3) to head (d4d1c4e).

Files	Patch %	Lines
hugr-core/src/builder/dataflow.rs	0.00%	0 Missing and 3 partials ⚠️
hugr-core/src/hugr/rewrite/inline_dfg.rs	0.00%	0 Missing and 3 partials ⚠️
hugr-core/src/hugr/validate/test.rs	75.00%	0 Missing and 1 partial ⚠️
hugr-core/src/ops/constant.rs	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1219      +/-   ##
==========================================
- Coverage   86.99%   86.98%   -0.02%     
==========================================
  Files         100      100              
  Lines       18975    18948      -27     
  Branches    16992    16965      -27     
==========================================
- Hits        16508    16481      -27     
  Misses       1689     1689              
  Partials      778      778              

Flag	Coverage Δ
rust	86.46% <78.37%> (-0.03%)	⬇️

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.

[acl-cqc]

Hmmm, I'm starting to think that having a new type BuilderFunctionTypeSpec (call it FTSpec or ContainerSpec or something maybe) with factory methods new_endo(impl Into<TypeRow>), new(impl Into<TypeRow>, impl Into<TypeRow>), new_fixed_exts(impl Into<TypeRow>, impl Into<TypeRow>, impl Into<ExtensionSet>), maybe alsonew_endo_fixed_exts might be the best way to go. (new_fixed_exts could also take a FunctionType but taking the three params will require less typing at use sites).

Roughly, we're gonna want to change the builder interface for DFGs, CFGs, and simple-basic-blocks, also non-simple basic blocks + Conditionals + TailLoops (these have an extra row of common inputs), that's gonna be a lot of new methods if we add _endo and _ft variants of each one...

[acl-cqc]

Ok, redone with a couple of helper methods. I admit/agree that ft1 and ft2 are extremely terse names...but that is part of the point.... ;-). (foo,) where foo is a row/type/etc. would be even shorter, but I've not gone there...(I could definitely be persuaded!)

[ss2165]

🎉

[ss2165]

nice