refactor: use type schemes in extension definitions wherever possible #678

ss2165 · 2023-11-10T15:28:30Z

Closes #658

As a drive by improve extensions test coverage as I went.

Definition can be a bit unintuitive, should improve after #676 is done.

codecov · 2023-11-10T15:33:40Z

Codecov Report

Merging #678 (b5159e9) into main (d32d033) will increase coverage by 1.77%.
The diff coverage is 98.23%.

@@            Coverage Diff             @@
##             main     #678      +/-   ##
==========================================
+ Coverage   81.97%   83.74%   +1.77%     
==========================================
  Files          66       66              
  Lines       12759    12754       -5     
  Branches    12759    12754       -5     
==========================================
+ Hits        10459    10681     +222     
+ Misses       1538     1314     -224     
+ Partials      762      759       -3

Files	Coverage Δ
src/extension/op_def.rs	`75.98% <100.00%> (+1.76%)`	⬆️
src/extension/prelude.rs	`88.46% <100.00%> (+0.46%)`	⬆️
src/std_extensions/arithmetic/conversions.rs	`100.00% <100.00%> (+24.63%)`	⬆️
src/std_extensions/arithmetic/float_ops.rs	`100.00% <100.00%> (+16.66%)`	⬆️
src/std_extensions/arithmetic/float_types.rs	`96.22% <100.00%> (+25.39%)`	⬆️
src/std_extensions/arithmetic/int_ops.rs	`99.16% <100.00%> (+20.67%)`	⬆️
src/std_extensions/arithmetic/int_types.rs	`95.00% <100.00%> (+34.71%)`	⬆️
src/std_extensions/collections.rs	`93.00% <100.00%> (+2.79%)`	⬆️
src/utils.rs	`97.01% <100.00%> (+10.11%)`	⬆️
src/std_extensions/logic.rs	`97.64% <85.71%> (+18.29%)`	⬆️
... and 1 more

... and 3 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

acl-cqc

Looks great! Thanks @ss2165. I can't quite bring myself to hit "approve" with 21 comments but certainly in principle ;)

acl-cqc · 2023-11-13T10:33:08Z

src/extension/op_def.rs

@@ -331,6 +331,23 @@ impl Extension {
            SignatureFunc::TypeScheme(type_scheme),
        )
    }
+
+    /// Create an OpDef with a signature (inputs+outputs) read from e.g.


acl-cqc · 2023-11-13T10:41:56Z

src/std_extensions/arithmetic/conversions.rs

+fn ftoi_sig(
+    int_type_var: Type,
+    temp_reg: &ExtensionRegistry,
+) -> Result<PolyFuncType, SignatureError> {


Yeah, so #676 bites harder than I expected here; without that, this would just be a PolyFuncType and verging on inlinable. (Shame we can't make it const!)

src/std_extensions/arithmetic/conversions.rs

acl-cqc · 2023-11-13T10:52:03Z

src/std_extensions/arithmetic/conversions.rs

+    let int_type_def = int_types_extension.get_type(&INT_TYPE_ID).unwrap();
+    let int_type_var = int_type_var(0, int_type_def).unwrap();
+    let temp_reg: ExtensionRegistry = [
+        extension.clone(),


#676 pending of course, but in the meantime, I suspect you don't actually need this one? (You don't define any types in this extension so you're not referring to them here)

src/std_extensions/arithmetic/int_types.rs

acl-cqc · 2023-11-13T11:18:40Z

src/std_extensions/collections.rs

        )
        .unwrap();
    extension
 }
+
+fn list_types(list_type_def: &TypeDef) -> (Type, Type) {


Did you mean to move this?

list_and_elem_type (or list_type_and_elem) might be a better name

acl-cqc · 2023-11-13T11:19:43Z

src/std_extensions/logic.rs

-                        }
-                        .into());
-                    }
+                let TypeArg::BoundedNat { n } = arg_values.iter().exactly_one().unwrap() else {


Suggested change

let TypeArg::BoundedNat { n } = arg_values.iter().exactly_one().unwrap() else {

let Ok(TypeArg::BoundedNat { n }) = arg_values.iter().exactly_one() else {

Good use of let...else tho!

acl-cqc · 2023-11-13T11:20:34Z

src/std_extensions/logic.rs

-                        }
-                        .into());
-                    }
+                let TypeArg::BoundedNat { n } = arg_values.iter().exactly_one().unwrap() else {


mainly `sum_with_error`

acl-cqc · 2023-11-13T12:59:42Z

src/std_extensions/arithmetic/float_ops.rs

@@ -40,78 +35,76 @@ pub fn extension() -> Extension {
        ExtensionSet::singleton(&super::float_types::EXTENSION_ID),
    );

+    let fcmp_sig = fcmp_sig();


You could just inline the functions here, these are the only callsites

acl-cqc

Looks great, thanks!

## 🤖 New release * `quantinuum-hugr`: 0.1.0 <details><summary>Changelog</summary> <blockquote> ## 0.1.0 (2024-01-15) ### Bug Fixes - Subgraph boundaries with copies ([#440](#440)) - [**breaking**] Use internal tag for SumType enum serialisation ([#462](#462)) - Check kind before unwrap in insert_identity ([#475](#475)) - Allow for variables to get solved in inference ([#478](#478)) - In IdentityInsertion add noop to correct parent ([#477](#477)) - Failing release tests ([#485](#485)) - [**breaking**] Serialise `Value`, `PrimValue`, and `TypeArg` with internal tags ([#496](#496)) - Serialise custom constants with internal tag ([#502](#502)) - [**breaking**] Reduce max int width in arithmetic extension to 64 ([#504](#504)) - HugrView::get_function_type ([#507](#507)) - TODO in resolve_extension_ops: copy across input_extensions ([#510](#510)) - Use given input extensions in `define_function` ([#524](#524)) - Lessen requirements for hugrs in outline_cfg ([#528](#528)) - Make unification logic less strict ([#538](#538)) - Simple replace incorrectly copying metadata ([#545](#545)) - Account for self-referencial constraints ([#551](#551)) - Consider shunted metas when comparing equality ([#555](#555)) - Join labels in issue workflow ([#563](#563)) - Comment out broken priority code ([#562](#562)) - Handling of issues with no priority label ([#573](#573)) - Don't insert temporary wires when extracting a subgraph ([#582](#582)) - Improve convexity checking and fix test ([#585](#585)) - Ignore input->output links in SiblingSubgraph::try_new_dataflow_subgraph ([#589](#589)) - Enforce covariance of SiblingMut::RootHandle ([#594](#594)) - Erratic stack overflow in infer.rs (live_var) ([#638](#638)) - Work harder in variable instantiation ([#591](#591)) - Actually add the error type to prelude ([#672](#672)) - Serialise dynamically computed opaqueOp signatures ([#690](#690)) - FuncDefns don't require that their extensions match their children ([#688](#688)) - Binary compute_signature returning a PolyFuncType with binders ([#710](#710)) - Use correct number of args for int ops ([#723](#723)) - [**breaking**] Add serde tag to TypeParam enum ([#722](#722)) - Allow widening and narrowing to same width. ([#735](#735)) - Case node should not have an external signature ([#749](#749)) - [**breaking**] Normalize input/output value/static/other ports in `OpType` ([#783](#783)) - No dataflow_signature for block types ([#792](#792)) - Ignore unsupported test in miri ([#794](#794)) - Include schema rather than read file ([#807](#807)) ### Documentation - Add operation constraint to quantum extension ([#543](#543)) - Coverage check section in DEVELOPMENT.md ([#648](#648)) - Remove "quantum extension" from HUGR spec. ([#694](#694)) - Improve crate-level docs, including example code. ([#698](#698)) - Spec cleanups and clarifications ([#742](#742)) - Spec clarifications ([#738](#738)) - Spec updates ([#741](#741)) - [spec] Remove references to causal cone and Order edges from Input ([#762](#762)) - Mention experimental inference in readme ([#800](#800)) - Collection of spec updates for 0.1 ([#801](#801)) - Add schema v0 ([#805](#805)) - Update spec wrt. polymorphism ([#791](#791)) ### Features - Derive things for builder structs ([#229](#229)) - Return a slice of types from the signature ([#238](#238)) - Move `dot_string` to `HugrView` ([#271](#271)) - [**breaking**] Change `TypeParam::USize` to `TypeParam::BoundedNat` and use in int extensions ([#445](#445)) - TKET2 compatibility requirements ([#450](#450)) - Split methods between `HugrMut` and `HugrMutInternals` ([#441](#441)) - Add `HugrView::node_connections` to get all links between nodes ([#460](#460)) - Location information in extension inference error ([#464](#464)) - Add T, Tdg, X gates ([#466](#466)) - [**breaking**] Add `ApplyResult` associated type to `Rewrite` ([#472](#472)) - Implement rewrite `IdentityInsertion` ([#474](#474)) - Instantiate inferred extensions ([#461](#461)) - Default DFG builders to open extension sets ([#473](#473)) - Some helper methods ([#482](#482)) - Extension inference for conditional nodes ([#465](#465)) - Add ConvexChecker ([#487](#487)) - Add clippy lint for mut calls in a debug_assert ([#488](#488)) - Default more builder methods to open extension sets ([#492](#492)) - Port is serializable ([#489](#489)) - More general portgraph references ([#494](#494)) - Add extension deltas to CFG ops ([#503](#503)) - Implement petgraph traits on Hugr ([#498](#498)) - Make extension delta a parameter of CFG builders ([#514](#514)) - [**breaking**] SiblingSubgraph does not borrow Hugr ([#515](#515)) - Validate TypeArgs to ExtensionOp ([#509](#509)) - Derive Debug & Clone for `ExtensionRegistry`. ([#530](#530)) - Const nodes are built with some extension requirements ([#527](#527)) - Some python errors and bindings ([#533](#533)) - Insert_hugr/insert_view return node map ([#535](#535)) - Add `SiblingSubgraph::try_from_nodes_with_checker` ([#547](#547)) - PortIndex trait for undirected port parameters ([#553](#553)) - Insert/extract subgraphs from a HugrView ([#552](#552)) - Add `new_array` operation to prelude ([#544](#544)) - Add a `DataflowParentID` node handle ([#559](#559)) - Make TypeEnum and it's contents public ([#558](#558)) - Optional direction check when querying a port index ([#566](#566)) - Extension inference for CFGs ([#529](#529)) - Better subgraph verification errors ([#587](#587)) - Compute affected nodes for `SimpleReplacement` ([#600](#600)) - Move `SimpleReplace::invalidation_set` to the `Rewrite` trait ([#602](#602)) - [**breaking**] Resolve extension ops (mutating Hugr) in (infer_and_->)update_validate ([#603](#603)) - Add accessors for node index and const values ([#605](#605)) - [**breaking**] Expose the value of ConstUsize ([#621](#621)) - Nicer debug and display for core types ([#628](#628)) - [**breaking**] Static checking of Port direction ([#614](#614)) - Builder and HugrMut add_op_xxx default to open extensions ([#622](#622)) - [**breaking**] Add panicking integer division ops ([#625](#625)) - Add hashable `Angle` type to Quantum extension ([#608](#608)) - [**breaking**] Remove "rotations" extension. ([#645](#645)) - Port.as_directed to match on either direction ([#647](#647)) - Impl GraphRef for PetgraphWrapper ([#651](#651)) - Provide+implement Replace API ([#613](#613)) - Require the node's metadata to always be a Map ([#661](#661)) - [**breaking**] Polymorphic function types (inc OpDefs) using dyn trait ([#630](#630)) - Make prelude error type public ([#669](#669)) - Shorthand for retrieving custom constants from `Const`, `Value` ([#679](#679)) - [**breaking**] HugrView API improvements ([#680](#680)) - Make FuncDecl/FuncDefn polymorphic ([#692](#692)) - [**breaking**] Simplify `SignatureFunc` and add custom arg validation. ([#706](#706)) - [**breaking**] Drop the `pyo3` feature ([#717](#717)) - [**breaking**] `OpEnum` trait for common opdef functionality ([#721](#721)) - MakeRegisteredOp trait for easier registration ([#726](#726)) - Getter for `PolyFuncType::body` ([#727](#727)) - `Into<OpType>` for custom ops ([#731](#731)) - Always require a signature in `OpaqueOp` ([#732](#732)) - Values (and hence Consts) know their extensions ([#733](#733)) - [**breaking**] Use ConvexChecker trait ([#740](#740)) - Custom const for ERROR_TYPE ([#756](#756)) - Implement RemoveConst and RemoveConstIgnore ([#757](#757)) - Constant folding implemented for core and float extension ([#769](#769)) - Constant folding for arithmetic conversion operations ([#720](#720)) - DataflowParent trait for getting inner signatures ([#782](#782)) - Constant folding for logic extension ([#793](#793)) - Constant folding for list operations ([#795](#795)) - Add panic op to prelude ([#802](#802)) - Const::from_bool function ([#803](#803)) ### HugrMut - Validate nodes for set_metadata/get_metadata_mut, too ([#531](#531)) ### HugrView - Validate nodes, and remove Base ([#523](#523)) ### Miscellaneous Tasks - [**breaking**] Update portgraph 0.10 and pyo3 0.20 ([#612](#612)) - [**breaking**] Hike MSRV to 1.75 ([#761](#761)) ### Performance - Use lazy static definittion for prelude registry ([#481](#481)) ### Refactor - Move `rewrite` inside `hugr`, `Rewrite` -> `Replace` implementing new 'Rewrite' trait ([#119](#119)) - Use an excluded upper bound instead of max log width. ([#451](#451)) - Add extension info to `Conditional` and `Case` ([#463](#463)) - `ExtensionSolution` only consists of input extensions ([#480](#480)) - Remove builder from more DFG tests ([#490](#490)) - Separate hierarchy views ([#500](#500)) - [**breaking**] Use named struct for float constants ([#505](#505)) - Allow NodeType::new to take any Into<Option<ExtensionSet>> ([#511](#511)) - Move apply_rewrite from Hugr to HugrMut ([#519](#519)) - Use SiblingSubgraph in SimpleReplacement ([#517](#517)) - CFG takes a FunctionType ([#532](#532)) - Remove check_custom_impl by inlining into check_custom ([#604](#604)) - Insert_subgraph just return HashMap, make InsertionResult new_root compulsory ([#609](#609)) - [**breaking**] Rename predicate to TupleSum/UnitSum ([#557](#557)) - Simplify infer.rs/report_mismatch using early return ([#615](#615)) - Move the core types to their own module ([#627](#627)) - Change &NodeType->&OpType conversion into op() accessor ([#623](#623)) - Infer.rs 'fn results' ([#631](#631)) - Be safe ([#637](#637)) - NodeType constructors, adding new_auto ([#635](#635)) - Constraint::Plus stores an ExtensionSet, which is a BTreeSet ([#636](#636)) - [**breaking**] Remove `SignatureDescription` ([#644](#644)) - [**breaking**] Remove add_op_<posn> by generalizing add_node_<posn> with "impl Into" ([#642](#642)) - Rename accidentally-changed Extension::add_node_xxx back to add_op ([#659](#659)) - [**breaking**] Remove quantum extension ([#670](#670)) - Use type schemes in extension definitions wherever possible ([#678](#678)) - [**breaking**] Flatten `Prim(Type/Value)` in to parent enum ([#685](#685)) - [**breaking**] Rename `new_linear()` to `new_endo()`. ([#697](#697)) - Replace NodeType::signature() with io_extensions() ([#700](#700)) - Validate ExtensionRegistry when built, not as we build it ([#701](#701)) - [**breaking**] One way to add_op to extension ([#704](#704)) - Remove Signature struct ([#714](#714)) - Use `MakeOpDef` for int_ops ([#724](#724)) - [**breaking**] Use enum op traits for floats + conversions ([#755](#755)) - Avoid dynamic dispatch for non-folding operations ([#770](#770)) - Simplify removeconstignore verify ([#768](#768)) - [**breaking**] Unwrap BasicBlock enum ([#781](#781)) - Make clear const folding only for leaf ops ([#785](#785)) - [**breaking**] `s/RemoveConstIgnore/RemoveLoadConstant` ([#789](#789)) - Put extension inference behind a feature gate ([#786](#786)) ### SerSimpleType - Use Vec not TypeRow ([#381](#381)) ### SimpleReplace+OutlineCfg - Use HugrMut methods rather than .hierarchy/.graph ([#280](#280)) ### Testing - Update inference test to not use DFG builder ([#550](#550)) - Strengthen "failing_sccs_test", rename to "sccs" as it's not failing! ([#660](#660)) - [**breaking**] Improve coverage in signature and validate ([#643](#643)) - Use insta snapshots to add dot_string coverage ([#682](#682)) - Miri ignore file-opening test ([#684](#684)) - Unify the serialisation tests ([#730](#730)) - Add schema validation to roundtrips ([#806](#806)) ### `ConstValue - :F64` and `OpaqueOp::new` ([#206](#206)) ### Cleanup - Remove outdated comment ([#536](#536)) ### Cosmetic - Format + remove stray TODO ([#444](#444)) ### Doc - Crate name as README title + add reexport ([#199](#199)) ### S/EdgeKind - :Const/EdgeKind::Static/ ([#201](#201)) ### Simple_replace.rs - Use HugrMut::remove_node, includes clearing op_types ([#242](#242)) ### Spec - Remove "Draft 3" from title of spec document. ([#590](#590)) - Rephrase confusing paragraph about TailLoop inputs/outputs ([#567](#567)) ### Src/ops/validate.rs - Common-up some type row calculations ([#254](#254)) </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: Agustín Borgna <[email protected]>

ss2165 requested a review from acl-cqc November 10, 2023 15:28

acl-cqc reviewed Nov 13, 2023

View reviewed changes

ss2165 force-pushed the refactor/use-type-schemes branch from 53452d5 to 0063b46 Compare November 13, 2023 12:32

ss2165 requested a review from acl-cqc November 13, 2023 12:42

ss2165 added 14 commits November 13, 2023 12:46

feat: add "simple" version of add_op_type_scheme

fffb696

refactor: use PolyFuncType for list definition

4ec9c75

refactor: use type scheme for logic not

7eb7bc8

test: improve logic extension test coverage

3f3e5ea

test: improve float_types coverage

8be903e

refactor: use PolyFuncType everywhere in float_ops

f5f994b

test: improve int_ops coverage

6355fa2

refactor: use PolyFuncType for conversions extension

76b1995

refactor: use PolyFunctType to define int ops extension

1508ead

refactor: use PolyFuncType for utils ops

09dbb1b

address some review comments

3ddd6ab

mainly `sum_with_error`

refactor: use int_type_var(usize) to simplify int_ops and conversions

c068c40

only call simple functions once

aeb7ef6

remove another unecessary extension

d93d5e6

ss2165 force-pushed the refactor/use-type-schemes branch from d6a9000 to d93d5e6 Compare November 13, 2023 12:47

ss2165 enabled auto-merge November 13, 2023 12:48

acl-cqc reviewed Nov 13, 2023

View reviewed changes

inline single use functions

b5159e9

ss2165 requested a review from acl-cqc November 13, 2023 13:34

acl-cqc approved these changes Nov 13, 2023

View reviewed changes

ss2165 added this pull request to the merge queue Nov 13, 2023

Merged via the queue into main with commit 201d1a2 Nov 13, 2023
10 checks passed

ss2165 deleted the refactor/use-type-schemes branch November 13, 2023 14:08

acl-cqc mentioned this pull request Nov 20, 2023

refactor!: one way to add_op to extension #704

Merged

github-actions bot mentioned this pull request Jan 3, 2024

chore: release v0.1.0 #766

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use type schemes in extension definitions wherever possible #678

refactor: use type schemes in extension definitions wherever possible #678

ss2165 commented Nov 10, 2023 •

edited

Loading

codecov bot commented Nov 10, 2023 •

edited

Loading

acl-cqc left a comment

acl-cqc Nov 13, 2023

acl-cqc Nov 13, 2023

acl-cqc Nov 13, 2023

acl-cqc Nov 13, 2023

acl-cqc Nov 13, 2023

acl-cqc Nov 13, 2023

acl-cqc Nov 13, 2023

acl-cqc Nov 13, 2023

acl-cqc left a comment

	let TypeArg::BoundedNat { n } = arg_values.iter().exactly_one().unwrap() else {
	let Ok(TypeArg::BoundedNat { n }) = arg_values.iter().exactly_one() else {

refactor: use type schemes in extension definitions wherever possible #678

refactor: use type schemes in extension definitions wherever possible #678

Conversation

ss2165 commented Nov 10, 2023 • edited Loading

codecov bot commented Nov 10, 2023 • edited Loading

Codecov Report

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

acl-cqc left a comment

Choose a reason for hiding this comment

ss2165 commented Nov 10, 2023 •

edited

Loading

codecov bot commented Nov 10, 2023 •

edited

Loading