Avoid silent failure for associated types.

[tadeohepperle]

Fixes #14

In the issue #14 it was raised that the type generation can currently override types with the same type path. This should only be valid if the generics of the two types match up, such that the types are essentially the same.

If you opt for deduplicating the type paths in the PortableRegistry with crate::utils::ensure_unique_type_paths this problem does not occur because then we generate two distinct types.

This PR adds error checks during the type generation when two types with the same path are encountered. Previously we would have just overridden the first type.

I added a test to verify that the guards save us from the erroneous behavior reported in #14

[jsdw]

I wonder whether scale_info::Path<PortableForm> can also be a reference now? Not if you need to alter the path here for de-duping but possible if you've already de-duped by then perhaps?

[jsdw]

Hehe whoops :D

[jsdw]

Is it worth unit testing this function?

[jsdw]

So right now this function Does a shallow compare, and to use an earlier example an issue is eg if we compare:

struct Foo<T = u64> {
    inner: Vec<u64>
}

struct Foo<T = u32> {
    inner: Vec<u32>
}

We would return false because inner would be pointing to a different type ID in either case, and require those types to be deduplicated into eg Foo and Foo2.

We could be smarter and notice that the T in Foo<T> is used as a param in Vec and so we only need to output one type during codegen, but it's more complicated than just recursively calling types_equal_extended_to_params because we need to track usages of the generic type, and I'm not sure if that's possible offhand?

Ie we could note that since T has type ID 32, then any instances of that ID are actually instances of T being used, but that might not be the case (eg struct Bar<T = u32> { a: T, b: u32 }). We could maybe use the typeName if that is equal to T too or something, in conjunction with the type ID? I'm curious if you have any ideas here @tadeohepperle!

All that said, for now it's perfeclty OK if it is more conservative as long as ensure_unique_type_paths then uses this to rename the type paths appropriately. Better to have more duplicated types being spat out in the codegen than to assume types can be merged when in fact they can't!

[niklasad1]

would be nice with some simple unit tests for fn types_equal_extended_to_params as well, if we don't have it already.

[jsdw]

Great to cover this with a test!

[jsdw]

Looks good to me!

We can def work on making the type "de-duplication" stronger, but it's better to be more conservative rather than less; ultiamtely this just emits an error to warn users where ensure_type_path isn't used and duple types exist, and fixes a small bug in the type comparison fn by the looks of it :)

[tadeohepperle]

When merging it with master I just discovered another bug with the current accounting for type params in the types_equal_extended_to_params function: When there are multiple params of the same type, e.g. Foo<A,B> parameterized as Foo<u8,u8> previously we just mapped the type id of each generic to one type param: That meant that the type id of u8 would be mapped only to A or B overwriting the other one when the HashMap is built. I changed this logic to consider collect all possible type params that a type id could refer to. This is caught by the more_than_1_generic_parameters test.

[niklasad1]

👍

[niklasad1]

So this will treat struct B<usize, String, bool> == struct A<(), (), usize> because both contains usize? Or am I missing something?

[niklasad1]

seems like like a.name == b.name is checked before this is called on composite/types should fine I think :)

[tadeohepperle]

No, it is a bit more subtle: This ids_equal function is called for every field of the type: Let's say we have a generic type like this:

struct A<T,U,V> { 
    f1: T,
    f2: U,
    f3: V
}

Let's say the concrete types A<usize, String, bool> and A<(), (), usize> have been registered in the type registry and the ids of the primitive types are: usize: id=0, String: id=1, bool: id=2, (): id=3. Then A<usize, String, bool> is represented as:

NamedComposite<T: id=0, U: id=1, V: id=2> {
    f1: id=0,
    f2: id=1,
    f3: id=2
}

and A<(), (), usize> is represented as

NamedComposite<T: id=3, U: id=3, V: id=0> {
    f1: id=3,
    f2: id=3,
    f3: id=0
}

Now we compare A<usize, String, bool> and A<(), (), usize> to find out if they are basically the same rust type (they should be, both are parameterizations of a generic A<T,U,V>).
Now for field f1, we see that the type ids do not equal (0 != 3). But: a_param_names will say, oh type id=0 actually maps to [T] and b_param_names will say, oh type_id 3 maps to [T, or U].
Now the a_param_names.iter().any(|a_p| b_param_names.contains(a_p)) returns true, saying "okay this field is just the generic parameter T for both registered types, so they are considered the same".
Then we continue with the next field f2 and so on.