Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unused_variables and unused_assignments lints do not catch array usage #65467

Open
jamesmunns opened this issue Oct 16, 2019 · 7 comments
Open
Labels
A-array Area: `[T; N]` A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@jamesmunns
Copy link
Member

I recently had a problem where I was accidentally implicitly copying an array with if let bindings, and the writes to the original array were never made (I needed to add ref in my bindings). I thought it was strange that there was no warnings for this, and noticed this is true for these lints in general. Here's a small reproduction (also on the playground).

fn main() {
    // Write but never read. No warning is emitted
    let mut arr = [0; 5];
    arr[0] = 4;
    [0][0] = 0;
    
    // Write but never read, the following warnings are emitted
    
    // warning: variable `x` is assigned to, but never used
    //  --> src/main.rs:7:13
    //   |
    // 7 |     let mut x = 4;
    //   |             ^
    //   |
    //   = note: `#[warn(unused_variables)]` on by default
    //   = note: consider using `_x` instead
    
    // warning: value assigned to `x` is never read
    //  --> src/main.rs:8:5
    //   |
    // 8 |     x = 12;
    //   |     ^
    //   |
    //   = note: `#[warn(unused_assignments)]` on by default
    //   = help: maybe it is overwritten before being read?
    let mut x = 4;
    x = 12;
}

I would expect these lints to fire here, is this expected behavior? I tried a quick search and wasn't able to find an existing issue.

@jonas-schievink jonas-schievink added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 16, 2019
@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Oct 16, 2019
@Centril
Copy link
Contributor

Centril commented Oct 16, 2019

Adding T-Lang for the "do we want to do this" aspect of the lint beyond it's execution (T-Compiler).

I would expect these lints to fire here, is this expected behavior?

The current behavior is expected. These lints are defined through a liveness analysis done on HIR rather than MIR and understanding arrays specially on HIR might not be worth the impl complexity.

@Centril
Copy link
Contributor

Centril commented Oct 16, 2019

However, @eddyb suggested that we rewrite liveness to work on MIR instead and then we should be able to make this work and then some. Cc @nikomatsakis @matthewjasper @tmandry

@estebank
Copy link
Contributor

If we extend rustc to detect this case it would be the same machinery necessary to support #58792, either to suggest split_at_mut or to make it unnecessary by keeping track of non-overlapping mutable borrows during borrowck.

@nikomatsakis
Copy link
Contributor

My opinion is that it would be good to detect cases like this -- probably we could say that any write to the 'owned content' of a variable suffices. I agree it'd probably be somewhat easier to do this sort of thing on MIR -- certainly I consider the existing 'liveness' code to be in need of a rewrite.

There are some questions as to how smart we want to be. One could imagine for example warning on a case like this

let mut foo =(0, 0);
foo.0 = 22;
println!("{}", foo.1);

Here, the write to foo.0 is dead.

@Centril
Copy link
Contributor

Centril commented Oct 18, 2019

Here, the write to foo.0 is dead.

It seems to me that declaring foo.0 dead in the lint here is more conservative than doing so for arrays so if we accept @jamesmunns's idea we ought to declare foo.0 dead as well.
We could also divide the aggressiveness into different lints (still same machinery, just different names for the lints.)

@pacak
Copy link
Contributor

pacak commented Dec 15, 2021

More examples, here neither q nor foo are read back.

#[derive(Copy, Clone)]
pub struct Foo {
    pub foo: usize,
}

pub fn foo(src: Foo) {
    let mut q = src;
    q.foo = 12;
}
    let mut foo = (0, 0);                                                                  
    foo.0 = 22;      

@camsteffen
Copy link
Contributor

I've linked a couple Clippy issues to this, for more examples.

Also it might be good to detect unused Futures in a similar way, as in rust-lang/rust-clippy#8126.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-array Area: `[T; N]` A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants