Uh oh!
There was an error while loading. Please reload this page.
Conversation
Non-derive proc macros are invoked without cfg being resolved. This adds quite a bit complexity to the macro because all of the macro needs to be careful to attach necessary cfgs. This becomes especially tricky for tuple structs. Thus, it is convenient if cfgs are all resolved before expansion. The most optimal way to handle this is via `TokenStream::expand_expr`, but that is still unstable. Implement an approach where we generate two cfg-gated macro invocations with cfg resolved within the invocation. This is the same approach as commit 3445a65 ("internal: rework how `#[pin_data]` handles cfg"). Signed-off-by: Gary Guo <gary@garyguo.net>
Add a test with 26 cfg options to test linear time behaviour of cfg resolution. If the cfg-expansion approach is exponential, this test will cause a timeout. Signed-off-by: Gary Guo <gary@garyguo.net>
BennoLossin
left a comment
Member
There was a problem hiding this comment.
I'm very happy with this strategy to handle cfg's :) I left some small suggestions, with those fixed, you can add my RB.
Comment on lines
+544
to
+546
| for attr in &self.attrs { | ||
| attr.to_tokens(tokens); | ||
| } |
Member
There was a problem hiding this comment.
Doesn't Vec<T>: ToTokens hold when T: ToTokens?
Comment on lines
+547
to
+549
| if let Some(this) = &self.this { | ||
| this.to_tokens(tokens); | ||
| } |
Comment on lines
+543
to
+561
| fn to_tokens(&self, tokens: &mut TokenStream) { | ||
| for attr in &self.attrs { | ||
| attr.to_tokens(tokens); | ||
| } | ||
| if let Some(this) = &self.this { | ||
| this.to_tokens(tokens); | ||
| } | ||
| self.path.to_tokens(tokens); | ||
| self.brace_token.surround(tokens, |tokens| { | ||
| self.fields.to_tokens(tokens); | ||
| if let Some((dotdot, expr)) = &self.rest { | ||
| dotdot.to_tokens(tokens); | ||
| expr.to_tokens(tokens); | ||
| } | ||
| }); | ||
| if let Some((question, ty)) = &self.error { | ||
| question.to_tokens(tokens); | ||
| ty.to_tokens(tokens); | ||
| } |
Member
There was a problem hiding this comment.
I usually prefer to exhaustively match in functions like these. With the other two suggestions:
Suggested change
| fn to_tokens(&self,tokens:&mutTokenStream){ | |
| for attr in&self.attrs{ | |
| attr.to_tokens(tokens); | |
| } | |
| ifletSome(this) = &self.this{ | |
| this.to_tokens(tokens); | |
| } | |
| self.path.to_tokens(tokens); | |
| self.brace_token.surround(tokens, |tokens| { | |
| self.fields.to_tokens(tokens); | |
| ifletSome((dotdot, expr)) = &self.rest{ | |
| dotdot.to_tokens(tokens); | |
| expr.to_tokens(tokens); | |
| } | |
| }); | |
| ifletSome((question, ty)) = &self.error{ | |
| question.to_tokens(tokens); | |
| ty.to_tokens(tokens); | |
| } | |
| fn to_tokens(&self,tokens:&mutTokenStream){ | |
| letSelf{ attrs, this, path, brace_token, fields, rest, error } = self; | |
| attrs.to_tokens(tokens); | |
| this.to_tokens(tokens); | |
| path.to_tokens(tokens); | |
| brace_token.surround(tokens, |tokens| { | |
| fields.to_tokens(tokens); | |
| ifletSome((dotdot, expr)) = rest { | |
| dotdot.to_tokens(tokens); | |
| expr.to_tokens(tokens); | |
| } | |
| }); | |
| ifletSome((question, ty)) = error { | |
| question.to_tokens(tokens); | |
| ty.to_tokens(tokens); | |
| } | |
| } |
The tuples might also support ToTokens, I haven't checked, in that case, this could be even shorter.
| expand(initializer, default_error, pinned, dcx) | ||
| } | ||
| pub(crate) fn expand( |
Member
There was a problem hiding this comment.
should we make this private now?
mqqz added a commit
to mqqz/pin-init
that referenced
this pull request
Aug 28, 2026
Extend the initializer syntax so that a field can be named by an index,
addressing tuple struct fields the same way a struct expression does:
pin_init!(Foo { 0: value, 1 <- initializer })
Tuple fields are not exposed by a `let` binding to the fields after them,
since they have no name to bind; `_0` would shadow a user variable.
`cfg` needs different treatment from named fields. Non-derive proc macros
are invoked before cfg is resolved, so the macro cannot know whether a
field survives, and dropping a tuple field renumbers every field after it.
That cannot be expressed by attaching a `cfg` attribute to the initializer
of a single field.
Resolve tuple field cfgs up front instead, by generating two cfg-gated
invocations of the macro with one field resolved in each. This is the
approach of commit 3445a65 ("internal: rework how `#[pin_data]`
handles cfg"), and it is linear because only one of the two branches is
ever expanded. Named fields do not renumber, so they keep using the
existing attribute-based handling.
Link: Rust-for-Linux#165
Suggested-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Mohamad Alsadhan <mo@sdhn.cc>mqqz added a commit
to mqqz/pin-init
that referenced
this pull request
Aug 28, 2026
Extend the initializer syntax so that a field can be named by an index,
addressing tuple struct fields the same way a struct expression does:
pin_init!(Foo { 0: value, 1 <- initializer })
Tuple fields are not exposed by a `let` binding to the fields after them,
since they have no name to bind; `_0` would shadow a user variable.
`cfg` needs different treatment from named fields. Non-derive proc macros
are invoked before cfg is resolved, so the macro cannot know whether a
field survives, and dropping a tuple field renumbers every field after it.
That cannot be expressed by attaching a `cfg` attribute to the initializer
of a single field.
Resolve tuple field cfgs up front instead, by generating two cfg-gated
invocations of the macro with one field resolved in each. This is the
approach of commit 3445a65 ("internal: rework how `#[pin_data]`
handles cfg"), and it is linear because only one of the two branches is
ever expanded. Named fields do not renumber, so they keep using the
existing attribute-based handling.
Link: Rust-for-Linux#165
Suggested-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Mohamad Alsadhan <mo@sdhn.cc>
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Non-derive proc macros are invoked without cfg being resolved. This adds quite a bit complexity to the macro because all of the macro needs to be careful to attach necessary cfgs. This becomes especially tricky for tuple structs. Thus, it is convenient if cfgs are all resolved before expansion.
The most optimal way to handle this is via
TokenStream::expand_expr, but that is still unstable. Implement an approach where we generate two cfg-gated macro invocations with cfg resolved within the invocation.This is the same approach as #161 but for init macros. This should hopefully also help #155 by removing the cfg limitation.