feat: LowerTypes pass allows replacing extension types and ops #1989
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1989 +/- ##
========================================
Coverage 83.71% 83.71%
========================================
Files 209 210 +1
Lines 39192 39655 +463
Branches 35863 36326 +463
========================================
+ Hits 32808 33199 +391
- Misses 4540 4596 +56
- Partials 1844 1860 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
acl-cqc
force-pushed
the
acl/lower_types
branch
from
836566c to
ad79e1b
Compare
acl-cqc
force-pushed
the
acl/lower_types
branch
from
ad79e1b to
c78d88a
Compare
| impl LowerTypes { | ||
| /// Sets the validation level used before and after the pass is run. | ||
| // Note the self -> Self style is consistent with other passes, but not the other methods here. | ||
| // TODO change the others? But we are planning to drop validation_level in https://github.com/CQCL/hugr/pull/1895 |
There was a problem hiding this comment.
Opinions welcome, if I'm gonna change the others I'll do it in this PR
| /// original op modulo the required type substitutions. (If signatures are incorrect, | ||
| /// it is likely that the wires in the Hugr will be invalid, so this gives an early warning | ||
| /// by instead raising [ChangeTypeError::SignatureMismatch].) | ||
| pub fn check_signatures(&mut self, check_sig: bool) { |
There was a problem hiding this comment.
I'm not really sure about this checking stuff, hence no tests. I could drop it altogether, or convert to a panic (as it was before a0ac6d6). Thoughts?
| if let Some(old_sig) = hugr.get_optype(n).dataflow_signature() { | ||
| let old_sig = old_sig.into_owned(); | ||
| let mut expected_sig = old_sig.clone(); | ||
| expected_sig.transform(self)?; |
There was a problem hiding this comment.
Hence can't use self.check_sig.then_some()
| let mut h = mb.finish_hugr().unwrap(); | ||
|
|
||
| assert!(lower_types(&ext).run(&mut h).unwrap()); | ||
| // We do not update the extension delta |
There was a problem hiding this comment.
If we need to update the delta, probably by rerunning extension "inference" to recompute union over children, we can do this, but would definitely prefer to leave for another PR unless it's urgent
| pub fn lower_parametric_type( | ||
| &mut self, | ||
| src: &TypeDef, | ||
| dest_fn: Box<dyn Fn(&[TypeArg]) -> Type>, // TODO should we return Option<Type> ? |
There was a problem hiding this comment.
Thoughts? Even returning Type doesn't actually force replacing all instances of the parametric type as the Fn could return the same type again (with the same or different typeargs), and returning Option allows better/easier "change checking" (None => return False for unchanged)
| pub fn lower_parametric_op( | ||
| &mut self, | ||
| src: &OpDef, | ||
| dest_fn: Box<dyn Fn(&[TypeArg]) -> Option<OpReplacement>>, // TODO or just OpReplacement? |
There was a problem hiding this comment.
I'm leaning towards keeping the Option here
| pub fn lower_consts( | ||
| &mut self, | ||
| src_ty: &CustomType, | ||
| const_fn: Box<dyn Fn(&OpaqueValue) -> Option<Value>>, // TODO should we return Value?? |
There was a problem hiding this comment.
Leaning towards keeping the Option. Oh, I should doc that None means "no change"
| OpType::OpaqueOp(_) => panic!("OpaqueOp should not be in a Hugr"), | ||
|
|
||
| OpType::AliasDecl(_) | OpType::Module(_) => Ok(false), | ||
| _ => todo!(), |
There was a problem hiding this comment.
This is the catch-all because OpType is #[non_exhaustive], I believe I've handled all current OpTypes and there is no more to do right now
Also added HugrMut::optype_mut.
Some rough edges - naming? (e.g. LowerTypes, ChangeTypeError).
If you replace a Copyable type with a Linear one, you'll either get a SignatureError (if this results in a type like
ArgMustBeCopyable[Qubit]) or an invalid Hugr. (Or you might get lucky, if every outgoing port of the type happens to have one successor!) Addingdup/freewill follow in a later PR.Deprecation of OpDef
lower_funcsandenum LowerFunc, and the previoushugr_passes::lower, to follow in another PR.