Skip to content

Conversation

@ascjones
Copy link
Contributor

@ascjones ascjones commented Apr 12, 2021

Removes the requirement for HasCompact::Type: TypeInfo bounds I had to add to some impls in substrate, see them removed in this commit paritytech/substrate@7da3bb8.

It does this by just registering T for a compact field in a Compact<T>. This relies on the fact that the blanket impl always has Type = Compact<T>.

@dvdplm maybe you already tried this as part of #53 and decided against it.

A speculative PR to see whether I can remove some `HasCompact::Type: TypeInfo` bounds I had to add to some impls in substrate.
@dvdplm
Copy link
Contributor

dvdplm commented Apr 15, 2021

Yeah so this is what I had in my very first implementation. I was wary about it mostly because I had a poor understanding of what Compact and HasCompact even was; it felt a bit too easy to be correct?

When a type is #[code(compact)] it means that instead of encoding the type we actually encode <the_type as HasCompact>::Type so it becomes a bit philosophical here: do we want TypeInfo for the_type or for the type actually used in the encoding, ::Type?
At the time of #53 I felt pretty convinced that we wanted TypeInfo for the type that would end up being encoded (and then we ran into fun compiler bugs and I didn't think too deeply about it).

If this works for substrate then I'm all for it.

@ascjones
Copy link
Contributor Author

do we want TypeInfo for the_type or for the type actually used in the encoding, ::Type

TypeInfo for ::Type, however we know this is always Compact<T> because of the blanket impl.

I'll leave this here as a draft for now, and come back to it once the substrate PR is in a good state, and maybe explore other ways to get around having those bounds. And consider whether this is philosophically/practically the right approach.

@ascjones ascjones mentioned this pull request May 6, 2021
9 tasks
@ascjones ascjones marked this pull request as ready for review June 6, 2021 16:28
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

LGTM

@ascjones ascjones merged commit 7c18977 into master Jun 14, 2021
@ascjones ascjones deleted the aj-remove-compact-type-bounds branch June 14, 2021 14:09
@ascjones ascjones mentioned this pull request Jun 25, 2021
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.

3 participants