Rust: expand attribute macros #19334
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
redsun82
force-pushed
the
redsun82/rust-expand-attr-macros
branch
from
7c8fc79 to
49cf173
Compare
redsun82
changed the title
getExpended on Items
| @@ -0,0 +1,2 @@ | |||
| | attr_macro_expansion.rs:1:1:2:11 | fn foo | attr_macro_expansion.rs:2:4:2:6 | Static | | |||
| | attr_macro_expansion.rs:1:1:2:11 | fn foo | attr_macro_expansion.rs:2:4:2:10 | fn foo | | |||
There was a problem hiding this comment.
Why is it not the attribute that is expanded? It seems weird that the function can expand to two things.
There was a problem hiding this comment.
that's what the attribute macro does: it takes in an AST (in this case, the definition of a function) and replaces it with possibly multiple items like in this case. So it's not really the attribute that is expanded, it's really the AST node it is attached to that gets entirely replaced.
There was a problem hiding this comment.
Ok, thanks for clarifying.
There was a problem hiding this comment.
Should the expanded items then be ordered, i.e. getExpanded(int i) instead of just getExpanded()?
There was a problem hiding this comment.
I looked a bit more into this. I went with reusing MacroCall's getExpanded, with the same signature. This is defined as returning an AstNode, but it really returns one of MacroStmts | MacroItems | Pat | Type | Expr. In case of attribute macros, this will always be MacroItems, which is just a wrapper around an ordered list of items.
I'm thinking about how we could be more precise here. We would need to have separate names for the property, as MacroCalls are Items, therefore macro call expansion could not have the same name as attribute or derive macro expansions. Maybe we could rename getExpanded to getMacroCallExpansion for MacroCall, and use getAttributeMacroExpansion (returning MacroItems) for this, getDeriveMacroExpansion (same) for the derive macros, and then reunite them all in a getMacroExpansion that would work for all cases (returning a more generic AstNode).
There was a problem hiding this comment.
I ended up separating macro call and attribute macro expansions in the DB. We can think whether we need a single predicate to do whatever macro expansion, but we can do that in QL if needed.
rust/schema/annotations.py
Outdated
| @@ -1945,4 +1944,4 @@ class FormatArgument(Locatable): | |||
|
|
|||
| @annotate(Item, add_bases=(Addressable,)) | |||
| class _: | |||
| pass | |||
| expanded: optional[AstNode] | child | rust.detach | doc("expanded attribute or procedural macro call of this item") | |||
There was a problem hiding this comment.
Would it be better to have a new base class, say Expandable, for this?
There was a problem hiding this comment.
considering that MacroCall is also an Item, and that the only remaining case to cover (derive macros) also applies to Items, I don't see the need for another class in the hierarchy. What do you think would be the advantage of such an additional class?
There was a problem hiding this comment.
In light of your reply to my question above, this question is no longer relevant.
|
I have started a DCA run. |
| { | ||
| generated::PathSegment::emit_trait_type_repr(label, t, &mut self.trap.writer) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn emit_item_expansion(&mut self, node: &ast::Item, label: Label<generated::Item>) { | ||
| (|| { |
There was a problem hiding this comment.
The code felt much more natural to write using ? to exit early in case of None results. After all
let x = foo()?;
reads (and writes) much nicer than
let Some(x) = foo() else { return }
But in order to use ?, one needs to be in a function returning Option or Result. So I did that in the context of a closure returning Option<()> which is immediately called
| let ExpandResult { | ||
| value: expanded, .. | ||
| } = semantics.expand_attr_macro(node)?; | ||
| // TODO emit err? |
There was a problem hiding this comment.
It would be nice to register any failure reason as a diagnostic, so we can keep track of how many macro expansions were successful.
There was a problem hiding this comment.
after looking a bit at the rust analyzer code, I have the feeling the err in the ExpandResult gets recovered again in parse_macro_expansion_error which is called by our emit_macro_expansion_parse_errors function called just below. So I think we should ignore the err here, as otherwise we would be emitting a diagnostic twice about it.
There was a problem hiding this comment.
Ah that makes sense. I guess that leaves the ? after the cast as a potential error that may be worth reporting. I suppose the result is always a MacroItem, but perhaps it could be just an Item if the macro expands to a single item, or an Error if something went wrong (although in that case I'd expect a parse error to be reported).
There was a problem hiding this comment.
yeah, maybe reporting an error there makes sense, just to be defensive. I think attribute and derive macros will always expand to MacroItems, as that's the only context in which they appear.
No description provided.