paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

Error on implementation of traits with default config points to the macro instead of expected item #14286

Closed gui1117 closed 1 year ago

gui1117 commented 1 year ago

Is there an existing issue?

Experiencing problems? Have you tried our Stack Exchange first?

Description of bug

when implementing a trait with default configs, if there is an error, then it just points to the macro instead of the correct item in the trait implementation> I believe this can be solved by adding some spans or generating the tokens differently>

Steps to reproduce

1- create a faulty implementation like:

diff --git a/frame/examples/default-config/src/lib.rs b/frame/examples/default-config/src/lib.rs
index adb2469e92f..ce79836dad6 100644
--- a/frame/examples/default-config/src/lib.rs
+++ b/frame/examples/default-config/src/lib.rs
@@ -135,7 +135,7 @@ pub mod tests {
                // these items are defined by frame-system as `no_default`, so we must specify them here.
                // Note that these are types that actually rely on the outer runtime, and can't sensibly
                // have an _independent_ default.
-               type BaseCallFilter = frame_support::traits::Everything;
+               type BaseCallFilter = traits::Everything;
                type RuntimeOrigin = RuntimeOrigin;
                type RuntimeCall = RuntimeCall;
                type RuntimeEvent = RuntimeEvent;

2- compile and see the error resulting

error[E0433]: failed to resolve: use of undeclared crate or module `traits`
   --> frame/examples/default-config/src/lib.rs:133:2
    |
133 |       #[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)]
    |       ^----------------------------------------------------------------------------------------------
    |       |
    |  _____in this procedural macro expansion
    | |
134 | |     impl frame_system::Config for Test {
135 | |         // these items are defined by frame-system as `no_default`, so we must specify them here.
136 | |         // Note that these are types that actually rely on the outer runtime, and can't sensibly
...   |
    |
    = note: this error originates in the macro `__import_tokens_attr_derive_impl_inner` which comes from the expansion of the attribute macro `derive_impl` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0433`.
error: could not compile `pallet-default-config-example` due to previous error

instead it should point to traits

bkchr commented 1 year ago

CC @sam0x17

sam0x17 commented 1 year ago

I believe this one is also addressed by https://github.com/sam0x17/macro_magic/issues/3 so should have something for this soon

gui1117 commented 1 year ago

@sam0x17 as you wrote here https://github.com/sam0x17/macro_magic/pull/5#discussion_r1218299839

extra is a string literal used pass through all the macros the tokens of the impl item. because impl item is converted to string and back to token stream the spans are lost.

you pointed the reason for string convertion is to have extra being in any position. I think we can also use {} block to capture any token stream.

so the export_tokens generated macro, instead of matching on

($(::)?$($tokens_var:ident)::*, $(::)?$($callback:ident)::*, $extra:expr) => {

it could match on

($(::)?$($tokens_var:ident)::*, $(::)?$($callback:ident)::*, { $($extra:tt)* }) => {

this way it could take others parameters like:

($(::)?$($tokens_var:ident)::*, $(::)?$($callback:ident)::*, { $($extra1:tt)* }, { $($extra2:tt)* }, $extra3:ident) => {

But I don't know if macro_rules call will keep the spans or not.

gui1117 commented 1 year ago

macro_rules keeps the spans, initial implementation is here https://github.com/sam0x17/macro_magic/pull/6

sam0x17 commented 1 year ago

This specific improvement will be really good for #13950 since we were having span issues there

gui1117 commented 1 year ago

should be fixed in https://github.com/sam0x17/macro_magic/pull/6 fix will come in substrate with next upgrade of macro magic