Should dead code count toward var liveness?

compiler
1

Currently, the following code triggers unused_variables and unreachable_code. I feel that the unused_variables lint is just noise, and would prefer that variables usages in dead code still "count".

let x = 1;
return;
dbg!(x);

I'm working through refactoring the liveness analysis. If this change is acceptable, it would reduce complexity.

2 Likes
2

If the code isn't reachable, how is it used?

3

I guess I am differentiating between "used in code" and "used at runtime". In practice, if I have code like the above, it's most likely because I am just trying something out randomly ("what happens if the rest of this function just doesn't run?"). The unreachable_code lint is sufficient to help me remember to fix it.

8 Likes
4

x is used in the sense that the code wouldn't compile without it:

return;
// error[E0425]: cannot find value `x` in this scope
dbg!(x);
10 Likes
5

Yeah @quaternic I think what you're alluding to is that, if the user does the logical thing and deletes the variable, surprise!

1 Like
6

Hmm, with NLL we've somewhat decided that dead uses aren't uses. If the dead use was a "use" (as it was in 1.9), then this still wouldn't compile: https://rust.godbolt.org/z/e3bh5jhaG

So arguably saying it's "unused" is correct.

But also it's a lint, so it's more important that it be useful than pedantic.

What's the new definition you were thinking of using for it? Some sort of HIR-level thing based on "name resolution resolved to it at least once" instead of a MIR-level thing?

3 Likes
7

It is already implemented in HIR and has special handling of return etc. I'm just proposing to not do that. My definition would be "fires if the variable is never referenced in code" (I think that's effectively the same as what you said).

In other words, "code is reachable" is orthogonal to "code references variable x". Or it least it could be thought of that way.

8

Is a variable only used within a cfg! or #[cfg] that is false "used"?

9

Interesting. No, I would say cfg is different, it's like "I really intend for this code to be (conditionally) unreachable". And "unused for the current config" should be linted. But control-flow-unreachable code is usually supposed to be reachable, but regardless of whether it is, there's unreachable_code to tell the user something's wrong.

10

Ah, that's interesting.

I think it should definitely be either

  • Control-flow unaware and on HIR (like the unsafe checking will soon be), or
  • Control-flow aware and on MIR (like NLL and mut checking)

If it's currently control-flow aware yet on HIR, that definitely sounds like it should change.

I don't think I have a confident answer which way is the right solution.

Maybe for now just remove the flow-awareness from the HIR lint, and we can always add a "value never read on any possible path" kind of MIR lint later?

3 Likes
11

cfg! gated use is definitionally the same as if false { use; }. (I.e. control-flow dependent use.)

#[cfg] gated use is definitionally the same as {}. (I.e. no use.)

12

As an illustrative example:

if false {
    println!(); // not considered unreachable_code
}
return;
println!(); // considered unreachable_code

Another illustrative example:

pub fn demo1() -> String {
    let mut s = "1".to_string();
    let b = &mut s;              // unused_variables
    s = "2".to_string();
    return s;
    *b = "3".to_string();        // unreachable_code
}

pub fn demo2() -> String {
    let mut s = "1".to_string();
    let b = &mut s;
    s = "2".to_string();
    if false {
        *b = "3".to_string();    // cannot assign to `s` because it is borrowed
    }
    return s;
}

So

isn't quite fully accurate; you also have to define that how "dead uses" is defined, and that's roughly "exists after a -> ! expression." Or restated again, MIR control-flow aware lints/checks/etc are done after pruning unreachable-due-to-divergence basic blocks, but before unreachable-due-to-control-flow basic blocks are pruned.

I'm fairly in agreement that unused_code should be on MIR / divergence-aware, and that unused_variables should be on HIR / divergence-unaware.

It does seem potentially reasonable to have a separate unused_except_unreachable lint over MIR (so long as it doesn't duplicate HIR unused lints). Choosing whether it's available/on is a question of choosing between

  • emitted warnings should be independently fixable
  • fixing warnings should not introduce new warnings (that are trivial to predict)

but one could make a reasonable argument that the OP unused_variables lint is D-incorrect, because the implied (though not outright suggested) fix to remove the variable results in an error.

The unused_variables lint could be extended to suggest a machine-applicable fix of removing the full variable if it checks the expression to be clippy::no_effect.

13

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.