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

assigning_clones: Wrong suggestion with deref #12437

Closed
taiki-e opened this issue Mar 8, 2024 · 3 comments · Fixed by #12473
Closed

assigning_clones: Wrong suggestion with deref #12437

taiki-e opened this issue Mar 8, 2024 · 3 comments · Fixed by #12473
Labels
C-bug Category: Clippy is not doing the correct thing I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied

Comments

@taiki-e
Copy link
Member

taiki-e commented Mar 8, 2024

Summary

assigning_clones suggests code that fails to compile when the original code has deref.

Mentioning @Kobzol, who implemented this lint in #12077.

Reproducer

I tried this code:

#![allow(dead_code)]
#![warn(clippy::assigning_clones)]

use std::task::{Context, Waker};

fn f(mut prev: Box<Waker>, cx: &mut Context<'_>) -> Box<Waker> {
    *prev = cx.waker().clone();
    prev
}

I expected to see this happen: lint suggests code that can compile

Instead, this happened:

warning: assigning the result of `Clone::clone()` may be inefficient
 --> src/lib.rs:7:5
  |
7 |     *prev = cx.waker().clone();
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `prev.clone_from(cx.waker())`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assigning_clones

Suggested code prev.clone_from(cx.waker()) fails to compile because deref (*prev) is dropped:

#![allow(dead_code)]
#![warn(clippy::assigning_clones)]

use std::task::{Context, Waker};

fn f(mut prev: Box<Waker>, cx: &mut Context<'_>) -> Box<Waker> {
    prev.clone_from(cx.waker());
    prev
}
error[E0308]: mismatched types
   --> src/lib.rs:7:21
    |
7   |     prev.clone_from(cx.waker());
    |          ---------- ^^^^^^^^^^ expected `&Box<Waker>`, found `&Waker`
    |          |
    |          arguments to this method are incorrect
    |
    = note: expected reference `&std::boxed::Box<std::task::Waker>`
               found reference `&std::task::Waker`

The correct code is (*prev).clone_from(cx.waker()); (preserve deref).

#![allow(dead_code)]
#![warn(clippy::assigning_clones)]

use std::task::{Context, Waker};

fn f(mut prev: Box<Waker>, cx: &mut Context<'_>) -> Box<Waker> {
    (*prev).clone_from(cx.waker());
    prev
}

Version

rustc 1.78.0-nightly (9c3ad802d 2024-03-07)
binary: rustc
commit-hash: 9c3ad802d9b9633d60d3a74668eb1be819212d34
commit-date: 2024-03-07
host: aarch64-apple-darwin
release: 1.78.0-nightly
LLVM version: 18.1.0

Additional Labels

@rustbot label +I-suggestion-causes-error

@taiki-e taiki-e added the C-bug Category: Clippy is not doing the correct thing label Mar 8, 2024
@rustbot rustbot added the I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied label Mar 8, 2024
@gwilymk
Copy link

gwilymk commented Mar 8, 2024

I'm seeing a similar issue but where the variable is being assigned to itself. Not sure if this counts as a separate issue.

Example code (taken from one of my projects where it locates the root of the repo)

use std::{env, error::Error, fmt::Display};

#[derive(Debug)]
struct NoJustfile;

impl Display for NoJustfile {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "{self:?}")
    }
}
impl Error for NoJustfile {}

fn main() -> Result<(), Box<dyn Error>> {
    let mut path_to_search = env::current_dir()?;

    while !path_to_search.join("justfile").exists() {
        path_to_search = path_to_search
            .parent()
            .ok_or_else(|| Box::new(NoJustfile))?
            .to_owned();
    }

    println!("{}", path_to_search.display());

    Ok(())
}

you get the suggestion:

    Checking test-crate v0.1.0 (/tmp/test-crate)
warning: assigning the result of `ToOwned::to_owned()` may be inefficient
  --> src/main.rs:17:9
   |
17 | /         path_to_search = path_to_search
18 | |             .parent()
19 | |             .ok_or_else(|| Box::new(NoJustfile))?
20 | |             .to_owned();
   | |_______________________^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assigning_clones
   = note: `#[warn(clippy::assigning_clones)]` on by default
help: use `clone_into()`
   |
17 ~         path_to_search
18 +             .parent()
19 ~             .ok_or_else(|| Box::new(NoJustfile))?.clone_into(&mut path_to_search);
   |

warning: `test-crate` (bin "test-crate") generated 1 warning (run `cargo clippy --fix --bin "test-crate"` to apply 1 suggestion)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.07s

but if you do that you get:

error[E0502]: cannot borrow `path_to_search` as mutable because it is also borrowed as immutable
  --> src/main.rs:20:25
   |
17 |         path_to_search
   |         -------------- immutable borrow occurs here
...
20 |             .clone_into(&mut path_to_search);
   |              ---------- ^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
   |              |
   |              immutable borrow later used by call

For more information about this error, try `rustc --explain E0502`.
error: could not compile `test-crate` (bin "test-crate") due to 1 previous error

Version:

rustc 1.78.0-nightly (9c3ad802d 2024-03-07)
binary: rustc
commit-hash: 9c3ad802d9b9633d60d3a74668eb1be819212d34
commit-date: 2024-03-07
host: x86_64-unknown-linux-gnu
release: 1.78.0-nightly
LLVM version: 18.1.0

@Kobzol
Copy link
Contributor

Kobzol commented Mar 10, 2024

Thanks for the report, I'll take a look.

@Kobzol
Copy link
Contributor

Kobzol commented Mar 12, 2024

#12473 should hopefully fix the original issue from @taiki-e.

The issue from @gwilymk is related to a different problem, and has been found in multiple other issues (see the linked PR).

@bors bors closed this as completed in 533c377 Jul 25, 2024
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 I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants