Add support for repetition to proc_macro::quote
#141608
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
moatom
commented
May 31, 2025
| @@ -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?
moatom
commented
May 31, 2025
| #[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
moatom
commented
May 31, 2025
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.
moatom
commented
May 31, 2025
| @@ -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.
moatom
commented
May 31, 2025
Comment on lines
+107
to
+109
| 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
moatom
commented
May 31, 2025
Comment on lines
-1
to
-4
| // 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?
|
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) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixed #140238
ToDo