From 072b83603c4cbf198445241c91110d169012823e Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 29 Oct 2024 14:17:17 +0100 Subject: [PATCH] Rust: Exclude results inside macro expansions from unused entity queries --- .../rust/elements/internal/AstNodeImpl.qll | 20 +++++++++++++------ .../elements/internal/generated/Comment.qll | 2 +- .../queries/unusedentities/UnreachableCode.ql | 2 ++ .../src/queries/unusedentities/UnusedValue.ql | 3 ++- .../queries/unusedentities/UnusedVariable.qll | 1 + .../unusedentities/UnusedValue.expected | 4 ---- .../test/query-tests/unusedentities/main.rs | 8 ++++---- 7 files changed, 24 insertions(+), 16 deletions(-) diff --git a/rust/ql/lib/codeql/rust/elements/internal/AstNodeImpl.qll b/rust/ql/lib/codeql/rust/elements/internal/AstNodeImpl.qll index 435d8e1519129..abb4605433034 100644 --- a/rust/ql/lib/codeql/rust/elements/internal/AstNodeImpl.qll +++ b/rust/ql/lib/codeql/rust/elements/internal/AstNodeImpl.qll @@ -26,21 +26,29 @@ module Impl { result = getImmediateParent(e) } - /** Gets the nearest enclosing parent of `ast` that is an `AstNode`. */ - private AstNode getParentOfAst(AstNode ast) { - result = getParentOfAstStep*(getImmediateParent(ast)) - } - class AstNode extends Generated::AstNode { + /** + * Gets the nearest enclosing parent of this node, which is also an `AstNode`, + * if any. + */ + AstNode getParent() { result = getParentOfAstStep*(getImmediateParent(this)) } + /** Gets the immediately enclosing callable of this node, if any. */ cached Callable getEnclosingCallable() { - exists(AstNode p | p = getParentOfAst(this) | + exists(AstNode p | p = this.getParent() | result = p or not p instanceof Callable and result = p.getEnclosingCallable() ) } + + /** Holds if this node is inside a macro expansion. */ + predicate isInMacroExpansion() { + this = any(MacroCall mc).getExpanded() + or + this.getParent().isInMacroExpansion() + } } } diff --git a/rust/ql/lib/codeql/rust/elements/internal/generated/Comment.qll b/rust/ql/lib/codeql/rust/elements/internal/generated/Comment.qll index 7a4ca595f113e..694219d13bef8 100644 --- a/rust/ql/lib/codeql/rust/elements/internal/generated/Comment.qll +++ b/rust/ql/lib/codeql/rust/elements/internal/generated/Comment.qll @@ -29,7 +29,7 @@ module Generated { /** * Gets the parent of this comment. */ - AstNode getParent() { + override AstNode getParent() { result = Synth::convertAstNodeFromRaw(Synth::convertCommentToRaw(this).(Raw::Comment).getParent()) } diff --git a/rust/ql/src/queries/unusedentities/UnreachableCode.ql b/rust/ql/src/queries/unusedentities/UnreachableCode.ql index ffe6ce9d1c6e0..bce77bac47ede 100644 --- a/rust/ql/src/queries/unusedentities/UnreachableCode.ql +++ b/rust/ql/src/queries/unusedentities/UnreachableCode.ql @@ -30,6 +30,8 @@ predicate hiddenNode(AstNode n) { not succ(_, n) or n instanceof ControlFlowGraphImpl::PostOrderTree // location is counter-intuitive + or + n.isInMacroExpansion() } /** diff --git a/rust/ql/src/queries/unusedentities/UnusedValue.ql b/rust/ql/src/queries/unusedentities/UnusedValue.ql index d820858417dd6..ab9e7bfdbaf4b 100644 --- a/rust/ql/src/queries/unusedentities/UnusedValue.ql +++ b/rust/ql/src/queries/unusedentities/UnusedValue.ql @@ -20,5 +20,6 @@ where not write = any(Ssa::WriteDefinition def).getWriteAccess().getAstNode() and // avoid overlap with the unused variable query not isUnused(v) and - not v instanceof DiscardVariable + not v instanceof DiscardVariable and + not write.isInMacroExpansion() select write, "Variable '" + v + "' is assigned a value that is never used." diff --git a/rust/ql/src/queries/unusedentities/UnusedVariable.qll b/rust/ql/src/queries/unusedentities/UnusedVariable.qll index 64cca6e237af9..e52b836823b33 100644 --- a/rust/ql/src/queries/unusedentities/UnusedVariable.qll +++ b/rust/ql/src/queries/unusedentities/UnusedVariable.qll @@ -10,5 +10,6 @@ predicate isUnused(Variable v) { not exists(v.getAnAccess()) and not exists(v.getInitializer()) and not v instanceof DiscardVariable and + not v.getPat().isInMacroExpansion() and exists(File f | f.getBaseName() = "main.rs" | v.getLocation().getFile() = f) // temporarily severely limit results } diff --git a/rust/ql/test/query-tests/unusedentities/UnusedValue.expected b/rust/ql/test/query-tests/unusedentities/UnusedValue.expected index ce89c8ff06bff..8df7c4c999c6f 100644 --- a/rust/ql/test/query-tests/unusedentities/UnusedValue.expected +++ b/rust/ql/test/query-tests/unusedentities/UnusedValue.expected @@ -11,11 +11,7 @@ | main.rs:87:9:87:9 | a | Variable 'a' is assigned a value that is never used. | | main.rs:108:9:108:10 | is | Variable 'is' is assigned a value that is never used. | | main.rs:131:13:131:17 | total | Variable 'total' is assigned a value that is never used. | -| main.rs:194:13:194:31 | res | Variable 'res' is assigned a value that is never used. | -| main.rs:206:9:206:24 | kind | Variable 'kind' is assigned a value that is never used. | -| main.rs:210:9:210:32 | kind | Variable 'kind' is assigned a value that is never used. | | main.rs:266:13:266:17 | total | Variable 'total' is assigned a value that is never used. | -| main.rs:334:5:334:39 | kind | Variable 'kind' is assigned a value that is never used. | | main.rs:359:9:359:9 | x | Variable 'x' is assigned a value that is never used. | | main.rs:367:17:367:17 | x | Variable 'x' is assigned a value that is never used. | | more.rs:24:9:24:11 | val | Variable 'val' is assigned a value that is never used. | diff --git a/rust/ql/test/query-tests/unusedentities/main.rs b/rust/ql/test/query-tests/unusedentities/main.rs index 07485e3af7140..2de0b2b7d5ac0 100644 --- a/rust/ql/test/query-tests/unusedentities/main.rs +++ b/rust/ql/test/query-tests/unusedentities/main.rs @@ -191,7 +191,7 @@ fn loops() { } for x in 1..10 { - _ = format!("x is {x}"); // $ SPURIOUS: Alert[rust/unused-value] + _ = format!("x is {x}"); } for x in 1..10 { @@ -203,11 +203,11 @@ fn loops() { } for x in 1..10 { - assert_eq!(x, 1); // $ SPURIOUS: Alert[rust/unused-value] + assert_eq!(x, 1); } for x in 1..10 { - assert_eq!(id(x), id(1)); // $ SPURIOUS: Alert[rust/unused-value] + assert_eq!(id(x), id(1)); } } @@ -331,7 +331,7 @@ fn if_lets_matches() { } let duration1 = std::time::Duration::new(10, 0); // ten seconds - assert_eq!(duration1.as_secs(), 10); // $ SPURIOUS: Alert[rust/unused-value] + assert_eq!(duration1.as_secs(), 10); let duration2: Result = Ok(std::time::Duration::new(10, 0)); match duration2 {