Derive does not add bounds for wrapped associated types correctly about scale-info

[dvdplm]

So how about doing what serde did and allow users to customize their trait bounds?

I like this idea, it's pragmatic and solves the problem. It does expose a bigger "API" to users which is always good to avoid. I don't have your deep knowledge/intuition about the possible future pitfalls so if you're convinced parsing the source is eventually going to fail then let's not go there. :)

[dvdplm]

@thiolliere You are correct and my example above is wrong. The code that I was trying to rewrite as a test is this.

To repeat:

• remove the manual impl here
• add #[cfg_attr(feature = "dogfood", derive(scale_info_derive::TypeInfo))] here
• run cargo c --features=dogfood

[dvdplm]

It seems to me it is lacking: Vec<T::Type>, but I may be wrong.

The underlying issue here, I think, is a very good illustration of Robins point about syntactical vs semantical analysis.
The code here defines a type named Type and makes use of a trait with an associated type that is also called Type. The missing bound comes from this check where we end up missing the generic because it has the same Ident as the type we're deriving.
I am sure there's a hacky way around that (using the full PathSegment?) but it's likely just going to be a stop-gap solution until the next corner case pops up.

To see this bug in action, change the name of this associated type from A to Assoc: the derived bounds will be wrong.

@thiolliere It could be that parity-scale-codec has a variant of the same issue somewhere around here, or maybe I'm reading the code wrong?

[Robbepop]

Notice the odd Vec<P::Property>: ::scale_info::TypeInfo + 'static, in the expansion.

This is to be expected since the macro generates (as all standard proc. macros) trait bounds for all fields and their types.

I think the best we can do is to allow users to customize their trait bounds manually since any hackery to solve this automatically will eventually break other scenarios. The same is done for example in serde.

[dvdplm]

I was a bit surprised that Vec<T>: TypeInfo + 'static is not enough, given that we already have impl<T> TypeInfo for Vec<T> where T: TypeInfo + 'static,.

Either way, what do you think about trying a Visit-or on the type of each field to recursively collect all the GenericParam and add the bounds?

[Robbepop]

Either way, what do you think about trying a Visit-or on the type of each field to recursively collect all the GenericParam and add the bounds?

I am always against doing syntactic analysis in proc. macros since you will always break your neck somewhere (mostly in some special cases) and end up with a half-baked solution anyways that will cause confusion to people and make things less transparent. So how about doing what serde did and allow users to customize their trait bounds?

[gui1117]

This issue is a bit related to paritytech/parity-scale-codec#217

I actually think parity-scale-codec bounds too much generics in some context by visiting every types when it is not needed.
But I never took the time to be sure of this.

The derive macro however does not provide the last one, P::Property: TypeInfo + 'static

I actually don't understand why you want to bound this, is it needed? AFAICT the Vec<P::Property>: ::scale_info::TypeInfo + 'static should be enough to ensure that the field properties has type info.

Either way, what do you think about trying a Visit-or on the type of each field to recursively collect all the GenericParam and add the bounds?

My 2 cents is that this macro should do similar bounding than parity-scale-codec.
And I agree that allowing user to define bounds manually similarly to serde can be useful. Maybe we should do this here and in parity-scale-codec.

[dvdplm]

The derive macro however does not provide the last one, P::Property: TypeInfo + 'static

I actually don't understand why you want to bound this, is it needed? AFAICT the Vec<P::Property>: ::scale_info::TypeInfo + 'static should be enough to ensure that the field properties has type info.

rustc seems to think it is needed, but I agree with you that the impl on Vec<T> should be enough. Maybe there's something deeper going on here. :/

And I agree that allowing user to define bounds manually similarly to serde can be useful. Maybe we should do this here and in parity-scale-codec.

We had a call just now about scale-info in general and agreed that adding this is useful. I don't think I'll have time to work on it before middle of next week so if you get it done first I'm always happy to steal code! :)

[gui1117]

EDIT: unrelated thus deleted

[dvdplm]

@thiolliere yeah that's a different bug, #71

[gui1117]

ah ok, in your example when I look at the expanded code I see:

impl <T: Form> crate::TypeInfo for Type<T> where
	Path<T>: crate::TypeInfo + 'static,
	TypeDef<T>: crate::TypeInfo + 'static,
	T: Form + crate::TypeInfo + 'static

It seems to me it is lacking: Vec<T::Type>, but I may be wrong.

Also the issue I linked paritytech/parity-scale-codec#217 is not really related, it is about recursive types.

[gui1117]

I tried locally:

trait Types {
	type Assoc;
}
#[derive(Encode, Decode)]
struct Assoc<T: Types> {
	a: Vec<T::Assoc>,
}

And parity-scale-codec bounds Vec<T::Assoc> correctly, because it checks that the first segment is of name "Assoc".

But

#[derive(Encode, Decode)]
struct Assoc<T: Types> {
	a: Vec<<T>::Assoc>,
}

Fails indeed, we should ensure qself is empty.

I open an issue: paritytech/parity-scale-codec#257