refactor!: Separate Signature from FunctionType #1044
Conversation
acl-cqc
force-pushed
the
row_typevar/separate_signature
branch
from
20346f3 to
9f53329
Compare
hugr/src/extension/op_def.rs
Outdated
| @@ -24,7 +24,7 @@ pub trait CustomSignatureFunc: Send + Sync { | |||
| arg_values: &[TypeArg], | |||
| def: &'o OpDef, | |||
| extension_registry: &ExtensionRegistry, | |||
| ) -> Result<PolyFuncType, SignatureError>; | |||
| ) -> Result<PolyFuncType<true>, SignatureError>; | |||
There was a problem hiding this comment.
@acl-cqc I think this should be false?
There was a problem hiding this comment.
We don't substitute args into the signature in SignatureFunc::compute_signature, but I think we could? I think this would make all uses of PolyFuncType disallow row variables (excepting perhaps funcdecl).
EDIT: no, I am confuesed
There was a problem hiding this comment.
For an OpDef, we allow row variables in the type scheme (i.e. any time it's polymorphic), as long as they are gone by the time we have a node Signature. We could possibly do that final substitution + (fallible-)conversion-to-Signature inside compute_signature but we'll still need a PolyFuncType somewhere even if maybe not as public as this. (We allow custom binary compute-signature functions to return a PolyFuncType<true> if they want.)
There was a problem hiding this comment.
Sorry I was unclear above. Yes I eventually came to understand that row variables should be allowed here.
|
Hey there and thank you for opening this pull request! 👋🏼 It looks like your proposed title indicates a breaking change. If that's the case, |
…kes impl Into, update Dataflow/Ops etc.
acl-cqc
force-pushed
the
row_typevar/separate_signature
branch
from
4e94de4 to
cac4efb
Compare
acl-cqc
force-pushed
the
row_typevar/separate_signature
branch
from
cac4efb to
2b8f0ef
Compare
|
Hey there and thank you for opening this pull request! 👋🏼 It looks like your proposed title indicates a breaking change. If that's the case, |
|
Closing in favour of #1138 |
Goal is to solve #1094 but not even building yet!
Plan?
inputs,outputsandextension_reqsnon-pubin a first PR, we should probably do that anywayFunctionType::new(...)(that although nominally variable-length, don't actually contain any row-vars)?