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

fix: add op's extension to signature check in resolve_opaque_op #1317

Merged
merged 17 commits into from
Jul 17, 2024
Merged

fix: add op's extension to signature check in resolve_opaque_op #1317

Changes from 8 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
4f3cda8
test: Add failing test case for #1315
doug-q Jul 16, 2024
7948129
add comment
doug-q Jul 16, 2024
c520ece
Update hugr-core/src/ops/custom.rs
doug-q Jul 16, 2024
d03d990
revert dup comment
doug-q Jul 16, 2024
143e507
fix: add op's extension to signature check in resolve_opaque_op
doug-q Jul 16, 2024
dd3bc4b
fmt
doug-q Jul 16, 2024
865ecfb
un-should_panic test
doug-q Jul 16, 2024
118152d
Merge branch 'main' into doug/resolve_opaque_op
doug-q Jul 16, 2024
3742ddd
rework
doug-q Jul 17, 2024
db6534b
Revert "fix(hugr-py): ops require their own extensions (#1303)"
doug-q Jul 17, 2024
e119e26
fix with_nonlinear_and_outputs and test_invalid
doug-q Jul 17, 2024
b5008f0
tidy
doug-q Jul 17, 2024
f319896
restore extension_reqs arg for endo
doug-q Jul 17, 2024
0aa0a30
Update hugr-core/src/ops/custom.rs
doug-q Jul 17, 2024
1a5f330
put back improvement from reverted commit
doug-q Jul 17, 2024
a0a0cab
Merge branch 'main' of gh:CQCL-DEV/hugr into doug/resolve_opaque_op
doug-q Jul 17, 2024
13d898c
Merge branch 'doug/resolve_opaque_op' of gh:CQCL-DEV/hugr into doug/r…
doug-q Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions hugr-core/src/ops/custom.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,9 +392,14 @@ pub fn resolve_opaque_op(
r.name().clone(),
));
};
let ext_op =
ExtensionOp::new(def.clone(), opaque.args.clone(), extension_registry).unwrap();
if opaque.signature != ext_op.signature {
let ext_op = ExtensionOp::new(def.clone(), opaque.args.clone(), extension_registry)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

The change from unwrap to ? here necessitates the addition of CustomOpError::SignatureError? Good move :)

// ops always require their own extensions, so we do not force OpaqueOps
Copy link
Contributor

@acl-cqc acl-cqc Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The comment I imagined was "OpaqueOps serialized prior to PR #1226 did not include their own extension. Add it manually for backwards compatibility when deserializing such early Hugr's".

Of course, my version doesn't allow that not all OpaqueOps result from serializing known ExtensionOps - you can also write OpaqueOps out by hand with any extension delta you like. So I guess your comment is fine**, but might be worth adding Especially for backwards compatibility with Hugrs serialized before PR#1226, say.

** indeed better, unless we want to make OpaqueOps automatically include such...

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 have added some comments, see what you think. I think it's better for OpaqueOps not to include their extension in their serialised signature, just for smaller serialisation. I haven't made that change.

// to add their extension to their signature
let opaque_sig = opaque
.signature
.clone()
.with_extension_delta(opaque.extension().clone());
if opaque_sig != ext_op.signature {
return Err(CustomOpError::SignatureMismatch {
extension: opaque.extension.clone(),
op: def.name().clone(),
Expand Down Expand Up @@ -425,6 +430,9 @@ pub enum CustomOpError {
stored: FunctionType,
computed: FunctionType,
},
/// An error in computing the signature of the ExtensionOp
#[error(transparent)]
SignatureError(#[from] SignatureError),
}

#[cfg(test)]
Expand Down Expand Up @@ -459,7 +467,6 @@ mod test {
}

#[test]
#[should_panic] // https://github.com/CQCL/hugr/issues/1315
fn resolve_opaque_op() {
let registry = &INT_OPS_REGISTRY;
let i0 = &INT_TYPES[0];
Expand Down
Loading