-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rust: expand attribute macros #19334
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
base: main
Are you sure you want to change the base?
Conversation
@@ -5,6 +5,7 @@ | |||
|
|||
private import internal.ItemImpl | |||
import codeql.rust.elements.Addressable | |||
import codeql.rust.elements.AstNode |
Check warning
Code scanning / CodeQL
Redundant import Warning
codeql.rust.elements.Addressable
Redundant import, the module is already imported inside
codeql.rust.elements.Stmt
7c8fc79
to
49cf173
Compare
getExpended
on Item
sThere 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.
Initial comments.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for clarifying.
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.
Should the expanded items then be ordered, i.e. getExpanded(int i)
instead of just getExpanded()
?
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to have a new base class, say Expandable
, for this?
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.
considering that MacroCall
is also an Item
, and that the only remaining case to cover (derive macros) also applies to Item
s, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In light of your reply to my question above, this question is no longer relevant.
I have started a DCA run. |
No description provided.