feat!: Add Visibility to FuncDefn/FuncDecl.

[acl-cqc]

Note: modified to come after #2412 as I want those Schema checks in place to check this works!
closes #2354, #1752.
Includes cherry-pick of #2343.

• Add enum Visibility. In fact *2 (hugr-model version is not serde-able so cannot be used in hugr-core).

• FuncDefn and FuncDecl gain this. (A good use-case for a private FuncDecl is building cyclic functions via builder define_declaration.)

  • I've not added to AliasDecl or AliasDefn - I could, it would be consistent, but we hardly use them....

• validate checks that the public children of a Module have unique names (except multiple FuncDecls may alias).

• FuncDefn::new defaults to private (also builder define_function / FunctionBuilder::new, and json-deserialize in both python and rust). There is a _vis taking an explicit Visibility parameter.

• FuncDecl::new defaults to public (also builder declare_function, and json-deserialize*2). Again there is _vis.

• hugr-model import/export + roundtrip + AST printing via new keyword pub. Note since hugr-core does not have visibility on aliases or extension operations I've defaulted these to private to avoid any change to textual output.

• add field to hugr-py, using "string"-enum (Literal["Public", "Private"])

• I've attempted to add hugr-model hugr-py bindings, but there are no tests of these yet.

Note all changes to hugr-passes delayed until a follow-up PR.

BREAKING CHANGE: hugr-model: Symbol has an extra field

[hugrbot]

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary

--- failure constructible_struct_adds_field: externally-constructible struct adds field ---

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
      ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.42.0/src/lints/constructible_struct_adds_field.ron

Failed in:
field Symbol.visibility in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-model/src/v0/ast/mod.rs:198
field Symbol.visibility in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-model/src/v0/table/mod.rs:307

[codecov]

Codecov Report

Attention: Patch coverage is 76.92308% with 66 lines in your changes missing coverage. Please review.

Project coverage is 82.01%. Comparing base (68415f4) to head (5a81bc0).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
hugr-model/src/v0/ast/python.rs	0.00%	17 Missing ⚠️
hugr-core/src/builder/module.rs	64.28%	13 Missing and 2 partials ⚠️
hugr-model/src/capnp/hugr_v0_capnp.rs	52.38%	10 Missing ⚠️
hugr-model/src/v0/binary/read.rs	70.00%	1 Missing and 8 partials ⚠️
hugr-core/src/builder/dataflow.rs	36.36%	7 Missing ⚠️
hugr-core/src/ops/module.rs	81.25%	6 Missing ⚠️
hugr-py/src/hugr/model/export.py	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2143      +/-   ##
==========================================
- Coverage   82.06%   82.01%   -0.05%     
==========================================
  Files         245      245              
  Lines       45289    45474     +185     
  Branches    41033    41210     +177     
==========================================
+ Hits        37166    37297     +131     
- Misses       6108     6172      +64     
+ Partials     2015     2005      -10     

Flag	Coverage Δ
python	85.38% <84.61%> (+0.02%)	⬆️
rust	81.66% <76.55%> (-0.06%)	⬇️

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.

🚀 New features to boost your workflow:

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

[doug-q]

I've not reviewed the code, but the description is so good I can give initial feedback from that"

• Add public: bool to FuncDefn (closes Decide on which functions are "public" #1752). For FuncDefns that are not direct children of a Module, this is likely meaningless, but I've not imposed any rule.

So when a FuncDefn with public == true is "lifted" to a module the public flag "turns on"? This seems a bit dangerous. Lifting functions to modules is very normal and useful. I don't see a better path though. This is a case for emitting a warning (non-module-child has link name)

• An alternative would be to have a separate field link_name: Option<String>. Preferences? Factory/builder methods might default to using the same string for both kinds of name. However, name then has no use in the code, so might be better replaced by name/description metadata....?

This is my preference. I expect the "String" to grow in complexity over time, linking is complicated. One might want to link two hugrs together, i.e. one uses a "public" function of the other, but that function doesn't want to be actually exposed as a public symbol after conversion to llvm. There is weak linking, ODR. I expect we will want more than zero of these features.

Yes name can be moved to metadata. Or it can stay where it is because it's Very Important Metadata.

Nice to have: it would be great if we were able to be unconcerned with naming generated functions (e.g. monomorphise)

• Or even just make name an Option - all (Module-child) functions are exported unless they are anonymous?

I think there is benefit in maintaining the distinction between "link name" and "user-written-name".

• validate checks that the public children of a Module have unique names (also FuncDecls - i.e. don't allow exporting a FuncDefn and importing the same name)

• Duplicate "name"s should be allowed.
• The link names of module child FuncDefns should be unique, and distinct from link names of module child FuncDecls
• multiple FuncDecls with the same signature should be able to share link names (the two funcdecls are aliased)
• It should be valid for FuncDecls to not have link names. Obviously you can't codegen this yet, but I expect such HUGRs to be useful.

• TODO: this should also incorporate AliasDefn/AliasDecl
  We don't use these, let's delete them

• It might/probably make(s) sense to use these in all dataflow analyses that care about entry points...e.g. that would include constant folding (i.e. default to joining inputs of public functions with Top unless explicitly disabled)

Yes, once we have a definition of public dataflow analysis should use this

• Various faffing around with factory methods and builder define_function methods

Unclear whether this would be part of 0.16.0 release but I suspect "too late for that deadline".
.

• TODO Also for now I have bodged the hugr-model import.rs (@zrho)
• TODO: Update hugr-py

BREAKING CHANGE: FuncDefn has an extra field, recommend using new, new_public or new_private

Perhaps we should mark FuncDefn (and everything else?) #[non_exhaustive] ?

Do you think we should change "FuncDecls must be children of module" here?

I do not think we should attempt to link Consts. One can just write a function that returns the const. Yes we need const de-duplication, but we need it without linking too.

[acl-cqc]

Thanks @doug-q, great points! :)

So when a FuncDefn with public == true is "lifted" to a module the public flag "turns on"? This seems a bit dangerous. Lifting functions to modules is very normal and useful.

Yes, good point. I've made it an error to have an exported FuncDefn that isn't at the top-level.

• An alternative would be to have a separate field link_name: Option<String>. Preferences? Factory/builder methods might default to using the same string for both kinds of name. However, name then has no use in the code, so might be better replaced by name/description metadata....?

This is my preference. I expect the "String" to grow in complexity over time, linking is complicated. One might want to link two hugrs together, i.e. one uses a "public" function of the other, but that function doesn't want to be actually exposed as a public symbol after conversion to llvm. There is weak linking, ODR. I expect we will want more than zero of these features.

Yes name can be moved to metadata. Or it can stay where it is because it's Very Important Metadata.

Yup ok, I've kept name but not enforced uniqueness, and added optional link_name, with uniqueness requirement

Nice to have: it would be great if we were able to be unconcerned with naming generated functions (e.g. monomorphise)

Right. So monomorphise now mangles link_name only, and leaves name unchanged. (Tests need updating here.)

• multiple FuncDecls with the same signature should be able to share link names (the two funcdecls are aliased)

yup, good point, done

• It should be valid for FuncDecls to not have link names. Obviously you can't codegen this yet, but I expect such HUGRs to be useful.

This is the one I haven't really seen the use case for. Seems to me that whereas a FuncDefn has both name and optionally link-name, a FuncDecl has only (but always) link-name. What else does a FuncDecl do except open the way for linking?

Perhaps we should mark FuncDefn (and everything else?) #[non_exhaustive] ?

Yup, done

Do you think we should change "FuncDecls must be children of module" here?

Now that every Hugr has a module root, no, I think we can insist every FuncDecl lives there.

I do not think we should attempt to link Consts. One can just write a function that returns the const. Yes we need const de-duplication, but we need it without linking too.

Removed.

[zrho]

I think it is important that functions, aliases, etc. can be referred to by a name. This is not immediately clear when just concentrating on writing the in-memory data structure of the compiler, but becomes much more apparent when considering other aspects, including but not limited to linking. These other aspects have been neglected so far, which I think can be felt very acutely. Names are in particular essential for the text format. That hugr-core has this design choice in contrast to every other IR known to me I think is caused by it being designed with only the in-memory representation in mind.

How about this: the current name becomes name_hint (or title or something like that) and is "Very Important Metadata". The proposed link_name becomes name. Both are optional. There are no rules for name_hint, but when a name is given then it must follow uniqueness/scoping rules. When exporting from core to model everything gets a name; if it didn't have one already, one is created from node id and name_hint. Stuff that is marked as public for linking must have a name in core. With that we can still avoid having to deal with the apparently intractable problem of alpha renaming in core, which appears to be the motivation behind the non-functional names so far, and at the same time acknlowedge that names are useful for more than just linking.

[acl-cqc]

Thanks, both. I like Lukas' suggestion (rename "Metadata" name to title or even description; and link_name to name). That name must be provided when the thing is public is what I'm aiming for here ;) but @lukas do you mean you want to allow a (ex-link_)name even when not public?

Also it seems odd for metadata that is used only by humans to be compulsory. I guess (if there is no uniqueness requirement) one can provide an empty string (which, incidentally, would be another way to declare that a function has no link-name; empty string is, after all, not a legal name).

Finally @doug-q the idea that monomorphization leaves human-name unchanged but mangles link-name is kinda annoying in that monomorphization still acts on functions that have no link-name, but then the human-name does not give any way to tell them apart. I guess one then has to inspect the type of the function, which is much more cumbersome.

[zrho]

Yes, there's an argument to allow for names on non-public things. This keeps names as the general mechanism to refer to symbols such as functions, but allows to internally go for a nameless representation when convenient, e.g. while rewriting. The philosophy then would be that everything has a name, but for some things we don't care what it is.

The human readable title should be optional. If hugr-core had a nicer interface for metadata already, it probably wouldn't even need to be blessed metadata.

[doug-q]

Yes, there's an argument to allow for names on non-public things. This keeps names as the general mechanism to refer to symbols such as functions, but allows to internally go for a nameless representation when convenient, e.g. while rewriting. The philosophy then would be that everything has a name, but for some things we don't care what it is.

Indeed, maybe we could just assign an int as a name? We could call it node id.

Renaming name and adding another field called name is a recipe for disaster. Renaming the field is unnecessary breakage, changing it's semantics as suggested above is sabotage.

I think we are agreed that we should add an optional field, say link_name to FuncDefn and FuncDecl. We are not agreed on how that field should be named or whether we should rename the existing name field.

The detail of which Func{Defn,Decl}s are allowed and/or required to have link_names is not resolved.

My suggestion is that any Func{Defn,Decl} is allowed a link_name, and that the link_name of children of Modules, if it is present, must be unique in that Module. This avoids any scoping issues.

What monomorphisation does is not material here. we can mangle name if we want.

[aborgna-q]

Use a default (public?) fall-back instead of requiring the new visibility field, so the format remains backwards-compatible.

[acl-cqc]

I view the defaults used when deserializing as a legacy backwards-compatibility mechanism. AFAICS serde correctly applies the defaults and there is no problem loading an "old" Hugr w/out visibility - at least outside the envelope mechanism, I'm working on a test using production/envelope-loading code.

Do you think that marking old hugrs as "not schema compliant" prevents loading them? I mean, do we require old Hugrs to be schema-compliant (not just loadable) in order to say backwards compatibility is maintained? (Do we really need the schema to encompass all supported legacy formats?) Your answer suggests "yes" to at least the last two questions here, is that really what you mean?

[aborgna-q]

The fields must be marked as default-if-not-present to be correctly loaded, no?

• For pydantic, add visibility = Field(default=...) on the definition
• For serde, add #[serde(default)] on the field
• You added a fallback on hugr-model already, so that should work fine

[acl-cqc]

Added a test using Hugr::load of a FuncDecl and a FuncDefn without any visibility. Defaults correctly applied....?

[aborgna-q]

Ugh, it looks like that's undocumented serde behaviour.
It doesn't hurt to be explicit though...

I tested a toy example for pydantic, and I'm pretty sure that we require the default there.

•   ValidationError: 1 validation error for Test
    b
      Field required [type=missing, input_value={'a': 1}, input_type=dict]
        For further information visit https://errors.pydantic.dev/2.11/v/missing

• https://docs.pydantic.dev/latest/errors/validation_errors/#missing

[acl-cqc]

Right - I was just thinking, what's the next thing to try - do the same thing in python.

Clearly this is more subtle than I expected and as such I think having some tests of loading legacy Hugrs really is warranted after all. Let's get your "toy example" in as well and then I can resolve (somehow)....

[acl-cqc]

Ok, so python test added using the same file in resources, skipped if it can't read it (e.g. sdist).
Defaults added to pydantic, sadly duplicating those for serde.

I think you can see all the changes here: https://github.com/CQCL/hugr/compare/2cbe99f44d123b3788fb0801a630bca5e0d9c735..acl/linking_prelims

That's on top of the other changes since approval - removing the special-case for FuncDefn called "main" (now just defaults to private), and all changes to hugr-passes (pending followup PR) - after discussion with @ss2165 .

[acl-cqc]

Also I tried adding a default "unspecified" to the hugr-model capnp, so Symbol can have Option. This is tidier in many ways (albeit more verbose), see here: https://github.com/CQCL/hugr/compare/acl/linking_prelims..acl/vis_unspec I could include that here (probably switching the model text format to "public" and "private" rather than "pub" and "priv") if you think it's worth it?