Skip to content
Merged
7 changes: 4 additions & 3 deletions rust/ql/src/queries/unusedentities/UnusedValue.ql
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ import UnusedVariable
from AstNode write, Ssa::Variable v
where
variableWrite(write, v) and
not v instanceof DiscardVariable and
not write.isInMacroExpansion() and
not isAllowableUnused(v) and
// SSA definitions are only created for live writes
not write = any(Ssa::WriteDefinition def).getWriteAccess().getAstNode() and
// avoid overlap with the unused variable query
not isUnused(v) and
not v instanceof DiscardVariable and
not write.isInMacroExpansion()
not isUnused(v)
select write, "Variable $@ is assigned a value that is never used.", v, v.getText()
17 changes: 17 additions & 0 deletions rust/ql/src/queries/unusedentities/UnusedVariable.qll
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,20 @@ predicate isUnused(Variable v) {
not exists(v.getInitializer())
}

/**
* A callable for which we have incomplete information, for example because an unexpanded
* macro call is present. These callables are prone to false positive results from unused
* entities queries, unless they are excluded from results.
*/
class IncompleteCallable extends Callable {
IncompleteCallable() {
exists(MacroExpr me |
me.getEnclosingCallable() = this and
not me.getMacroCall().hasExpanded()
)
}
}

/**
* Holds if variable `v` is in a context where we may not find a use for it,
* but that's expected and should not be considered a problem.
Expand All @@ -24,6 +38,9 @@ predicate isAllowableUnused(Variable v) {
// in a macro expansion
v.getPat().isInMacroExpansion()
or
// declared in an incomplete callable
v.getEnclosingCfgScope() instanceof IncompleteCallable
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this works, it would perhaps be more correct to only exclude results when there is an unexpandable macro in the scope that the variable is defined. However, I realize that the concept of variable scope is not currently exposed, so perhaps that is better done follow-up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is required to expose the concept of a variable scope?

I've added a test that exposes the situation I think we're talking about here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would have to expose the VariableScope class from VariableImpl.qll.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've added it to the existing improvements issue for rust/unused-variable. It sounds like a straightforward follow-up, apart from the need to go through another cycle of DCA / testing.

Is there any reason VariableScope shouldn't be exposed publically?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason VariableScope shouldn't be exposed publically?

Only that we haven't needed it before. We may also have to change it slightly, for example right now x and y are considered to be in the same scope in

let x = ...;
let y = ...;

or
// a 'self' variable
v.getText() = "self"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
extractionWarning
| undefined_macros/main.rs:1:1:1:1 | semantic analyzer unavailable (not included as a module) |
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
| main.rs:284:13:284:17 | total | Variable $@ is assigned a value that is never used. | main.rs:252:13:252:17 | total | total |
| main.rs:377:9:377:9 | x | Variable $@ is assigned a value that is never used. | main.rs:377:9:377:9 | x | x |
| main.rs:385:17:385:17 | x | Variable $@ is assigned a value that is never used. | main.rs:385:17:385:17 | x | x |
| main.rs:486:9:486:9 | c | Variable $@ is assigned a value that is never used. | main.rs:486:9:486:9 | c | c |
| main.rs:519:9:519:20 | var_in_macro | Variable $@ is assigned a value that is never used. | main.rs:519:9:519:20 | var_in_macro | var_in_macro |
| main.rs:531:9:531:20 | var_in_macro | Variable $@ is assigned a value that is never used. | main.rs:531:9:531:20 | var_in_macro | var_in_macro |
| main.rs:540:9:540:9 | c | Variable $@ is assigned a value that is never used. | main.rs:540:9:540:9 | c | c |
| more.rs:44:9:44:14 | a_ptr4 | Variable $@ is assigned a value that is never used. | more.rs:44:9:44:14 | a_ptr4 | a_ptr4 |
| more.rs:59:9:59:13 | d_ptr | Variable $@ is assigned a value that is never used. | more.rs:59:9:59:13 | d_ptr | d_ptr |
| more.rs:65:13:65:17 | f_ptr | Variable $@ is assigned a value that is never used. | more.rs:65:13:65:17 | f_ptr | f_ptr |
78 changes: 60 additions & 18 deletions rust/ql/test/query-tests/unusedentities/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -468,16 +468,70 @@ fn traits() {

// --- macros ---

fn macros() {
macro_rules! set_value {
($x:expr,$y:expr) => {
$x = $y
};
}

macro_rules! use_value {
($x:expr) => {
println!("{}", $x)
};
}

fn macros1() {
let a: u16;
let b: u16 = 2;
set_value!(a, 1);
use_value!(b);

match std::env::args().nth(1).unwrap().parse::<u16>() {
Ok(n) => {
use_value!(n);
}
_ => {}
}
}

fn macros2() {
let a: u16 = 3;
println!("{}", a);

match std::env::args().nth(1).unwrap().parse::<u16>() {
Ok(n) => {
println!("{}", n);
}
_ => {}
}
}

fn macros3() {
let x;
println!(
"The value of x is {}",
({
x = 10; // $ MISSING: Alert[rust/unused-value]
10
})
)
);
}

macro_rules! let_in_macro {
($e:expr) => {{
let var_in_macro = 0;
$e
}};
}

// Our analysis does not currently respect the hygiene rules of Rust macros
// (https://veykril.github.io/tlborm/decl-macros/minutiae/hygiene.html), because
// all we have access to is the expanded AST
fn hygiene_mismatch() {
let var_in_macro = 0; // $ SPURIOUS: Alert[rust/unused-value]
let_in_macro!(var_in_macro);
}

// --- references ---

fn references() {
Expand Down Expand Up @@ -505,21 +559,6 @@ trait MyTrait {
fn my_func2(&self, x: i32) -> i32;
}

macro_rules! let_in_macro {
($e:expr) => {{
let var_in_macro = 0;
$e
}};
}

// Our analysis does not currently respect the hygiene rules of Rust macros
// (https://veykril.github.io/tlborm/decl-macros/minutiae/hygiene.html), because
// all we have access to is the expanded AST
fn hygiene_mismatch() {
let var_in_macro = 0; // $ SPURIOUS: Alert[rust/unused-value]
let_in_macro!(var_in_macro);
}

// --- main ---

fn main() {
Expand All @@ -534,7 +573,10 @@ fn main() {
shadowing();
func_ptrs();
folds_and_closures();
macros();
macros1();
macros2();
macros3();
hygiene_mismatch();
references();

generics();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@

// --- undefined macro calls ---

fn undefined_macros1() {
let a: u16;

undefined_macro_call!(d);
}

fn undefined_macros2() {
{
let a: u16 = 1; // $ MISSING: Alert[rust/unused-value]
}

undefined_macro_call!(5);

let b: u16; // $ MISSING: Alert[rust/unused-variable]
}

fn undefined_macros3() {
match std::env::args().nth(1).unwrap().parse::<u16>() {
Ok(n) => {
undefined_macro_call!(n);
}
_ => {}
}
}

fn main() {
undefined_macros1();
undefined_macros2();
undefined_macros3();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
qltest_cargo_check: false