Add support for repetition to proc_macro::quote
#141608
Conversation
rustbot
added
the
T-compiler
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
moatom
changed the title
proc_macro::quoteproc_macro::quote
| @@ -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.
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.
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.
| #[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.
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.
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.
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.
@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.
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.
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.
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.
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.
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.
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.
| // FIXME(quote): `proc_macro::quote!` doesn't support repetition at the moment, so the stderr is | ||
| // expected to be incorrect. | ||
| //@ known-bug: #54722 | ||
|
|
There was a problem hiding this comment.
Is it sufficient to simply remove these, or is some additional workaround needed?
E.g.:
//@ check-pass
//@ force-host
//@ no-prefer-dynamic
//@ compile-flags: -Z unpretty=expanded
//@ needs-unwind compiling proc macros with panic=abort causes a warning
//@ edition: 2015There was a problem hiding this comment.
plus any new tests in tests/ui/proc-macro/quote for things we should reject
Do you have any examples besides these?
There was a problem hiding this comment.
Is it sufficient to simply remove these, or is some additional workaround needed?
Removing is sufficient. If you are comparing to debug.rs, it has more flags since it wants to actually expand and check the stdout. Just make sure the tests all have a comment about what they are intended to check.
There was a problem hiding this comment.
plus any new tests in tests/ui/proc-macro/quote for things we should reject
Do you have any examples besides these?
Basically ensure that our tests/ui/proc-macro/quote includes all of quote's tests somewhere https://github.com/dtolnay/quote/tree/62fd385a800f7398ab416c00100664479261a86e/tests, or mentioned in the "not yet supported" list. It looks like you probably have everything relevant now.
|
rustbot has assigned @petrochenkov. Use |
rustbot
added
the
S-waiting-on-review
|
|
r? @tgross35 |
|
I’ll leave a review but David knows this area much better, so r? dtolnay |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
| 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.
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.
| // FIXME(quote): `proc_macro::quote!` doesn't support repetition at the moment, so the stderr is | ||
| // expected to be incorrect. | ||
| //@ known-bug: #54722 | ||
|
|
There was a problem hiding this comment.
Is it sufficient to simply remove these, or is some additional workaround needed?
Removing is sufficient. If you are comparing to debug.rs, it has more flags since it wants to actually expand and check the stdout. Just make sure the tests all have a comment about what they are intended to check.
| // FIXME(quote): `proc_macro::quote!` doesn't support repetition at the moment, so the stderr is | ||
| // expected to be incorrect. | ||
| //@ known-bug: #54722 | ||
|
|
There was a problem hiding this comment.
plus any new tests in tests/ui/proc-macro/quote for things we should reject
Do you have any examples besides these?
Basically ensure that our tests/ui/proc-macro/quote includes all of quote's tests somewhere https://github.com/dtolnay/quote/tree/62fd385a800f7398ab416c00100664479261a86e/tests, or mentioned in the "not yet supported" list. It looks like you probably have everything relevant now.
| if after_dollar { | ||
| after_dollar = false; | ||
| match tree { | ||
| TokenTree::Group(inner) => { |
There was a problem hiding this comment.
Could you add comments in this section about what is happening?
| @@ -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.
Could you add a doc comment?
|
You should also rebase when you update this, I think the CI failure was from something spurious. |
Fixed #140238
ToDo