feat!: support multi-argument/extern functions in expressions #446
Conversation
|
erichulburd
left a comment
erichulburd
left a comment
There was a problem hiding this comment.
I've a added a couple discussions to resolve before approving.
erichulburd
left a comment
erichulburd
left a comment
There was a problem hiding this comment.
I encourage making the documentation comment above, but I otherwise agree to merge as-is.
I do think we should create three follow on issues (or add a comment where they might otherwise exist):
- Implement argument resolution for
FunctionCallExpression. Essentially, we get theExternSignatureMapfromProgram::try_extern_signature_map_from_pragma_mapand then do something analogous toCall:resolve_arguments. This may not be strictly required for downstream implementations, but it seems most appropriate to keep them in quil-rs. If this turns out to be sub-optimal for some reason or other, we can let the issue languish or close it out with an explanation. - Implement type checking for
CALLandFunctionCallExpression. Will build on the mechanics established in (1). - Support expressions in CALL, MOVE, ADD, and other classical instructions. This otherwise imposes unnecessary restrictions in Quil expressiveness. We should open a PR to get this change into the spec prior to implementation.
| Expression::FunctionCall(FunctionCallExpression { expression, .. }) => { | ||
| expression.get_memory_references() | ||
| } | ||
| Expression::FunctionCall(FunctionCallExpression { arguments, .. }) => arguments |
There was a problem hiding this comment.
I think somewhere we should explicitly reference the quote you pulled from the spec:
Extern CALLs may appear in arithmetic expressions with some restrictions: the extern MUST have a declared function signature, which MUST include a return type, and which MUST NOT include any mutable parameters.
This is why we can simply return Vec<MemoryReference> here (all presumed to be reads), while Call::get_memory_accesses returns a MemoryAccessesResult.
2 participants
This branch adds support for multi-argument functions in Quil expressions, so you can write e.g.
The major weakness of this PR is that I had to manually expand a call to
rigetti_pyo3::py_wrap_union_enum!inquil_py/src/expression.rsand edit the result due to some type errors I couldn't fix. This has proven very difficult to address.One question for this MR, that can block merge or not as we decide, is what to do about type checking for extern function calls in expressions. Typechecking can be more complex now; there's a "FUTURE WORK" comment in
quil_rs::program::type_check::should_be_realwhere we simply punt on checking extern functions. This isn't a regression, as it doesn't affect existing programs, and it's similar to how we don't checkCALLinstructions now (although for those we always succeed, and for extern functions in expressions we always fail). Fixing this would require either (1) ignoring non-real function arguments, or (2) doing actual type-checking on those. Both are entirely doable but maybe not necessary for v1.