Derivation adds unnecessary bounds that can't be satisfied for certain recursive types

[pkhry]

Error encountered

error[E0277]: the trait bound `BrakesOthers<u32>: Decode` is not satisfied
   --> tests/mod.rs:898:14
    |
898 |     pub broken: runtime::BrakesOthers<u32>,
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Decode` is not implemented for `BrakesOthers<u32>`
    |
    = help: the trait `Decode` is implemented for `BrakesOthers<_1>`

error[E0277]: the trait bound `BrakesOthers<u32>: Encode` is not satisfied
   --> tests/mod.rs:896:24
    |
896 | #[derive(DeriveDecode, DeriveEncode, Debug)]
    |                        ^^^^^^^^^^^^ the trait `Encode` is not implemented for `BrakesOthers<u32>`, which is required by `&BrakesOthers<u32>: Encode`
    |
    = help: the trait `Encode` is implemented for `BrakesOthers<_1>`
    = note: required for `&BrakesOthers<u32>` to implement `Encode`
    = note: this error originates in the derive macro `DeriveEncode` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.

Minimal Repro:

#[derive(DeriveDecode, DeriveEncode, Debug)]
pub struct Foo2 {
	pub broken: runtime::BrakesOthers<u32>,
}

pub mod runtime {
	use std::marker::PhantomData;

	use parity_scale_codec_derive::{Decode as DeriveDecode, Encode as DeriveEncode};

	#[derive(DeriveEncode, DeriveDecode, Debug)]
	pub struct BrakesOthers<_1> {
		twtwt: Box<super::runtime::BrakesOthers<_1>>,
		marker: PhantomData<_1>,
	}
}

Produces trait with extra bounds(expansion below, note extra Containter<Self> bounds introduced):

Click me(`Encode` as the example, although same happens for `Decode` too)

// Recursive expansion of DeriveEncode macro
// ==========================================

#[allow(deprecated)]
const _: () = {
	#[automatically_derived]
	impl<_1> ::parity_scale_codec::Encode for BrakesOthers<_1>
	where
		Box<super::runtime::BrakesOthers<_1>>: ::parity_scale_codec::Encode,
		Box<super::runtime::BrakesOthers<_1>>: ::parity_scale_codec::Encode,
		PhantomData<_1>: ::parity_scale_codec::Encode,
		PhantomData<_1>: ::parity_scale_codec::Encode,
	{
		fn size_hint(&self) -> usize {
			0_usize
				.saturating_add(::parity_scale_codec::Encode::size_hint(&self.twtwt))
				.saturating_add(::parity_scale_codec::Encode::size_hint(&self.marker))
		}
		fn encode_to<__CodecOutputEdqy: ::parity_scale_codec::Output + ?::core::marker::Sized>(
			&self,
			__codec_dest_edqy: &mut __CodecOutputEdqy,
		) {
			::parity_scale_codec::Encode::encode_to(&self.twtwt, __codec_dest_edqy);
			::parity_scale_codec::Encode::encode_to(&self.marker, __codec_dest_edqy);
		}
	}
	#[automatically_derived]
	impl<_1> ::parity_scale_codec::EncodeLike for BrakesOthers<_1>
	where
		Box<super::runtime::BrakesOthers<_1>>: ::parity_scale_codec::Encode,
		Box<super::runtime::BrakesOthers<_1>>: ::parity_scale_codec::Encode,
		PhantomData<_1>: ::parity_scale_codec::Encode,
		PhantomData<_1>: ::parity_scale_codec::Encode,
	{
	}
};

changing this line to

  for segment in i.path.segments.iter() {

seems to solve the issue of correctly detecting the recursive ty_ident.

Tests pass after the change, however it might break things not tested in the ecosystem(?)
also see: paritytech/subxt#1603

[jsdw]

To summarize my understanding of the issue here:

Say you have a generic type like Foo<T>. The Encode ( and presumably Decode) derives will (AFAIU) scan through each of the fields of Foo<T> and add where bounds for any generic-containing-types found in the fields.

In order to avoid an issue, it will skip doing this when the field is recursively referencing itself (ie when Foo<T> is mentioned) because that would never resolve. However, if a type references itself recursively using a path (eg super::foo::Foo<T>) and not just ident (Foo<T>), then this skip check fails and we end up spitting out where bounds like Box<Foo<T>>: Encode, which will never work out since they are recursive.

The suggested fix above won't quite work because what if we have two types with the same name in different modules that reference eachother (eg Foo<T> { inner: Box<some::other::path::Foo<T>> }.

Can we simplify the where bounds that we are spitting out though to avoid this issue entirely?

@ascjones you may know something about this :)

[bkchr]

The problem is the algorithm that tries to determine the trait bounds. It tries to be better than what we have in the standard library. However, we don't access to the type system in macros and we need to "guess".

For the situation like you are facing, you can use the attribute codec(dumb_trait_bound). This should fix the compilation for you.

[jsdw]

My feeling is that we should simplify the trait bound stuff in this lib so that it is a bit dumber by default and just applies basic bounds on each generic like the std library, and then let users use #[codec(..)] if they want to be smarter. Trying to be smarter like we are here opens the door for werid bugs like this one, because you have to do stuffl ike matching on idents.

Maybe there were good reasons to make it smarter though which I'm missing, ie maybe we did it to avoid issues where we use generic associated types like struct Foo<T: Config> which I imagine being a bit more common in Substrate?

For the situation like you are facing, you can use the attribute codec(dumb_trait_bound). This should fix the compilation for you.

Yeah good point, we'll maybe do that in our codegen to avoid this issue :)

[jsdw]

Oh!! #[codec(dumb_trait_bound)] is actually an attribute! I thought that you were just meaning to use #[codec(trait_bounds(A: Decode, B: Decode))] or whatever and "dumb_trait_bound" was just a dummy placeholder :D