-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Add support for repetition to proc_macro::quote
#141608
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
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
proc_macro::quote
proc_macro::quote
library/proc_macro/src/lib.rs
Outdated
@@ -1613,3 +1614,202 @@ pub mod tracked_path { | |||
crate::bridge::client::FreeFunctions::track_path(path); | |||
} | |||
} | |||
|
|||
#[doc(hidden)] | |||
#[unstable(feature = "proc_macro_quote", issue = "54722")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure whether these annotations are appropriate. (I added them just based on my speculation.)
In addition,
It's probably easiest to just copy quote's logic here, which uses an extension trait to facilitate this.
do we need to take care of its license?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure whether these annotations are appropriate. (I added them just based on my speculation.)
You can always try removing them and see if it complains - but in general, all crate-public items need a stability gate. Try to make as little as possible actually pub
of course, but that's difficult because this needs a lot of helpers to be public.
This all doesn't need to be in lib.rs
though, could you move the additions here to the quote
module? And then reexport only what is needed.
It's probably easiest to just copy quote's logic here, which uses an extension trait to facilitate this.
do we need to take care of its license?
quote
is MIT AND Apache-2.0
, which is the same as rust-lang/rust so there is no problem here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all doesn't need to be in lib.rs though, could you move the additions here to the quote module? And then reexport only what is needed.
DONE.
FYI, I used #[unstable(feature = "proc_macro_quote", issue = "54722")]
instead of something like #[unstable(feature = "proc_macro_quote_span", issue = "140238")]
, because this PR modifies the quote function, which is already part of the "proc_macro_quote"
feature.
library/proc_macro/src/lib.rs
Outdated
#[unstable(feature = "proc_macro_quote", issue = "54722")] | ||
pub mod ext { | ||
use core::slice; | ||
use std::collections::btree_set::{self, BTreeSet}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an alternative for alloc::collections
library/proc_macro/src/quote.rs
Outdated
} | ||
|
||
self.tokens[self.pos] = self.iter.next(); | ||
let token_opt = self.tokens[self.pos].clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helper struct may require significant refactoring. (Especially, the use of .clone()
might be avoidable.)
Do you have any comments or suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth using just a Peekable
. I think it would be better to use it at the expense of some extra parsing code, than using a custom lookahead iterator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ora-0
I think Peekable
only supports single-element lookahead.
However, placing a simplified and extended version of Peekable
here might be a better approach than defining the completely original lookahead iterator from scratch. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if a lookahead is necessary. Would this work? I can't be completely sure since I haven't tested this code.
let mut iter = stream.into_iter();
// ...
let mut sep_opt: Option<TokenTree> = None;
if let next @ Some(TokenTree::Punct(token_1)) = iter.next() {
if token_1.as_char() != '*' {
sep_opt = next;
if !matches!(iter.next(), Some(TokenTree::Punct(token_2)) if token_2 == "*") {
panic!("`$(...)` must be followed by `*` in `quote!`");
}
}
}
Since we are panicking at the wildcard we could just use .next()
s. It is a bit less declarative but it avoids the complexity of another data structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nvm, turns out it is necessary, because the separator itself may end up being a star. But I think we only need one lookahead. The code ends up being pretty similar to the current:
let mut iter = stream.into_iter().peekable();
// ...
let sep_opt: Option<TokenTree> = match (iter.next(), iter.peek()) {
(Some(TokenTree::Punct(sep)), Some(&TokenTree::Punct(star)))
if sep.spacing() == Spacing::Joint && star.as_char() == '*' =>
{
iter.next();
Some(TokenTree::Punct(sep))
}
(Some(TokenTree::Punct(star)), _) if star.as_char() == '*' => None,
_ => panic!("`$(...)` must be followed by `*` in `quote!`"),
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think we only need one lookahead.
Thank you for the great point! We must consume at least one *
here.
@@ -71,10 +160,97 @@ pub fn quote(stream: TokenStream) -> TokenStream { | |||
let mut after_dollar = false; | |||
|
|||
let mut tokens = crate::TokenStream::new(); | |||
for tree in stream { | |||
let mut iter = LookaheadIter::new(stream); | |||
while let Some(tree) = iter.next() { | |||
if after_dollar { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after_dollar
can be combined with LookaheadIter
, if you prefer.
assert_eq!("X, X, X, X,", quote!($($primes,)*).to_string()); | ||
|
||
assert_eq!("X, X, X, X", quote!($($primes),*).to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adjusted the expected spacing around SEP
in the original test code. Is it appropriate?
Cf. https://github.com/dtolnay/quote/blob/62fd385a800f7398ab416c00100664479261a86e/tests/test.rs#L84
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proc_macro
and proc_macro2
unquote whitespace slightly differently, is this what you are referring to? If so, I don't think there is any problem.
rustbot has assigned @petrochenkov. Use |
This comment has been minimized.
This comment has been minimized.
r? @tgross35 |
I’ll leave a review but David knows this area much better, so r? dtolnay |
This comment has been minimized.
This comment has been minimized.
assert_eq!("X, X, X, X,", quote!($($primes,)*).to_string()); | ||
|
||
assert_eq!("X, X, X, X", quote!($($primes),*).to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proc_macro
and proc_macro2
unquote whitespace slightly differently, is this what you are referring to? If so, I don't think there is any problem.
library/proc_macro/src/quote.rs
Outdated
if after_dollar { | ||
after_dollar = false; | ||
match tree { | ||
TokenTree::Group(inner) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add comments in this section about what is happening?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added them, but
// Append setup code for a
while
, where recursively quotedCONTENTS
// andSEP_OPT
are repeatedly processed, toREP_EXPANDED
.
the word choice of "setup code" may be ambiguous.
Moreover, it would be better to carefully distinguish the terms "expanded", "processed", and "quoted".
library/proc_macro/src/quote.rs
Outdated
@@ -155,6 +276,30 @@ pub fn quote(stream: TokenStream) -> TokenStream { | |||
} | |||
} | |||
|
|||
fn collect_meta_vars(stream: TokenStream) -> Vec<Ident> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a doc comment?
You should also rebase when you update this, I think the CI failure was from something spurious. |
dcca29e
to
e6cb946
Compare
e6cb946
to
23e35c6
Compare
@tgross35 @dtolnay
This label was added because I mistakenly pushed incorrectly rebased commits. Feel free to remove it if you'd like. |
Thanks for working on this feature! I see some parsing differences here compared to what the #![feature(proc_macro_quote)]
use proc_macro::TokenStream;
macro_rules! decl {
($($iter:tt)*) => {
stringify!($($iter) << *)
};
}
#[proc_macro]
pub fn repro(input: TokenStream) -> TokenStream {
// macro_rules macro
let tokens = decl!(a b c);
eprintln!("{}", tokens);
// quote crate
let input2 = proc_macro2::TokenStream::from(input.clone());
let iter2 = input2.into_iter();
let tokens = quote::quote!(#(#iter2) << *);
eprintln!("{}", tokens);
// libproc_macro
let iter = input.into_iter();
let tokens = proc_macro::quote!($($iter) << *);
eprintln!("{}", tokens);
TokenStream::new()
} Macro_rules macro: error: proc macro panicked
--> src/lib.rs:25:18
|
25 | let tokens = proc_macro::quote!($($iter) << *);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: message: `$(...)` must be followed by `*` in `quote!` Another example: error[E0425]: cannot find value `j` in this scope
--> src/lib.rs:25:47
|
25 | let tokens = proc_macro::quote!($$ j $($$ j $iter)*);
| ^ not found in this scope The parsing logic will need some more scrutiny in followup PRs before the macro can be stabilized. @bors r+ |
I removed the tag to close #140238 and added your comment as a todo item there so we don't lose this follow up. |
Add support for repetition to `proc_macro::quote` Progress toward: rust-lang#140238
Rollup of 11 pull requests Successful merges: - #140809 (Reduce special casing for the panic runtime) - #141608 (Add support for repetition to `proc_macro::quote`) - #141864 (Handle win32 separator for cygwin paths) - #142216 (Miscellaneous RefCell cleanups) - #142517 (Windows: Use anonymous pipes in Command) - #142570 (Reject union default field values) - #142584 (Handle same-crate macro for borrowck semicolon suggestion) - #142585 (Update books) - #142586 (Fold unnecessary `visit_struct_field_def` in AstValidator) - #142595 (Revert overeager warning for misuse of `--print native-static-libs`) - #142598 (Set elf e_flags on ppc64 targets according to abi) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 14 pull requests Successful merges: - #141574 (impl `Default` for `array::IntoIter`) - #141608 (Add support for repetition to `proc_macro::quote`) - #142100 (rustdoc: make srcIndex no longer a global variable) - #142371 (avoid `&mut P<T>` in `visit_expr` etc methods) - #142517 (Windows: Use anonymous pipes in Command) - #142520 (alloc: less static mut + some cleanup) - #142588 (Generic ctx imprv) - #142605 (Don't unwrap in enzyme builds in case of missing llvm-config) - #142608 (Refresh module-level docs for `rustc_target::spec`) - #142618 (Lint about `console` calls in rustdoc JS) - #142620 (Remove a panicking branch in `BorrowedCursor::advance`) - #142631 (Dont suggest remove semi inside macro expansion for redundant semi lint) - #142632 (Update cargo) - #142635 (Temporarily add back -Zwasm-c-abi=spec) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 14 pull requests Successful merges: - rust-lang/rust#141574 (impl `Default` for `array::IntoIter`) - rust-lang/rust#141608 (Add support for repetition to `proc_macro::quote`) - rust-lang/rust#142100 (rustdoc: make srcIndex no longer a global variable) - rust-lang/rust#142371 (avoid `&mut P<T>` in `visit_expr` etc methods) - rust-lang/rust#142517 (Windows: Use anonymous pipes in Command) - rust-lang/rust#142520 (alloc: less static mut + some cleanup) - rust-lang/rust#142588 (Generic ctx imprv) - rust-lang/rust#142605 (Don't unwrap in enzyme builds in case of missing llvm-config) - rust-lang/rust#142608 (Refresh module-level docs for `rustc_target::spec`) - rust-lang/rust#142618 (Lint about `console` calls in rustdoc JS) - rust-lang/rust#142620 (Remove a panicking branch in `BorrowedCursor::advance`) - rust-lang/rust#142631 (Dont suggest remove semi inside macro expansion for redundant semi lint) - rust-lang/rust#142632 (Update cargo) - rust-lang/rust#142635 (Temporarily add back -Zwasm-c-abi=spec) r? `@ghost` `@rustbot` modify labels: rollup
Progress toward: #140238