-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge type id and definition #3
Conversation
# Conflicts: # src/impls.rs
Stack overflow in derive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all looks good to me. I have 2 important additions that we could do as follow-up or directly integrate into this PR since they introducing massively breaking and disruptive changes.
src/impls.rs
Outdated
fn type_info() -> Type { | ||
TypeComposite::new("BTreeMap", Namespace::prelude()) | ||
.type_params(tuple_meta_type![(K, V)]) | ||
.fields(Fields::named().field_of::<[(K, V)]>("elems")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have had a chat with Jaco about conceptualizing encoded structures. E.g. for mappings we define a concept that is that types are mappings if they are a sequence over a type that has a key
and a value
field. So the decoder side (JS API) could then query for such conceptual types and deduce that they are seen as mappings and provide semantics control for them. In this case we could say that BTreeMap
is really just a sequence over a custom type (that we define) that simple has a key
and value
field for K
and V
respectively. The custom type could be:
pub struct KeyValue<K, V> {
key: K,
value: V,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/impls.rs
Outdated
fn type_info() -> Type { | ||
TypeVariant::new("Option", Namespace::prelude()) | ||
.type_params(tuple_meta_type![T]) | ||
.variants( | ||
Variants::with_fields() | ||
.variant_unit("None") | ||
.variant("Some", Fields::unnamed().field_of::<T>()), | ||
) | ||
.into() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing this we still have the problem that our encoding is not generics-aware which it should imo. So we end up having a different type definition for each Option<T>
with a different T
instead of re-using the one generic definition. I have made some thoughts about this and came to the conclusion that it should even be fairly simple to actually implement this with some additional built-ins.
There are actually 2 ways we could implement this:
By introducing two new built-in type classes generic_type
and generic_param
:
{
"strings": {
"Option", # ID 1
"Some", # ID 2
"None", # ID 3
"T", # ID 4
"Foo", # ID 5
"Bar", # ID 6
"Baz", # ID 7
}
"types": {
{ # ID 1
"variant": {
"path" [ 1 ],
"params": [ 4 ]
"variants": [
{
"name": [ 2 ],
"type": [ 2 ],
},
{
"name": [ 3 ],
}
],
}
},
{ # ID 2
# Generic parameter with name `"T"`.
"generic_param": {
"name": 4
}
},
{ # ID 3
"primitive": "u32"
},
{ # ID 4
"primitive": "bool"
},
{ # ID 5
# Forwards to the `Option<u32>` type.
"generic_type": {
"type": 1,
"params": 3,
}
},
{ # ID 6
# Demonstrates usage of a type that contains a Option<u32> type.
"composite": {
"path": [ 5 ],
"fields": [
{
"type": 5
}
],
}
},
{ # ID 7
# Forwards to the `Option<bool>` type.
"generic_type": {
"type": 1,
"params": 4,
}
},
{ # ID 8
# Demonstrates usage of a type that contains a Option<bool> type.
"composite": {
"path": [ 6 ],
"fields": [
{
"type": 7
}
],
}
},
{ # ID 9
# Demonstrates usage of a non-generic type.
"composite": {
"path": [ 7 ],
"field": [
{
"type": 3,
},
{
"type": 4,
},
]
}
}
}
}
By introducing flags for all type
fields:
{
"strings": {
"Option", # ID 1
"Some", # ID 2
"None", # ID 3
"T", # ID 4
"Foo", # ID 5
"Bar", # ID 6
"Baz", # ID 7
}
"types": {
{ # ID 1
"variant": {
"path" [ 1 ],
"params": [ 4 ]
"variants": [
{
"name": [ 2 ],
"type": [
{
# 4 indexes into the string table and must have
# the same index as one of the identifier in `params`.
"parameter": 4
}
],
},
{
"name": [ 3 ],
}
],
}
},
{ # ID 3
"primitive": "u32"
},
{ # ID 4
"primitive": "bool"
},
{ # ID 5
# Demonstrates usage of a type that contains a Option<u32> type.
"composite": {
"path": [ 5 ],
"fields": [
{
# Demonstrates usage of the generic type at index 1 (`Option<T>`)
# with a parameter of 4 where 4 indexes into the type table as `u32`.
"type": {
"generic": {
"type": 1,
"params": 3,
},
}
}
],
}
},
{ # ID 6
# Demonstrates usage of a type that contains a Option<bool> type.
"composite": {
"path": [ 6 ],
"fields": [
{
"type": {
# Demonstrates usage of the generic type at index 1 (`Option<T>`)
# with a parameter of 4 where 4 indexes into the type table as `bool`.
"generic": {
"type": 1,
"params": 4,
},
}
}
],
}
},
{ # ID 7
# Demonstrates usage of a non-generic type.
"composite": {
"path": [ 7 ],
"field": [
{
"type": {
# 3 indexes into the type table and refers to a non
# generic (aka concrete) type, in this case `u32`.
"concrete": 3
},
},
{
"type": {
# 4 indexes into the type table and refers to a non
# generic (aka concrete) type, in this case `bool`.
"concrete": 4
}
},
]
}
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/impls.rs
Outdated
@@ -104,7 +104,8 @@ where | |||
T: Metadata + 'static, | |||
{ | |||
fn type_info() -> Type { | |||
TypeVariant::new("Option", Namespace::prelude()) | |||
TypeVariant::new() | |||
.path(Path::new().ident("Option")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather go for a special constructor so that users have to opt-in to create a Voldemord type which should be a rather rare thing instead of making users opt-in to create a common named type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. we could have 4 constructors
Path::voldemord
Path::prelude
(with prelude namespace)Path::new(name, namespace)
(or so)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And maybe Path::prelude
should actually be hidden from users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All nits fixed. LGTM!
Honor of merging is yours.
Migrated from type-metadata/type-metadata#42
TypeId
toType
and merges inTypeDef
HasTypeDef
trait etc.Composite
(for structs) andVariant
(for enums) for user defined typesField
with optional name. Builders enforce construction of either all named (structs) or all unnamed (tuples).Variant
with optional fields. For "C-like" enums, a builder enforces that all variants have no fields and an optional discriminant. For enums with fields, we reuse theField
type and builders for "struct" and "tuple" variants.Downstream PRs
todo
TypeId
struct?ink!
comments from legacy PR
path
or not Merge type def into custom type id type-metadata/type-metadata#42 (comment) (keep it flat)