Carry over where clauses specified in Config trait to other trait impls

[mustermeiszer]

I experience an issue, where I have a complex trait bound on my pallets trait Config. But not all automatical implementations for Pallet<T> do take over those bounds.

Example

trait Config
where
    <Self::GetItem as SomeOtherTraitA>::ItemA: SomeOtherTraitB<ItemB = u32>,
{
   type GetItem: SomeOtherTrait;
}

As far as I could see in the expansion the following impls do not take over the trait bounds

• frame_support::traits::Hooks
• frame_support::traits::OnFinalize
• frame_support::traits::OnIdle
• frame_support::traits::OnInitialize
• frame_support::traits::OnRuntimeUpgrade
• frame_support::traits::OffchainWorker
• frame_support::traits::IntegrityTest

[KiChjang]

This might not be a good idea after all.

In vanilla Rust, one would not expect to have the where clause on one trait to be carried over onto another trait bound -- it is expected that you rewrite the bound, because it is entirely possible for the other trait to be implemented under different conditions.

Having such a replication of the where clause brings in more weirdness than it is worth IMHO, because now you could not specify a more lenient trait bound on the Config trait without it also affecting the other trait impls.

[bkchr]

@mustermeiszer did you tried to add the where bound to the Hooks implementation? Doesn't this work?

In vanilla Rust, one would not expect to have the where clause on one trait to be carried over onto another trait bound -- it is expected that you rewrite the bound, because it is entirely possible for the other trait to be implemented under different conditions.

I mean I would also expect the same for vanilla Rust. I still think that this is a bug/short coming of the current rustc implementation and that chalk will fix it. The point here being that you can only implement the trait when all the given bounds are matching. No need to carry it around all the time. It should be enough to check this on the implementation side.

[mustermeiszer]

@bkchr I removed it from the trait and just use the bounds on the impls. This works. I was just not sure, how the compiler ensures, I am wireing the types with the right bounds at the runtime level. I guess it will, but wanted to have it on the trait level to be sure.

Anyways, it works like this. I will check if the runtime adheres to the bounds. If you have any insight on this, happy hear already now.

[mustermeiszer]

@KiChjang and @bkchr, I think it would be nice to carry this over. The trait Config only exists in this version and can not exist without adhering to the given traitbounds. Its rather a shortcoming of syntax and compiler than anything else. And although it works to only restrict the impls, it would be better IMO to see the bounds on a pallets config directly in the trait Config rather than in any underlying impl blocks.

[bkchr]

Hmm. I mean I get your frustration. But the current FRAME macros also try to be as close as possible to normal rust. However, we could maybe support that if you don't provide these bounds manually that we add them for you.

[mustermeiszer]

No, can totally understand this. It's just a bit ugly this way, as I need an empty hooks impl and append it everywhere.

Will close this then. It's rather an edge case anyways ^^

[KiChjang]

I mean I would also expect the same for vanilla Rust. I still think that this is a bug/short coming of the current rustc implementation and that chalk will fix it.

Actually, now that I think about it, you're right, because we've already trait bounded the Config trait, and so any time we try to access the GetItem associated type, we should expect its ItemA associated type to be trait bounded by SomeOtherTraitB<ItemB = 32>, because otherwise you would not have been able to implement the Config trait on the concrete type.

It does now sound like it's a Rust compiler issue, so when we finally have chalk, this issue should be resolved.

[mustermeiszer]

I wanted to reopen this, as this occurs more often now on my side. @KiChjang WDYT about this now?

[KiChjang]

I'm not particularly enthusiastic about implementing this, because this is code that eventually will be removed as the compiler improves. We are basically monkey-patching the compiler trait logic by using our own macros for the benefit of saving a few keystrokes now.

I can be convinced otherwise if there is a demonstration of this being a high priority issue that negatively affects pallet development, otherwise I would honestly encourage you to open a compiler issue on this, as I believe that should be the correct abstraction layer to solve this bug.

[mustermeiszer]

But your macros are already doing this. Just not consistently.

What is mainly of annoyance here ist that I need associated types where I could use more concrete trait bounds.

[KiChjang]

How exactly do our macros carry over where clauses from the Config trait to other trait impls?

[mustermeiszer]

See above:

These do NOT take over the bounds:

• frame_support::traits::Hooks
• frame_support::traits::OnFinalize
• frame_support::traits::OnIdle
• frame_support::traits::OnInitialize
• frame_support::traits::OnRuntimeUpgrade
• frame_support::traits::OffchainWorker
• frame_support::traits::IntegrityTest

These DO take over the bounds:

• frame_support::traits::PalletsInfoAccess
• frame_support::traits::StorageInfoTrait
• frame_support::traits::PalletInfoAccess
• frame_support::traits::OnGenesis
• frame_support::traits::GetStorageVersion
• Impl for const metadata

  impl<T: Config> Pallet<T>
          where
              <T::Tokens as Inspect<T::AccountId>>::AssetId: From<T::InvestmentId>,
          {
              #[doc(hidden)]
              pub fn pallet_constants_metadata(
              ) -> frame_support::sp_std::vec::Vec<frame_support::metadata::PalletConstantMetadata>
              {
                  ::alloc::vec::Vec::new()
              }
          }

Lists are probably not exhaustive.

[KiChjang]

The commonality in the traits that you listed as "carrying over" are all because they're generated by the pallet macro. For the others that you mentioned, they aren't traits impls that are generated during macro expansion, and instead are explicitly written out by the developer.

This is why on the very first comment on this issue, I said that it isn't necessarily a good idea, since the macro is respecting exactly what you have written in your trait impl. If you did not specify any where clauses on the trait impls that you had explicitly written, then the macro would not attempt to be smart and add where clauses when none are written.

As such, I would only consider adding the where clauses to trait impls that we generate (i.e. when you skip writing #[pallet::hook] and the macro automatically generates the trait impl for you), but if the user explicitly specifies a trait impl, then it should respect what is written.

[mustermeiszer]

As such, I would only consider adding the where clauses to trait impls that we generate (i.e. when you skip writing #[pallet::hook] and the macro automatically generates the trait impl for you), but if the user explicitly specifies a trait impl, then it should respect what is written.

Ah, now I understand what you meant. Yes, that would be great and I totally agree, that the macro shouldn't change explicit impls made by the dev.

And just to clarify: I did not implement hooks or call, and the macro expansion is NOT automatically adding my where clause. Changing this us what I think would be a valuable adaption.