From 8634dd5f9849e277fe03d4c0ceb1749aa1585e99 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 2 Oct 2024 09:54:09 +0200 Subject: [PATCH] Rust: Implement `UnusedVariable.ql` --- rust/ql/lib/codeql/rust/elements/Variable.qll | 4 + .../rust/elements/internal/VariableImpl.qll | 44 ++++ .../queries/unusedentities/UnusedVariable.ql | 11 +- .../variables/variables.expected | 106 +++++++++ .../test/library-tests/variables/variables.ql | 6 + .../CONSISTENCY/CfgConsistency.expected | 12 +- .../unusedentities/UnusedVariable.expected | 4 + .../test/query-tests/unusedentities/main.rs | 204 +++++++++--------- 8 files changed, 279 insertions(+), 112 deletions(-) diff --git a/rust/ql/lib/codeql/rust/elements/Variable.qll b/rust/ql/lib/codeql/rust/elements/Variable.qll index 76c2d9f19f72..31cc17a263f8 100644 --- 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; diff --git a/rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll b/rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll index 58c9a4fb6ebf..0f7968079157 100644 --- 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,29 @@ module Impl { override string getAPrimaryQlClass() { result = "VariableAccess" } } + /** Holds if `e` occurs in the LHS of a (compound) assignment. */ + private predicate assignLhs(Expr e) { + exists(BinaryExpr be | + be.getOperatorName().regexpMatch(".*=") and + e = be.getLhs() + ) + or + exists(Expr mid | + assignLhs(mid) 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 } + } + cached private module Cached { cached diff --git a/rust/ql/src/queries/unusedentities/UnusedVariable.ql b/rust/ql/src/queries/unusedentities/UnusedVariable.ql index a057db7d1734..5fb0cdfecdc1 100644 --- 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." diff --git a/rust/ql/test/library-tests/variables/variables.expected b/rust/ql/test/library-tests/variables/variables.expected index ba563aeab6ef..c86e30d4bb57 100644 --- 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 | diff --git a/rust/ql/test/library-tests/variables/variables.ql b/rust/ql/test/library-tests/variables/variables.ql index b96f6ceabcae..dcca73d5cf28 100644 --- 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" } diff --git a/rust/ql/test/query-tests/unusedentities/CONSISTENCY/CfgConsistency.expected b/rust/ql/test/query-tests/unusedentities/CONSISTENCY/CfgConsistency.expected index f88f86c81777..1fa846391eca 100644 --- a/rust/ql/test/query-tests/unusedentities/CONSISTENCY/CfgConsistency.expected +++ b/rust/ql/test/query-tests/unusedentities/CONSISTENCY/CfgConsistency.expected @@ -1,8 +1,8 @@ deadEnd -| main.rs:14:2:14:23 | ExprStmt | -| main.rs:38:2:38:23 | ExprStmt | -| main.rs:93:2:93:44 | ExprStmt | -| main.rs:107:2:107:20 | LetStmt | -| main.rs:151:2:151:53 | ExprStmt | +| main.rs:13:5:13:26 | ExprStmt | +| main.rs:37:5:37:26 | ExprStmt | +| main.rs:92:5:92:47 | ExprStmt | +| main.rs:106:5:106:23 | LetStmt | +| main.rs:151:5:151:56 | ExprStmt | scopeNoFirst -| main.rs:125:1:133:1 | statics | +| main.rs:126:1:133:1 | statics | diff --git a/rust/ql/test/query-tests/unusedentities/UnusedVariable.expected b/rust/ql/test/query-tests/unusedentities/UnusedVariable.expected index e69de29bb2d1..f3aadfdff0d8 100644 --- 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. | diff --git a/rust/ql/test/query-tests/unusedentities/main.rs b/rust/ql/test/query-tests/unusedentities/main.rs index 8066f7c783e0..c58693ed8667 100644 --- 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; - - 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] - - 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); - - 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); - - _ = 1; // (deliberately unused) [NOT DETECTED] + 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] + + 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); + + 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] + + 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] } // --- 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; - - 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()); - - f = MyStruct {val : 6 }; // BAD: unused value [NOT DETECTED] - f.val = 7; // BAD: unused value [NOT DETECTED] + 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); + + 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] } // --- 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 // BAD: unused variable [SPURIOUS: 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)); }