feat: No polymorphic closures #906

acl-cqc · 2024-04-02T16:24:35Z

Type::Function now stores only a FunctionType, not a PolyFuncType
PolyFuncType remains, for OpDef's and FuncDefn/Decl's
EdgeKind::Static now replaced by EdgeKind::Const (a type) EdgeKind::Static (a PolyFuncType)
Remove LeafOp::TypeApply, repurpose validation code onto Call
Thus, progressively remove all impl Substitutions except for struct SubstValues, which can become Substitution
Update spec, introducing "Static" and "Dataflow" edge kinds as broader classes of the other edge kinds, and polymorphic "type schemes" vs monomorphic "Function types".
Update serialization schema and add roundtrip test of a Noop operating on a value of function type

fixes #904

This should enable resolving #788 and related capture/closure issues if we forbid edges into a FuncDefn from outside (@doug-q)

BREAKING CHANGE: EdgeKind::{Static -> Const}, add new EdgeKind::Function, Type contains only monomorphic functions, remove TypeApply.

Rm nested-binder tests (cannot make such a testcase anymore) Const: generalize error in case value is a polymorphic function impl TryFrom<PolyFuncType> for FunctionType

inline apply_typevar

codecov · 2024-04-02T17:13:10Z

Codecov Report

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

Project coverage is 85.46%. Comparing base (100e2cc) to head (1777329).

Files	Patch %	Lines
hugr/src/hugr/rewrite/replace.rs	0.00%	8 Missing ⚠️
hugr/src/hugr/validate.rs	75.00%	8 Missing ⚠️
hugr/src/ops/constant.rs	53.84%	5 Missing and 1 partial ⚠️
hugr/src/ops/dataflow.rs	72.22%	4 Missing and 1 partial ⚠️
hugr/src/hugr/serialize.rs	62.50%	0 Missing and 3 partials ⚠️
hugr/src/hugr/views/render.rs	78.57%	3 Missing ⚠️
hugr/src/extension.rs	50.00%	1 Missing ⚠️
hugr/src/extension/op_def.rs	0.00%	0 Missing and 1 partial ⚠️
hugr/src/types.rs	95.23%	1 Missing ⚠️
hugr/src/types/poly_func.rs	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #906      +/-   ##
==========================================
- Coverage   85.63%   85.46%   -0.18%     
==========================================
  Files          78       78              
  Lines       14444    14176     -268     
  Branches    14444    14176     -268     
==========================================
- Hits        12369    12115     -254     
+ Misses       1436     1432       -4     
+ Partials      639      629      -10

Flag	Coverage Δ
rust	`85.46% <72.38%> (-0.18%)`	⬇️

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 · 2024-04-03T14:59:10Z

quantinuum-hugr/src/hugr/validate.rs

-                    || OpTag::Function.is_superset(from_optype.tag()))
-                {
-                    return Err(InterGraphEdgeError::InvalidConstSrc {
+                let reqd_optype = match typ {


TBH I think this check is more of an assertion about the spec, than something we really need to check at runtime; there is no other way to construct nodes with such out-ports, and as such I don't think it's even possible to test the error path here. So I'd be happy to remove both these checks and the relevant error variant....?

acl-cqc · 2024-04-03T15:00:56Z

quantinuum-hugr/src/types.rs

-    /// A reference to a static value definition.
-    Static(Type),
+    /// A reference to a static value definition - Const or Function
+    Static(StaticEdgeData),


This is the bit I'm least sure about. The other possibility is to have two distinct EdgeKinds (i.e. one new) - ideally I'd call them Const (rather than Static) and Function, or perhaps StaticValue and StaticFunction. We even had ConstE edges in the spec at one point....

I like Const and Function. I think that would make some of the code clearer, versus this hierarchy of edge types. You do comment that many statements about Static edges in the spec apply equally to both kinds, so not entirely sure, but even so a little repetition in the spec may be a price worth paying.

acl-cqc · 2024-04-03T15:02:33Z

specification/hugr.md

@@ -124,7 +124,9 @@ carry an edge weight:
 - `Value` edges carry typed data at runtime. They have a *port* at each end, associated
  with the source and target nodes. They have an `AnyType`as an edge weight.
 - `Static` edges are similar to `Value` edges but carry static data (knowable at
-  compilation time). They have a `CopyableType` as an edge weight.
+  compilation time). These come in two subkinds with different edge weights:


See also https://github.com/CQCL/hugr/pull/906/files#r1549924162, although there is no requirement to go the same way in the spec as the code. However I found many references to Static edges in the spec that were true for both value and function subkinds!

cqc-alec · 2024-04-04T09:38:45Z

quantinuum-hugr/src/hugr/validate.rs

-                    || OpTag::Function.is_superset(from_optype.tag()))
-                {
-                    return Err(InterGraphEdgeError::InvalidConstSrc {
+                let reqd_optype = match typ {


Yeah I agree if the error path is unreachable we should delete the check.

cqc-alec · 2024-04-04T09:59:09Z

quantinuum-hugr/src/types.rs

-    /// A reference to a static value definition.
-    Static(Type),
+    /// A reference to a static value definition - Const or Function
+    Static(StaticEdgeData),


I like Const and Function. I think that would make some of the code clearer, versus this hierarchy of edge types. You do comment that many statements about Static edges in the spec apply equally to both kinds, so not entirely sure, but even so a little repetition in the spec may be a price worth paying.

cqc-alec · 2024-04-04T11:58:07Z

specification/hugr.md

 ```

+Note that both TypeArgs of kind both Type and Extensions may also include variables.


Suggested change

Note that both TypeArgs of kind both Type and Extensions may also include variables.

Note that TypeArgs of kind both Type and Extensions may also include variables.

Good, but I realized these lines were just repeating the -- comments in the fragment above, so went with the comments

ss2165 · 2024-04-05T13:06:46Z

quite possible that test coverage wasn't there. I'm out of date on the serialization I think, tagging in @aborgna-q

aborgna-q · 2024-04-05T13:13:28Z

My changes only made sure that the schema we are checking against is up-to-date, but the tests remain quite spotty.

@doug-q was working on adding more coverage over all serialisable hugrs.

cqc-alec · 2024-04-08T12:11:21Z

specification/hugr.md

- `Static` edges are similar to `Value` edges but carry static data (knowable at
-  compilation time). They have a `CopyableType` as an edge weight.
+- `Const` edges are similar to `Value` edges but carry static data (knowable at
+  compilation time). These has as edge weight a `CopyableType`.


Suggested change

compilation time). These has as edge weight a `CopyableType`.

compilation time). These have as edge weight a `CopyableType`.

cqc-alec · 2024-04-08T12:14:39Z

specification/hugr.md

+* *Static* edges are the union of the `Const` and `Function` edges
+* *dataflow* edges are the union of `Value` and Static (thus, `Value`, `Const` and `Function`)


Suggested change

* *Static* edges are the union of the `Const` and `Function` edges

* *dataflow* edges are the union of `Value` and Static (thus, `Value`, `Const` and `Function`)

* *Static* edges are the union of the `Const` and `Function` edges.

* *Dataflow* edges are the union of `Value` and Static (thus, `Value`, `Const` and `Function`).

Yes, I guess so. Quite a few existing uses of "dataflow edges" thus also capitalized.

Actually I was only suggesting to make each bullet here a full sentence, hence the capitalization. But capitalizing Dataflow more generally seems reasonable.

cqc-alec · 2024-04-08T12:18:50Z

specification/hugr.md

   The operation declares the arguments required by the `compute_signature` function as a list
-   of TypeParams; if this successfully returns a `Function` type, then that may then require
-   additional TypeParams.
+   of TypeParams; if this successfully returns a typ scheme, that may have additional TypeParams


Suggested change

of TypeParams; if this successfully returns a typ scheme, that may have additional TypeParams

of TypeParams; if this successfully returns a type scheme, that may have additional TypeParams

Ha (thank you!) - maybe I meant a tip scheme, for dividing gratuities between team members

cqc-alec · 2024-04-08T12:23:35Z

specification/hugr.md

+| `LoadConstant` | 0, 1    | +, ✱    | 1, 0    | 0, 0       | 0, 0          | 1, 0        |          |
+| `Input`        | 0, ✱    | 0, ✱    | 0, 0    | 0, 0       | 0, 0          | 1, 0        |          |
+| `Output`       | ✱, 0    | ✱, 0    | 0, 0    | 0, 0       | 0, 0          | 1, 0        |          |
+| `LeafOp`       | ✱, ✱    | ✱, ✱    | ✱, 0    | *, 0       | 0, 0          | 1, 0        |          |


Can a LeafOp have Function inputs?

Good spot! At the time I wasn't really sure what a LeafOp was, and thought it might include some of the other rows such as LoadConstant or Call, hence Static: *,0. However I now see that LeafOp should be MakeTuple, UnpackTuple, Tag, Lift, Noop and extension ops, so indeed, no Const or Function inputs or outputs. Done, thanks :-)

acl-cqc · 2024-04-08T20:21:51Z

Thanks, Alec!

I've added a new roundtrip serialization test with an op on function values, which test was passing on main but failing to satisfy the schema here; and then updated the schema so it passes. So I think everything I originally wanted to include is now in here.

@aborgna-q serial_hugr.py was introduced at v1 in #888 but the version was left at v1 in intervening PRs e.g. #891. I am therefore assuming that I should not be bumping that to v2 here?

aborgna-q · 2024-04-09T08:35:10Z

@aborgna-q serial_hugr.py was introduced at v1 in #888 but the version was left at v1 in intervening PRs e.g. #891. I am therefore assuming that I should not be bumping that to v2 here?

Correct. We still haven't released any new (non-alpha) version with the v1 schema, so no need to bump it.

cqc-alec · 2024-04-09T09:30:17Z

specification/schema/hugr_schema_v1.json

I notice that the description of PolyFuncType is identical to that of FunctionType. Perhaps this was a copy-and-paste error?

Good spot, these were autogenerated from the python which has its own docstrings independent of the rust. I've done some updating of all of these.

## 🤖 New release * `hugr`: 0.3.0-alpha.1 -> 0.3.0-alpha.2 (⚠️ API breaking changes) ### ⚠️ `hugr` breaking changes ``` --- 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.30.0/src/lints/enum_missing.ron Failed in: enum hugr::ops::custom::ExternalOp, previously in file /tmp/.tmpZslYkR/hugr/src/ops/custom.rs:20 enum hugr::ops::leaf::LeafOp, previously in file /tmp/.tmpZslYkR/hugr/src/ops/leaf.rs:21 enum hugr::ops::LeafOp, previously in file /tmp/.tmpZslYkR/hugr/src/ops/leaf.rs:21 --- 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.30.0/src/lints/enum_variant_added.ron Failed in: variant ConstTypeError:NotMonomorphicFunction in /tmp/.tmpoZAGEw/hugr/hugr/src/ops/constant.rs:98 variant ConstTypeError:NotMonomorphicFunction in /tmp/.tmpoZAGEw/hugr/hugr/src/ops/constant.rs:98 variant SignatureError:CallIncorrectlyAppliesType in /tmp/.tmpoZAGEw/hugr/hugr/src/extension.rs:172 --- 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.30.0/src/lints/enum_variant_missing.ron Failed in: variant InterGraphEdgeError::InvalidConstSrc, previously in file /tmp/.tmpZslYkR/hugr/src/hugr/validate.rs:774 variant OpType::LeafOp, previously in file /tmp/.tmpZslYkR/hugr/src/ops.rs:30 variant SignatureError::TypeApplyIncorrectCache, previously in file /tmp/.tmpZslYkR/hugr/src/extension.rs:171 variant EdgeKind::Static, previously in file /tmp/.tmpZslYkR/hugr/src/types.rs:44 variant ConstTypeError::FunctionTypeMissing, previously in file /tmp/.tmpZslYkR/hugr/src/ops/constant.rs:99 variant ConstTypeError::FunctionTypeMissing, previously in file /tmp/.tmpZslYkR/hugr/src/ops/constant.rs:99 --- failure inherent_method_missing: pub method removed or renamed --- Description: A publicly-visible method or associated fn 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.30.0/src/lints/inherent_method_missing.ron Failed in: OpType::as_leaf_op, previously in file /tmp/.tmpZslYkR/hugr/src/ops.rs:103 OpType::is_leaf_op, previously in file /tmp/.tmpZslYkR/hugr/src/ops.rs:103 --- failure method_parameter_count_changed: pub method parameter count changed --- Description: A publicly-visible method now takes a different number of parameters. ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.30.0/src/lints/method_parameter_count_changed.ron Failed in: hugr::types::PolyFuncType::validate now takes 2 parameters instead of 3, in /tmp/.tmpoZAGEw/hugr/hugr/src/types/poly_func.rs:76 --- failure struct_missing: pub struct removed or renamed --- Description: A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct 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.30.0/src/lints/struct_missing.ron Failed in: struct hugr::ops::leaf::TypeApplication, previously in file /tmp/.tmpZslYkR/hugr/src/ops/leaf.rs:82 ``` <details><summary>Changelog</summary> <blockquote> ## 0.3.0-alpha.2 (2024-04-15) ### Documentation - Specify direct children in `HugrView::children` ([#921](#921)) - Add logo svg to readme and spec ([#925](#925)) ### Features - [**breaking**] No polymorphic closures ([#906](#906)) - [**breaking**] Flatten `LeafOp` ([#922](#922)) ### Refactor - Combine ExtensionSolutions (no separate closure) ([#884](#884)) - [**breaking**] Merge `CustomOp` and `ExternalOp`. ([#923](#923)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Agustin Borgna <agustin.borgna@quantinuum.com>

🤖 I have created a release *beep* *boop* --- ## 0.1.0 (2024-04-15) ### ⚠ BREAKING CHANGES * Flatten `LeafOp` ([#922](#922)) * EdgeKind::{Static -> Const}, add new EdgeKind::Function, Type contains only monomorphic functions, remove TypeApply. * **py:** Rename package to `hugr` ([#913](#913)) ### Features * Flatten `LeafOp` ([#922](#922)) ([3598913](3598913)) * No polymorphic closures ([#906](#906)) ([b05dd6b](b05dd6b)) * **py:** Rename package to `hugr` ([#913](#913)) ([9fe65db](9fe65db)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Agustín Borgna <121866228+aborgna-q@users.noreply.github.com>

Also some driveby updates e.g. missed in #906. Closes #1097, #745.

acl-cqc added 9 commits April 2, 2024 15:48

Remove TypeApply op, use validation code for Call instead

39ecc87

Remove now-unused instantiate_poly

1a1d08d

Add EdgeKind::Function for FuncDefn/Decl, Call

bc08af4

Type::Function is a FunctionType not PolyFuncType

ce429d5

Rm nested-binder tests (cannot make such a testcase anymore) Const: generalize error in case value is a polymorphic function impl TryFrom<PolyFuncType> for FunctionType

Rm PolyFunc::substitute

913414b

Rm InsideBinders

0edf898

Rm Renumber

6dee540

change 'trait Substitution' into its only impl (SubstValues)

ec78a49

inline apply_typevar

Update doclinks

9d90dfa

acl-cqc added 2 commits April 3, 2024 12:51

Combine EdgeKind::{Static,Function} by nested enum StaticEdgeData

9c5dad7

spec

d81781e

acl-cqc commented Apr 3, 2024

View reviewed changes

acl-cqc requested a review from cqc-alec April 3, 2024 15:07

acl-cqc mentioned this pull request Apr 3, 2024

Handle ExtensionSet type parameters in inference #713

Closed

cqc-alec reviewed Apr 4, 2024

View reviewed changes

acl-cqc added 12 commits April 5, 2024 09:26

Remove InvalidStaticSrc

298e3b8

Refactor validate_edge using matches!

d9aa038

validate_edge: refactor: early return if local to skip non-local checks

a01ebda

Rm StaticEdgeData, switch to EdgeKind::Const and EdgeKind::Function

ffcbe05

validate_edge only returns InterGraphEdgeError not ValidationError

7d2e068

Break out validate_port_kind returning SignatureError

5a3e6a6

Add EdgeKind::is_static

01de858

Update spec Static = Const union Function

25306ae

Update spec using 'type scheme'

f76683d

cleanup: PolyFuncType::validate never receives external TypeParams

fd87403

We are no longer using deBruijn as there are never nested binders

0c6429d

fix doclinks

b2ac160

cqc-alec reviewed Apr 8, 2024

View reviewed changes

acl-cqc added 7 commits April 8, 2024 20:42

Add serialization test of function - passes on main but failing here

916efc2

Update python schema, undo manual schema changes...

fcb7e15

....and regenerate schema, fixing test

8b38333

These has -> These have

b81e090

dataflow edge -> Dataflow edge

c192f95

typ scheme -> type scheme

66a200a

LeafOp: no constant or static inputs

b070a5f

acl-cqc requested a review from cqc-alec April 8, 2024 20:24

cqc-alec approved these changes Apr 9, 2024

View reviewed changes

Update PolyFuncType-vs-FunctionType docs, in rust+py, regenerate schema

1777329

acl-cqc added this pull request to the merge queue Apr 9, 2024

Merged via the queue into main with commit b05dd6b Apr 9, 2024
18 of 19 checks passed

acl-cqc deleted the feat/no_polymorphic_closures branch April 9, 2024 11:15

github-actions bot mentioned this pull request Apr 4, 2024

chore: release v0.3.0-alpha.2 #898

Merged

acl-cqc mentioned this pull request Apr 15, 2024

Can you LoadConstant a FuncDefn? #829

Closed

This was referenced Apr 15, 2024

chore: release v0.3.1 #928

Merged

chore(main): release hugr-py 0.1.0 #932

Merged

mark-koch mentioned this pull request Apr 16, 2024

Call serialisation schema is broken #941

Closed

acl-cqc mentioned this pull request Apr 25, 2024

HugrView::get_function_type makes no sense #957

Closed

acl-cqc mentioned this pull request Aug 8, 2024

docs: update spec for Row Variables and no-input-extensions #1402

Merged

github-merge-queue bot pushed a commit that referenced this pull request Aug 20, 2024

docs: update spec for Row Variables and no-input-extensions (#1402)

67f0668

Also some driveby updates e.g. missed in #906. Closes #1097, #745.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: No polymorphic closures #906

feat: No polymorphic closures #906

acl-cqc commented Apr 2, 2024 •

edited

Loading

codecov bot commented Apr 2, 2024 •

edited

Loading

acl-cqc Apr 3, 2024

acl-cqc Apr 3, 2024

cqc-alec Apr 4, 2024

acl-cqc Apr 3, 2024

cqc-alec Apr 4, 2024

acl-cqc Apr 5, 2024

cqc-alec Apr 4, 2024

cqc-alec Apr 4, 2024

acl-cqc Apr 8, 2024

ss2165 commented Apr 5, 2024

aborgna-q commented Apr 5, 2024

cqc-alec Apr 8, 2024

cqc-alec Apr 8, 2024

acl-cqc Apr 8, 2024

cqc-alec Apr 9, 2024

cqc-alec Apr 8, 2024

acl-cqc Apr 8, 2024

cqc-alec Apr 8, 2024

acl-cqc Apr 8, 2024

acl-cqc commented Apr 8, 2024

aborgna-q commented Apr 9, 2024

cqc-alec Apr 9, 2024

acl-cqc Apr 9, 2024

		```

		Note that both TypeArgs of kind both Type and Extensions may also include variables.

	Note that both TypeArgs of kind both Type and Extensions may also include variables.
	Note that TypeArgs of kind both Type and Extensions may also include variables.

	compilation time). These has as edge weight a `CopyableType`.
	compilation time). These have as edge weight a `CopyableType`.

		* Static edges are the union of the `Const` and `Function` edges
		* dataflow edges are the union of `Value` and Static (thus, `Value`, `Const` and `Function`)

	of TypeParams; if this successfully returns a typ scheme, that may have additional TypeParams
	of TypeParams; if this successfully returns a type scheme, that may have additional TypeParams

feat: No polymorphic closures #906

feat: No polymorphic closures #906

Conversation

acl-cqc commented Apr 2, 2024 • edited Loading

codecov bot commented Apr 2, 2024 • edited Loading

Codecov Report

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

ss2165 commented Apr 5, 2024

aborgna-q commented Apr 5, 2024

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 commented Apr 8, 2024

aborgna-q commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc commented Apr 2, 2024 •

edited

Loading

codecov bot commented Apr 2, 2024 •

edited

Loading