Skip to content

Conversation

@acl-cqc
Copy link
Contributor

@acl-cqc acl-cqc commented May 27, 2025

closes #2262

The test seems fragile - i.e. somewhat lucky to have reproduced the bug in the first place - so I've included trying a bunch of cases that worked before (every possible entrypoint of the failing Hugr, and "hugr-1.hugr" and "hugr-3.hugr" i.e. the valid ones) as well.

@codecov
Copy link

codecov bot commented May 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.17%. Comparing base (ede8e7b) to head (806c08c).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2263      +/-   ##
==========================================
+ Coverage   82.15%   82.17%   +0.01%     
==========================================
  Files         239      239              
  Lines       42989    43012      +23     
  Branches    38901    38924      +23     
==========================================
+ Hits        35319    35346      +27     
+ Misses       5679     5675       -4     
  Partials     1991     1991              
Flag Coverage Δ
python 85.34% <ø> (ø)
rust 81.84% <100.00%> (+0.02%) ⬆️

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.

@acl-cqc acl-cqc marked this pull request as ready for review May 27, 2025 15:09
@acl-cqc acl-cqc requested a review from a team as a code owner May 27, 2025 15:09
@acl-cqc acl-cqc requested a review from zrho May 27, 2025 15:09
@acl-cqc acl-cqc force-pushed the acl/fix_canonical branch from 14235d9 to c68315e Compare May 30, 2025 16:46
@acl-cqc acl-cqc force-pushed the acl/fix_canonical branch from c68315e to 75cd2a1 Compare May 30, 2025 17:02
Copy link
Contributor

@zrho zrho left a comment

Choose a reason for hiding this comment

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

I think this should work, but I'm slightly confused why it's done this way.

If I understand this correctly, ordered can be seen as a map from new node id to old node id. Therefore the new entrypoint is the node with id target such that ordered[target] == self.entrypoint. You can do one check for this just after the let mut source = \\ ... line, before you follow the permutation chain. Am I missing something?

@acl-cqc
Copy link
Contributor Author

acl-cqc commented Jun 2, 2025

I think this should work, but I'm slightly confused why it's done this way.

If I understand this correctly, ordered can be seen as a map from new node id to old node id. Therefore the new entrypoint is the node with id target such that ordered[target] == self.entrypoint. You can do one check for this just after the let mut source = \\ ... line, before you follow the permutation chain. Am I missing something?

Thanks - yes, you are right; ordered is not mutated (I've refactored to make that clearer). So the old check was kind of correct, if we had done it before following the permutation chain, rather than after. (I had been scratching my head over this a bit before, thank you for figuring it out!)

I've simplified using that, and made some of the logic a bit more explicit using an Option. And to increase confidence I've generalized the test to include more cases...how's that? :)

@acl-cqc acl-cqc requested a review from zrho June 2, 2025 12:33
Copy link
Contributor

@zrho zrho left a comment

Choose a reason for hiding this comment

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

LGTM!

@acl-cqc acl-cqc added this pull request to the merge queue Jun 2, 2025
Merged via the queue into main with commit ead071b Jun 2, 2025
30 checks passed
@acl-cqc acl-cqc deleted the acl/fix_canonical branch June 2, 2025 13:17
@hugrbot hugrbot mentioned this pull request Jun 2, 2025
github-merge-queue bot pushed a commit that referenced this pull request Jun 3, 2025
## 🤖 New release

* `hugr-model`: 0.20.0 -> 0.20.1
* `hugr-core`: 0.20.0 -> 0.20.1 (✓ API compatible changes)
* `hugr-llvm`: 0.20.0 -> 0.20.1 (✓ API compatible changes)
* `hugr-passes`: 0.20.0 -> 0.20.1 (✓ API compatible changes)
* `hugr`: 0.20.0 -> 0.20.1 (✓ API compatible changes)
* `hugr-cli`: 0.20.0 -> 0.20.1 (✓ API compatible changes)

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

## `hugr-model`

<blockquote>

##
[0.20.0](hugr-model-v0.19.0...hugr-model-v0.20.0)
- 2025-05-14

### New Features

- [**breaking**] Mark all Error enums as non_exhaustive
([#2056](#2056))
- [**breaking**] Bump MSRV to 1.85
([#2136](#2136))
- Export and import entrypoints via metadata in `hugr-model`.
([#2172](#2172))
- Define text-model envelope formats
([#2188](#2188))
- Import CFG regions without adding an entry block.
([#2200](#2200))
- Symbol applications can leave out prefixes of wildcards.
([#2201](#2201))
</blockquote>

## `hugr-core`

<blockquote>

##
[0.20.1](hugr-core-v0.20.0...hugr-core-v0.20.1)
- 2025-06-03

### Bug Fixes

- check well-definedness of DFG wires in validate
([#2221](#2221))
- Check for order edges in SiblingSubgraph::from_node
([#2223](#2223))
- Make SumType::Unit(N) equal to SumType::General([(); N])
([#2250](#2250))
- canonicalize_nodes sometimes mangles the entrypoint
([#2263](#2263))

### New Features

- Add PersistentHugr ([#2080](#2080))
- Add `Type::used_extensions`
([#2224](#2224))
- Add boundary edge traversal in SimpleReplacement
([#2231](#2231))
- Add signature map function for DFGs
([#2239](#2239))
- PersistentHugr implements HugrView
([#2202](#2202))
- PersistentHugr Walker API
([#2168](#2168))
- Hugr::store_with_exts and auto-include in serde_as
([#2280](#2280))

### Refactor

- tidies/readability improvements to PersistentHugr
([#2251](#2251))
- Deprecate ValidationError::ExtensionError
([#2281](#2281))

### Testing

- Ignore miri errors in tests involving `assert_snapshot`
([#2261](#2261))
</blockquote>

## `hugr-llvm`

<blockquote>

##
[0.20.1](hugr-llvm-v0.20.0...hugr-llvm-v0.20.1)
- 2025-06-03

### Bug Fixes

- Make SumType::Unit(N) equal to SumType::General([(); N])
([#2250](#2250))

### Testing

- Add exec tests for widen op
([#2043](#2043))
</blockquote>

## `hugr-passes`

<blockquote>

##
[0.20.1](hugr-passes-v0.20.0...hugr-passes-v0.20.1)
- 2025-06-03

### Bug Fixes

- Dataflow analysis produces unsound results on Hugrs with entrypoint
([#2255](#2255))

### New Features

- LocalizeEdges pass ([#2237](#2237))
</blockquote>

## `hugr`

<blockquote>

##
[0.20.1](hugr-v0.20.0...hugr-v0.20.1)
- 2025-06-03

### Bug Fixes

- Dataflow analysis produces unsound results on Hugrs with entrypoint
([#2255](#2255))
- check well-definedness of DFG wires in validate
([#2221](#2221))
- Check for order edges in SiblingSubgraph::from_node
([#2223](#2223))
- Make SumType::Unit(N) equal to SumType::General([(); N])
([#2250](#2250))
- canonicalize_nodes sometimes mangles the entrypoint
([#2263](#2263))

### New Features

- LocalizeEdges pass ([#2237](#2237))
- Add PersistentHugr ([#2080](#2080))
- Add `Type::used_extensions`
([#2224](#2224))
- Add boundary edge traversal in SimpleReplacement
([#2231](#2231))
- Add signature map function for DFGs
([#2239](#2239))
- PersistentHugr implements HugrView
([#2202](#2202))
- PersistentHugr Walker API
([#2168](#2168))
- Hugr::store_with_exts and auto-include in serde_as
([#2280](#2280))

### Refactor

- tidies/readability improvements to PersistentHugr
([#2251](#2251))
- Deprecate ValidationError::ExtensionError
([#2281](#2281))

### Testing

- Ignore miri errors in tests involving `assert_snapshot`
([#2261](#2261))
</blockquote>

## `hugr-cli`

<blockquote>

##
[0.20.1](hugr-cli-v0.20.0...hugr-cli-v0.20.1)
- 2025-06-03

### New Features

- support external subcommands via PATH
([#1343](#1343))
([#2278](#2278))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).
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.

canonicalize_nodes does not always preserve entrypoint

3 participants