Skip to content

Unify transparent wrapper types e.g. references#26

Merged
ascjones merged 17 commits intomasterfrom
aj-recursive-types
Nov 20, 2020
Merged

Unify transparent wrapper types e.g. references#26
ascjones merged 17 commits intomasterfrom
aj-recursive-types

Conversation

@ascjones
Copy link
Contributor

@ascjones ascjones commented Nov 12, 2020

Background

A bug was discovered by @Robbepop with the following recursive type using Box:

#[derive(TypeInfo)]
pub enum Tree {
	Leaf { value: i32 },
	Node { right: Box<Tree>, left: Box<Tree> },
}

This should result in a self-referential type definition in the type registry, instead it was creating two equivalent definitions: Tree and Box<Tree>.

The cause is that any::TypeId::of<T>() is used to uniquely identify types, and Tree and Box<Tree> are different types and therefore have different TypeIds. This regression was likely introduced in #3, which removed the original separation between a type id and its definition.

"Transparent" wrapper types in SCALE

Box is an example of what in parity-scale-codec is known as a "wrapper" type. Such types are marked by the WrapperTypeEncode trait:

A marker trait for types that wrap other encodable type. Such types should not carry any additional information that would require to be encoded, because the encoding is assumed to be the same as the wrapped type.

For our purposes that means that the SCALE encoded representation of T is the same as that of Box<T>, therefore we consider it a "transparent" wrapper type. Such a type would be erased in the type registry since it has the same definition.

Other types which implement WrapperTypeEncode are

image

Faux specialization

EDIT: I abandoned this approach because it doesn't work with generics e.g. GenericStruct<Box<GenericStruct>>

Therefore what we need is to be able to say that meta_type(T) == metatype(Box<T>) == metadtype(&T) ...). To do this, for any type which implements WrapperTypeEncode we need to identify by any::TypeId::of<T>() (the id of the wrapped type). This way the same type definition is used for the wrapped type and it is erased during type registration.

The initial approach in this PR is adapted from the autoref based specialization. Here is a playground demonstrating the concept.

Adding an associated type to the TypeInfo trait

This is a simple alternative, which adds the associated type MetaType to TypeInfo. This will be set to Self by the derive macro and for most primitive type definitions. For the transparent wrapper types described above it will be set to the underlying wrapped type e.g. the T in Box<T>.

The associated type will then be used when constructing the metatype to get the unique id with core::any::TypeId::of<T::MetaType>(). So for any type T, the equivalent Box<T> will have the same type id, and there will appear only a single type definition in the type registry.

@ascjones ascjones marked this pull request as ready for review November 16, 2020 12:01
@ascjones ascjones changed the title Represent transparent wrapper types as the same type Unify transparent wrapper types e.g. references Nov 16, 2020
@ascjones ascjones requested a review from Robbepop November 16, 2020 12:07
Copy link
Contributor

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I like the simplicity of this solution.
The only thing I'd like is to rename the generic MetaType to something more concrete such as UnderlyingType, IdentityType or just Identity something that really describes its semantics.

@ascjones ascjones merged commit ad3f72a into master Nov 20, 2020
@ascjones ascjones deleted the aj-recursive-types branch November 20, 2020 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants