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.

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

New export_tokens path strategy #8

Merged
merged 5 commits into from
Jun 12, 2023
Merged

New export_tokens path strategy #8

merged 5 commits into from
Jun 12, 2023

Conversation

sam0x17
Copy link
Owner

@sam0x17 sam0x17 commented Jun 9, 2023

This fulfills item 2 from #3

With this new strategy, we can now properly access #[export_tokens] declarations as real items within the modules in which they are defined, instead of faking it and accessing them from the #[macro_export] created at the crate root.

This is a breaking change since it is no longer possible (at least with how the macros are written currently) to do two things that used to be possible:

  1. access #[export_tokens] declarations that are inside of things like functions that are not accessible at the module level
  2. access #[export_tokens] declarations that are in private modules

I think this is largely OK if we have to drop support for this, but I would still like to brainstorm some possible workarounds so we can still make these work, possibly with some additional opt-in macro argument.

@sam0x17 sam0x17 added the enhancement New feature or request label Jun 9, 2023
@sam0x17 sam0x17 self-assigned this Jun 9, 2023
@sam0x17 sam0x17 mentioned this pull request Jun 9, 2023
3 tasks
@sam0x17
Copy link
Owner Author

sam0x17 commented Jun 9, 2023

would love your thoughts on this if you have any @thiolliere

@gui1117
Copy link
Contributor

gui1117 commented Jun 10, 2023

This is a breaking change since it is no longer possible (at least with how the macros are written currently) to do two things that used to be possible:

1. access `#[export_tokens]` declarations that are _inside of_ things like functions that are not accessible at the module level

2. access `#[export_tokens]` declarations that are in private modules

I see now that it is limiting the features of export_tokens.
As you say we could still re-enable it with an attribute like [export_tokens(declare_at_crate_root)].

The main advantage of this PR implementation IMO is that path feels more natural and there is no conflict if 2 pallets declare the same reexport: TestDefaultConfig

But if we don't want that we can fix the issue paritytech/substrate#14287 by requiring in attribute the correct path which should points to the item at the crate root, and search for __export_tokens_tt_test_default_config at this path.
e.g. the call would look like this:

#[derive_impl(bar::frame_system::TestDefaultConfig as frame_system::DefaultConfig)]

instead of

#[derive_impl(bar::frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)]

and then we search for the __export_tokens_tt_test_default_config at the path except last segment so: bar::frame_system

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

Yes implementation is good to me 👍

Comment on lines +405 to +415
let Some(last_seg) = item_path.segments.last() else { unreachable!("must have at least one segment") };
let mut leading_segs = item_path
.segments
.iter()
.cloned()
.map(|seg| seg.ident)
.collect::<Vec<_>>()[0..item_path.segments.len() - 1]
.to_vec();
let last_seg = export_tokens_macro_ident(&last_seg.ident);
leading_segs.push(last_seg);
parse_quote!(#(#leading_segs)::*)
Copy link
Contributor

@gui1117 gui1117 Jun 10, 2023

Choose a reason for hiding this comment

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

maybe this is slightly cleaner as we don't loose the path leading colon, what do you think?

Suggested change
let Some(last_seg) = item_path.segments.last() else { unreachable!("must have at least one segment") };
let mut leading_segs = item_path
.segments
.iter()
.cloned()
.map(|seg| seg.ident)
.collect::<Vec<_>>()[0..item_path.segments.len() - 1]
.to_vec();
let last_seg = export_tokens_macro_ident(&last_seg.ident);
leading_segs.push(last_seg);
parse_quote!(#(#leading_segs)::*)
let mut macro_path = item_path.clone();
let Some(syn::punctuated::Pair::End(last_seg)) = macro_path.segments.pop()
else { unreachable!("Path must have a last segment") };
let last_seg = export_tokens_macro_ident(&last_seg.ident);
macro_path.segments.push(last_seg.into());
macro_path

@gui1117
Copy link
Contributor

gui1117 commented Jun 10, 2023

Thinking again, we could do 2 attribute macros: export_tokens and export_tokens_at_crate_root

@sam0x17
Copy link
Owner Author

sam0x17 commented Jun 10, 2023

Yes, am working on something like the 2 attribute approach you mentioned

@sam0x17
Copy link
Owner Author

sam0x17 commented Jun 12, 2023

Yeah upon further thought and investigation I think it is worth it to do a breaking change and drop this (accessing private items) feature for now. No one I have spoken to is using this, and to be fair, if someone has enough control over the code to add #[export_tokens], they also have enough control to make the module public. So I will merge this as is

@sam0x17 sam0x17 merged commit fcf29fc into main Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants