feat: Type of constants in core Terms.
#2411
Conversation
zrho
changed the title
core Terms.core Terms.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2411 +/- ##
==========================================
- Coverage 82.56% 82.54% -0.02%
==========================================
Files 247 247
Lines 45894 45926 +32
Branches 41528 41545 +17
==========================================
+ Hits 37891 37911 +20
- Misses 5981 5992 +11
- Partials 2022 2023 +1
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:
|
zrho
force-pushed
the
push-kklpqyyxpskx
branch
3 times, most recently
from
3962e45 to
2c329cd
Compare
zrho
force-pushed
the
push-kklpqyyxpskx
branch
3 times, most recently
from
675c2ca to
b9acbff
Compare
There was a problem hiding this comment.
Generally fine, a couple of suggestions - naming?
And, it's a bit of a shocker to have only 35% coverage! Obviously one cannot get very far without any terms of type Term::Const(....) but I think at the least you could add some roundtrip serialization tests and things like that?
| @@ -139,6 +139,9 @@ pub enum Term { | |||
| /// - see [`Term::new_var_use`] | |||
| #[display("{_0}")] | |||
| Variable(TermVar), | |||
|
|
|||
| /// The type of constants for a runtime type. | |||
There was a problem hiding this comment.
| /// The type of constants for a runtime type. | |
| /// The type of constants, i.e. terms that can be turned into runtime values of a given (runtime) type. |
hugr-core/src/types/type_param.rs
Outdated
| @@ -139,6 +139,9 @@ pub enum Term { | |||
| /// - see [`Term::new_var_use`] | |||
| #[display("{_0}")] | |||
| Variable(TermVar), | |||
|
|
|||
| /// The type of constants for a runtime type. | |||
| Const(Box<Type>), | |||
There was a problem hiding this comment.
I reckon this should be called ConstType, paralleling BytesType or RuntimeType. (In hindsight, using a Param suffix, rather than ...Type, would have been more similar to the python!).
There may never be a Const constructor (i.e. in the family of TypeArgs, a term that is/produces a runtime value) - rather, there will likely be a number of different ones, but I think we can still try to be consistent wrt. naming.
| @@ -685,6 +696,7 @@ pub fn check_term_type(term: &Term, type_: &Term) -> Result<(), TermTypeError> { | |||
| (Term::ListType { .. }, Term::StaticType) => Ok(()), | |||
| (Term::TupleType(_), Term::StaticType) => Ok(()), | |||
| (Term::RuntimeType(_), Term::StaticType) => Ok(()), | |||
| (Term::Const(_), Term::StaticType) => Ok(()), | |||
|
|
|||
There was a problem hiding this comment.
I guess we do not want to say that e.g. Term::BoundedNat is a const (e.g. of Runtime type usize) but rather wait for a different term that does that?
There was a problem hiding this comment.
Indeed! A Term::BoundedNat is a compile time value. It can be used as a parameter to a custom term that describes how to produce a particular type of runtime integer from it.
| @@ -101,6 +102,7 @@ def test_tys_sum_str(ty: Type, string: str, repr_str: str): | |||
| "(Linear, Nat(3))", | |||
| ), | |||
| (ListParam(StringParam()), "[String]"), | |||
| (ConstParam(Qubit), "Const(Qubit)"), | |||
There was a problem hiding this comment.
I'm sure this is valid** but it's a bit misleading, we are never going to have a Term that produces a Qubit "constant" are we?
** this is why we said constants have to be Copyable in hugr-core; I still feel it is a shame not to be able to here
There was a problem hiding this comment.
Glad that I randomly picked that example then! Constants are descriptions on how to produce runtime values, so we can certainly have constants that produce qubits. Loading such a constant would mean preparing a qubit in a particular state.
(declare-ctr quantum.const_zero (core.const quantum.qubit))
(declare-ctr quantum.const_one (core.const quantum.qubit))
(declare-ctr quantum.const_plus (core.const quantum.qubit))
(declare-ctr quantum.const_minus (core.const quantum.qubit))Note that the constant is always copyable since it is only the blueprint. The result of loading the constant need not be copyable. This is one of the advantages that are afforded by this system of constants.
|
Yeah, will add some roundtrip tests. I am looking forward to the day when we have custom terms so that PRs like this will become trivial. |
This PR adds a
Term::Constvariant, representing the type of constants for a given runtime type. This corresponds to thecore.consttype inhugr-model.Closes #2304.