Rust: add macro expansion to the extractor #17659

aibaars · 2024-10-03T15:01:39Z

No description provided.

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

@@ -5,6 +5,7 @@

 private import internal.MacroCallImpl
 import codeql.rust.elements.AssocItem
+import codeql.rust.elements.AstNode


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

+ */
+
+private import internal.MacroStmtsImpl
+import codeql.rust.elements.AstNode


rust/ql/lib/codeql/rust/elements/internal/generated/MacroCall.qll

@@ -7,6 +7,7 @@
 private import codeql.rust.elements.internal.generated.Synth
 private import codeql.rust.elements.internal.generated.Raw
 import codeql.rust.elements.internal.AssocItemImpl::Impl as AssocItemImpl
+import codeql.rust.elements.AstNode


This avoids problems with files containing invalid utf-8 data, which may cause panic's like: ``` thread 'main' panicked at external/rules_rust~~_crate~ql~~r~r__ra_ap_salsa-0.0.232/src/input.rs:91:32: no value set for CompressedFileTextQuery(FileId(2429)) stack backtrace: 0: rust_begin_unwind at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:665:5 1: core::panicking::panic_fmt at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:74:14 2: <salsa::input::InputStorage<Q> as salsa::plumbing::QueryStorageOps<Q>>::fetch 3: <DB as ra_ap_base_db::SourceDatabase>::compressed_file_text::__shim 4: <ra_ap_base_db::FileTextQuery as salsa::plumbing::QueryFunction>::execute 5: salsa::Cycle::catch 6: salsa::derived_lru::slot::Slot<Q,MP>::execute 7: salsa::derived_lru::slot::Slot<Q,MP>::read 8: <salsa::derived_lru::DerivedStorage<Q,MP> as salsa::plumbing::QueryStorageOps<Q>>::fetch 9: <DB as ra_ap_base_db::SourceDatabase>::file_text::__shim 10: <DB as ra_ap_base_db::SourceDatabase>::file_text 11: <ra_ap_base_db::ParseQuery as salsa::plumbing::QueryFunction>::execute 12: salsa::Cycle::catch 13: salsa::derived_lru::slot::Slot<Q,MP>::execute 14: salsa::derived_lru::slot::Slot<Q,MP>::read 15: <salsa::derived_lru::DerivedStorage<Q,MP> as salsa::plumbing::QueryStorageOps<Q>>::fetch 16: <DB as ra_ap_base_db::SourceDatabase>::parse::__shim 17: <DB as ra_ap_base_db::SourceDatabase>::parse 18: ra_ap_hir::semantics::SemanticsImpl::parse 19: single_arch_extractor::main ```

The locations are "clipped" to the ranges of the parent node of a token, and the root node of the parse tree for errors.

This way we have only one "project" database in-memory at a time. This should avoid running out of memory when analyzing large mono-repos.

geoffw0

I haven't reviewed this (at this stage), just want to chip in and say that the changes to query-tests look fantastic!

redsun82

hey, thanks for the tremendous effort here, much appreciated!

Apart from a couple of minor nits I'm slightly worried by the file-to-workspace loading strategy...

redsun82 · 2024-10-10T14:39:08Z

rust/extractor/src/translate/base.rs

    pub trap: TrapFile,
    label: trap::Label,
    line_index: LineIndex,
+    semi: Option<Semantics<'a, RootDatabase>>,


why semi? maybe semantics, or sema? The semi prefix usually means "half" or "partial"

rust/extractor/src/rust_analyzer.rs

redsun82 · 2024-10-10T14:54:31Z

rust/extractor/src/rust_analyzer.rs

+        while let Some(parent) = p.parent() {
+            p = parent;
+            if let Some((vfs, db)) = self.workspace.get(parent) {
+                if let Some(file_id) = Utf8PathBuf::from_path_buf(path.to_path_buf())


I see two problems here:

I suspect this algorithm can be broken by using a relative path attribute: theoretically with that attribute a source file can be pulled in in a workspace without being beneath the workspace directory. It's dirty and might be rare, but Rob did tell us to beware of what dirty things Rust users have come up with 😅

if a file ends up in multiple workspaces with different cfg values, we would probably only extract one version of it. Although this case we might want to come back later to, as it's a tricky one which probably requires careful trap keying or maybe using linkage awareness.

In any case I wonder if we shouldn't rather:

iterate through all source files in the workspaces, ticking them off from the input files and outputting a semantic extraction of them

after doing that, go through the files that were not ticked off and output a non-semantic extraction of those

redsun82 · 2024-10-10T15:14:58Z

rust/extractor/src/translate/base.rs

+    pub(crate) fn extract_macro_call_expanded(
+        &mut self,
+        mcall: &ast::MacroCall,
+        label: Label<generated::MacroCall>,
+    ) {


this function is a mouthful... could we maybe split it into smaller ones?

redsun82 · 2024-10-11T08:56:42Z

also, I think it'd be worth adding to the existing integration tests a query for diagnostics, which should show how the different loading strategy (with semantics and without) applies to files, thanks to the warning when semantics are not available (in one of the integration tests there is a file not in the workspace).

rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll

hvitved · 2024-10-11T11:51:29Z

rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll

+
+  override predicate succ(AstNode pred, AstNode succ, Completion c) { none() }
+
+  override predicate propagatesAbnormal(AstNode child) { none() }


child = super.getExpanded().

hvitved · 2024-10-11T11:52:47Z

rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll

@@ -391,8 +403,40 @@ class ForExprTree extends LoopingExprTree instanceof ForExpr {
  }
 }

-// TODO: replace with expanded macro once the extractor supports it
-class MacroExprTree extends LeafTree, MacroExpr { }
+class MacroCallTree extends ControlFlowTree instanceof MacroCall {


Can't this simply be

class MacroCallTree extends StandardPostOrderTree, MacroCall { override AstNode getChildNode(int i) { i = 0 and result = this.getExpanded() } }

?

MacroCalls can also be statements and patterns which are pre-order. By dropping the MacroCall node altogether it "inherits" the order of the expanded tree.

redsun82

Thanks for addressing my comments. I agree that the more far-reaching comment about the loading strategy can be addressed later as a follow-up.

hvitved · 2024-10-11T15:29:40Z

rust/ql/test/query-tests/unusedentities/UnusedVariable.expected

@@ -1,10 +1,7 @@
 | main.rs:25:9:25:9 | a | Variable is not used. |
 | main.rs:90:13:90:13 | d | Variable is not used. |
-| main.rs:114:9:114:9 | k | Variable is not used. |


The SPURIOUS comments should be removed in main.rs.

@redsun82 let me know if you're already on this, I have a bit of follow-up to do after this PR anyway so I could fix this as well?

Perhaps the comments should be converted to inline test expecations?

Here's the fix: #17744

And there's already an issue for converting them into inline expectations, but that's waiting on having inline expectations for query tests. Is that work finished yet?

No, not finished yet.

hvitved · 2024-10-11T18:05:00Z

rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll

@@ -661,7 +704,9 @@ module PatternTrees {

  class LiteralPatTree extends LeafTree, LiteralPat { }

-  class MacroPatTree extends LeafTree, MacroPat { } // todo
+  class MacroPatTree extends PreOrderPatTree, MacroPat {


We need a case for MatroPat in isIrrefutablePattern and condPropagateExpr.

Does this look good for isIrrefutablePattern? #17746

Shouldn't condPropagateExpr be covered already, MacroPat because is defined as a PreOrderPatTree. Or does it need something else?

github-actions bot added the Rust Pull requests that update Rust code label Oct 3, 2024

aibaars force-pushed the aibaars/rust-macros branch from c2f9557 to 3286ec7 Compare October 3, 2024 15:23

aibaars added the no-change-note-required This PR does not need a change note label Oct 3, 2024

github-advanced-security bot found potential problems Oct 3, 2024

View reviewed changes

aibaars force-pushed the aibaars/rust-macros branch 15 times, most recently from 8bd6bee to a86d676 Compare October 8, 2024 15:06

aibaars marked this pull request as ready for review October 9, 2024 09:28

aibaars added 10 commits October 10, 2024 15:19

Rust: avoid double '.' in trap extension

7986fc7

Rust: add MacroStmts and MacroItems

69f0e8b

Rust: regenerate code

6ede20c

Rust: integrate Rust Analyzer's Semantic module into extractor

cad2b74

Rust: macro expansion

0b3b95a

Rust: update expected output

796da12

Rust: set RUST_BACKTRACE=1

0c10f08

Rust: reduce log output

faa1689

Rust: check that TextRanges are for the correct file

ae19b2f

Rust: do not fail on bad Cargo.toml files

3e877ff

aibaars added 7 commits October 10, 2024 15:53

Rust: ensure error and token locations are valid

8372a2e

The locations are "clipped" to the ranges of the parent node of a token, and the root node of the parse tree for errors.

Rust: use macro to inject extraction functions for detached properties

db28f1b

Rust: extract files on a per-project basis

ca27785

This way we have only one "project" database in-memory at a time. This should avoid running out of memory when analyzing large mono-repos.

Rust: lower default verbosity to WARN

88e5ce3

Rust: clippy fixes

f97a159

Rust: add QL doc for MacroStmts and MacroItems

6d43eed

geoffw0 reviewed Oct 10, 2024

View reviewed changes

redsun82 reviewed Oct 10, 2024

View reviewed changes

aibaars force-pushed the aibaars/rust-macros branch from 8847599 to 59be922 Compare October 10, 2024 20:10

Rust: implement CFG for macros

9fad541

geoffw0 reviewed Oct 11, 2024

View reviewed changes

rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll Outdated Show resolved Hide resolved

Rust: address comments

6ade2a8

aibaars force-pushed the aibaars/rust-macros branch from 59be922 to 6ade2a8 Compare October 11, 2024 11:13

Merge remote-tracking branch 'upstream/main' into aibaars/rust-macros

b4bb24a

github-advanced-security bot found potential problems Oct 11, 2024

View reviewed changes

rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll Fixed Show resolved Hide resolved

Rust: fix CodeQL alert

659ce6f

hvitved reviewed Oct 11, 2024

View reviewed changes

redsun82 previously approved these changes Oct 11, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into aibaars/rust-macros

5f45402

aibaars dismissed redsun82’s stale review via 5f45402 October 11, 2024 13:44

Rust: address comment

fc298b2

aibaars force-pushed the aibaars/rust-macros branch from 6eb0972 to fc298b2 Compare October 11, 2024 14:01

redsun82 approved these changes Oct 11, 2024

View reviewed changes

redsun82 merged commit 5b57826 into main Oct 11, 2024
14 checks passed

redsun82 deleted the aibaars/rust-macros branch October 11, 2024 15:29

hvitved requested changes Oct 11, 2024

View reviewed changes

geoffw0 mentioned this pull request Oct 11, 2024

Rust: Unused variable follow-up work #17744

Merged

hvitved reviewed Oct 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust: add macro expansion to the extractor #17659

Rust: add macro expansion to the extractor #17659

aibaars commented Oct 3, 2024

geoffw0 left a comment

redsun82 left a comment

redsun82 Oct 10, 2024 •

edited

Loading

redsun82 Oct 10, 2024

redsun82 Oct 10, 2024

redsun82 commented Oct 11, 2024

hvitved Oct 11, 2024

hvitved Oct 11, 2024

aibaars Oct 11, 2024

redsun82 left a comment

hvitved Oct 11, 2024

geoffw0 Oct 11, 2024

aibaars Oct 11, 2024

geoffw0 Oct 11, 2024

geoffw0 Oct 11, 2024

hvitved Oct 11, 2024

hvitved Oct 11, 2024

aibaars Oct 12, 2024

aibaars Oct 12, 2024


		override predicate succ(AstNode pred, AstNode succ, Completion c) { none() }

		override predicate propagatesAbnormal(AstNode child) { none() }

Rust: add macro expansion to the extractor #17659

Rust: add macro expansion to the extractor #17659

Conversation

aibaars commented Oct 3, 2024

geoffw0 left a comment

Choose a reason for hiding this comment

redsun82 left a comment

Choose a reason for hiding this comment

redsun82 Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redsun82 commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redsun82 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redsun82 Oct 10, 2024 •

edited

Loading