Skip to content

Conversation

@lmondada
Copy link
Contributor

@lmondada lmondada commented Jul 28, 2025

This PR's goal is to expose the new LineConvexChecker in portgraph in the hugr API. For details see the original PR: CQCL/portgraph#240

This PR will require a (non-breaking) portgraph release. Currently pinning portgraph main (to be reverted).

There are also two functions whose API could be generalised -- but that would be a breaking change. I've marked them as TODOs in the code.

Breaking change?

Confirm: hiking the patch version of portgraph is considered non-breaking, right?

The current "breaking change" detected by semver-checks is due to struct TopoConvexChecker now being a type synonym for ConvexChecker<'g, Base, pg:TopoConvexChecker<CheckerRegion<'g, Base>>>. It behaves exactly identically from a user's perspective, so I would be in favour of ignoring that warning, unless someone can spot an issue with it?

Alternatively, I could define a newtype struct TopoConvexChecker(ConvexChecker) that delegates all methods to the inner struct. Presumably semver-checks would accept that as non-breaking (but it would be ugly, verbose and would need to be undone before the next breaking release).

@codecov
Copy link

codecov bot commented Jul 28, 2025

Codecov Report

❌ Patch coverage is 77.02703% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.66%. Comparing base (c3370fe) to head (8076a45).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
hugr-core/src/hugr/views/sibling_subgraph.rs 77.02% 34 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2487      +/-   ##
==========================================
- Coverage   82.67%   82.66%   -0.01%     
==========================================
  Files         251      251              
  Lines       46661    46786     +125     
  Branches    42190    42315     +125     
==========================================
+ Hits        38576    38675      +99     
- Misses       6034     6060      +26     
  Partials     2051     2051              
Flag Coverage Δ
python 91.38% <ø> (ø)
rust 81.74% <77.02%> (-0.01%) ⬇️

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 Jul 28, 2025

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 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.42.0/src/lints/struct_missing.ron

Failed in:
struct hugr_core::hugr::views::sibling_subgraph::TopoConvexChecker, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/views/sibling_subgraph.rs:601

@lmondada lmondada force-pushed the lm/lineconvchecker branch 15 times, most recently from 56bf271 to 90abe47 Compare July 31, 2025 08:33
@lmondada lmondada force-pushed the lm/lineconvchecker branch from 90abe47 to 3fe880e Compare August 5, 2025 12:25
@lmondada lmondada force-pushed the lm/lineconvchecker branch from 3fe880e to 86be906 Compare August 5, 2025 12:36
@lmondada lmondada marked this pull request as ready for review August 5, 2025 12:53
@lmondada lmondada requested a review from a team as a code owner August 5, 2025 12:53
@lmondada lmondada requested a review from croyzor August 5, 2025 12:53
@lmondada
Copy link
Contributor Author

lmondada commented Aug 6, 2025

@aborgna-q my thoughts

  • I cannot think of a scenario for which swapping an opaque struct with a type alias would break code
  • even if there is an edge case that we are not thinking of right now, I highly doubt someone will stumble on it given that (afaik) the only user of this is tket

So rather than whether this is breaking, for me the question is whether it's worth the hassle to overwrite semver-checks (because I think it creates issues when generating the changelog, right?)

Whilst we're at it, can you confirm that hiking the portgraph dependency wouldn't be breaking? It would be a patch upgrade on portgraph.

@lmondada
Copy link
Contributor Author

lmondada commented Aug 6, 2025

An alternative is to play it safe (i.e. define TopoConvexChecker not as a type alias but a newtype wrapping ConvexChecker), and then already create the breaking PR that will undo this (as well as generalising the fn signatures currently marked as TODO). Then we'd leave the breaking PR hanging around until the next breaking release?

There shouldn't be too many merge conflicts arising, given that it only touches this one file.

@aborgna-q
Copy link
Collaborator

Relevant semver-checks issue: obi1kenobi/cargo-semver-checks#609 (comment).

I think we're fine overwriting the breaking change marker here.
We should do the overriden patch release soon after merging this to avoid headaches later.

@lmondada
Copy link
Contributor Author

lmondada commented Aug 6, 2025

(nit)

Relevant semver-checks issue: obi1kenobi/cargo-semver-checks#609 (comment).

Our case is slightly different: the type alias and struct are both defined within this crate, so the justification does not apply. However, in our case, the aliased struct isn't equal to the old one (it is equal once some of the generics are fixed).

Do you think I should open an issue in semver-checks?

@aborgna-q
Copy link
Collaborator

Uhm, you're right though it's quite the edge case.
May be worth reporting it if you have some free at some point.

@lmondada lmondada added this pull request to the merge queue Aug 6, 2025
Merged via the queue into main with commit 671a55f Aug 6, 2025
24 of 26 checks passed
@lmondada lmondada deleted the lm/lineconvchecker branch August 6, 2025 11:13
@hugrbot hugrbot mentioned this pull request Aug 5, 2025
github-merge-queue bot pushed a commit that referenced this pull request Aug 6, 2025
## 🤖 New release

* `hugr-model`: 0.22.1 -> 0.23.0 (✓ API compatible changes)
* `hugr-core`: 0.22.1 -> 0.23.0 (⚠ API breaking changes)
* `hugr-llvm`: 0.22.1 -> 0.23.0 (✓ API compatible changes)
* `hugr-passes`: 0.22.1 -> 0.23.0 (✓ API compatible changes)
* `hugr-persistent`: 0.2.1 -> 0.2.2 (✓ API compatible changes)
* `hugr`: 0.22.1 -> 0.23.0 (✓ API compatible changes)
* `hugr-cli`: 0.22.1 -> 0.23.0 (✓ API compatible changes)

### ⚠ `hugr-core` breaking changes

```text
--- 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.41.0/src/lints/struct_missing.ron

Failed in:
  struct hugr_core::hugr::views::sibling_subgraph::TopoConvexChecker, previously in file /tmp/.tmpA42bjc/hugr-core/src/hugr/views/sibling_subgraph.rs:601
```

<details><summary><i><b>Changelog</b></i></summary><p>

## `hugr-model`

<blockquote>

##
[0.23.0](hugr-model-v0.22.1...hugr-model-v0.23.0)
- 2025-08-06

### New Features

- Type of constants in `core` `Term`s.
([#2411](#2411))
</blockquote>

## `hugr-core`

<blockquote>

##
[0.23.0](hugr-core-v0.22.1...hugr-core-v0.23.0)
- 2025-08-06

### New Features

- Type of constants in `core` `Term`s.
([#2411](#2411))
- Support LineConvexChecker
([#2487](#2487))
</blockquote>

## `hugr-llvm`

<blockquote>

##
[0.23.0](hugr-llvm-v0.22.1...hugr-llvm-v0.23.0)
- 2025-08-06

### Bug Fixes

- added public func getter for EmitFuncContext
([#2482](#2482))
- *(hugr-llvm)* Set llvm function linkage based on Visibility hugr node
field ([#2502](#2502))
</blockquote>

## `hugr-passes`

<blockquote>

##
[0.22.1](hugr-passes-v0.22.0...hugr-passes-v0.22.1)
- 2025-07-28

### New Features

- Include copy_discard_array in DelegatingLinearizer::default
([#2479](#2479))
- Inline calls to functions not on cycles in the call graph
([#2450](#2450))
</blockquote>

## `hugr-persistent`

<blockquote>

##
[0.2.0](hugr-persistent-v0.1.0...hugr-persistent-v0.2.0)
- 2025-07-24

### New Features

- [**breaking**] Update portgraph dependency to 0.15
([#2455](#2455))
</blockquote>

## `hugr`

<blockquote>

##
[0.23.0](hugr-v0.22.1...hugr-v0.23.0)
- 2025-08-06

### New Features

- Type of constants in `core` `Term`s.
([#2411](#2411))
- Support LineConvexChecker
([#2487](#2487))
</blockquote>

## `hugr-cli`

<blockquote>

##
[0.23.0](hugr-cli-v0.22.1...hugr-cli-v0.23.0)
- 2025-08-06

### New Features

- *(cli)* Deprecate HugrInputArgs::get_hugr
([#2506](#2506))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).

---------

Co-authored-by: Luca Mondada <[email protected]>
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.

5 participants