Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Macros to use path instead of ident #1474

Merged
merged 34 commits into from
Oct 14, 2023
Merged

Macros to use path instead of ident #1474

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
7102f28
use path instead of ident
juangirini Sep 8, 2023
933fd3f
fix clippy
juangirini Sep 8, 2023
dc43fa5
add path changes to frame_support
juangirini Sep 11, 2023
16668ca
add frame conditional
juangirini Sep 11, 2023
ebd13e3
remove references to the frame-crate
juangirini Sep 11, 2023
270a6b7
update fn description
juangirini Sep 11, 2023
8be65aa
update fn description
juangirini Sep 11, 2023
850a77e
make benchmarks to use generate_crate_access_2018
juangirini Sep 11, 2023
5ae6e07
undo unrelated changes
juangirini Sep 12, 2023
04dc503
undo unrelated changes
juangirini Sep 12, 2023
95444ae
update macro description
juangirini Sep 12, 2023
6a773d0
refactor `generate_crate_access`
juangirini Sep 12, 2023
bc17557
unhardcode codec import
juangirini Sep 13, 2023
6d9887b
bring in hypotetical frame crate
juangirini Sep 14, 2023
35747a1
refactor expected_system_config and add tests
juangirini Sep 15, 2023
330af4a
".git/.scripts/commands/fmt/fmt.sh"
Sep 15, 2023
10bf65c
make zepter happy
juangirini Sep 15, 2023
97bfec4
improve comment
juangirini Sep 21, 2023
6a07bb3
add event_type_bound_system_config_assoc_type test
juangirini Sep 22, 2023
f303276
implement review suggestions
juangirini Oct 2, 2023
963be94
implement review suggestions
juangirini Oct 2, 2023
76de90d
add dummy frame crate for testing
juangirini Oct 3, 2023
32f2f1e
add pallet test example with frame crate
juangirini Oct 4, 2023
877fdc3
wip
juangirini Oct 4, 2023
c89c0c7
make dummy pallet work with frame crate
juangirini Oct 4, 2023
8184085
add dummy pallet with frame crate
juangirini Oct 4, 2023
1ee920b
add min runtime ui test
juangirini Oct 6, 2023
5050625
simplify example pallet
juangirini Oct 12, 2023
55daf79
update comment
juangirini Oct 12, 2023
43ca2c2
zepter improvement
juangirini Oct 12, 2023
2a2b45f
Merge branch 'master' into jg/frame-crate-path
juangirini Oct 12, 2023
b55615b
fix broken test
juangirini Oct 12, 2023
d253b66
update compile test
juangirini Oct 13, 2023
d2009d3
fix todo
juangirini Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 17 additions & 12 deletions substrate/frame/support/procedural/src/benchmark.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
//! Home of the parsing and expansion code for the new pallet benchmarking syntax

use derive_syn_parse::Parse;
use frame_support_procedural_tools::generate_crate_access_2018;
use frame_support_procedural_tools::generate_crate_access_from_frame_or_deps;
use proc_macro::TokenStream;
use proc_macro2::{Ident, Span, TokenStream as TokenStream2};
use quote::{quote, ToTokens};
Expand Down Expand Up @@ -418,7 +418,8 @@ pub fn benchmarks(
true => quote!(T: Config<I>, I: 'static),
};

let krate = generate_crate_access_2018("frame-benchmarking")?;
let krate = generate_crate_access_from_frame_or_deps("frame-benchmarking")?;
let frame_system = generate_crate_access_from_frame_or_deps("frame-system")?;

// benchmark name variables
let benchmark_names_str: Vec<String> = benchmark_names.iter().map(|n| n.to_string()).collect();
Expand Down Expand Up @@ -488,7 +489,7 @@ pub fn benchmarks(
}
#[cfg(any(feature = "runtime-benchmarks", test))]
impl<#type_impl_generics> #krate::Benchmarking for Pallet<#type_use_generics>
where T: frame_system::Config, #where_clause
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
where T: #frame_system::Config, #where_clause
{
fn benchmarks(
extra: bool,
Expand Down Expand Up @@ -535,7 +536,7 @@ pub fn benchmarks(
_ => return Err("Could not find extrinsic.".into()),
};
let mut whitelist = whitelist.to_vec();
let whitelisted_caller_key = <frame_system::Account<
let whitelisted_caller_key = <#frame_system::Account<
T,
> as #krate::__private::storage::StorageMap<_, _,>>::hashed_key_for(
#krate::whitelisted_caller::<T::AccountId>()
Expand Down Expand Up @@ -571,8 +572,8 @@ pub fn benchmarks(
>::instance(&selected_benchmark, c, verify)?;

// Set the block number to at least 1 so events are deposited.
if #krate::__private::Zero::is_zero(&frame_system::Pallet::<T>::block_number()) {
frame_system::Pallet::<T>::set_block_number(1u32.into());
if #krate::__private::Zero::is_zero(&#frame_system::Pallet::<T>::block_number()) {
#frame_system::Pallet::<T>::set_block_number(1u32.into());
}

// Commit the externalities to the database, flushing the DB cache.
Expand Down Expand Up @@ -654,7 +655,7 @@ pub fn benchmarks(
}

#[cfg(test)]
impl<#type_impl_generics> Pallet<#type_use_generics> where T: ::frame_system::Config, #where_clause {
impl<#type_impl_generics> Pallet<#type_use_generics> where T: #frame_system::Config, #where_clause {
/// Test a particular benchmark by name.
///
/// This isn't called `test_benchmark_by_name` just in case some end-user eventually
Expand Down Expand Up @@ -719,10 +720,14 @@ fn expand_benchmark(
where_clause: TokenStream2,
) -> TokenStream2 {
// set up variables needed during quoting
let krate = match generate_crate_access_2018("frame-benchmarking") {
let krate = match generate_crate_access_from_frame_or_deps("frame-benchmarking") {
Ok(ident) => ident,
Err(err) => return err.to_compile_error().into(),
};
let frame_system = match generate_crate_access_from_frame_or_deps("frame-system") {
Ok(path) => path,
Err(err) => return err.to_compile_error().into(),
};
let codec = quote!(#krate::__private::codec);
let traits = quote!(#krate::__private::traits);
let setup_stmts = benchmark_def.setup_stmts;
Expand Down Expand Up @@ -762,7 +767,7 @@ fn expand_benchmark(
Expr::Cast(t) => {
let ty = t.ty.clone();
quote! {
<<T as frame_system::Config>::RuntimeOrigin as From<#ty>>::from(#origin);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all of this seem to have been hardcoded 🙈 well, not like anything broke because of it, but indeed not a good practice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless if was intentional? but I can't see a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see a reason either, and we need this unhardcoded moving forward to work with the frame umbrella crate

<<T as #frame_system::Config>::RuntimeOrigin as From<#ty>>::from(#origin);
}
},
_ => quote! {
Expand Down Expand Up @@ -932,7 +937,7 @@ fn expand_benchmark(
}

#[cfg(test)]
impl<#type_impl_generics> Pallet<#type_use_generics> where T: ::frame_system::Config, #where_clause {
impl<#type_impl_generics> Pallet<#type_use_generics> where T: #frame_system::Config, #where_clause {
#[allow(unused)]
fn #test_ident() -> Result<(), #krate::BenchmarkError> {
let selected_benchmark = SelectedBenchmark::#name;
Expand All @@ -951,8 +956,8 @@ fn expand_benchmark(
>::instance(&selected_benchmark, &c, true)?;

// Set the block number to at least 1 so events are deposited.
if #krate::__private::Zero::is_zero(&frame_system::Pallet::<T>::block_number()) {
frame_system::Pallet::<T>::set_block_number(1u32.into());
if #krate::__private::Zero::is_zero(&#frame_system::Pallet::<T>::block_number()) {
#frame_system::Pallet::<T>::set_block_number(1u32.into());
}

// Run execution + verification
Expand Down
14 changes: 7 additions & 7 deletions substrate/frame/support/procedural/src/construct_runtime/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ mod parse;

use cfg_expr::Predicate;
use frame_support_procedural_tools::{
generate_crate_access, generate_crate_access_2018, generate_hidden_includes,
generate_crate_access, generate_crate_access_from_frame_or_deps, generate_hidden_includes,
};
use itertools::Itertools;
use parse::{ExplicitRuntimeDeclaration, ImplicitRuntimeDeclaration, Pallet, RuntimeDeclaration};
Expand Down Expand Up @@ -271,7 +271,7 @@ fn construct_runtime_implicit_to_explicit(
input: TokenStream2,
definition: ImplicitRuntimeDeclaration,
) -> Result<TokenStream2> {
let frame_support = generate_crate_access_2018("frame-support")?;
let frame_support = generate_crate_access_from_frame_or_deps("frame-support")?;
let mut expansion = quote::quote!(
#frame_support::construct_runtime! { #input }
);
Expand All @@ -282,7 +282,7 @@ fn construct_runtime_implicit_to_explicit(
expansion = quote::quote!(
#frame_support::__private::tt_call! {
macro = [{ #pallet_path::tt_default_parts }]
frame_support = [{ #frame_support }]
your_tt_return = [{ #frame_support::__private::tt_return }]
~~> #frame_support::match_and_insert! {
target = [{ #expansion }]
pattern = [{ #pallet_name: #pallet_path #pallet_instance }]
Expand All @@ -307,7 +307,7 @@ fn construct_runtime_explicit_to_explicit_expanded(
input: TokenStream2,
definition: ExplicitRuntimeDeclaration,
) -> Result<TokenStream2> {
let frame_support = generate_crate_access_2018("frame-support")?;
let frame_support = generate_crate_access_from_frame_or_deps("frame-support")?;
let mut expansion = quote::quote!(
#frame_support::construct_runtime! { #input }
);
Expand All @@ -318,7 +318,7 @@ fn construct_runtime_explicit_to_explicit_expanded(
expansion = quote::quote!(
#frame_support::__private::tt_call! {
macro = [{ #pallet_path::tt_extra_parts }]
frame_support = [{ #frame_support }]
your_tt_return = [{ #frame_support::__private::tt_return }]
~~> #frame_support::match_and_insert! {
target = [{ #expansion }]
pattern = [{ #pallet_name: #pallet_path #pallet_instance }]
Expand Down Expand Up @@ -371,7 +371,7 @@ fn construct_runtime_final_expansion(
let scrate = generate_crate_access(hidden_crate_name, "frame-support");
let scrate_decl = generate_hidden_includes(hidden_crate_name, "frame-support");

let frame_system = generate_crate_access_2018("frame-system")?;
let frame_system = generate_crate_access_from_frame_or_deps("frame-system")?;
let block = quote!(<#name as #frame_system::Config>::Block);
let unchecked_extrinsic = quote!(<#block as #scrate::sp_runtime::traits::Block>::Extrinsic);

Expand Down Expand Up @@ -783,7 +783,7 @@ fn decl_static_assertions(
quote! {
#scrate::__private::tt_call! {
macro = [{ #path::tt_error_token }]
frame_support = [{ #scrate }]
your_tt_return = [{ #scrate::__private::tt_return }]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall this was one of the BIG BIG fixes in this PR when I was working on it.

Try this for yourself: if you are familiar with the tt_call pattern a bit, instead of passing in the your_tt_return, pass frame_support as it was in the past to the caller. Then have the caller call #frame_support::__private::tt_return instead of plain your_tt_return. These two seem equivalent, but the former never worked.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, hearing what I just said, in the scope of this PR, it is pretty unclear why we are doing this. I recall my working example was minimal-runtime. Would be good if you also bring back some sort of example here to demonstrate that this change is actually needed. Ideally, this example will just a test in frame-support and not the frame crate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two seem equivalent, but the former never worked.

Yes, that's what looks at a glance... did you find out what's the reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good if you also bring back some sort of example here to demonstrate that this change is actually needed

@kianenigma Probably the best example is by using the frame-crate itself, which makes me reconsider if bringing this change here was a good idea and instead bring it in the 'main' frame umbrella PR. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I highly suspect that it didn't work before because the "receiver" didn't expect to see any "arguments" named your_tt_return and have been coded to only recognize frame_support as an "argument". If you take a look at tt_default_parts.rs, you'll see what I mean.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If my memory is good, the former didn't work because there was another error when giving a path instead of a single ident:
Like code was something like this:

				frame_support = [{ $($frame_support:ident)::* }]
			} => {
				$($frame_support::)*__private::tt_return! {

but it should have been like this for path to work

				frame_support = [{ $($frame_support:ident)::* }]
			} => {
// see the semicolon is after the parentheses here
				$($frame_support)::*__private::tt_return! {

but when you implemented with your_tt_return you fixed this kind of error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

macro_magic might make this easier to reason about perhaps 😇

Copy link
Contributor

@sam0x17 sam0x17 Oct 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the specific workaround for passing paths to these btw is to do an interpolation of idents segmented by :: and with an optional leading ::. For some reason that just works positionally where path doesn't,

see: https://github.com/sam0x17/macro_magic/blob/main/core/src/lib.rs#L511

~~> #scrate::assert_error_encoded_size! {
path = [{ #path }]
runtime = [{ #runtime }]
Expand Down
4 changes: 2 additions & 2 deletions substrate/frame/support/procedural/src/crate_version.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
//! Implementation of macros related to crate versioning.

use super::get_cargo_env_var;
use frame_support_procedural_tools::generate_crate_access_2018;
use frame_support_procedural_tools::generate_crate_access_from_frame_or_deps;
use proc_macro2::{Span, TokenStream};
use syn::{Error, Result};

Expand All @@ -42,7 +42,7 @@ pub fn crate_to_crate_version(input: proc_macro::TokenStream) -> Result<TokenStr
let patch_version = get_cargo_env_var::<u8>("CARGO_PKG_VERSION_PATCH")
.map_err(|_| create_error("Patch version needs to fit into `u8`"))?;

let crate_ = generate_crate_access_2018("frame-support")?;
let crate_ = generate_crate_access_from_frame_or_deps("frame-support")?;

Ok(quote::quote! {
#crate_::traits::CrateVersion {
Expand Down
7 changes: 6 additions & 1 deletion substrate/frame/support/procedural/src/key_prefix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use frame_support_procedural_tools::generate_crate_access_from_frame_or_deps;
use proc_macro2::{Span, TokenStream};
use quote::{format_ident, quote, ToTokens};
use syn::{Ident, Result};
Expand All @@ -27,6 +28,7 @@ pub fn impl_key_prefix_for_tuples(input: proc_macro::TokenStream) -> Result<Toke
}

let mut all_trait_impls = TokenStream::new();
let frame_support = generate_crate_access_from_frame_or_deps("frame-support")?;

for i in 2..=MAX_IDENTS {
let current_tuple = (0..i)
Expand Down Expand Up @@ -64,7 +66,10 @@ pub fn impl_key_prefix_for_tuples(input: proc_macro::TokenStream) -> Result<Toke
#(#hashers: ReversibleStorageHasher,)*
#(#kargs: EncodeLike<#prefixes>),*
> HasReversibleKeyPrefix<( #( #kargs, )* )> for ( #( Key<#hashers, #current_tuple>, )* ) {
fn decode_partial_key(key_material: &[u8]) -> Result<Self::Suffix, codec::Error> {
fn decode_partial_key(key_material: &[u8]) -> Result<
Self::Suffix,
#frame_support::__private::codec::Error,
> {
<#suffix_keygen>::decode_final_key(key_material).map(|k| k.0)
}
}
Expand Down
10 changes: 5 additions & 5 deletions substrate/frame/support/procedural/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ mod storage_alias;
mod transactional;
mod tt_macro;

use frame_support_procedural_tools::generate_crate_access_2018;
use frame_support_procedural_tools::generate_crate_access_from_frame_or_deps;
use macro_magic::import_tokens_attr;
use proc_macro::TokenStream;
use quote::{quote, ToTokens};
Expand Down Expand Up @@ -754,9 +754,9 @@ pub fn storage_alias(attributes: TokenStream, input: TokenStream) -> TokenStream
#[import_tokens_attr {
format!(
"{}::macro_magic",
match generate_crate_access_2018("frame-support") {
match generate_crate_access_from_frame_or_deps("frame-support") {
Ok(path) => Ok(path),
Err(_) => generate_crate_access_2018("frame"),
Err(_) => generate_crate_access_from_frame_or_deps("frame"),
}
.expect("Failed to find either `frame-support` or `frame` in `Cargo.toml` dependencies.")
.to_token_stream()
Expand Down Expand Up @@ -1647,9 +1647,9 @@ pub fn pallet_section(attr: TokenStream, tokens: TokenStream) -> TokenStream {
#[import_tokens_attr {
format!(
"{}::macro_magic",
match generate_crate_access_2018("frame-support") {
match generate_crate_access_from_frame_or_deps("frame-support") {
Ok(path) => Ok(path),
Err(_) => generate_crate_access_2018("frame"),
Err(_) => generate_crate_access_from_frame_or_deps("frame"),
}
.expect("Failed to find either `frame-support` or `frame` in `Cargo.toml` dependencies.")
.to_token_stream()
Expand Down
8 changes: 4 additions & 4 deletions substrate/frame/support/procedural/src/pallet/expand/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ pub fn expand_error(def: &mut Def) -> proc_macro2::TokenStream {
macro_rules! #error_token_unique_id {
{
$caller:tt
frame_support = [{ $($frame_support:ident)::* }]
your_tt_return = [{ $my_tt_return:path }]
} => {
$($frame_support::)*__private::tt_return! {
$my_tt_return! {
$caller
}
};
Expand Down Expand Up @@ -170,9 +170,9 @@ pub fn expand_error(def: &mut Def) -> proc_macro2::TokenStream {
macro_rules! #error_token_unique_id {
{
$caller:tt
frame_support = [{ $($frame_support:ident)::* }]
your_tt_return = [{ $my_tt_return:path }]
} => {
$($frame_support::)*__private::tt_return! {
$my_tt_return! {
$caller
error = [{ #error_ident }]
}
Expand Down
3 changes: 2 additions & 1 deletion substrate/frame/support/procedural/src/pallet/expand/genesis_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

use crate::{pallet::Def, COUNTER};
use frame_support_procedural_tools::get_doc_literals;
use quote::ToTokens;
use syn::{spanned::Spanned, Ident};

///
Expand Down Expand Up @@ -79,7 +80,7 @@ pub fn expand_genesis_config(def: &mut Def) -> proc_macro2::TokenStream {
let genesis_config_item =
&mut def.item.content.as_mut().expect("Checked by def parser").1[genesis_config.index];

let serde_crate = format!("{}::__private::serde", frame_support);
let serde_crate = format!("{}::__private::serde", frame_support.to_token_stream());

match genesis_config_item {
syn::Item::Enum(syn::ItemEnum { attrs, .. }) |
Expand Down
16 changes: 7 additions & 9 deletions substrate/frame/support/procedural/src/pallet/expand/tt_default_parts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,18 +85,16 @@ pub fn expand_tt_default_parts(def: &mut Def) -> proc_macro2::TokenStream {
// wrapped inside of braces and finally prepended with double colons, to the caller inside
// of a key named `tokens`.
//
// We need to accept a frame_support argument here, because this macro gets expanded on the
// crate that called the `construct_runtime!` macro, and said crate may have renamed
// frame-support, and so we need to pass in the frame-support path that said crate
// recognizes.
// We need to accept a path argument here, because this macro gets expanded on the
// crate that called the `construct_runtime!` macro, and the actual path is unknown.
#[macro_export]
#[doc(hidden)]
macro_rules! #default_parts_unique_id {
{
$caller:tt
frame_support = [{ $($frame_support:ident)::* }]
your_tt_return = [{ $my_tt_return:path }]
} => {
$($frame_support)*::__private::tt_return! {
$my_tt_return! {
$caller
tokens = [{
expanded::{
Expand All @@ -112,7 +110,7 @@ pub fn expand_tt_default_parts(def: &mut Def) -> proc_macro2::TokenStream {
pub use #default_parts_unique_id as tt_default_parts;


// This macro is similar to the `tt_default_parts!`. It expands the pallets thare are declared
// This macro is similar to the `tt_default_parts!`. It expands the pallets that are declared
// explicitly (`System: frame_system::{Pallet, Call}`) with extra parts.
//
// For example, after expansion an explicit pallet would look like:
Expand All @@ -124,9 +122,9 @@ pub fn expand_tt_default_parts(def: &mut Def) -> proc_macro2::TokenStream {
macro_rules! #extra_parts_unique_id {
{
$caller:tt
frame_support = [{ $($frame_support:ident)::* }]
your_tt_return = [{ $my_tt_return:path }]
} => {
$($frame_support)*::__private::tt_return! {
$my_tt_return! {
$caller
tokens = [{
expanded::{
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/support/procedural/src/pallet/parse/composite.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ impl CompositeDef {
pub fn try_from(
attr_span: proc_macro2::Span,
index: usize,
scrate: &proc_macro2::Ident,
scrate: &syn::Path,
item: &mut syn::Item,
) -> syn::Result<Self> {
let item = if let syn::Item::Enum(item) = item {
Expand Down
Loading
Loading