Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat!: Make the rewrite errors more useful #1174

Merged
merged 3 commits into from
Jun 7, 2024
Merged

feat!: Make the rewrite errors more useful #1174

merged 3 commits into from
Jun 7, 2024

Conversation

aborgna-q
Copy link
Collaborator

@aborgna-q aborgna-q commented Jun 6, 2024 •
edited
Loading

Adds some context to the errors.

BREAKING CHANGE: Added more attributes to the InvalidReplacement and InvalidSubgraph error enums.

@aborgna-q aborgna-q requested a review from a team as a code owner June 6, 2024 16:19
@aborgna-q aborgna-q requested review from acl-cqc and removed request for a team June 6, 2024 16:19
Base automatically changed from ab/extract_hugr to main June 6, 2024 16:42
@codecov Codecov
Copy link

codecov bot commented Jun 7, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 23.33333% with 23 lines in your changes missing coverage. Please review.

Project coverage is 87.01%. Comparing base (15c3ea9) to head (a7dfad8).

Files Patch % Lines
hugr-core/src/hugr/views/sibling_subgraph.rs 23.33% 23 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1174      +/-   ##
==========================================
- Coverage   87.09%   87.01%   -0.09%     
==========================================
  Files          97       97              
  Lines       19297    19317      +20     
  Branches    18432    18452      +20     
==========================================
+ Hits        16807    16808       +1     
- Misses       1636     1655      +19     
  Partials      854      854
Flag Coverage Δ
rust 86.90% <23.33%> (-0.09%) ⬇️

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.

@aborgna-q aborgna-q changed the title feat: Make the rewrite errors more useful feat!: Make the rewrite errors more useful Jun 7, 2024
acl-cqc
acl-cqc approved these changes Jun 7, 2024
View reviewed changes
Copy link
Contributor

@acl-cqc acl-cqc left a comment

Choose a reason for hiding this comment

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

Thanks, Agustin! A couple of suggestions for improvements but I guess they are kinda problems that existed already

hugr-core/src/hugr/views/sibling_subgraph.rs Outdated
#[error("Replacement graph boundary size mismatch.")]
InvalidSignature,
#[error(
"Replacement graph boundary size mismatch. Expected {expected}, got {}.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Replacement graph boundary size mismatch. Expected {expected}, got {}.",
"Replacement graph boundary type mismatch. Expected {expected}, got {}.",

Or maybe just "graph type" without the "boundary"

Copy link
Contributor

Choose a reason for hiding this comment

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

(And the comment on the previous line too)

hugr-core/src/hugr/views/sibling_subgraph.rs Outdated
}
let Some([rep_input, rep_output]) = replacement.get_io(rep_root) else {
return Err(InvalidReplacement::InvalidDataflowParent);
return Err(InvalidReplacement::InvalidDataflowParent {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a strangely named error. It's not that the optype isn't a DataflowParent - that's guaranteed from the previous check that it's a Dfg. (Odd that we are checking OpTag::Dfg.is_superset(dfg_optype) rather than OpTag::Dfg == dfg_optype but there.)

So, it's that the input/output are missing. Indeed that's what the #[error] message for InvalidDataflowParent says! I'd suggest renaming to, say, MissingInputOutput.

(Also we kind of know what the op is, it's an OpType::DFG - so this just puts the input/output typerows in, well I guess that's fair.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we can replace this error with an expect. It can only trigger with invalid hugrs.

hugr-core/src/hugr/views/sibling_subgraph.rs Outdated
#[error("Not a sibling subgraph.")]
NoSharedParent,
#[error(
"Invalid sibling subgraph. {first_node} has parent {}, but {other_node} has parent {}.",
Copy link
Contributor

Choose a reason for hiding this comment

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

I quite like "not a" sibling subgraph (the definition of such is that they are all siblings!), but ok

@aborgna-q aborgna-q added this pull request to the merge queue Jun 7, 2024
Merged via the queue into main with commit bb75522 Jun 7, 2024
19 of 20 checks passed
@aborgna-q aborgna-q deleted the ab/replacement-errors branch June 7, 2024 15:20
@hugrbot hugrbot mentioned this pull request Jun 7, 2024
Closed
This was referenced Jun 28, 2024
Closed
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jul 1, 2024
@hugrbot @doug-q
chore: release (#1240)
7dadadf 
## 🤖 New release
* `hugr`: 0.5.1 -> 0.6.0
* `hugr-core`: 0.2.0 -> 0.3.0
* `hugr-passes`: 0.2.0 -> 0.3.0
* `hugr-cli`: 0.1.1 -> 0.1.2

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

## `hugr`
<blockquote>

## 0.6.0 (2024-06-28)

### Bug Fixes

- SimpleReplacement panic on multiports
([#1191](#1191))
- Add some validation for const nodes
([#1222](#1222))
- Cfg not validating entry/exit types
([#1229](#1229))
- `extract_hugr` not removing root node ports
([#1239](#1239))

### Documentation

- Fix documentation of `ValidationError::ConstTypeError`
([#1227](#1227))

### Features

- CircuitBuilder::add_constant
([#1168](#1168))
- [**breaking**] Make the rewrite errors more useful
([#1174](#1174))
- [**breaking**] Validate Extensions using hierarchy, ignore
input_extensions, RIP inference
([#1142](#1142))
- [**breaking**] Infer extension deltas for Case, Cfg, Conditional,
DataflowBlock, Dfg, TailLoop
([#1195](#1195))
- Helper functions for requesting inference, use with builder in tests
([#1219](#1219))

### Refactor

- [**breaking**] FunctionBuilder takes impl Into<PolyFuncType>
([#1220](#1220))
- [**breaking**] Remove NodeType and input_extensions
([#1183](#1183))
</blockquote>

## `hugr-core`
<blockquote>

## 0.3.0 (2024-06-28)

### Bug Fixes

- SimpleReplacement panic on multiports
([#1191](#1191))
- Add some validation for const nodes
([#1222](#1222))
- Cfg not validating entry/exit types
([#1229](#1229))
- `extract_hugr` not removing root node ports
([#1239](#1239))

### Documentation

- Fix documentation of `ValidationError::ConstTypeError`
([#1227](#1227))

### Features

- CircuitBuilder::add_constant
([#1168](#1168))
- [**breaking**] Make the rewrite errors more useful
([#1174](#1174))
- [**breaking**] Validate Extensions using hierarchy, ignore
input_extensions, RIP inference
([#1142](#1142))
- [**breaking**] Infer extension deltas for Case, Cfg, Conditional,
DataflowBlock, Dfg, TailLoop
([#1195](#1195))
- Helper functions for requesting inference, use with builder in tests
([#1219](#1219))

### Refactor

- [**breaking**] Remove NodeType and input_extensions
([#1183](#1183))
- [**breaking**] FunctionBuilder takes impl Into<PolyFuncType>
([#1220](#1220))
</blockquote>

## `hugr-passes`
<blockquote>

## 0.3.0 (2024-06-28)

### Features

- [**breaking**] Validate Extensions using hierarchy, ignore
input_extensions, RIP inference
([#1142](#1142))
- Helper functions for requesting inference, use with builder in tests
([#1219](#1219))

</blockquote>

## `hugr-cli`
<blockquote>

## 0.1.1 (2024-06-07)

### Features

- Reexport `clap::Parser` and `clap_verbosity_flag::Level` from hugr_cli
([#1146](#1146))

### Refactor

- Move binary to hugr-cli
([#1134](#1134))
</blockquote>


</p></details>

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

---------

Co-authored-by: Douglas Wilson <douglas.wilson@quantinuum.com>
@hugrbot hugrbot mentioned this pull request Jul 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acl-cqc acl-cqc acl-cqc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aborgna-q @acl-cqc