Deduplicated structs in codegen

[dandanlen]

I have encountered some surprising (to me) behaviour.

I saw #13, it looks relevant but I don't understand why deduplication is the default behaviour.

The following is quite a common pattern in substrate:

pub trait Config {
    type Inner: TypeInfo;
}

#[derive(TypeInfo)]
pub struct A;
#[derive(TypeInfo)]
pub struct B;

impl Config for A {
    type Inner = ();
}

impl Config for B {
    type Inner = u32;
}

#[derive(TypeInfo)]
#[scale_info(skip_type_params(T))]
pub struct X<T: Config> {
    pub inner: T::Inner,
}


#[test]
fn associated_types() {
    let test_mod = Testgen::new()
        .with::<X<A>>()
        .with::<X<B>>()
		.gen_tests_mod(subxt_settings());

    println!("Code: {}", test_mod.to_string());
}

The generated code is:

pub struct X {
    pub inner: ::core::primitive::u32,
}

I would have expected either:

• two types, one with inner: () and another with inner: u32
• single generic struct X<T> { inner: T }

I can't make the codegen do this.

If first noticed the issue when using subxt codegen for Chainflip. For example for testnet:
subxt codegen --url wss://perseverance.chainflip.xyz:443

There is a type TrackedData here that has three entries in the metadata but only one in the generated code. Some similarly-defined types have multiple definitions (for example there is a type VaultRotationStatus, VaultRotationStatus2, VaultRotationStatus3). I tried to debug this and ended up digging down in to this crate.

I'm not sure if this is a bug or if i'm misusing subxt / scale-info / typegen.

Any help would be appreciated.

[jsdw]

I suspect that the reason for this is likely the skip_type_params(T) attribute on X. This means that the type information doesn't contain information about T, and so from the codegen point of view, the type has no generic params (that it can do anything about, at least). skip_type_params has caused issues a few times now, and more generally I think there is some limitation around type params with associated types (ie they aren't well represented in scale-info).

That all said, I'm a bit sketchy on the details and that may not be 100% accurate; we'll need to dig into this a bit more thoroughly to arrive at a proper conclusion!

[dandanlen]

I've tried lots of combinations... When I remove skip_type_params, it just adds a generic phantom member instead:

        pub struct X<_0> {
            pub inner: ::core::primitive::u32,
            #[codec(skip)]
            pub __ignore: ::core::marker::PhantomData<_0>,
        }

[dandanlen]

I think one solution might be to avoid using this kind of type definition with scale_info, and instead use this pattern:

struct X<Inner> { inner: Inner }

type SForT<T> = S<<T as Config>::Inner>

But this seems like a shame - for one, it's ugly! And looking at the metadata, there seems to be enough information to resolve the types correctly.

In fact it seems like the codegen is almost there. The types are represented in the metadata, but they have the same paths and I think the codegen is deduplicating by path. Ie. it can resolve the X { u32 } and the X { () } fine but then it deduplicates the structs at the path level.

[jsdw]

I've tried lots of combinations... When I remove skip_type_params, it just adds a generic phantom member instead

Mmm, I think that this is because the type information then knows that there's a generic type, but doesn't understand that the generic type has an associated type which is then used on inner.

The types are represented in the metadata, but they have the same paths and I think the codegen is deduplicating by path. Ie. it can resolve the X { u32 } and the X { () } fine but then it deduplicates the structs at the path level.

And yeah, from what I recall, every concrete instance of a type is represented in the metadata/type registry, and then when generating the code we have to figure out what to do with duplicates like Foo<u32>(u32) and Foo<u64>(u64), which is where de-duplication comes in (ie recognising that it's a generic and using a generic Foo<T>(T) instead.

I think one solution might be to avoid using this kind of type definition with scale_info

I can see that this would work because you keep the generic (so we know how to de-duplicate it properly) but remove the associated type link (which I think we don't have the information to deal with in scale-info but we are checking!

Anyways, we are looking more into this anyway so we'll have a more concrete response in the next day or so; just wanted to respond sooner!

[tadeohepperle]

I opened a PR that adds some checks to at least avoid that the type override happens. I think it is not really possible for us to generate a single struct X that is generic over the type T: MyTrait, and there is no notion of trait bounds in scale-info. So all associated types have to be concreted at some point and the only way I can see that happen is by generating two distinct types X1 and X2.
This can be achieved by calling scale_typegen::utils::ensure_unique_type_paths on the portable registry before generating rust code. The PR adds checks to error in the cases where we had a silent failure before and points the user towards using ensure_unique_type_paths in this case instead.

[dandanlen]

Thanks for this!

I guess my question then is, why is ensure_unique_type_paths not called by default?

Also - do you know if / when / how this would be propagated to subxt codegen?

[jsdw]

I guess my question then is, why is ensure_unique_type_paths not called by default?

I could go either way on this. I think that right now the reason it's not the default is becasue it messes with the type registry that's handed scale-typegen, and so as it stands, it's a conscious decision from somebody whether to accept that the registry needs to be "messed with" to make it generate valid types, or whether to simply error if we can't generate valid types.

Currently we have a duplicate of that method in Subxt which we use instead (which is another reason we can't just call it automatically right now I think), so we'll probbaly need to break some things apart so that we can reuse this function in both places wihtout pulling in unwanted dependencies. I'll try to have a look in the next weeks, but we're going to be short on manpower for a while and have some higher priority things to focus on so it might take a while to do that bit!

[dandanlen]

Thanks for the clarification, I totally understand. I would volunteer to help out here, but am in a similar position wrt. priorities.

I'm not sure I follow what you mean regarding messing with the registry. The registry in this case clearly contains two X<T> types: X<u32> and X<()>. They each have the same 'path', but the type_def section is different. Currently, the codegen is taking the decision that 'there may only be a single type at any given path', and arbitrarily discards one of them. Surely this is going against what the metadata is saying?

[jsdw]

I'm not sure I follow what you mean regarding messing with the registry. The registry in this case clearly contains two X types: X and X<()>. They each have the same 'path', but the type_def section is different. Currently, the codegen is taking the decision that 'there may only be a single type at any given path', and arbitrarily discards one of them. Surely this is going against what the metadata is saying?

With this PR, I think there will be two possible outcomes:

1. You try to generate code using metadata with eg X { inner: u32 } and X { inner: () }, and get back an error because there are two types that would override eachother. (The error I think is new here)
2. You decide to use ensure_unique_type_paths, and this messes with the metadata (ie it renames the above to X { inner: u32 } and X2 { inner: () } or whatever) in order to make it that we can generate everything properly.

Without the PR, we are silently overwriting one of the types during codegen which is not ideal :)

After this lands, there are some other followup things to think about:

• Should we call ensure_unique_type_paths by default always?
• Can we make sure that subxt uses this same impl and not an outdated one that doesn't do such a good job in some cases.
• More generally, I wonder whether we can have a better internal representation for types-to-be-used-for-codegen than keeping the PortableRegistry thing and modifying it as we do currently. Right now we have two steps which could prob turn into one step to ensure they work properly together:
  • ensure_unique_type_paths which "de duplicates" types that shouldn't overlap (ie renames X to X2)
  • The actual codegen phase which iterates over all of the types and overrides ones with the same paths, assuming that all such types are ok to be overridden. (if I understand it right)

But yeah; we are short on time/manpower and have some other priorities until end of Q2 so this might be picked up again after that!

[dandanlen]

Without the PR, we are silently overwriting one of the types during codegen which is not ideal :)

Yes exactly! What I'm suggesting is that that renaming the types according to some reasonable convention, while still not ideal, is "more ideal" than silently dropping one of them.

I feel like both 1 and 2 are messing with the metadata in some way (1 is dropping a type, 2 is renaming). And the name of the type is not what's important: its 'shape' (and the fact it exists in the first place!) is more important than the name.

Should we call ensure_unique_type_paths by default always?

My opinion is that this is not something the codegen can ensure. If the metadata tells you "there are two distinct types at this path", then the codegen has to accept that :)

[jsdw]

I feel like both 1 and 2 are messing with the metadata in some way (1 is dropping a type, 2 is renaming). And the name of the type is not what's important: its 'shape' (and the fact it exists in the first place!) is more important than the name.

Well, with this PR, 1 will give you an error if there would be an overlap (rather than dropping anything), and 2 will try to rename things to avoid an overlap. Only 2 actually alters the metadata though :)