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

Remove need for use_attr and use_proc #5

Merged
merged 6 commits into from
Jun 6, 2023

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Jun 4, 2023

implements first part of #3

what it does:
import_tokens macros call themself instead of calling their related inner_sig
forward_tokens_inner_internal will add a keyword at the start of the token stream.
if import_tokens see the keyword at the start of the token stream then it call their inner_sig which is declared inlined.

tests are successful

TODO:

  • update docs
  • discuss the keyword and whereas we should expose a forward_token which doesn't add this extra token in order not to break usage of forward_token
  • is it correct to assume usage of extra is equivalent to having been called by an attribute macro, or should we pass along this information in another way.

@sam0x17
Copy link
Owner

sam0x17 commented Jun 5, 2023

This is amazing, didn't think this would be possible without a stack of macros calling each other. I will have to go over carefully to understand how you got around that!

@sam0x17
Copy link
Owner

sam0x17 commented Jun 5, 2023

for context, extra right now is a sort of dumping ground for things that need to be passed from the initial macro call to the final macro call that were added on as later features from the initial version. It's sort of horrible how I pass it and use string manipulation to escape it, really it should be done on the export_tokens macro itself but this was a convenient way to quickly add a few features.

@sam0x17
Copy link
Owner

sam0x17 commented Jun 5, 2023

I am also OK with changing / breaking the original signature of forward_tokens if needed -- I don't think anyone is using it directly and I already see this as a 0.4x thing so OK to have a breaking change

@sam0x17
Copy link
Owner

sam0x17 commented Jun 5, 2023

So yeah if you can get the cargo fmt to pass I can merge this and we can tackle the next pieces

core/src/lib.rs Outdated
let tokens_forwarded_keyword = keywords::__private_macro_magic_tokens_forwarded::default();
let pound = Punct::new('#', Spacing::Alone);
match parsed.extra {
// no extra, used by attr, so expand to attribute macro
Copy link
Contributor Author

@gui1117 gui1117 Jun 5, 2023

Choose a reason for hiding this comment

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

@sam0x17 what I wrote here looks a bit abusive to me, but as long as we use the extra token stuff only for attribute macros we are good.
but maybe it should be refactored together with extra token feature.

otherwise we could have forward_tokens_to_attribute_macro and forward_tokens_to_proc_macro.
the implementation would redirect to forward_tokens_inner(attr: true, ...) and forward_tokens_inner(attr: false, ...). and then same for forward_tokens_internal.

Copy link
Owner

@sam0x17 sam0x17 Jun 5, 2023

Choose a reason for hiding this comment

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

Yeah also if I remember correctly the main blocker to having a more general thing like a tt here is anything but an ident or a literal limits what types of position you can use forward_tokens stuff with because of how decl macros work. This is the same reason I have to do that crazy manual interpolation of idents with :: instead of just a path in the export tokens macro. Extra as a string works in all positions.

Maybe a tuple like this would be better though:

("extra item 1", "extra item 2", "extra item 3", "extra item 4")

I don't think that would mess up the position capabilities at all, and at least then we wouldn't have to do string interpolation and escaping

Copy link
Owner

@sam0x17 sam0x17 Jun 5, 2023

Choose a reason for hiding this comment

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

But yes I think it is fine to expand to a proc macro here, and I'd be happy to merge it this way

@gui1117
Copy link
Contributor Author

gui1117 commented Jun 5, 2023

yes formatted, I only have the little concern above #5 (comment) but the PR is ready.

I will attend buidl asia, I won't work on the other points soon I think

@sam0x17
Copy link
Owner

sam0x17 commented Jun 5, 2023

OK I will see if I can get the other parts working and update the docs 👍🏻

core/src/lib.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants