feat!: No nested FuncDefns (or AliasDefns)

[acl-cqc]

....instead require them all at the top level. This is intended as a preliminary to linking.

Thus:

• no flattening (and less name-mangling) in monomorphization
• less validation (reverting #1061 as such an arrangement is now impossible).
• Added HugrBuilder::module_root_builder, which you can use to add FuncDefns to the module (outside the main XXXBuilder<Hugr>); the downside is you have to hang on to the module-root-builder while you use builders that borrow it. This could be made easier by adding instead define_function itself (and add_alias_defn) to HugrBuilder - doesn't really seem the principled thing but would be slightly more ergonomic; thoughts?
  • And similarly in hugr-py, tho there's no you-have-to-hang-onto-the-module_root_builder there :).
• In hugr-llvm:
  • SimpleTestConfig produces Hugr's where entrypoint is now the main function, and has lost some (currently-disabled) debug printouts, see comment.
  • I've dropped diverse_dfg_children test as Consts within DFGs are well-covered by other tests.
  • FatExt::new_root is now the root (and always a Module), new_entrypoint gets the entrypoint

Follows #2255 because that seemed the best way to sort out problems in the dataflow analysis tests.

We'll also need to modify insert_hugr and friends to (configurably) transfer FuncDefns, etc., at the Module level, from inserted into insertee, including SimpleReplacement, as before this PR, such FuncDefns could be included as children/descendants of the entrypoint (e.g. a DFG). However we don't have any cases like that yet in the testsuite, so that can follow in another PR.

BREAKING CHANGE: FuncDefns must be moved to beneath Module. Container::define_function is gone, use HugrBuilder::module_root_builder; similarly in hugr-py DefinitionBuilder (define_function -> module_root_builder().define_function). In hugr-llvm, some uses of FatExt::new_root should become new_entrypoint.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 92.22798% with 15 lines in your changes missing coverage. Please review.

Project coverage is 82.28%. Comparing base (f8a7490) to head (108dbad). Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
hugr-core/src/hugr/validate/test.rs	63.15%	1 Missing and 6 partials :warning:
hugr-core/src/hugr/patch/inline_call.rs	69.23%	1 Missing and 3 partials :warning:
hugr-core/src/builder/module.rs	81.25%	1 Missing and 2 partials :warning:
hugr-llvm/src/utils/fat.rs	75.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2256      +/-   ##
==========================================
+ Coverage   82.18%   82.28%   +0.10%     
==========================================
  Files         239      240       +1     
  Lines       43017    43385     +368     
  Branches    38928    39295     +367     
==========================================
+ Hits        35353    35699     +346     
- Misses       5673     5715      +42     
+ Partials     1991     1971      -20     

Flag	Coverage Δ
python	85.40% <100.00%> (+<0.01%)	:arrow_up:
rust	81.95% <91.62%> (+0.11%)	:arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[acl-cqc]

@ss2165 yes, indeed we can make this non-breaking in the technical API sense (code, using the hugr family of crates, that compiles before, will still compile). Of course we can't guarantee it will still work....although at least Hugrs should still be valid... so yeah, definition of "breaking change" ??

(I think this is still breaking for hugr-py, may be able to address that too, not sure yet.)

[ss2165]

yes, indeed we can make this non-breaking in the technical API sense

Unless we can upgrade serialised HUGRs with functions inside DFG regions by pulling them out, it has to be breaking I think

[acl-cqc]

yes, indeed we can make this non-breaking in the technical API sense

Unless we can upgrade serialised HUGRs with functions inside DFG regions by pulling them out, it has to be breaking I think

AFAICS - I'm not very familiar with Envelope etc. - but we totally can do this. Whether it's worth the effort to make the PR non-breaking is a different question. My take is that in read_envelope just before returning the Hugr we would just lift all FuncDefns to the top. (If there's already a place set up for doing backwards-compat then that would be better - I thought there was for entrypoints but I fail to find it right now. However the key requirement is just that no validation is done before we transform.)

• This never makes the Hugr invalid: the rules for static edges always allow the source to be lifted upwards without affecting anything. (And, we have never required uniqueness of function name, so no problem there either).
• I don't see any mention of Hugr version within the Envelope (?) but we could do that to all Hugrs: it's the identity transformation for any Hugr made after this PR.
• We'd want some serialized Hugr with nested FuncDefns to be a test....

My concern is that we're still changing behaviour; code that worked before, would still compile but might fail to work afterwards.

[ss2165]

code that worked before, would still compile but might fail to work afterwards.

Agreed, this is a breaking change.

[hugrbot]

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary

--- failure enum_no_repr_variant_discriminant_changed: enum variant had its discriminant change value ---

Description:
The enum's variant had its discriminant value change. This breaks downstream code that used its value via a numeric cast like `as isize`.
      ref: https://doc.rust-lang.org/reference/items/enumerations.html#assigning-discriminant-values
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/enum_no_repr_variant_discriminant_changed.ron

Failed in:
variant InterGraphEdgeError::NonCFGAncestor 2 -> 1 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/validate.rs:749
variant InterGraphEdgeError::MissingOrderEdge 3 -> 2 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/validate.rs:760
variant InterGraphEdgeError::NoRelation 4 -> 3 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/validate.rs:771
variant InterGraphEdgeError::NonDominatedAncestor 5 -> 4 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/validate.rs:781

--- failure enum_variant_missing: pub enum variant removed or renamed ---

Description:
A publicly-visible enum has at least one variant that is no longer available under its prior name. It may have been renamed or removed entirely.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/enum_variant_missing.ron

Failed in:
variant InterGraphEdgeError::ValueEdgeIntoFunc, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/validate.rs:765

--- failure trait_method_missing: pub trait method removed or renamed ---

Description:
A trait method is no longer callable, and may have been renamed or removed entirely.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#major-any-change-to-trait-item-signatures
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_method_missing.ron

Failed in:
method define_function of trait Container, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/builder/build_traits.rs:92

--- failure trait_method_requires_different_generic_type_params: trait method now requires a different number of generic type parameters ---

Description:
A trait method now requires a different number of generic type parameters than it used to. Calls or implementations of this trait method using the previous number of generic types will be broken.
      ref: https://doc.rust-lang.org/reference/items/generics.html
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_method_requires_different_generic_type_params.ron

Failed in:
FatExt::fat_root (1 -> 0 generic types) in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-llvm/src/utils/fat.rs:376

[acl-cqc]

@ss2165 thanks for comments. I've added module_root_builder to hugr-py too and done most other things as suggested. (The deprecated version of define_function has been removed after all - I see only one use in tket2, in #[test] fn qsystem_pass, that looks straightforward to update to use module_root_builder.) btw inhugr-py I note that Cfg does not extend DefinitionBuilder and probably should, but haven't done that here....

Think it'd be good to get some feedback from @doug-q about the hugr-llvm SimpleTestConfig change, but otherwise I think good for another review.