feat!: Allow "Row Variables" declared as List<Type> #804

acl-cqc · 2024-01-10T15:47:49Z

Add a RowVariable variant of Type(Enum) that stands for potentially multiple types, created via Type::new_row_var_use.
This can appear in a TypeRow, i.e. the TypeRow is then of variable length; and can be instantiated with a list of types (including row vars) or a single row variable
Validation enforces that RowVariables are not used directly as wire/port types, but can appear inside other types
OpDef's may be polymorphic over RowVariables (allowing "varargs"-like operators equivalent to e.g. MakeTuple); these must be replaced by non-rowvar types when instantiating the OpDef to an OpType
FuncDefn's/FuncDecl's may also be polymorphic over RowVariables as long as these are not directly argument/result types
Also add TypeParam::new_sequence

closes #787

BREAKING CHANGE: Type::validate takes extra bool (allow_rowvars); renamed {FunctionType, PolyFuncType}::(validate=>validate_var_len).

…_typevar_in_row

codecov · 2024-01-10T15:52:22Z

Codecov Report

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

Project coverage is 86.72%. Comparing base (02f3864) to head (98c1fc9).
Report is 6 commits behind head on main.

Files	Patch %	Lines
hugr/src/hugr/validate/test.rs	85.14%	0 Missing and 15 partials ⚠️
hugr/src/types/type_param.rs	95.60%	7 Missing and 1 partial ⚠️
hugr/src/types.rs	86.95%	6 Missing ⚠️
hugr/src/builder/dataflow.rs	92.85%	0 Missing and 2 partials ⚠️
hugr/src/types/poly_func.rs	97.84%	0 Missing and 2 partials ⚠️
hugr/src/extension/op_def.rs	90.90%	0 Missing and 1 partial ⚠️
hugr/src/types/signature.rs	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #804      +/-   ##
==========================================
+ Coverage   86.59%   86.72%   +0.12%     
==========================================
  Files          83       88       +5     
  Lines       17582    18467     +885     
  Branches    17582    18074     +492     
==========================================
+ Hits        15226    16015     +789     
- Misses       1536     1612      +76     
- Partials      820      840      +20

Flag	Coverage Δ
python	`83.20% <100.00%> (∅)`
rust	`86.79% <93.45%> (+0.19%)`	⬆️

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-01-15T09:45:47Z

The "real" way to do this would be to change TypeRow to store elements of Either<RowVariable, Type> (where struct RowVariable(usize, TypeBound)), but I think that's just gonna be too messy with type-rows everywhere.

Thinking about this, it might not be too bad (i.e. to make TypeRow contain a list of RowVariableOrType) - given enough impl and Into magic. After all, most uses are all Types, and type_row! is a macro that can jump around typechecking issues...is it worth a try, do you think?

ss2165

I agree that the "proper way" is worth a quick try (time bounded!). The macro does help us but the annoying part will likely be that most uses of rows treat it like a slice of types (iterating, getting etc.). I'm imagining moving the current TypeRow impls to a new ConcreteTypeRow struct and then adding a panicking as_concrete() to a TypeRow enum. Maybe there is something less onerous?

src/types.rs

aborgna-q · 2024-03-20T16:31:20Z

@acl-cqc What is the status of this? Did you get to try the TypeRow / ConcreteTypeRow alternative?

acl-cqc · 2024-03-22T16:55:46Z

@acl-cqc What is the status of this? Did you get to try the TypeRow / ConcreteTypeRow alternative?

Yes, or at least, partly, in branch feat/row_typevar2. ConcreteTypeRow is called TypeRow and non-concrete is TypeRowV. From what I remember we didn't think this looked very promising, but perhaps with more tidying up?? Both are quite out of date, certainly I can update this branch but I think I'd not update feat/row_typevar2 unless we really thought that was the best way to go...

doug-q · 2024-04-11T11:07:15Z

I admit that I am not convinced of the value of this feature. Taking the value on faith, may I suggest a generalisation:

EDIT: on further investigation the current implementaton already works as I describe!
As written, some typerows are granted the ability to be a single RowVariable which can then be substituted for a concrete typerow of arbitrary length. e.g. [tr]/{tr => [i32, i64]} => [i32,i64]. I suggest generalising this to: some typerows are granted the ability to contain RowVariables which can be substituted for a typerow of arbitrary length, which is spliced into the typerow. e.g. [i16, tr, i128]/{tr => [i32,i64]} => [i16,i32,164,i128]. I think this is more general at no cost.

acl-cqc · 2024-04-15T08:42:44Z

The value of the feature is to allow us to make operations like UnpackTuple, MakeTuple, etc., as library operations in the prelude, rather than the current LeafOp enum of ops which (without this PR) cannot be expressed in the extension system...

doug-q · 2024-04-15T09:08:29Z

The value of the feature is to allow us to make operations like UnpackTuple, MakeTuple, etc., as library operations in the prelude, rather than the current LeafOp enum of ops which (without this PR) cannot be expressed in the extension system...

I disagree. There is only aesthetic value in making these ops library operations.

I instead understand the value to be to allow operations with variadic inputs/outputs to be defined. The cost of not allowing these to be defined is that one is forced, by their absence, to instead work with tuples. The requisite UnpackTuple operations introduce barriers to parallelism.

acl-cqc · 2024-04-15T10:51:55Z

@doug - moved discussion to #787

acl-cqc · 2024-04-15T12:01:18Z

hugr/src/types.rs

+    /// New use (occurrence) of the row variable with specified index.
+    /// `bound` must match that with which the variable was declared
+    /// (i.e. as a [TypeParam::List]` of a `[TypeParam::Type]` of that bound).
+    /// For use in [OpDef], not [FuncDefn], type schemes only.


This is the critical distinction, which is neither explained nor probably made, as clear(ly) as it needs to be. (So far, in this PR!)

acl-cqc · 2024-05-20T16:47:01Z

Removed "Port" from TypeRow on main in refactor!: No Ports in TypeRow #1087, now merged in here
Moved not-indexing-over-rowvars check from TypeRow to FunctionType via helper, made non-debug assert
Type::validate{_1type,_in_row} renamed/combined to validate with extra bool first argument
FunctionType/PolyFuncType validate_varargs renamed to validate_var_len for consistency with TypeRow

acl-cqc · 2024-05-21T08:43:18Z

Oh, and fixed a failing proptest using a new Type::non_row_var strategy for the element type of list values. This is sufficient for the test, but still (generally speaking) far from valid; a list-value must have a closed type without any type variables at all (row or type vars, at the top level or elsewhere) whereas this only rules out top-level row vars.

doug-q

LGTM. I am unsure about debug_assert/assert. debug_assert doesn't impose a performance cost.

doug-q · 2024-05-21T09:08:24Z

hugr/src/types.rs

+            pub fn any_non_row_var() -> BoxedStrategy<Self> {
+                any::<Self>()
+                    .prop_filter("Cannot be a Row Variable", |t| !t.is_row_var())
+                    .boxed()


This is fine, but for your notes: this is not the best way to do this. Filtering is to be avoided, because if too many values are filtered out tests will fail. One should instead generate the values that you want. Use filtering only when this is impractical.

To "generate values that you want", change the Arbitrary impl for Type. Change Parameters to struct ArbitraryTypeParamerters(RecursionDepth,bool) and in arbitrary_with only include the RowVariable strategy when this is new bool is true. You will likely need to use Union to achieve this, I think there is an example in the codebase.

doug-q · 2024-05-21T09:11:55Z

hugr/src/types/signature.rs

+    let idx = port.index();
+    if idx > 0 {
+        // Check we have not skipped over / indexed past any row variables
+        assert!(!row.iter().take(idx - 1).any(Type::is_row_var));


I worry this is too expensive for an assert. I would prefer to disallow any row variables, even after the index i.e. removing the take.

doug-q · 2024-05-21T09:14:06Z

hugr/src/types/type_param.rs

+        check_type_arg(&row_arg, &row_param).unwrap();
+
+        // Now say a row variable referring to *that* row was used
+        // to instantiate an outer "row parameter" (list of type).


Yes I think so, but not necessary in this PR. create an issue.

doug-q · 2024-05-21T09:18:19Z

I edited the title to mark a breaking change. Can your review the commit message, in particular remove references to commits that will be squashed.

…w_variables

…ort indexing)

…xtension_with_eval_parallel

doug-q

Great! just one nit.

doug-q · 2024-05-22T08:36:08Z

hugr/src/types/signature.rs

@@ -193,6 +193,17 @@ impl FunctionType {
 }

 impl FunctionType {
+    /// If this FunctionType contains any row variables, return one.
+    pub fn find_rowvar(&self) -> Option<(usize, TypeBound)> {


not sure this should be pub?

I was going to have has_rowvars(&self) -> bool and thought that definitely should be pub. Then I realized that when it returned true I'd have to scan through to find one to raise the error I wanted, so why not do that in the same go. I think having some kind of are-there-any-rowvars is a good method to have, I agree this is a slightly non-obvious form for it but it's no worse than the bool one no?

Could return a list of vars, or Vec<(Direction, usize, (usize, TypeBound)> (i.e. where each one was found) or something like that I guess?

It's not the return type it's that I don't think this will have any users, so preferring not to pollute the namespace + docs. If you disagree, no problem.

I think we should have a query method, yes. When we split FunTypeVarArgs and SignatureType it belongs only on the former (as necessarily false/None for Signature).

## 🤖 New release * `hugr`: 0.4.0 -> 0.5.0 * `hugr-cli`: 0.1.0 * `hugr-core`: 0.1.0 * `hugr-passes`: 0.1.0 <details><summary>Changelog</summary> ## `hugr` <blockquote> ## 0.5.0 (2024-05-29) ### Bug Fixes - Missing re-exports in `hugr::hugr` ([#1127](#1127)) - Set initial version of hugr-core to 0.1.0 ([#1129](#1129)) ### Features - [**breaking**] Remove `PartialEq` impl for `ConstF64` ([#1079](#1079)) - [**breaking**] Allow "Row Variables" declared as List<Type> ([#804](#804)) - Hugr binary cli tool ([#1096](#1096)) - [**breaking**] Move passes from `algorithms` into a separate crate ([#1100](#1100)) - [**breaking**] Disallow nonlocal value edges into FuncDefn's ([#1061](#1061)) - [**breaking**] Move cli in to hugr-cli sub-crate ([#1107](#1107)) - Add verbosity, return `Hugr` from `run`. ([#1116](#1116)) - Unseal and make public the traits `HugrInternals` and `HugrMutInternals` ([#1122](#1122)) ### Refactor - [**breaking**] No Ports in TypeRow ([#1087](#1087)) - Add a `hugr-core` crate ([#1108](#1108)) </blockquote> ## `hugr-core` <blockquote> ## 0.1.0 (2024-05-29) ### Bug Fixes - Set initial version of hugr-core to 0.1.0 ([#1129](#1129)) ### Features - [**breaking**] Move cli in to hugr-cli sub-crate ([#1107](#1107)) - Make internals of int ops and the "int" CustomType more public. ([#1114](#1114)) - Unseal and make public the traits `HugrInternals` and `HugrMutInternals` ([#1122](#1122)) ### Refactor - Add a `hugr-core` crate ([#1108](#1108)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Co-authored-by: Agustin Borgna <[email protected]>

🤖 I have created a release *beep* *boop* --- ## [0.3.0](hugr-py-v0.2.1...hugr-py-v0.3.0) (2024-07-03) ### ⚠ BREAKING CHANGES * * `add_child_op`(`_with_parent`), etc., gone; use `add_child_node`(`_with_parent`) with an (impl Into-)OpType. * `get_nodetype` gone - use `get_optype`. * `NodeType` gone - use `OpType` directly. * Various (Into<)Option<ExtensionSet> params removed from builder methods especially {cfg_,dfg_}builder. * `input_extensions` removed from serialization schema. * the Signature class is gone, but it's not clear how or why you might have been using it... * TailLoop node and associated builder functions now require specifying an ExtensionSet; extension/validate.rs deleted; some changes to Hugrs validated/rejected when the `extension_inference` feature flag is turned on * Type::validate takes extra bool (allow_rowvars); renamed {FunctionType, PolyFuncType}::(validate=>validate_var_len). ### Features * Allow "Row Variables" declared as List<Type> ([#804](#804)) ([3ea4834](3ea4834)) * **hugr-py:** add builders for Conditional and TailLoop ([#1210](#1210)) ([43569a4](43569a4)) * **hugr-py:** add CallIndirect, LoadFunction, Lift, Alias ([#1218](#1218)) ([db09193](db09193)), closes [#1213](#1213) * **hugr-py:** add values and constants ([#1203](#1203)) ([f7ea178](f7ea178)), closes [#1202](#1202) * **hugr-py:** automatically add state order edges for inter-graph edges ([#1165](#1165)) ([5da06e1](5da06e1)) * **hugr-py:** builder for function definition/declaration and call ([#1212](#1212)) ([af062ea](af062ea)) * **hugr-py:** builder ops separate from serialised ops ([#1140](#1140)) ([342eda3](342eda3)) * **hugr-py:** CFG builder ([#1192](#1192)) ([c5ea47f](c5ea47f)), closes [#1188](#1188) * **hugr-py:** define type hierarchy separate from serialized ([#1176](#1176)) ([10f4c42](10f4c42)) * **hugr-py:** only require input type annotations when building ([#1199](#1199)) ([2bb079f](2bb079f)) * **hugr-py:** python hugr builder ([#1098](#1098)) ([23408b5](23408b5)) * **hugr-py:** store children in node weight ([#1160](#1160)) ([1cdaeed](1cdaeed)), closes [#1159](#1159) * **hugr-py:** ToNode interface to treat builders as nodes ([#1193](#1193)) ([1da33e6](1da33e6)) * Validate Extensions using hierarchy, ignore input_extensions, RIP inference ([#1142](#1142)) ([8bec8e9](8bec8e9)) ### Bug Fixes * Add some validation for const nodes ([#1222](#1222)) ([c05edd3](c05edd3)) * **hugr-py:** more ruff lints + fix some typos ([#1246](#1246)) ([f158384](f158384)) * **py:** get rid of pydantic config deprecation warnings ([#1084](#1084)) ([52fcb9d](52fcb9d)) ### Documentation * **hugr-py:** add docs link to README ([#1259](#1259)) ([d2a9148](d2a9148)) * **hugr-py:** build and publish docs ([#1253](#1253)) ([902fc14](902fc14)) * **hugr-py:** docstrings for builder ([#1231](#1231)) ([3e4ac18](3e4ac18)) ### Code Refactoring * Remove "Signature" from hugr-py ([#1186](#1186)) ([65718f7](65718f7)) * Remove NodeType and input_extensions ([#1183](#1183)) ([ea5213d](ea5213d)) --- 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: hugrbot <[email protected]> Co-authored-by: Seyon Sivarajah <[email protected]>

acl-cqc added 2 commits January 10, 2024 14:56

Refactor: break out fn valid_row for validating a TypeRow

7b242ac

Type::{validate,substitute}_in_row, {Substitution,SubstValues}::apply…

ba05010

…_typevar_in_row

acl-cqc added 6 commits January 12, 2024 14:16

[test] types.rs::construct - also validate

59c5854

[tests] poly_func.rs: test row variables

72b4fb2

[test] validate Hugr passing around Function of unknown arity

2cf1e7f

[tests]Validation prevents row variables being used in node signatures

8899806

Simplify default apply_typevar_in_row

e1f1825

Separate TypeEnum::RowVariable

1074093

acl-cqc changed the title ~~feat: Allow TypeVars of kind List<Type> in rows~~ feat: Allow "Row Variables" declared as List<Type> Jan 12, 2024

acl-cqc requested a review from ss2165 January 12, 2024 17:08

acl-cqc marked this pull request as ready for review January 15, 2024 09:00

ss2165 reviewed Jan 17, 2024

View reviewed changes

src/types.rs Outdated Show resolved Hide resolved

acl-cqc mentioned this pull request Apr 2, 2024

[WIP] Row Variables, the hard (but good?) way #903

Closed

acl-cqc mentioned this pull request Apr 15, 2024

Allow TypeArg for a row of types #787

Closed

acl-cqc added 2 commits April 15, 2024 12:34

Merge remote-tracking branch 'origin/main' into feat/row_typevar

4291848

Doc updates - no DeBruijn, ref OpDef vs FuncDefn

98c75a2

acl-cqc commented Apr 15, 2024

View reviewed changes

Remove unused test setup changes

b918bee

acl-cqc force-pushed the feat/row_typevar branch from 88cd17e to b918bee Compare April 15, 2024 12:04

acl-cqc marked this pull request as draft April 29, 2024 10:42

acl-cqc added 2 commits May 9, 2024 18:13

Merge remote-tracking branch 'origin/main' into feat/row_typevar

a096f1a

Combine Type::substitute(_in_row), doc, add TODOs

4b0e23f

Merge 'origin/main' into feat/row_typevar. Proptest failing...

b77e3c8

acl-cqc mentioned this pull request May 20, 2024

refactor!: No Ports in TypeRow #1087

Merged

acl-cqc added 6 commits May 20, 2024 17:26

Fix proptest by local reject

91317ae

Temp remove TypeRow checks

3cbccc7

Merge remote-tracking branch 'origin/main' into feat/row_typevar

a43e0a1

Add non-debug asserts in FunctionType

e0c7162

Combine Type::validate_1type / validate_in_row to validate(bool,...)

7ebb596

validate_varargs => validate_var_len

bc2d358

acl-cqc requested a review from doug-q May 20, 2024 16:47

doug-q approved these changes May 21, 2024

View reviewed changes

doug-q changed the title ~~feat: Allow "Row Variables" declared as List<Type>~~ feat!: Allow "Row Variables" declared as List<Type> May 21, 2024

acl-cqc added 4 commits May 21, 2024 15:10

RowVarOutsideRow => RowVarWhereTypeExpected

5b63d0c

check_no_rowvars - ignore index, check whole row - breaks no_outer_ro…

2a28052

…w_variables

FunctionType::find_row_var, check in builder and validation (before p…

c559abc

…ort indexing)

Tests (rewrite w/out builder; add builder test, using now-pub-crate e…

98c1fc9

…xtension_with_eval_parallel

doug-q approved these changes May 22, 2024

View reviewed changes

acl-cqc added this pull request to the merge queue May 22, 2024

Merged via the queue into main with commit 3ea4834 May 22, 2024
22 of 23 checks passed

acl-cqc deleted the feat/row_typevar branch May 22, 2024 09:33

aborgna-q mentioned this pull request May 22, 2024

chore(main): release hugr-py 0.3.0 #1092

Merged

This was referenced May 22, 2024

chore: release #1088

Closed

chore: release #1104

Closed

This was referenced May 29, 2024

chore: release #1125

Closed

chore: release #1128

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Allow "Row Variables" declared as List<Type> #804

feat!: Allow "Row Variables" declared as List<Type> #804

acl-cqc commented Jan 10, 2024 •

edited

Loading

codecov bot commented Jan 10, 2024 •

edited

Loading

acl-cqc commented Jan 15, 2024

ss2165 left a comment

aborgna-q commented Mar 20, 2024

acl-cqc commented Mar 22, 2024

doug-q commented Apr 11, 2024 •

edited

Loading

acl-cqc commented Apr 15, 2024

doug-q commented Apr 15, 2024

acl-cqc commented Apr 15, 2024

acl-cqc Apr 15, 2024

acl-cqc commented May 20, 2024 •

edited

Loading

acl-cqc commented May 21, 2024

doug-q left a comment

doug-q May 21, 2024

doug-q May 21, 2024

doug-q May 21, 2024

doug-q commented May 21, 2024

doug-q left a comment

doug-q May 22, 2024

acl-cqc May 22, 2024

doug-q May 22, 2024

acl-cqc May 22, 2024

feat!: Allow "Row Variables" declared as List<Type> #804

feat!: Allow "Row Variables" declared as List<Type> #804

Conversation

acl-cqc commented Jan 10, 2024 • edited Loading

codecov bot commented Jan 10, 2024 • edited Loading

Codecov Report

acl-cqc commented Jan 15, 2024

ss2165 left a comment

Choose a reason for hiding this comment

aborgna-q commented Mar 20, 2024

acl-cqc commented Mar 22, 2024

doug-q commented Apr 11, 2024 • edited Loading

acl-cqc commented Apr 15, 2024

doug-q commented Apr 15, 2024

acl-cqc commented Apr 15, 2024

Choose a reason for hiding this comment

acl-cqc commented May 20, 2024 • edited Loading

acl-cqc commented May 21, 2024

doug-q 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

doug-q commented May 21, 2024

doug-q 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

acl-cqc commented Jan 10, 2024 •

edited

Loading

codecov bot commented Jan 10, 2024 •

edited

Loading

doug-q commented Apr 11, 2024 •

edited

Loading

acl-cqc commented May 20, 2024 •

edited

Loading