Break out primitives into a v1 module and make generic.

[rphmeier]

Let's set up a series of types that are defined as "v1" storage. We can do a migration to v2 storage all in one fell swoop. The runtime then doesn't need to worry about upgrades to one storage type at a time and the node-side code can use a primitives-version runtime API to know exactly which types to provide at any time.

The runtime has a hard time with types that refer to the block hash or number type concretely, as it has to use system::BlockNumber and system::Hash types and can't specialize them.

So we should set up types as generic, typically as

struct SomePrimitive<H = Hash, N = BlockNumber> {
    relay_parent: H,
    relay_parent_number: N,
}

All primitive types should be rewritten this way.
And all code using primitive types should be retargeted towards v1 primitives.
Runtime code should be using explicitly instantiated versions with <T as system::Trait>::BlockNumber and <T as system::Trait>::Hash (usually T::BlockNumber and T::Hash for short), while node-side code will use the default instantiations.

This is considered substantial only because of the amount of code that needs to be changed. However the default instantiations should cut down on that a lot.

When we move onto v2, most types will be exactly the same. Instead of re-declaring them I figure we can just define stuff like this

mod v2 {
    pub use v1::{TypeA, TypeB, TypeC};

    struct TypeD<H = Hash, N = BlockNumber> { ... }
}

And we can also establish some global primitives like BlockNumber and Hash, which should never change, but best to leave those as minimal as possible.

[coriolinus]

In the example, I think you mean

struct SomePrimitive<H = Hash, N = BlockNumber> {
    relay_parent: H,
    relay_parent_number: N,
}

?

Making sure I understand the proposal: this struct is generic on H and N, but we've provided not constraints but defaults. If we have any hash, and any block number, then we can still construct a SomePrimitive { relay_parent: hash, relay_parent_number: num }. This improves the situation for the runtime, which is defined in terms of the trait associated types (?) system::Hash and system::BlockNumber.

If that's all true, then I'm concerned that we're giving up a lot of typesafety to do this: we suggest appropriate default types for H and N, but there are no constraints there. I suspect we're going to see a lot of errors of the form SomePrimitive<Foo, Bar> cannot be compared to SomePrimitive<Foo, Bat>.

We could mitigate, but not eliminate, that risk, by ensuring that for every generic, we specify not only a default but also a bound:

struct SomePrimitive<H: HashTrait = Hash, N: BlockNumberTrait = BlockNumber> {
    relay_parent: H,
    relay_parent_number: N,
}

If we do this, the generic declarations very quickly get gory. I'd suggest developing something like this:

#[generify]
struct SomePrimitive {
    relay_parent: Hash (HashTrait),
    relay_parent_number: BlockNumber (BlockNumberTrait),
}

which desugars to the version above.

[coriolinus]

Bonus: it should be straightforward to write the generify macro such that you can slap it onto any valid struct declaration without change, and get the generic version without trait bounds. I'd have fun writing that macro.

[coriolinus]

There exists the temptation to also propose a generify_all macro which is applied to a module scope which auto-applies generify to all struct and enum definitions contained within. At first glance, that's not useful, because it doesn't enable specifying appropriate trait bounds. At second glance, it might be useful after all, if we specify a table of type to trait mappings within the generify_all invocation. Not 100% sure it would be worth the effort, but it would also be a fun macro to write.

[bkchr]

How can you give up type safety when using generics?

I'm strongly against writing a macro for this, the time it takes to rewrite the code once is way less than writing a proper macro for this stuff.

[bkchr]

And you should also never put trait bounds into the declaration of a type if you can prevent it. The bound should only be added to the impl block.

[coriolinus]

You're right; type safety is the wrong term. Rust will still ensure that things stay safe; it won't compile if you use this wrong. My concern is that someone will accidentally plug the wrong type into an unconstrained generic, and then nothing will compile, in hard-to-debug ways.

Not putting trait bounds into a type declaration is a good idea in the general case of a truly generic type. In this case, we're using generics to work around a limitation of the compiler's ability to notice that our Hash type is the same as the system::Hash; we don't actually want to allow arbitrary types to be used. I therefore think this is an exception to the general rule.

[bkchr]

Code that doesn't compile is not broken and we also should not optimize for such use cases.

Putting trait bounds into the type declaration will not change anything on type resolution of the compiler. It will fail to compile in the moment you try to use a function on the restricted impl, because your type x does not implement hash.

[rphmeier]

In the example, I think you mean

Yes, thanks. Fixed.

We will run into quite a few errors regarding type equality in the runtime, but they are easily fixed - T::BlockNumber and T::Hash are available everywhere.

In my opinion this is very low friction. These errors are caught by the compiler, which is ideal, and are easy to fix. And we don't force node-side code to make everything generic, which would have fallout of thousands of scattered lines.

What are the trait bounds HashTrait and BlockNumberTrait supposed to do? Are these sealed traits where we are going to exploit some flaw in Rust's visibility rules to ensure nobody can implement them wrongly?

Also, I think we'd run into the same set of issues if we bound on traits. Of course what I'd like to do in the runtime is something like this rather than screw around with the primitive type definitions:

trait Trait: system::Trait<BlockNumber=BlockNumber, Hash=Hash> { }

but that actually doesn't work in the frame macro system, and the generics proposed are a workaround for this. The trait bounds you propose would require

trait Trait: system::Trait 
	where <Self as system::Trait>::Hash: HashTrait, <Self as system::Trait>::BlockNumber: BlockNumberTrait
{ }

I haven't tried this, but I am very skeptical that it would work in the runtime.

[bkchr]

decl_module! supports custom trait bounds, but it is very dumb as we replicate the bounds to all generated types.

[rphmeier]

I think doing this with #1276 would make a lot of sense.

w.r.t. type defaults, we should stick with my initial proposed approach. We have done that piecewise to some extent to great success.