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

let-underscore-must-use is triggered when Debug is derived #4980

Closed
ArekPiekarz opened this issue Jan 1, 2020 · 4 comments · Fixed by #5082
Closed

let-underscore-must-use is triggered when Debug is derived #4980

ArekPiekarz opened this issue Jan 1, 2020 · 4 comments · Fixed by #5082
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy T-macros Type: Issues with macros and macro expansion

Comments

@ArekPiekarz
Copy link

ArekPiekarz commented Jan 1, 2020

cargo clippy -V: clippy 0.0.212 (c807fbc 2019-12-29)
rustc -V: rustc 1.42.0-nightly (da3629b 2019-12-29)

Summary

Deriving Debug on a struct with at least one field triggers let-underscore-must-use lint.

Steps to reproduce

Given the following code:

#[derive(Debug)]
struct Foo {
    field: i32,
}

fn main() {
}

When cargo clippy -- -W clippy::let-underscore-must-use is run it emits a warning:

warning: non-binding let on an expression with #[must_use] type
 --> src/main.rs:1:10
  |
1 | #[derive(Debug)]
  |          ^^^^^
  |
  = note: requested on the command line with `-W clippy::let-underscore-must-use`
  = help: consider explicitly using expression value
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_must_use

If we run cargo expand, we can see the compiler generates an anonymous binding indeed:

#[automatically_derived]
#[allow(unused_qualifications)]
impl ::core::fmt::Debug for Foo {
    fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
        match *self {
            Foo {
                field: ref __self_0_0,
            } => {
                let mut debug_trait_builder = f.debug_struct("Foo");
                let _ = debug_trait_builder.field("field", &&(*__self_0_0));
                debug_trait_builder.finish()
            }
        }
    }
}
@ghost
Copy link

ghost commented Jan 2, 2020

Here is some analysis of what is going on.

Running cargo expand on the sample shows that this impl block is generated:

#[automatically_derived]
#[allow(unused_qualifications)]
impl ::core::fmt::Debug for Foo {
    fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
        match *self {
            Foo {
                field: ref __self_0_0,
            } => {
                let mut debug_trait_builder = f.debug_struct("Foo");
                let _ = debug_trait_builder.field("field", &&(*__self_0_0));
                debug_trait_builder.finish()
            }
        }
    }
}

debug_trait_builder has type DebugStruct. This type is must_use because it has the attribute #[must_use = "must eventually call ``finish()`` on Debug builders"]. field() is a builder method which returns a reference todebug_trait_builder. (Normally you'd just chain these calls like the DebugStruct documentation shows.) finish is correctly called on this struct on the next line.

It's a bug in the lint but I'm not sure what the best way of fixing it would be.

@ArekPiekarz
Copy link
Author

There are two separate problems here:

  1. Detecting if DebugStruct is used.
  2. Detecting if DebugStruct is used correctly.

The second one relates to the need to call finish() after configuring the builder. I think currently it's not possible to express that relation in the language. The only thing that comes close is using a typestate pattern, so the finish method would consume the builder, but currently it accepts &mut self. Because of that this point could be deferred into the future.

The first one could be solved by checking if the object pointed by the reference is used, not just the reference returned from the function. I don't know if Clippy is capable of such call analysis, especially when in general the object could be passed away from the place of its construction and used much later in some other part of the program.

@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy T-macros Type: Issues with macros and macro expansion labels Jan 4, 2020
@flip1995
Copy link
Member

flip1995 commented Jan 4, 2020

I think it's save to just disable this lint in external macros.

@flip1995 flip1995 added the C-bug Category: Clippy is not doing the correct thing label Jan 4, 2020
bors added a commit that referenced this issue Jan 23, 2020
disable let_underscore_must_use in external macros

changelog: disable let_underscore_must_use in external macros
Closes #4980
@bors bors closed this as completed in 3237b7a Jan 23, 2020
@jendrikw
Copy link
Contributor

jendrikw commented Aug 6, 2022

This is still an issue when using other crates to derive traits. Here is an example using derivative to derive Debug, which fires a FP.

playground

#![deny(clippy::let_underscore_must_use)]

use derivative::Derivative; // 2.2.0

#[derive(Derivative)]
#[derivative(Debug)]
struct S {
    x: i32,
}

Output from clippy:

error: non-binding let on an expression with `#[must_use]` type
 --> src/lib.rs:7:8
  |
7 | struct S {
  |        ^
  |
note: the lint level is defined here
 --> src/lib.rs:1:9
  |
1 | #![deny(clippy::let_underscore_must_use)]
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  = help: consider explicitly using expression value
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_must_use

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy T-macros Type: Issues with macros and macro expansion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants