update macro_magic to 0.4.1

[sam0x17]

• fixes derive_impl doesn't seems to handle path correctly. Errors rise when using reexport #14287
• unblocks [FRAME Core] Adds ability to split a pallet across multiple files #13950
• improves [FRAME Core] Default Pallet Config Trait / derive_impl #13454

This brings in the new (breaking) changes from macro_magic 0.4.0 (see https://github.com/sam0x17/macro_magic/releases/tag/v0.4.0 and sam0x17/macro_magic#3)

I would say this release is noteworthy because anyone using derive_impl will now no longer need to use #[use_attr] to import it, but happy to remove that label if we don't think this is worth mentioning.

WIP while I get derive_impl working properly (have to make 1-2 more changes).

Still WIP while I get the expr-based frame_support import working

ready to merge once UI tests issue with CI is resolved

[sam0x17]

@thiolliere feel free to add any issues I forgot to link to in the bullets, I think that's all of them

[bkchr]

I would say this release is noteworthy because anyone using derive_impl will now no longer need to use #[use_attr] to import it, but happy to remove that label if we don't think this is worth mentioning.

You print a warning if someone uses use_attr or? So no need to make this noteworthy. And TBH, I don't think that anyone is already using this right now.

[sam0x17]

cc @gupnik cc @kianenigma

[sam0x17]

note that we have 3 choices:

Choice 1 (I did this):

use pallet::config_preludes::*;
#[derive_impl(TestDefaultConfig as pallet::DefaultConfig)]

Choice 2:

use pallet::config_preludes;
#[derive_impl(config_preludes::TestDefaultConfig as pallet::DefaultConfig)]

Choice 3:

#[derive_impl(pallet::config_preludes::TestDefaultConfig as pallet::DefaultConfig)]

The reason we can't just do:

use pallet::config_preludes::TestDefaultConfig;
#[derive_impl(TestDefaultConfig as pallet::DefaultConfig)]

is that the #[export_tokens] for TestDefaultConfig will not be brought into scope (the * import does bring it in, so (1) provides a slick way of doing this). Note that this worked before because it just so happened the root of the crate the TestDefaultConfig was being imported from is blob-imported, so coincidentally that made this work before.

Conversely when we access by an explicit path, it can get it properly, as shown above

[sam0x17]

Almost forgot, still need to add the generate_crate_access_2018 stuff now that we have support for Expr

[gui1117]

The reason we can't just do:

use pallet::config_preludes::TestDefaultConfig;
#[derive_impl(TestDefaultConfig as pallet::DefaultConfig)]

is that the #[export_tokens] for TestDefaultConfig will not be brought into scope (the * import does bring it in, so (1) provides a slick way of doing this). Note that this worked before because it just so happened the root of the crate the TestDefaultConfig was being imported from is blob-imported, so coincidentally that made this work before.

Conversely when we access by an explicit path, it can get it properly, as shown above

Ah I missed this case, in this case use_attr would be needed, ah sorry.
but I actually wonder now, isn't it possible to just name the macro TestDefaultConfig? or use in both case the same name to refer to the token export? Like in this context it would be named TestDefaultConfigImpl maybe.

EDIT: Otherwise I can bring the use_attr back?
EDIT: I think I'm confused now I need to look carefully again.
cc @sam0x17

should we introduce a use_token_export attribute macro when importing TestDefaultConfig in scope.
or should we make export_token take an identifier as attribute and we generate a macro which export tokens under a name which is not hidden?

[sam0x17]

My thinking is right where yours is -- might be useful to have use_export or something like that

And yes, sorry for the confusion, before there were two things that potentially needed this (the proc macros themselves, and the exports). Now thanks to your recursive stuff, it's just the latter.

[sam0x17]

So right now export_tokens already can take an ident to be used as a disambiguation name (needed for Items that don't have an inherent ident, or (previously) items that would otherwise collide with another item name-wise at the crate level). Now that we don't have to deal with crate-wide collisions, I think it would be reasonable to re-purpose this so that the ident you specify becomes the real name of the item.

The problem with doing this, though, is that then from the perspective of import_tokens_attr and import_tokens_proc we have no way of knowing which kind it is, one with a fake name or one with a real ident, and currently in Rust there is no way to be like "does this symbol exist? if not do this".

Now all of that said, let's pretend for a moment that we don't have to worry about names colliding with the item we are attaching #[export_tokens] to. If that were the case, then we could have #[export_tokens] emit something like this:

#[export_tokens]
struct MyStruct {
    field: usize,
}

=>

mod MyStruct {
    macro_rules! __export_tokens_tt_my_struct_0 {
        // ...
    }
}

Then we would be able to do an import like use my::crate::MyItem and MyItem::__export_tokens_tt_my_item_0 would work without issue.

The problem of course with this approach is the name collides with the original item, so it would only work with the no_emit version of #[export_tokens].

So at the end of the day I agree that the best we can do right now until rust adds the ability to ask if a symbol is defined at the current compilation step is to write some sort of #[use_export] macro

The problem with writing such a macro, though, is we will have to know the COUNTER for that particular export_tokens ident...

So it could be that we require specifying an ident like #[export_tokens(MyIdent)] for #[export_tokens] attributes that we want to be able to use with #[use_export], since for those we won't have the COUNTER problem because they will have an explicit name.

Yeah I think that is the solution. The unfortunate thing is I don't think there is a way to get an intelligent compile-error here pushing people towards adding an ident to their #[export_tokens] other than the built in error which is just going to say "type my::crate::MyItem is a type not an expression" or something like that.

[sam0x17]

Now all of that said, I think it would be wise to merge as is and address this #[use_export] stuff in a future PR, since it is a minor usability thing and the existing implementation we are replacing is significantly more frustrating than what is in this PR

[kianenigma]

I

Given that we are iterating fast on derive_impl, I would be happy to merge this if it already remove the needs for use_attr or other peculiarities.

[sam0x17]

Given that we are iterating fast on derive_impl, I would be happy to merge this if it already remove the needs for use_attr or other peculiarities.

Yeah my plan is to merge this as soon as https://github.com/paritytech/ci_cd/issues/816 / #14389 is in since I can't produce UI test output that the CI likes now without downgrading to the previous stable rust

[sam0x17]

btw word is from the CI people we will have to skip Rust 1.70 because of some ICE issues, so have manually downgraded to 1.69 to get CI to pass and will merge like this

[sam0x17]

bot merge