Rust: expand attribute macros #19334

redsun82 · 2025-04-17T16:10:04Z

No description provided.

rust/ql/lib/codeql/rust/elements/Item.qll

hvitved

Initial comments.

hvitved · 2025-04-28T06:57:40Z

rust/ql/test/extractor-tests/attribute_macro_expansion/test.expected

@@ -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 |


Why is it not the attribute that is expanded? It seems weird that the function can expand to two things.

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.

Ok, thanks for clarifying.

Should the expanded items then be ordered, i.e. getExpanded(int i) instead of just getExpanded()?

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).

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

hvitved · 2025-04-28T13:00:26Z

I have started a DCA run.

aibaars · 2025-04-30T11:53:36Z

rust/extractor/src/translate/base.rs

            {
                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>) {
+        (|| {


Why is this a closure?

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

aibaars · 2025-04-30T13:30:50Z

rust/extractor/src/translate/base.rs

+            let ExpandResult {
+                value: expanded, ..
+            } = semantics.expand_attr_macro(node)?;
+            // TODO emit err?


It would be nice to register any failure reason as a diagnostic, so we can keep track of how many macro expansions were successful.

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.

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).

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.

aibaars · 2025-05-02T15:29:14Z

rust/ql/test/extractor-tests/macro_expansion/macro_expansion.rs

@@ -0,0 +1,2 @@
+#[ctor::ctor]
+fn foo() {}


Could you add some more extensive tests? Covering items with more than one macro attribute, and also some macro attributes that expand to "nothing", like #[cfg(all(unix, not(unix)))] or perhaps even a macro attribute that adds another macro attribute?

in order to do that we would need to define our own attribute macro. That requires a separate crate, so it will probably require some additional work on the qltest side to pull off. Trying something that expand to nothing is easy though.

If a custom attribute macro in the test requires tweaking the qltest code, I would very much prefer to do that in a follow-up PR

btw, I'm not even sure cfg is really implemented as an attribute macro, even if it behaves like one. There's definitely no instance of regular attribute macro definitions in the standard library, this I checked (I would have used those instead of relying on a third party crate).

yeah, I can confirm cfg does not work through attribute macro expansion

I've added an additional integration test (to be able to use an additional crate for the macros) covering the cases you mentioned (using a repeat attribute macro).

aibaars · 2025-05-02T15:30:12Z

rust/ql/test/extractor-tests/macro_expansion/test.ql

@@ -0,0 +1,6 @@
+import rust
+import TestUtils


Perhaps some "printAst"-like output would be helpful to show the expanded AST?

I thought about that, but I find printAst-heavy tests to tend to need a lot of updating. But we can have it in if you prefer.

added a PrintAst test

aibaars · 2025-05-14T11:56:47Z

rust/ql/integration-tests/macro-expansion/test.expected

+| src/lib.rs:6:1:8:11 | fn bar | 0 | src/lib.rs:7:1:8:10 | fn bar_0 |
+| src/lib.rs:6:1:8:11 | fn bar | 1 | src/lib.rs:7:1:8:10 | fn bar_1 |


I had not expected fn bar_0 and fn bar_1 here. Shouldn't the expansion be as follows?

#[repeat(2)] #[repeat(3)] fn bar() {}

-->

#[repeat(3)] fn bar_0() {} #[repeat(3)] fn bar_1() {}

-->

fn bar_0_1() {} fn bar_0_2() {} fn bar_0_3() {} fn bar_1_1() {} fn bar_1_2() {} fn bar_1_3() {}

indeed, that is the expansion, but all steps of the expansion are in the database. So you get the immediate expansion bar_0 and bar_1 with their repeat attribute attached to them, and then you get bar_i_j as getAttributeMacroExpansion() under each bar_i

this is similar to what we already do with macro call expansions: we don't get the full expansion right away, and each macro call gets in turn expanded within the expansion of the top-level macro call

Perfect , that makes sense. I was just wondering.

github-actions bot added the Rust Pull requests that update Rust code label Apr 17, 2025

github-advanced-security bot found potential problems Apr 17, 2025

View reviewed changes

rust/ql/lib/codeql/rust/elements/Item.qll Fixed Show fixed Hide fixed

Rust: expand attribute macros

49cf173

redsun82 force-pushed the redsun82/rust-expand-attr-macros branch from 7c8fc79 to 49cf173 Compare April 25, 2025 14:17

Rust: accept test changes

adeaceb

redsun82 changed the title ~~Rust: extract getExpended on Items~~ Rust: expand attribute macros Apr 25, 2025

redsun82 marked this pull request as ready for review April 25, 2025 15:43

redsun82 requested a review from a team as a code owner April 25, 2025 15:43

hvitved reviewed Apr 28, 2025

View reviewed changes

rust/schema/annotations.py Outdated Show resolved Hide resolved

Rust: add missing expected files

2d32c36

Rust: separate attribute macro and macro call expansions

a7a887c

aibaars reviewed Apr 30, 2025

View reviewed changes

redsun82 added 2 commits April 30, 2025 15:25

Rust: fix QL compilation errors

ecd80fb

Merge branch 'main' into redsun82/rust-expand-attr-macros

8ffe4d6

aibaars reviewed Apr 30, 2025

View reviewed changes

redsun82 added 4 commits April 30, 2025 16:38

Rust: fix downgrade script

6ecaf65

Rust: add diagnostics about item expansion not working properly

82736ea

Merge branch 'main' into redsun82/rust-expand-attr-macros

557f468

Rust: fix location emission

6748007

aibaars reviewed May 2, 2025

View reviewed changes

Paolo Tranquilli added 2 commits May 13, 2025 16:21

Merge branch 'main' into redsun82/rust-expand-attr-macros

a99556e

Rust: enhance macro expansion testing

f543839

redsun82 requested a review from aibaars May 14, 2025 08:12

Rust: accept dummy test output

3d38d77

aibaars reviewed May 14, 2025

View reviewed changes

aibaars approved these changes May 14, 2025

View reviewed changes

redsun82 merged commit e4b7b91 into main May 14, 2025
21 checks passed

redsun82 deleted the redsun82/rust-expand-attr-macros branch May 14, 2025 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust: expand attribute macros #19334

Rust: expand attribute macros #19334

redsun82 commented Apr 17, 2025

hvitved left a comment

hvitved Apr 28, 2025

redsun82 Apr 28, 2025

hvitved Apr 28, 2025

hvitved Apr 28, 2025

redsun82 Apr 29, 2025

redsun82 Apr 30, 2025

hvitved commented Apr 28, 2025

aibaars Apr 30, 2025

redsun82 Apr 30, 2025

aibaars Apr 30, 2025

redsun82 Apr 30, 2025

aibaars Apr 30, 2025

redsun82 Apr 30, 2025

aibaars May 2, 2025

redsun82 May 12, 2025

redsun82 May 12, 2025

redsun82 May 13, 2025

redsun82 May 13, 2025

redsun82 May 14, 2025

aibaars May 2, 2025

redsun82 May 12, 2025

redsun82 May 14, 2025

aibaars May 14, 2025

redsun82 May 14, 2025

redsun82 May 14, 2025

aibaars May 14, 2025

		@@ -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 \|

		\| src/lib.rs:6:1:8:11 \| fn bar \| 0 \| src/lib.rs:7:1:8:10 \| fn bar_0 \|
		\| src/lib.rs:6:1:8:11 \| fn bar \| 1 \| src/lib.rs:7:1:8:10 \| fn bar_1 \|

Rust: expand attribute macros #19334

Rust: expand attribute macros #19334

Conversation

redsun82 commented Apr 17, 2025

hvitved left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvitved commented Apr 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment