Skip to content

feat!: Generalise array op builers to arbitrary array kinds #2119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 30 commits into from
May 6, 2025

Conversation

mark-koch
Copy link
Contributor

  • Renamed ArrayOpBuilder trait to GenericOpBuilder whose methods are generic over the array implementation
  • Defined new ArrayOpBuilder and VArrayOpBuilder traits that are implemented in terms of the generic version. I moved those into the array and value_array modules respectively, so they don't come into scope at the same time
  • Moved all_array_ops ops function out of the tests module so we can reuse it across other tests (in particular getting rid of the code duplication in hugr-llvm::extension::collections::array::test

BREAKING CHANGE: ArrayOpBuilder was moved from hugr_core::std_extensions::collections::array::op_builder to hugr_core::std_extensions::collections::array

@mark-koch mark-koch requested a review from aborgna-q April 28, 2025 08:14
@mark-koch mark-koch requested a review from a team as a code owner April 28, 2025 08:14
Copy link

codecov bot commented Apr 28, 2025

Codecov Report

Attention: Patch coverage is 72.19731% with 62 lines in your changes missing coverage. Please review.

Please upload report for BASE (feat/arrays@ae02e43). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...core/src/std_extensions/collections/value_array.rs 0.00% 60 Missing ⚠️
hugr-core/src/std_extensions/collections/array.rs 97.33% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##             feat/arrays    #2119   +/-   ##
==============================================
  Coverage               ?   83.32%           
==============================================
  Files                  ?      226           
  Lines                  ?    42847           
  Branches               ?    38900           
==============================================
  Hits                   ?    35703           
  Misses                 ?     5289           
  Partials               ?     1855           
Flag Coverage Δ
python 85.68% <ø> (?)
rust 83.08% <72.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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hugrbot
Copy link
Collaborator

hugrbot commented Apr 28, 2025

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary

--- failure trait_added_supertrait: non-sealed trait added new supertraits ---

Description:
A non-sealed trait added one or more supertraits, which breaks downstream implementations of the trait
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#generic-bounds-tighten
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_added_supertrait.ron

Failed in:
trait hugr_core::std_extensions::collections::array::ArrayOpBuilder gained GenericArrayOpBuilder in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/std_extensions/collections/array.rs:176

--- failure trait_allows_fewer_generic_type_params: trait now allows fewer generic type parameters ---

Description:
A trait now allows fewer generic type parameters than it used to. Uses of this trait that supplied all previously-supported generic types will be broken.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-parameter-no-default
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_allows_fewer_generic_type_params.ron

Failed in:
trait ArrayOpBuilder allows 1 -> 0 generic types in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/std_extensions/collections/array.rs:176

--- failure trait_missing: pub trait removed or renamed ---

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

Failed in:
trait hugr_core::std_extensions::collections::array::op_builder::ArrayOpBuilder, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/std_extensions/collections/array/op_builder.rs:15

--- failure trait_removed_supertrait: supertrait removed or renamed ---

Description:
A supertrait was removed from a trait. Users of the trait can no longer assume it can also be used like its supertrait.
      ref: https://doc.rust-lang.org/reference/items/traits.html#supertraits
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_removed_supertrait.ron

Failed in:
supertrait hugr_core::builder::Dataflow of trait ArrayOpBuilder in file /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/std_extensions/collections/array.rs:176

Base automatically changed from arrays/llvm to feat/arrays April 29, 2025 11:42
Copy link
Collaborator

@aborgna-q aborgna-q left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nice solutions to the duplicated impl problem :)

@mark-koch mark-koch merged commit 7a2fcff into feat/arrays May 6, 2025
25 checks passed
@mark-koch mark-koch deleted the arrays/builder branch May 6, 2025 11:01
github-merge-queue bot pushed a commit that referenced this pull request May 6, 2025
Feature branch for improved array lowering.

* The old `array` type is now called `value_array` and lives in a
separate extension
* The default `array` is now a linear type with additional `clone` and
`discard` operations
* To avoid code duplication, array operations and values are now defined
generically over a new `ArrayKind` trait that is instantiated with
`Array` (the linear one) and `VArray` (the copyable one) to generate the
`array` and `value_array` extensions
* An `array<n, T>` is now lowered to a fat pointer `{ptr, usize}` where
`ptr` is a heap allocated pointer of size at least `n * sizeof(T)` and
the `usize` is an offset pointing to the first element (i.e. the first
element is at `ptr + offset * sizeof(T)`). The rational behind the
additional offset is the `pop_left` operation which bumps the offset
instead of mutating the pointer. This way, we can still free the
original pointer when the array is discarded after a pop.

Tracked PRs:
* #2097 (closes #2066)
* #2100
* #2101
* #2110
* #2112 (closes #2067)
* #2119
* #2125 (closes #2124)

BREAKING CHANGE: `std.collections.array` is now a linear type, even if
the contained elements are copyable. Use the new
`std.collections.value_array` for an array with the previous copyable
semantics.

BREAKING CHANGE: `std.collections.array.get` now also returns the passed
array as an extra output

BREAKING CHANGE: `ArrayOpBuilder` was moved from
`hugr_core::std_extensions::collections::array::op_builder` to
`hugr_core::std_extensions::collections::array`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants