Merge pull request #17642 from hvitved/rust/unused-variable

Rust: Implement `UnusedVariable.ql`
2025-12-17 01:03:14 +01:00 · 2024-10-02 15:41:24 +02:00
parent 3fa52ad680 3a1f6efce4
commit d6415cd0c8
7 changed files with 272 additions and 96 deletions
--- a/rust/ql/lib/codeql/rust/elements/Variable.qll
+++ b/rust/ql/lib/codeql/rust/elements/Variable.qll
@@ -7,3 +7,7 @@ private import internal.VariableImpl
 final class Variable = Impl::Variable;

 final class VariableAccess = Impl::VariableAccess;
+
+final class VariableWriteAccess = Impl::VariableWriteAccess;
+
+final class VariableReadAccess = Impl::VariableReadAccess;
--- a/rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll
+++ b/rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll
@@ -98,6 +98,27 @@ module Impl {

    /** Gets an access to this variable. */
    VariableAccess getAnAccess() { result.getVariable() = this }
+
+    /**
+     * Gets the pattern that declares this variable.
+     *
+     * Normally, the pattern is unique, except when introduced in an or pattern:
+     *
+     * ```rust
+     * match either {
+     *    Either::Left(x) | Either::Right(x) => println!(x),
+     * }
+     * ```
+     */
+    IdentPat getPat() { variableDecl(definingNode, result, name) }
+
+    /** Gets the initial value of this variable, if any. */
+    Expr getInitializer() {
+      exists(LetStmt let |
+        this.getPat() = let.getPat() and
+        result = let.getInitializer()
+      )
+    }
  }

  /** A path expression that may access a local variable. */
@@ -366,6 +387,38 @@ module Impl {
    override string getAPrimaryQlClass() { result = "VariableAccess" }
  }

+  /** Holds if `e` occurs in the LHS of an assignment or compound assignment. */
+  private predicate assignLhs(Expr e, boolean compound) {
+    exists(BinaryExpr be, string op |
+      op = be.getOperatorName().regexpCapture("(.*)=", 1) and
+      e = be.getLhs()
+    |
+      op = "" and compound = false
+      or
+      op != "" and compound = true
+    )
+    or
+    exists(Expr mid |
+      assignLhs(mid, compound) and
+      getImmediateParent(e) = mid
+    )
+  }
+
+  /** A variable write. */
+  class VariableWriteAccess extends VariableAccess {
+    VariableWriteAccess() { assignLhs(this, _) }
+  }
+
+  /** A variable read. */
+  class VariableReadAccess extends VariableAccess {
+    VariableReadAccess() {
+      not this instanceof VariableWriteAccess
+      or
+      // consider LHS in compound assignments both reads and writes
+      assignLhs(this, true)
+    }
+  }
+
  cached
  private module Cached {
    cached
--- a/rust/ql/src/queries/unusedentities/UnusedVariable.ql
+++ b/rust/ql/src/queries/unusedentities/UnusedVariable.ql
@@ -3,13 +3,16 @@
 * @description Unused variables may be an indication that the code is incomplete or has a typo.
 * @kind problem
 * @problem.severity recommendation
- * @precision medium
+ * @precision high
 * @id rust/unused-variable
 * @tags maintainability
 */

 import rust

-from Locatable e
-where none() // TODO: implement query
-select e, "Variable is not used."
+from Variable v
+where
+  not exists(v.getAnAccess()) and
+  not exists(v.getInitializer()) and
+  not v.getName().charAt(0) = "_"
+select v, "Variable is not used."
--- a/rust/ql/test/library-tests/variables/variables.expected
+++ b/rust/ql/test/library-tests/variables/variables.expected
@@ -146,3 +146,109 @@ variableAccess
 | variables.rs:306:15:306:16 | n2 | variables.rs:304:9:304:10 | n2 |
 | variables.rs:313:12:313:12 | v | variables.rs:310:9:310:9 | v |
 | variables.rs:314:19:314:22 | text | variables.rs:312:9:312:12 | text |
+variableWriteAccess
+| variables.rs:17:5:17:6 | x2 | variables.rs:15:13:15:14 | x2 |
+| variables.rs:266:9:266:10 | c2 | variables.rs:259:13:259:14 | c2 |
+| variables.rs:267:9:267:10 | b4 | variables.rs:258:13:258:14 | b4 |
+| variables.rs:268:9:268:11 | a10 | variables.rs:257:13:257:15 | a10 |
+variableReadAccess
+| variables.rs:11:15:11:16 | x1 | variables.rs:10:9:10:10 | x1 |
+| variables.rs:16:15:16:16 | x2 | variables.rs:15:13:15:14 | x2 |
+| variables.rs:18:15:18:16 | x2 | variables.rs:15:13:15:14 | x2 |
+| variables.rs:23:15:23:16 | x3 | variables.rs:22:9:22:10 | x3 |
+| variables.rs:25:9:25:10 | x3 | variables.rs:22:9:22:10 | x3 |
+| variables.rs:26:15:26:16 | x3 | variables.rs:24:9:24:10 | x3 |
+| variables.rs:31:15:31:16 | x4 | variables.rs:30:9:30:10 | x4 |
+| variables.rs:34:19:34:20 | x4 | variables.rs:33:13:33:14 | x4 |
+| variables.rs:36:15:36:16 | x4 | variables.rs:30:9:30:10 | x4 |
+| variables.rs:55:15:55:16 | a1 | variables.rs:47:13:47:14 | a1 |
+| variables.rs:56:15:56:16 | b1 | variables.rs:48:13:48:14 | b1 |
+| variables.rs:57:15:57:15 | x | variables.rs:51:13:51:13 | x |
+| variables.rs:58:15:58:15 | y | variables.rs:52:13:52:13 | y |
+| variables.rs:66:9:66:10 | p1 | variables.rs:62:9:62:10 | p1 |
+| variables.rs:67:15:67:16 | a2 | variables.rs:64:12:64:13 | a2 |
+| variables.rs:68:15:68:16 | b2 | variables.rs:65:12:65:13 | b2 |
+| variables.rs:75:11:75:12 | s1 | variables.rs:72:9:72:10 | s1 |
+| variables.rs:76:19:76:20 | s2 | variables.rs:74:21:74:22 | s2 |
+| variables.rs:85:15:85:16 | x5 | variables.rs:81:14:81:15 | x5 |
+| variables.rs:92:11:92:12 | x6 | variables.rs:89:9:89:10 | x6 |
+| variables.rs:97:23:97:24 | y1 | variables.rs:94:14:94:15 | y1 |
+| variables.rs:102:15:102:16 | y1 | variables.rs:90:9:90:10 | y1 |
+| variables.rs:108:11:108:17 | numbers | variables.rs:106:9:106:15 | numbers |
+| variables.rs:114:23:114:27 | first | variables.rs:110:13:110:17 | first |
+| variables.rs:115:23:115:27 | third | variables.rs:111:13:111:17 | third |
+| variables.rs:116:23:116:27 | fifth | variables.rs:112:13:112:17 | fifth |
+| variables.rs:120:11:120:17 | numbers | variables.rs:106:9:106:15 | numbers |
+| variables.rs:126:23:126:27 | first | variables.rs:122:13:122:17 | first |
+| variables.rs:127:23:127:26 | last | variables.rs:124:13:124:16 | last |
+| variables.rs:135:11:135:12 | p2 | variables.rs:133:9:133:10 | p2 |
+| variables.rs:138:24:138:25 | x7 | variables.rs:137:16:137:17 | x7 |
+| variables.rs:149:11:149:13 | msg | variables.rs:147:9:147:11 | msg |
+| variables.rs:152:24:152:34 | id_variable | variables.rs:151:17:151:27 | id_variable |
+| variables.rs:157:23:157:24 | id | variables.rs:156:26:156:27 | id |
+| variables.rs:168:11:168:16 | either | variables.rs:167:9:167:14 | either |
+| variables.rs:170:26:170:27 | a3 | variables.rs:169:9:169:44 | a3 |
+| variables.rs:182:11:182:12 | tv | variables.rs:181:9:181:10 | tv |
+| variables.rs:184:26:184:27 | a4 | variables.rs:183:9:183:81 | a4 |
+| variables.rs:186:11:186:12 | tv | variables.rs:181:9:181:10 | tv |
+| variables.rs:188:26:188:27 | a5 | variables.rs:187:9:187:83 | a5 |
+| variables.rs:190:11:190:12 | tv | variables.rs:181:9:181:10 | tv |
+| variables.rs:192:26:192:27 | a6 | variables.rs:191:9:191:83 | a6 |
+| variables.rs:198:11:198:16 | either | variables.rs:197:9:197:14 | either |
+| variables.rs:200:16:200:17 | a7 | variables.rs:199:9:199:44 | a7 |
+| variables.rs:201:26:201:27 | a7 | variables.rs:199:9:199:44 | a7 |
+| variables.rs:209:11:209:16 | either | variables.rs:207:9:207:14 | either |
+| variables.rs:213:23:213:25 | a11 | variables.rs:211:14:211:51 | a11 |
+| variables.rs:215:15:215:15 | e | variables.rs:210:13:210:13 | e |
+| variables.rs:216:28:216:30 | a12 | variables.rs:214:33:214:35 | a12 |
+| variables.rs:232:11:232:12 | fv | variables.rs:231:9:231:10 | fv |
+| variables.rs:234:26:234:28 | a13 | variables.rs:233:9:233:109 | a13 |
+| variables.rs:244:15:244:16 | a8 | variables.rs:239:5:239:6 | a8 |
+| variables.rs:245:15:245:16 | b3 | variables.rs:241:9:241:10 | b3 |
+| variables.rs:246:15:246:16 | c1 | variables.rs:242:9:242:10 | c1 |
+| variables.rs:252:15:252:16 | a9 | variables.rs:250:6:250:41 | a9 |
+| variables.rs:261:15:261:17 | a10 | variables.rs:257:13:257:15 | a10 |
+| variables.rs:262:15:262:16 | b4 | variables.rs:258:13:258:14 | b4 |
+| variables.rs:263:15:263:16 | c2 | variables.rs:259:13:259:14 | c2 |
+| variables.rs:270:9:270:11 | a10 | variables.rs:257:13:257:15 | a10 |
+| variables.rs:271:9:271:10 | b4 | variables.rs:258:13:258:14 | b4 |
+| variables.rs:272:9:272:10 | c2 | variables.rs:259:13:259:14 | c2 |
+| variables.rs:274:15:274:17 | a10 | variables.rs:257:13:257:15 | a10 |
+| variables.rs:275:15:275:16 | b4 | variables.rs:258:13:258:14 | b4 |
+| variables.rs:276:15:276:16 | c2 | variables.rs:259:13:259:14 | c2 |
+| variables.rs:283:23:283:25 | a10 | variables.rs:280:13:280:15 | a10 |
+| variables.rs:284:23:284:24 | b4 | variables.rs:281:13:281:14 | b4 |
+| variables.rs:288:15:288:17 | a10 | variables.rs:257:13:257:15 | a10 |
+| variables.rs:289:15:289:16 | b4 | variables.rs:258:13:258:14 | b4 |
+| variables.rs:295:9:295:9 | x | variables.rs:294:10:294:10 | x |
+| variables.rs:297:9:297:23 | example_closure | variables.rs:293:9:293:23 | example_closure |
+| variables.rs:298:15:298:16 | n1 | variables.rs:296:9:296:10 | n1 |
+| variables.rs:303:9:303:9 | x | variables.rs:302:10:302:10 | x |
+| variables.rs:305:9:305:26 | immutable_variable | variables.rs:301:9:301:26 | immutable_variable |
+| variables.rs:306:15:306:16 | n2 | variables.rs:304:9:304:10 | n2 |
+| variables.rs:313:12:313:12 | v | variables.rs:310:9:310:9 | v |
+| variables.rs:314:19:314:22 | text | variables.rs:312:9:312:12 | text |
+variableInitializer
+| variables.rs:10:9:10:10 | x1 | variables.rs:10:14:10:16 | "a" |
+| variables.rs:15:13:15:14 | x2 | variables.rs:15:18:15:18 | 4 |
+| variables.rs:22:9:22:10 | x3 | variables.rs:22:14:22:14 | 1 |
+| variables.rs:24:9:24:10 | x3 | variables.rs:25:9:25:14 | ... + ... |
+| variables.rs:30:9:30:10 | x4 | variables.rs:30:14:30:16 | "a" |
+| variables.rs:33:13:33:14 | x4 | variables.rs:33:18:33:20 | "b" |
+| variables.rs:62:9:62:10 | p1 | variables.rs:62:14:62:37 | RecordExpr |
+| variables.rs:72:9:72:10 | s1 | variables.rs:72:14:72:41 | CallExpr |
+| variables.rs:89:9:89:10 | x6 | variables.rs:89:14:89:20 | CallExpr |
+| variables.rs:90:9:90:10 | y1 | variables.rs:90:14:90:15 | 10 |
+| variables.rs:106:9:106:15 | numbers | variables.rs:106:19:106:35 | TupleExpr |
+| variables.rs:133:9:133:10 | p2 | variables.rs:133:14:133:37 | RecordExpr |
+| variables.rs:147:9:147:11 | msg | variables.rs:147:15:147:38 | RecordExpr |
+| variables.rs:167:9:167:14 | either | variables.rs:167:18:167:33 | CallExpr |
+| variables.rs:181:9:181:10 | tv | variables.rs:181:14:181:36 | CallExpr |
+| variables.rs:197:9:197:14 | either | variables.rs:197:18:197:33 | CallExpr |
+| variables.rs:207:9:207:14 | either | variables.rs:207:18:207:33 | CallExpr |
+| variables.rs:231:9:231:10 | fv | variables.rs:231:14:231:35 | CallExpr |
+| variables.rs:293:9:293:23 | example_closure | variables.rs:294:9:295:9 | ClosureExpr |
+| variables.rs:296:9:296:10 | n1 | variables.rs:297:9:297:26 | CallExpr |
+| variables.rs:301:9:301:26 | immutable_variable | variables.rs:302:9:303:9 | ClosureExpr |
+| variables.rs:304:9:304:10 | n2 | variables.rs:305:9:305:29 | CallExpr |
+| variables.rs:310:9:310:9 | v | variables.rs:310:13:310:41 | RefExpr |
--- a/rust/ql/test/library-tests/variables/variables.ql
+++ b/rust/ql/test/library-tests/variables/variables.ql
@@ -5,6 +5,12 @@ query predicate variable(Variable v) { any() }

 query predicate variableAccess(VariableAccess va, Variable v) { v = va.getVariable() }

+query predicate variableWriteAccess(VariableWriteAccess va, Variable v) { v = va.getVariable() }
+
+query predicate variableReadAccess(VariableReadAccess va, Variable v) { v = va.getVariable() }
+
+query predicate variableInitializer(Variable v, Expr e) { e = v.getInitializer() }
+
 module VariableAccessTest implements TestSig {
  string getARelevantTag() { result = "access" }

--- a/rust/ql/test/query-tests/unusedentities/UnusedVariable.expected
+++ b/rust/ql/test/query-tests/unusedentities/UnusedVariable.expected
@@ -0,0 +1,4 @@
+| main.rs:23:9:23:9 | a | Variable is not used. |
+| main.rs:88:13:88:13 | d | Variable is not used. |
+| main.rs:112:9:112:9 | k | Variable is not used. |
+| main.rs:139:5:139:5 | y | Variable is not used. |
--- a/rust/ql/test/query-tests/unusedentities/main.rs
+++ b/rust/ql/test/query-tests/unusedentities/main.rs
@@ -1,118 +1,119 @@
-
 //fn cond() -> bool;

 // --- locals ---

 fn locals_1() {
-	let a = 1; // BAD: unused value [NOT DETECTED]
-	let b = 1;
-	let c = 1;
-	let d = String::from("a"); // BAD: unused value [NOT DETECTED]
-	let e = String::from("b");
-	let _ = 1; // (deliberately unused)
+    let a = 1; // BAD: unused value [NOT DETECTED]
+    let b = 1;
+    let c = 1;
+    let d = String::from("a"); // BAD: unused value [NOT DETECTED]
+    let e = String::from("b");
+    let _ = 1; // (deliberately unused)

-	println!("use {}", b);
+    println!("use {}", b);

-	if cond() {
-		println!("use {}", c);
-	}
+    if cond() {
+        println!("use {}", c);
+    }

-	println!("use {}", e);
+    println!("use {}", e);
 }

 fn locals_2() {
-	let a: i32;
-	let b: i32; // BAD: unused variable [NOT DETECTED]
-	let mut c: i32;
-	let mut d: i32;
-	let mut e: i32;
-	let mut f: i32;
-	let g: i32;
-	let h: i32;
-	let i: i32;
+    let a: i32; // BAD: unused variable
+    let b: i32;
+    let mut c: i32;
+    let mut d: i32;
+    let mut e: i32;
+    let mut f: i32;
+    let g: i32;
+    let h: i32;
+    let i: i32;

-	b = 1; // BAD: unused value [NOT DETECTED]
+    b = 1; // BAD: unused value [NOT DETECTED]

-	c = 1; // BAD: unused value [NOT DETECTED]
-	c = 2;
-	println!("use {}", c);
-	c = 3; // BAD: unused value [NOT DETECTED]
+    c = 1; // BAD: unused value [NOT DETECTED]
+    c = 2;
+    println!("use {}", c);
+    c = 3; // BAD: unused value [NOT DETECTED]

-	d = 1;
-	if cond() {
-		d = 2; // BAD: unused value [NOT DETECTED]
-		d = 3;
-	} else {
-	}
-	println!("use {}", d);
+    d = 1;
+    if cond() {
+        d = 2; // BAD: unused value [NOT DETECTED]
+        d = 3;
+    } else {
+    }
+    println!("use {}", d);

-	e = 1; // BAD: unused value [NOT DETECTED]
-	if cond() {
-		e = 2;
-	} else {
-		e = 3;
-	}
-	println!("use {}", e);
+    e = 1; // BAD: unused value [NOT DETECTED]
+    if cond() {
+        e = 2;
+    } else {
+        e = 3;
+    }
+    println!("use {}", e);

-	f = 1;
-	f += 1;
-	println!("use {}", f);
-	f += 1; // BAD: unused value [NOT DETECTED]
-	f = 1;
-	f += 1; // BAD: unused value [NOT DETECTED]
+    f = 1;
+    f += 1;
+    println!("use {}", f);
+    f += 1; // BAD: unused value [NOT DETECTED]
+    f = 1;
+    f += 1; // BAD: unused value [NOT DETECTED]

-	g = if cond() { 1 } else { 2 }; // BAD: unused value (x2) [NOT DETECTED]
-	h = if cond() { 3 } else { 4 };
-	i = if cond() { h } else { 5 };
-	println!("use {}", i);
+    g = if cond() { 1 } else { 2 }; // BAD: unused value (x2) [NOT DETECTED]
+    h = if cond() { 3 } else { 4 };
+    i = if cond() { h } else { 5 };
+    println!("use {}", i);

-	_ = 1; // (deliberately unused) [NOT DETECTED]
+    _ = 1; // (deliberately unused) [NOT DETECTED]
 }

 // --- structs ---

 #[derive(Debug)]
 struct MyStruct {
-	val: i64
+    val: i64,
 }

 impl MyStruct {
-	fn my_get(&mut self) -> i64 {
-		return self.val;
-	}
+    fn my_get(&mut self) -> i64 {
+        return self.val;
+    }
 }

 fn structs() {
-	let a = MyStruct {val : 1 }; // BAD: unused value [NOT DETECTED]
-	let b = MyStruct {val : 2 };
-	let c = MyStruct {val : 3 };
-	let mut d : MyStruct; // BAD: unused variable [NOT DETECTED]
-	let mut e : MyStruct;
-	let mut f : MyStruct;
+    let a = MyStruct { val: 1 }; // BAD: unused value [NOT DETECTED]
+    let b = MyStruct { val: 2 };
+    let c = MyStruct { val: 3 };
+    let mut d: MyStruct; // BAD: unused variable
+    let mut e: MyStruct;
+    let mut f: MyStruct;

-	println!("lets use {:?} and {}", b, c.val);
+    println!("lets use {:?} and {}", b, c.val);

-	e = MyStruct {val : 4 };
-	println!("lets use {}", e.my_get());
-	e.val = 5;
-	println!("lets use {}", e.my_get());
+    e = MyStruct { val: 4 };
+    println!("lets use {}", e.my_get());
+    e.val = 5;
+    println!("lets use {}", e.my_get());

-	f = MyStruct {val : 6 }; // BAD: unused value [NOT DETECTED]
-	f.val = 7; // BAD: unused value [NOT DETECTED]
+    f = MyStruct { val: 6 }; // BAD: unused value [NOT DETECTED]
+    f.val = 7; // BAD: unused value [NOT DETECTED]
 }

 // --- arrays ---

 fn arrays() {
-	let is = [1, 2, 3]; // BAD: unused values (x3) [NOT DETECTED]
-	let js = [1, 2, 3];
-	let ks = [1, 2, 3];
+    let is = [1, 2, 3]; // BAD: unused values (x3) [NOT DETECTED]
+    let js = [1, 2, 3];
+    let ks = [1, 2, 3];

-	println!("lets use {:?}", js);
+    println!("lets use {:?}", js);

-	for k in ks {
-		println!("lets use {}", k);
-	}
+    for k // SPURIOUS: unused variable [macros not yet supported]
+	in ks
+	{
+        println!("lets use {}", k);
+    }
 }

 // --- constants and statics ---
@@ -123,30 +124,29 @@ static mut STAT1: i32 = 1;
 static mut STAT2: i32 = 2; // BAD: unused value [NOT DETECTED]

 fn statics() {
-	static mut STAT3: i32 = 0;
-	static mut STAT4: i32 = 0; // BAD: unused value [NOT DETECTED]
+    static mut STAT3: i32 = 0;
+    static mut STAT4: i32 = 0; // BAD: unused value [NOT DETECTED]

-	unsafe
-	{
-		let total = CON1 + STAT1 + STAT3;
-	}
+    unsafe {
+        let total = CON1 + STAT1 + STAT3;
+    }
 }

 // --- parameters ---

 fn parameters(
-		x: i32,
-		y: i32, // BAD: unused variable [NOT DETECTED]
-		_z: i32 // (`_` is asking the compiler, and by extension us, to not warn that this is unused)
-	) -> i32 {
-	return x;
+    x: i32,
+    y: i32,  // BAD: unused variable
+    _z: i32, // (`_` is asking the compiler, and by extension us, to not warn that this is unused)
+) -> i32 {
+    return x;
 }

 fn main() {
-	locals_1();
-	locals_2();
-	structs();
-	arrays();
-	statics();
-	println!("lets use result {}", parameters(1, 2, 3));
+    locals_1();
+    locals_2();
+    structs();
+    arrays();
+    statics();
+    println!("lets use result {}", parameters(1, 2, 3));
 }