From bcf62b8030ae744b5c08971e4a0b633d03b8ce4a Mon Sep 17 00:00:00 2001 From: Nikhil Gupta <17176722+gupnik@users.noreply.github.com> Date: Wed, 28 Feb 2024 16:53:58 +0530 Subject: [PATCH 1/7] Computes disambiguation path --- .../frame/examples/default-config/src/lib.rs | 2 +- .../support/procedural/src/derive_impl.rs | 42 +++++++++++++------ 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/substrate/frame/examples/default-config/src/lib.rs b/substrate/frame/examples/default-config/src/lib.rs index cd1653e6c7643..69eca055965eb 100644 --- a/substrate/frame/examples/default-config/src/lib.rs +++ b/substrate/frame/examples/default-config/src/lib.rs @@ -149,7 +149,7 @@ pub mod tests { } ); - #[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)] + #[derive_impl(frame_system::config_preludes::TestDefaultConfig)] impl frame_system::Config for Runtime { // these items are defined by frame-system as `no_default`, so we must specify them here. type Block = Block; diff --git a/substrate/frame/support/procedural/src/derive_impl.rs b/substrate/frame/support/procedural/src/derive_impl.rs index d6d5bf68efd56..5caf66274663d 100644 --- a/substrate/frame/support/procedural/src/derive_impl.rs +++ b/substrate/frame/support/procedural/src/derive_impl.rs @@ -172,6 +172,31 @@ fn combine_impls( final_impl } +fn compute_disambiguation_path( + disambiguation_path: Option, + foreign_impl: ItemImpl, + default_impl_path: Path, +) -> Result { + match (disambiguation_path, foreign_impl.clone().trait_) { + (Some(disambiguation_path), _) => Ok(disambiguation_path), + // In case `as [disambiguation_path]` is not specified in the macro attr, we try to + // infer the disambiguation path from the `foreign_impl_path` and the computed scope. + // Else, we default to the foreign_impl_path. + (None, Some((_, foreign_impl_path, _))) => + if default_impl_path.segments.len() > 1 { + let scope = default_impl_path.segments.first(); + Ok(parse_quote!(#scope :: #foreign_impl_path)) + } else { + Ok(foreign_impl_path) + }, + _ => Err(syn::Error::new( + default_impl_path.span(), + "Impl statement must have a defined type being implemented \ + for a defined type such as `impl A for B`", + )), + } +} + /// Internal implementation behind [`#[derive_impl(..)]`](`macro@crate::derive_impl`). /// /// `default_impl_path`: the module path of the external `impl` statement whose tokens we are @@ -194,18 +219,11 @@ pub fn derive_impl( let foreign_impl = parse2::(foreign_tokens)?; let default_impl_path = parse2::(default_impl_path)?; - // have disambiguation_path default to the item being impl'd in the foreign impl if we - // don't specify an `as [disambiguation_path]` in the macro attr - let disambiguation_path = match (disambiguation_path, foreign_impl.clone().trait_) { - (Some(disambiguation_path), _) => disambiguation_path, - (None, Some((_, foreign_impl_path, _))) => foreign_impl_path, - _ => - return Err(syn::Error::new( - foreign_impl.span(), - "Impl statement must have a defined type being implemented \ - for a defined type such as `impl A for B`", - )), - }; + let disambiguation_path = compute_disambiguation_path( + disambiguation_path, + foreign_impl.clone(), + default_impl_path.clone(), + )?; // generate the combined impl let combined_impl = combine_impls( From 06f022b0af8389c6213a9bc4c68600f283242adf Mon Sep 17 00:00:00 2001 From: Nikhil Gupta <17176722+gupnik@users.noreply.github.com> Date: Wed, 28 Feb 2024 17:28:24 +0530 Subject: [PATCH 2/7] Adds a test --- .../support/procedural/src/derive_impl.rs | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/substrate/frame/support/procedural/src/derive_impl.rs b/substrate/frame/support/procedural/src/derive_impl.rs index 5caf66274663d..fdbc4c85a5ecd 100644 --- a/substrate/frame/support/procedural/src/derive_impl.rs +++ b/substrate/frame/support/procedural/src/derive_impl.rs @@ -275,3 +275,36 @@ fn test_runtime_type_with_doc() { } } } + +#[test] +fn test_disambugation_path() { + let foreign_impl: ItemImpl = parse_quote!(impl SomeTrait for SomeType {}); + let default_impl_path: Path = parse_quote!(SomeScope::SomeType); + + // disambiguation path is specified + let disambiguation_path = compute_disambiguation_path( + Some(parse_quote!(SomeScope::SomePath)), + foreign_impl.clone(), + default_impl_path.clone(), + ); + assert_eq!(disambiguation_path.unwrap(), parse_quote!(SomeScope::SomePath)); + + // disambiguation path is not specified and the default_impl_path has more than one segment + let disambiguation_path = compute_disambiguation_path( + None, + foreign_impl.clone(), + default_impl_path.clone(), + ); + assert_eq!( + disambiguation_path.unwrap(), + parse_quote!(SomeScope::SomeTrait) + ); + + // disambiguation path is not specified and the default_impl_path has only one segment + let disambiguation_path = compute_disambiguation_path( + None, + foreign_impl.clone(), + parse_quote!(SomeType), + ); + assert_eq!(disambiguation_path.unwrap(), parse_quote!(SomeTrait)); +} From 65af25266149cf6b41d1fc61b121ed7a51008f4d Mon Sep 17 00:00:00 2001 From: Nikhil Gupta <17176722+gupnik@users.noreply.github.com> Date: Wed, 28 Feb 2024 17:31:38 +0530 Subject: [PATCH 3/7] Fmt --- .../support/procedural/src/derive_impl.rs | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/substrate/frame/support/procedural/src/derive_impl.rs b/substrate/frame/support/procedural/src/derive_impl.rs index fdbc4c85a5ecd..7ae82d2eecd77 100644 --- a/substrate/frame/support/procedural/src/derive_impl.rs +++ b/substrate/frame/support/procedural/src/derive_impl.rs @@ -290,21 +290,12 @@ fn test_disambugation_path() { assert_eq!(disambiguation_path.unwrap(), parse_quote!(SomeScope::SomePath)); // disambiguation path is not specified and the default_impl_path has more than one segment - let disambiguation_path = compute_disambiguation_path( - None, - foreign_impl.clone(), - default_impl_path.clone(), - ); - assert_eq!( - disambiguation_path.unwrap(), - parse_quote!(SomeScope::SomeTrait) - ); + let disambiguation_path = + compute_disambiguation_path(None, foreign_impl.clone(), default_impl_path.clone()); + assert_eq!(disambiguation_path.unwrap(), parse_quote!(SomeScope::SomeTrait)); // disambiguation path is not specified and the default_impl_path has only one segment - let disambiguation_path = compute_disambiguation_path( - None, - foreign_impl.clone(), - parse_quote!(SomeType), - ); + let disambiguation_path = + compute_disambiguation_path(None, foreign_impl.clone(), parse_quote!(SomeType)); assert_eq!(disambiguation_path.unwrap(), parse_quote!(SomeTrait)); } From 94984794a38003d37d92a9f48898cbb313a88c2b Mon Sep 17 00:00:00 2001 From: Nikhil Gupta <17176722+gupnik@users.noreply.github.com> Date: Thu, 29 Feb 2024 10:56:44 +0530 Subject: [PATCH 4/7] Adds PrDoc --- prdoc/pr_3505.prdoc | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 prdoc/pr_3505.prdoc diff --git a/prdoc/pr_3505.prdoc b/prdoc/pr_3505.prdoc new file mode 100644 index 0000000000000..4b9f738b56d36 --- /dev/null +++ b/prdoc/pr_3505.prdoc @@ -0,0 +1,28 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Removes `as [disambiguation_path]` from the required syntax in `derive_impl` + +doc: + - audience: Runtime Dev + description: | + This PR removes the need to specify `as [disambiguation_path]` for cases where the trait + definition resides within the same scope as default impl path. + + For example, in the following macro invocation + ```rust + #[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)] + impl frame_system::Config for Runtime { + ... + } + ``` + the trait `DefaultConfig` lies within the `frame_system` scope and `TestDefaultConfig` + impls the `DefaultConfig` trait. Using this information, we can compute the + disambiguation path internally, thus removing the need of an explicit specification. + + In cases where the trait lies outside this scope, we would still need to specify it explicitly, + but this should take care of most (if not all) uses of `derive_impl` within FRAME's context. + +crates: + - name: frame-support-procedural + - name: pallet-default-config-example From c60afce82d3cd0fd2dd233328621effc8bf86c98 Mon Sep 17 00:00:00 2001 From: Nikhil Gupta <17176722+gupnik@users.noreply.github.com> Date: Thu, 29 Feb 2024 11:09:09 +0530 Subject: [PATCH 5/7] Updates docs --- substrate/frame/support/procedural/src/lib.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/substrate/frame/support/procedural/src/lib.rs b/substrate/frame/support/procedural/src/lib.rs index 5840377d72259..ed920d6a1da45 100644 --- a/substrate/frame/support/procedural/src/lib.rs +++ b/substrate/frame/support/procedural/src/lib.rs @@ -639,6 +639,11 @@ pub fn storage_alias(attributes: TokenStream, input: TokenStream) -> TokenStream /// default to `A` from the `impl A for B` part of the default impl. This is useful for scenarios /// where all of the relevant types are already in scope via `use` statements. /// +/// In case the `default_impl_path` is scoped to a different module such as +/// `some::path::TestTraitImpl`, the same scope is assumed for the `disambiguation_path`, i.e. +/// `some::A`. This enables the use of `derive_impl` attribute without having to specify the +/// `disambiguation_path` in most (if not all) uses within FRAME's context. +/// /// Conversely, the `default_impl_path` argument is required and cannot be omitted. /// /// Optionally, `no_aggregated_types` can be specified as follows: From 28c58dbaf9f162f62e20c71ce76096e1e9c7b303 Mon Sep 17 00:00:00 2001 From: Nikhil Gupta <17176722+gupnik@users.noreply.github.com> Date: Thu, 29 Feb 2024 11:12:13 +0530 Subject: [PATCH 6/7] Updates PrDoc --- prdoc/pr_3505.prdoc | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/prdoc/pr_3505.prdoc b/prdoc/pr_3505.prdoc index 4b9f738b56d36..08ad909739c7d 100644 --- a/prdoc/pr_3505.prdoc +++ b/prdoc/pr_3505.prdoc @@ -16,9 +16,17 @@ doc: ... } ``` - the trait `DefaultConfig` lies within the `frame_system` scope and `TestDefaultConfig` - impls the `DefaultConfig` trait. Using this information, we can compute the - disambiguation path internally, thus removing the need of an explicit specification. + the trait `DefaultConfig` lies within the `frame_system` scope and `TestDefaultConfig` impls + the `DefaultConfig` trait. + + Using this information, we can compute the disambiguation path internally, thus removing the + need of an explicit specification: + ```rust + #[derive_impl(frame_system::config_preludes::TestDefaultConfig)] + impl frame_system::Config for Runtime { + ... + } + ``` In cases where the trait lies outside this scope, we would still need to specify it explicitly, but this should take care of most (if not all) uses of `derive_impl` within FRAME's context. From fd025f1a88bc09fa04d00d82491afe076efd93a6 Mon Sep 17 00:00:00 2001 From: Nikhil Gupta <17176722+gupnik@users.noreply.github.com> Date: Fri, 1 Mar 2024 18:12:00 +0530 Subject: [PATCH 7/7] Updates docs --- substrate/frame/support/procedural/src/derive_impl.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/substrate/frame/support/procedural/src/derive_impl.rs b/substrate/frame/support/procedural/src/derive_impl.rs index 7ae82d2eecd77..8740ccd401abe 100644 --- a/substrate/frame/support/procedural/src/derive_impl.rs +++ b/substrate/frame/support/procedural/src/derive_impl.rs @@ -172,6 +172,11 @@ fn combine_impls( final_impl } +/// Computes the disambiguation path for the `derive_impl` attribute macro. +/// +/// When specified explicitly using `as [disambiguation_path]` in the macro attr, the +/// disambiguation is used as is. If not, we infer the disambiguation path from the +/// `foreign_impl_path` and the computed scope. fn compute_disambiguation_path( disambiguation_path: Option, foreign_impl: ItemImpl, @@ -179,9 +184,6 @@ fn compute_disambiguation_path( ) -> Result { match (disambiguation_path, foreign_impl.clone().trait_) { (Some(disambiguation_path), _) => Ok(disambiguation_path), - // In case `as [disambiguation_path]` is not specified in the macro attr, we try to - // infer the disambiguation path from the `foreign_impl_path` and the computed scope. - // Else, we default to the foreign_impl_path. (None, Some((_, foreign_impl_path, _))) => if default_impl_path.segments.len() > 1 { let scope = default_impl_path.segments.first();