Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
pkhry opened this issue May 31, 2024 · 4 comments

Comments

@pkhry
Copy link

pkhry commented May 31, 2024

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
Copy link
Contributor

jsdw commented May 31, 2024

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
Copy link
Member

bkchr commented Jun 1, 2024

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
Copy link
Contributor

jsdw commented Jun 3, 2024

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
Copy link
Contributor

jsdw commented Jun 3, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants