feat: Emulate `TypeBound`s on parameters via constraints. #1624

zrho · 2024-10-31T17:27:15Z

This PR translates some TypeBounds in hugr-core to the nonlinear constraint in hugr-model. This translation only occurs on parameters that take a runtime type directly.

As a driveby change before the model stabilises, this PR also moves constraints out of the parameter lists into their own list. In its previous form this could have led to confusions about which parameter a local variable index refers to when a constraint is situated between two parameters in the list. We also remove constraints from aliases for now.

Closes #1637.

codecov · 2024-10-31T17:32:50Z

Codecov Report

Attention: Patch coverage is 78.73563% with 37 lines in your changes missing coverage. Please review.

Project coverage is 85.64%. Comparing base (935c61b) to head (a05ae29).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
hugr-core/src/import.rs	61.53%	9 Missing and 6 partials ⚠️
hugr-model/src/v0/text/print.rs	71.79%	0 Missing and 11 partials ⚠️
hugr-model/src/v0/text/parse.rs	80.00%	0 Missing and 5 partials ⚠️
hugr-core/src/export.rs	90.47%	4 Missing ⚠️
hugr-model/src/v0/binary/read.rs	88.23%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1624      +/-   ##
==========================================
+ Coverage   85.50%   85.64%   +0.13%     
==========================================
  Files         136      136              
  Lines       25252    25373     +121     
  Branches    22164    22285     +121     
==========================================
+ Hits        21592    21730     +138     
+ Misses       2456     2430      -26     
- Partials     1204     1213       +9

Flag	Coverage Δ
python	`92.42% <ø> (ø)`
rust	`84.70% <78.73%> (+0.16%)`	⬆️

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.

hugrbot · 2024-11-13T10:45:42Z

This PR contains breaking changes to the public Rust API.
Please deprecate the old API instead (if possible), or mark the PR with a ! to indicate a breaking change.

cargo-semver-checks summary


--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
      ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/auto_trait_impl_removed.ron

Failed in:
type TopoConvexChecker is no longer Sync, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/views/sibling_subgraph.rs:490
type TopoConvexChecker is no longer RefUnwindSafe, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/hugr/views/sibling_subgraph.rs:490

--- 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.36.0/src/lints/constructible_struct_adds_field.ron

Failed in:
field ConstructorDecl.constraints in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-model/src/v0/mod.rs:425
field OperationDecl.constraints in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-model/src/v0/mod.rs:438
field FuncDecl.constraints in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-model/src/v0/mod.rs:401

--- failure enum_missing: pub enum removed or renamed ---

Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself 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.36.0/src/lints/enum_missing.ron

Failed in:
enum hugr_model::v0::Param, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-model/src/v0/mod.rs:672

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/enum_variant_added.ron

Failed in:
variant Term:NonLinearConstraint in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-model/src/v0/mod.rs:673

ss2165 · 2024-11-14T10:37:55Z

hugr-core/src/import.rs

@@ -938,7 +956,7 @@ impl<'a> Context<'a> {
            model::Term::FuncType { .. } => Err(error_unsupported!("`(fn ...)` as `TypeParam`")),

            model::Term::ListType { item_type } => {
-                let param = Box::new(self.import_type_param(*item_type)?);
+                let param = Box::new(self.import_type_param(*item_type, TypeBound::Any)?);


Not sure why this is any (also not covered by tests), is this a todo waiting for a list constraint?

The item type is nested within the list and since we only constrain parameters directly here, we can not express that the items of the list should be copyable. Therefore the TypeBound::Any.

ss2165 · 2024-11-14T10:55:34Z

hugr-model/tests/fixtures/model-constraints.edn

+  (where (copy ?t))
+  (where (discard ?t))


this looks like it could get quite verbose. Will we need to specify things that are copy but not discard? Even if we do, I feel that ought to be the more verbose specification. Ideally I guess I want to write:

Nothing for things that are copyable and discardable

"linear" or "not copy" for things that are such

"affine" when I can't copy but can discard.

Is this just a handy syntax change or are we running in to subtyping problems again?

Yeah, I feel that the "sales blurb" in the description that "The translation in this PR requires the two to be used together since TypeBound does not have the facility to express one without the other. Once we move to constraints in hugr-core, that restriction would be lifted." misses the point. We could have had separate copy/discard/affine/etc. in TypeBound, but we discussed this and did not decide that we wanted it. So I see no reason to do so now.

Indeed, if we change our minds are decide we want to add this in the future, there is still an easy mechanism (right?) - we just add two new constraints (say copy and discard) alongside the old copyable and then define that copyable holds if both copy and discard hold (and that each of the latter/two hold if copyable does), right? (so ok we need a cyclicity check in the constraint solver.) The problem would just be better naming, we can't really have both copy and copyable....

So (the more I think about it, the more) I think we should stick with one combined constraint rather than two for now.

Yeah, if we can find a decent name for the combined constraint, I'd be happy to go for that. Any ideas?

nonlinear ..?

I'm fine with nonlinear. @acl-cqc ?

The two constraints are joined into a nonlinear constraint now.

acl-cqc

Generally I would say this looks pretty good, but I vote for combining copy+discard, also a couple of other ideas (/confusions)...

acl-cqc · 2024-11-14T12:25:16Z

hugr-core/src/export.rs

@@ -46,6 +46,8 @@ struct Context<'a> {
    term_map: FxHashMap<model::Term<'a>, model::TermId>,
    /// The current scope for local variables.
    local_scope: Option<model::NodeId>,
+    /// Constraints to be added to the local scope.


Is local scope == [Self::local_scope]? Do we actually plan to modify the local scope by adding these? Or are these "Constraints to be added to those from the local scope" (i.e., extra constraints from somewhere else)? "Constraints in addition to those from the local scope" would be clearer still that we don't intend to mutate some list.

When exporting a PolyFuncType, we gather constraints in local_constraints. PolyFuncType itself can not express constraints, but if one of the parameters takes a copyable type, local_constraints is where we record the corresponding constraint for that parameter.

acl-cqc · 2024-11-14T12:28:31Z

hugr-model/src/v0/mod.rs

@@ -397,6 +397,8 @@ pub struct FuncDecl<'a> {
    pub name: &'a str,
    /// The static parameters of the function.
    pub params: &'a [Param<'a>],
+    /// The constraints on the static parameters.
+    pub constraints: &'a [TermId],


Good point (in the PR description) about separating these out! :)

acl-cqc · 2024-11-14T12:29:55Z

hugr-model/src/v0/mod.rs

@@ -408,6 +410,8 @@ pub struct AliasDecl<'a> {
    pub name: &'a str,
    /// The static parameters of the alias.
    pub params: &'a [Param<'a>],
+    /// The constraints on the static parameters.
+    pub constraints: &'a [TermId],


So the alias also introduces more constraints. I'd say that might make it slightly more than an alias; I can't introduce constraints just anywhere (without an alias), can I?

And when we have Terms in hugr-core, Alias's are likely to disappear after import (as we can get sharing via TermId, right?) - are we sure we want/need this?

Or maybe the idea is that the constraints here are copied(+substituted) from/according-to the RHS of the alias (they are the constraints that must apply anyway, rather than the Alias being able to specify new ones to introduce). If so, let's be clear in the comment....

Yes, good catch. I can imagine some niche uses, but for now it's probably best to remove constraints on aliases. If we find a good use case and mental model for this, we can add them again.

Constraints on aliases have been removed.

acl-cqc · 2024-11-14T12:31:20Z

hugr-model/src/v0/mod.rs

 }

 /// A parameter to a function or alias.
 ///
 /// Parameter names must be unique within a parameter list.
 /// Implicit and explicit parameters share a namespace.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
-pub enum Param<'a> {
-    /// An implicit parameter that should be inferred, unless a full application form is used
+pub struct Param<'a> {


+1 to this commoning up, amen.

I am a long way from convinced that we need implicit/explicit, but that's another issue and definitely not for this PR!

(nice that Constraints are now separate from Parameters here too! 👍 )

acl-cqc · 2024-11-14T12:34:39Z

hugr-model/src/v0/mod.rs

@@ -662,40 +670,42 @@ pub enum Term<'a> {
    ///
    /// `ctrl : static`
    ControlType,
+
+    /// Constraint that requires a runtime type to be copyable.
+    CopyConstraint {


I would not object to a general Term::Constraint { name: str, args: Vec<TermId> } with the name looked up in data that is static for now (i.e. just the one/two constraints and none user-defined). It does kinda say that there will be more constraints coming, but with the data being static there is no possibility of adding them.

There is a possibility and moreover a mechanism for adding them: Your Term::Constraint { name: str, args: Vec<TermId> } is Term::Apply/Term::ApplyFull for a constructor that declares a constraint.

There are good arguments for keeping Term as minimal as possible by defining anything that does not need special handling as a custom constructor in a standard extension. In particular it'd remove a bunch of boilerplate code. There's also arguments for having anything that is structurally important as builtin; for example adts since the control flow mechanism depends on it, or copyability constraints since they emerge just from the connectivity pattern of ports. I could go either way.

acl-cqc · 2024-11-14T12:39:43Z

hugr-model/src/v0/text/print.rs

-                for param in decl.params {
-                    this.print_param(*param)?;
-                }
+                this.print_params(decl.params)?;


If we are sure we want constraints in Alias too (I argue against elsewhere), then it is true that everywhere you introduce params you also introduce constraints; maybe a struct ParamsAndConstraints<'a> (maybe ConstrainedParams?) would be useful, as it'd allow combining all these print_params(...); print_constraints(...)

I agree that we'd probably want to avoid constraints on aliases for now.

acl-cqc · 2024-11-14T12:43:25Z

hugr-model/src/v0/text/print.rs

@@ -460,25 +446,37 @@ impl<'p, 'a: 'p> PrintContext<'p, 'a> {
        }
    }

+    fn print_params(&mut self, params: &'a [Param<'a>]) -> PrintResult<()> {
+        for param in params {


nit: can you just params.iter().try_for_each(self.print_param) (maybe .copied() or something) ?

acl-cqc · 2024-11-14T12:51:30Z

hugr-model/tests/fixtures/model-constraints.edn

+  (where (copy ?t))
+  (where (discard ?t))


Yeah, I feel that the "sales blurb" in the description that "The translation in this PR requires the two to be used together since TypeBound does not have the facility to express one without the other. Once we move to constraints in hugr-core, that restriction would be lifted." misses the point. We could have had separate copy/discard/affine/etc. in TypeBound, but we discussed this and did not decide that we wanted it. So I see no reason to do so now.

Indeed, if we change our minds are decide we want to add this in the future, there is still an easy mechanism (right?) - we just add two new constraints (say copy and discard) alongside the old copyable and then define that copyable holds if both copy and discard hold (and that each of the latter/two hold if copyable does), right? (so ok we need a cyclicity check in the constraint solver.) The problem would just be better naming, we can't really have both copy and copyable....

So (the more I think about it, the more) I think we should stick with one combined constraint rather than two for now.

acl-cqc · 2024-11-14T12:53:13Z

hugr-model/tests/fixtures/model-constraints.edn

+  (forall ?t type)
+  (where (copy ?t))
+  (where (discard ?t))
+  [(@ foo.tuple ?t)] [(@ foo.tuple ?t) (@ foo.tuple ?t)] (ext))


What is foo.tuple ??

It's some arbitrary custom constructor to trigger a particular case in import/export for more test coverage. tuple is for "historical" reasons since the example was different at first... Can rename it into something more nondescript.

…straint.

Emulate TypeBounds on parameters via constraints.

b9e75fc

zrho force-pushed the zrho/model-bounds branch from f9e97a7 to b9e75fc Compare November 13, 2024 10:40

zrho added 3 commits November 13, 2024 13:15

Separate parameters and constraints in declarations.

0293340

Simplified param representation.

1cba000

Write constraint lists properly in capnp.

f22581e

zrho requested review from ss2165 and acl-cqc November 13, 2024 13:50

zrho marked this pull request as ready for review November 13, 2024 13:51

zrho requested a review from a team as a code owner November 13, 2024 13:51

Removed model_unstable from default features.

d377fcc

ss2165 reviewed Nov 14, 2024

View reviewed changes

acl-cqc reviewed Nov 14, 2024

View reviewed changes

zrho added 3 commits November 18, 2024 12:44

Replaced copy and discardable constraints with a single nonlinear con…

7d0f344

…straint.

try_for_each

719af15

No constraints in aliases

a05ae29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Emulate `TypeBound`s on parameters via constraints. #1624

feat: Emulate `TypeBound`s on parameters via constraints. #1624

zrho commented Oct 31, 2024 •

edited

Loading

codecov bot commented Oct 31, 2024 •

edited

Loading

hugrbot commented Nov 13, 2024 •

edited

Loading

ss2165 Nov 14, 2024

zrho Nov 14, 2024

ss2165 Nov 14, 2024

acl-cqc Nov 14, 2024

zrho Nov 14, 2024

ss2165 Nov 14, 2024

zrho Nov 14, 2024

zrho Nov 18, 2024

acl-cqc left a comment

acl-cqc Nov 14, 2024

zrho Nov 14, 2024

acl-cqc Nov 14, 2024

acl-cqc Nov 14, 2024

zrho Nov 14, 2024

zrho Nov 18, 2024

acl-cqc Nov 14, 2024

acl-cqc Nov 14, 2024

acl-cqc Nov 14, 2024

zrho Nov 14, 2024

acl-cqc Nov 14, 2024

zrho Nov 14, 2024

acl-cqc Nov 14, 2024

acl-cqc Nov 14, 2024

acl-cqc Nov 14, 2024

zrho Nov 14, 2024

feat: Emulate TypeBounds on parameters via constraints. #1624

Are you sure you want to change the base?

feat: Emulate TypeBounds on parameters via constraints. #1624

Conversation

zrho commented Oct 31, 2024 • edited Loading

codecov bot commented Oct 31, 2024 • edited Loading

Codecov Report

hugrbot commented Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: Emulate `TypeBound`s on parameters via constraints. #1624

feat: Emulate `TypeBound`s on parameters via constraints. #1624

zrho commented Oct 31, 2024 •

edited

Loading

codecov bot commented Oct 31, 2024 •

edited

Loading

hugrbot commented Nov 13, 2024 •

edited

Loading