Interactions between resource definitions and instances #315
Conversation
ss2165
force-pushed
the
typdef-tag
branch
2 times, most recently
from
2d8206d
to
f6fc034
Compare
src/types/type_param.rs
Outdated
| @@ -67,6 +67,19 @@ impl TypeArg { | |||
| } | |||
| } | |||
|
|
|||
| impl TypeArg { | |||
There was a problem hiding this comment.
This has gone in already (PR needs merging), right?
src/resource.rs
Outdated
| @@ -323,6 +417,42 @@ pub struct TypeDef { | |||
| pub tag: TypeDefTag, | |||
| } | |||
|
|
|||
| /// Concrete instantiations of types and operations defined in resources. | |||
| pub trait CustomConcrete: sealed::SealedConcrete { | |||
There was a problem hiding this comment.
This is fine, but I wonder how much benefit there really is in having a trait to generalize over....things you might get by calling to_custom on one of the only-two-possible instances of TypeParametrisedInternal?
There was a problem hiding this comment.
What happens if you combine this with SealedConcrete i.e. hide the whole thing - can external folk still call its methods ?
src/resource.rs
Outdated
| /// Type arguments. | ||
| fn args(&self) -> &[TypeArg]; | ||
| /// Parent resource. | ||
| fn resource(&self) -> &ResourceId; |
There was a problem hiding this comment.
Note that there is a difference in meaning here - for ops, the resource means, you'll need access to that resource in order to do anything much to the op; as per resource checking. But for types, it doesn't really matter what the resource is - using a type from a resource doesn't mean you need the resource definition, or anything else, really; resource checking does not infer that an operation uses the resource just if it accepts/returns a type defined therein.
So again fine, but if you didn't have the trait, I don't think you need to pay as much attention to there being CustomType::resource() as OpaqueOp::resource()
src/resource.rs
Outdated
| @@ -139,6 +144,72 @@ impl Debug for LowerFunc { | |||
| } | |||
| } | |||
|
|
|||
| /// Sealed trait for shared type parametrised functionality between [`TypeDef`] and [`OpDef`]. | |||
| pub trait TypeParametrisedInternal: sealed::SealedDef { | |||
There was a problem hiding this comment.
This is not Internal - it's pub ! (Maybe you should merge this into SealedDef tho I think it's more useful to keep than CustomConcrete). The use case for parameterising over types <T: TypeParametrisedInternal> is small but I admit non-zero.
Also, the correct spelling is parameterised
src/resource.rs
Outdated
| @@ -139,6 +144,72 @@ impl Debug for LowerFunc { | |||
| } | |||
| } | |||
|
|
|||
| /// Sealed trait for shared type parametrised functionality between [`TypeDef`] and [`OpDef`]. | |||
There was a problem hiding this comment.
| /// Sealed trait for shared type parametrised functionality between [`TypeDef`] and [`OpDef`]. | |
| /// Sealed trait for type-parameterised functionality shared between [`TypeDef`] and [`OpDef`]. |
src/resource.rs
Outdated
| /// | ||
| /// This function will return an error if the provided arguments are not | ||
| /// valid instances of the TypeDef parameters. | ||
| fn to_custom(&self, args: impl Into<Vec<TypeArg>>) -> Result<Self::Concrete, SignatureError>; |
There was a problem hiding this comment.
Let's rename this - how about instantiate? Or to_concrete if you must (we're calling instantiations concrete, which I'm happy with, but we could be consistent)
src/resource.rs
Outdated
| /// | ||
| /// This function will return an error if the type of the instance does not | ||
| /// match the definition. | ||
| fn check_custom(&self, custom: &Self::Concrete) -> Result<(), SignatureError> { |
just use shared trait code method implementations
|
I've made both new traits private and added public methods to each struct for the functionality |
`CustomType::new_simple` was removed in #315 but I don't see why we can't. This version allows specifying type-arguments too. There are uses of `CLASSIC_T` (all in tests) in `src/ops/constant.rs`, `src/resource/type_def.rs`, `src/types/simple/serialize.rs` and a mention in `src/types/simple.rs`. We *could* now create local versions of `CLASSIC_T` for each of those (whereas before there was only one place that we could construct such). But for test code I'm not feeling worried. If you think I should be less lazy then please shout and I will do so ;-)
Currently build on #305 but not strictly dependent.
Bring TypeDef and CustomType to parity with OpDef and OpaqueOp (most importantly by making them point to the resource they belong to).
Then generalise behaviour across the definition structs and the instance structs using sealed traits.
Then implement methods in to generate instances from definitions by providing TypeArgs, and then to check those instances are valid against the definitions.