From cdc8d197e6d487ef54f7e16767b5c1ab041c8b10 Mon Sep 17 00:00:00 2001
From: gupnik <nikhilgupta.iitk@gmail.com>
Date: Sat, 2 Mar 2024 18:19:12 +0530
Subject: [PATCH] Remove `as frame_system::DefaultConfig` from the required
syntax in `derive_impl` (#3505)
Step in https://github.com/paritytech/polkadot-sdk/issues/171
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.
---
prdoc/pr_3505.prdoc | 36 ++++++++++
.../frame/examples/default-config/src/lib.rs | 2 +-
.../support/procedural/src/derive_impl.rs | 68 +++++++++++++++----
substrate/frame/support/procedural/src/lib.rs | 5 ++
4 files changed, 98 insertions(+), 13 deletions(-)
create mode 100644 prdoc/pr_3505.prdoc
diff --git a/prdoc/pr_3505.prdoc b/prdoc/pr_3505.prdoc
new file mode 100644
index 00000000000..08ad909739c
--- /dev/null
+++ b/prdoc/pr_3505.prdoc
@@ -0,0 +1,36 @@
+# 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:
+ ```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.
+
+crates:
+ - name: frame-support-procedural
+ - name: pallet-default-config-example
diff --git a/substrate/frame/examples/default-config/src/lib.rs b/substrate/frame/examples/default-config/src/lib.rs
index cd1653e6c76..69eca055965 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 d6d5bf68efd..8740ccd401a 100644
--- a/substrate/frame/support/procedural/src/derive_impl.rs
+++ b/substrate/frame/support/procedural/src/derive_impl.rs
@@ -172,6 +172,33 @@ 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<Path>,
+ foreign_impl: ItemImpl,
+ default_impl_path: Path,
+) -> Result<Path> {
+ match (disambiguation_path, foreign_impl.clone().trait_) {
+ (Some(disambiguation_path), _) => Ok(disambiguation_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 +221,11 @@ pub fn derive_impl(
let foreign_impl = parse2::<ItemImpl>(foreign_tokens)?;
let default_impl_path = parse2::<Path>(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(
@@ -257,3 +277,27 @@ 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));
+}
diff --git a/substrate/frame/support/procedural/src/lib.rs b/substrate/frame/support/procedural/src/lib.rs
index 5840377d722..ed920d6a1da 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:
--
GitLab