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

[needless_borrows_for_generic_args]: FP for &mut in generic argument #12856

Closed
amircodota opened this issue May 26, 2024 · 4 comments · Fixed by #12892
Closed

[needless_borrows_for_generic_args]: FP for &mut in generic argument #12856

amircodota opened this issue May 26, 2024 · 4 comments · Fixed by #12892
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have L-style Lint: Belongs in the style lint group

Comments

@amircodota
Copy link

Summary

When I run clippy on the reproducer code I get

warning: the borrowed expression implements the required traits
   --> src/lib.rs:124:39
    |
124 |         let mut result = Aligned::new(&self.data);
    |                                       ^^^^^^^^^^ help: change this to: `self.data`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
    = note: `#[warn(clippy::needless_borrows_for_generic_args)]` on by default

However, if I remove the borrow, I get

cannot move out of `self.data` which is behind a shared reference
   --> src/lib.rs:124:39
    |
124 |         let mut result = Aligned::new(self.data);
    |                                       ^^^^^^^^^ move occurs because `self.data` has type `&mut [T]`, which does not implement the `Copy` trait

For more information about this error, try `rustc --explain E0507`.

Reproducer

I tried this code:

#![allow(clippy::manual_slice_size_calculation)]

use std::{
    alloc::Layout,
    fmt::{self, Debug},
    ops::{Deref, DerefMut},
};

pub struct Aligned<T: Copy + 'static> {
    data: &'static mut [T],
    len: usize,
}

const ALIGNMENT: usize = 128;

impl<T: Copy> Aligned<T> {
    pub fn new(v: impl AsRef<[T]>) -> Self {
        let v = v.as_ref();
        let layout =
            Layout::from_size_align(v.len() * std::mem::size_of::<T>(), ALIGNMENT).unwrap();
        unsafe {
            let ptr = std::alloc::alloc(layout);
            let ptr = ptr.cast::<T>();
            let new_v = std::slice::from_raw_parts_mut(ptr, v.len());
            new_v.copy_from_slice(v);
            Aligned {
                data: new_v,
                len: v.len(),
            }
        }
    }

    pub fn zeroed(len: usize) -> Self {
        let layout = Layout::from_size_align(len * std::mem::size_of::<T>(), ALIGNMENT).unwrap();
        unsafe {
            let ptr = std::alloc::alloc_zeroed(layout);
            let ptr = ptr.cast::<T>();
            let data = std::slice::from_raw_parts_mut(ptr, len);
            Aligned { data, len }
        }
    }

    pub fn extend(&mut self, input: &[T]) {
        let new_len = self.len + input.len();
        assert!(new_len <= self.data.len(), "no room to extend");
        self.data[self.len..new_len].copy_from_slice(input);
        self.len = new_len;
    }

    pub fn shorten(&mut self, amount: usize) {
        assert!(amount <= self.len, "no room to shorten");
        self.len -= amount;
    }

    pub fn truncate(&mut self, len: usize) {
        assert!(
            len <= self.len,
            "truncating to later position (len={}, truncating to {})",
            self.len,
            len
        );
        self.len = len;
    }

    pub fn as_u8_mut(&mut self) -> &mut [u8] {
        let slice: &mut [T] = &mut *self;
        let ptr = slice.as_mut_ptr();
        let len = slice.len() * std::mem::size_of::<T>();
        unsafe {
            let ptr = ptr.cast::<u8>();
            std::slice::from_raw_parts_mut(ptr, len)
        }
    }

    pub fn capacity(&self) -> usize {
        self.data.len()
    }
}

impl<T: Copy + Default> Aligned<T> {
    pub fn with_capacity(x: usize) -> Self {
        let mut result = Self::new(vec![T::default(); x]);
        result.len = 0;
        result
    }
}

impl<T: Copy> Drop for Aligned<T> {
    fn drop(&mut self) {
        let layout =
            Layout::from_size_align(self.data.len() * std::mem::size_of::<T>(), ALIGNMENT).unwrap();
        unsafe {
            let ptr = self.data.as_mut_ptr().cast::<u8>();
            std::alloc::dealloc(ptr, layout);
        }
    }
}

impl<T: Copy> Deref for Aligned<T> {
    type Target = [T];
    fn deref(&self) -> &[T] {
        &self.data[..self.len]
    }
}

impl<T: Copy> DerefMut for Aligned<T> {
    fn deref_mut(&mut self) -> &mut [T] {
        &mut self.data[..self.len]
    }
}

impl Debug for Aligned<half::f16> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        (**self)
            .iter()
            .map(|x| f32::from(*x))
            .collect::<Vec<_>>()
            .fmt(f)
    }
}

impl<T: Copy + 'static> Clone for Aligned<T> {
    fn clone(&self) -> Aligned<T> {
        let mut result = Aligned::new(&self.data);
        result.len = self.len;
        result
    }
}

In stable clippy this works fine.

In latest nightly I get the above error (in the summary).

Version

rustc 1.80.0-nightly (1ba35e9bb 2024-05-25)
binary: rustc
commit-hash: 1ba35e9bb44d416fc2ebf897855454258b650b01
commit-date: 2024-05-25
host: x86_64-unknown-linux-gnu
release: 1.80.0-nightly
LLVM version: 18.1.6

Additional Labels

No response

@amircodota amircodota added the C-bug Category: Clippy is not doing the correct thing label May 26, 2024
@J-ZhengLi J-ZhengLi added I-false-positive Issue: The lint was triggered on code it shouldn't have L-style Lint: Belongs in the style lint group labels May 27, 2024
@jwiesler
Copy link

jwiesler commented May 27, 2024

Another example using loops:

pub fn serialize_to_buffer<F: serde::Serialize>(
    mut buffer: &mut Vec<u8>,
    iter: &[&str],
) {
    for item in iter {
        serde_json::to_writer(&mut buffer, &item).unwrap();
        //                    ^ suggestion: remove &mut
        // obviously this makes buffer get moved after first iteration
        buffer.extend(b"\n");
    }
}

@J-ZhengLi
Copy link
Member

minimal repro (playground) thanks to @jwiesler:

fn foo<T: std::io::Write>(_buffer: T) {}

fn bar(mut buffer: &mut Vec<u8>) {
    foo(&mut buffer);
    buffer.extend(b"\n");
}

@meithecatte
Copy link
Contributor

Bisected this to this clippy update: rust-lang/rust@72d8d8d

Running a bisect on the clippy repo now

@meithecatte
Copy link
Contributor

Perhaps unsurprisingly, the first bad commit is 79a14de. I'm planning to look into this, but in case I get distracted and drop this, don't hesitate to ping me if you'd like to take over.

meithecatte added a commit to meithecatte/rust-clippy that referenced this issue Jun 5, 2024
This commit fixes a bug introduced in rust-lang#12706, where the behavior of the
lint has been changed, to avoid suggestions that introduce a move. The
motivation in the commit message is quite poor (if the detection for
significant drops is not sufficient because it's not transitive, the
proper fix would be to make it transitive). However, rust-lang#12454, the linked
issue, provides a good reason for the change — if the value being
borrowed is bound to a variable, then moving it will only introduce
friction into future refactorings.

Thus rust-lang#12706 changes the logic so that the lint triggers if the value
being borrowed is Copy, or is the result of a function call, simplifying
the logic to the point where analysing "is this the only use of this
value" isn't necessary.

However, said PR also introduces an undocumented carveout, where
referents that themselves are mutable references are treated as Copy,
to catch some cases that we do want to lint against. However, that is
not sound — it's possible to consume a mutable reference by moving it.

To avoid emitting false suggestions, this PR reintroduces the
referent_used_exactly_once logic and runs that check for referents that
are themselves mutable references.

Thinking about the code shape of &mut x, where x: &mut T, raises the
point that while removing the &mut outright won't work, the extra
indirection is still undesirable, and perhaps instead we should suggest
reborrowing: &mut *x. That, however, is left as possible future work.

Fixes rust-lang#12856
meithecatte added a commit to meithecatte/rust-clippy that referenced this issue Jun 5, 2024
This commit fixes a bug introduced in rust-lang#12706, where the behavior of the
lint has been changed, to avoid suggestions that introduce a move. The
motivation in the commit message is quite poor (if the detection for
significant drops is not sufficient because it's not transitive, the
proper fix would be to make it transitive). However, rust-lang#12454, the linked
issue, provides a good reason for the change — if the value being
borrowed is bound to a variable, then moving it will only introduce
friction into future refactorings.

Thus rust-lang#12706 changes the logic so that the lint triggers if the value
being borrowed is Copy, or is the result of a function call, simplifying
the logic to the point where analysing "is this the only use of this
value" isn't necessary.

However, said PR also introduces an undocumented carveout, where
referents that themselves are mutable references are treated as Copy,
to catch some cases that we do want to lint against. However, that is
not sound — it's possible to consume a mutable reference by moving it.

To avoid emitting false suggestions, this PR reintroduces the
referent_used_exactly_once logic and runs that check for referents that
are themselves mutable references.

Thinking about the code shape of &mut x, where x: &mut T, raises the
point that while removing the &mut outright won't work, the extra
indirection is still undesirable, and perhaps instead we should suggest
reborrowing: &mut *x. That, however, is left as possible future work.

Fixes rust-lang#12856
meithecatte added a commit to meithecatte/rust-clippy that referenced this issue Jun 6, 2024
This commit fixes a bug introduced in rust-lang#12706, where the behavior of the
lint has been changed, to avoid suggestions that introduce a move. The
motivation in the commit message is quite poor (if the detection for
significant drops is not sufficient because it's not transitive, the
proper fix would be to make it transitive). However, rust-lang#12454, the linked
issue, provides a good reason for the change — if the value being
borrowed is bound to a variable, then moving it will only introduce
friction into future refactorings.

Thus rust-lang#12706 changes the logic so that the lint triggers if the value
being borrowed is Copy, or is the result of a function call, simplifying
the logic to the point where analysing "is this the only use of this
value" isn't necessary.

However, said PR also introduces an undocumented carveout, where
referents that themselves are mutable references are treated as Copy,
to catch some cases that we do want to lint against. However, that is
not sound — it's possible to consume a mutable reference by moving it.

To avoid emitting false suggestions, this PR reintroduces the
referent_used_exactly_once logic and runs that check for referents that
are themselves mutable references.

Thinking about the code shape of &mut x, where x: &mut T, raises the
point that while removing the &mut outright won't work, the extra
indirection is still undesirable, and perhaps instead we should suggest
reborrowing: &mut *x. That, however, is left as possible future work.

Fixes rust-lang#12856
@xFrednet xFrednet changed the title Error with latest nightly [needless_borrows_for_generic_args]: FP for &mut in generic argument Jul 23, 2024
xFrednet pushed a commit to xFrednet/rust-clippy that referenced this issue Jul 23, 2024
This commit fixes a bug introduced in rust-lang#12706, where the behavior of the
lint has been changed, to avoid suggestions that introduce a move. The
motivation in the commit message is quite poor (if the detection for
significant drops is not sufficient because it's not transitive, the
proper fix would be to make it transitive). However, rust-lang#12454, the linked
issue, provides a good reason for the change — if the value being
borrowed is bound to a variable, then moving it will only introduce
friction into future refactorings.

Thus rust-lang#12706 changes the logic so that the lint triggers if the value
being borrowed is Copy, or is the result of a function call, simplifying
the logic to the point where analysing "is this the only use of this
value" isn't necessary.

However, said PR also introduces an undocumented carveout, where
referents that themselves are mutable references are treated as Copy,
to catch some cases that we do want to lint against. However, that is
not sound — it's possible to consume a mutable reference by moving it.

To avoid emitting false suggestions, this PR reintroduces the
referent_used_exactly_once logic and runs that check for referents that
are themselves mutable references.

Thinking about the code shape of &mut x, where x: &mut T, raises the
point that while removing the &mut outright won't work, the extra
indirection is still undesirable, and perhaps instead we should suggest
reborrowing: &mut *x. That, however, is left as possible future work.

Fixes rust-lang#12856
xFrednet pushed a commit to xFrednet/rust-clippy that referenced this issue Jul 23, 2024
This commit fixes a bug introduced in rust-lang#12706, where the behavior of the
lint has been changed, to avoid suggestions that introduce a move. The
motivation in the commit message is quite poor (if the detection for
significant drops is not sufficient because it's not transitive, the
proper fix would be to make it transitive). However, rust-lang#12454, the linked
issue, provides a good reason for the change — if the value being
borrowed is bound to a variable, then moving it will only introduce
friction into future refactorings.

Thus rust-lang#12706 changes the logic so that the lint triggers if the value
being borrowed is Copy, or is the result of a function call, simplifying
the logic to the point where analysing "is this the only use of this
value" isn't necessary.

However, said PR also introduces an undocumented carveout, where
referents that themselves are mutable references are treated as Copy,
to catch some cases that we do want to lint against. However, that is
not sound — it's possible to consume a mutable reference by moving it.

To avoid emitting false suggestions, this PR reintroduces the
referent_used_exactly_once logic and runs that check for referents that
are themselves mutable references.

Thinking about the code shape of &mut x, where x: &mut T, raises the
point that while removing the &mut outright won't work, the extra
indirection is still undesirable, and perhaps instead we should suggest
reborrowing: &mut *x. That, however, is left as possible future work.

Fixes rust-lang#12856
bors added a commit that referenced this issue Jul 25, 2024
needless_borrows_for_generic_args: Fix for &mut

This commit fixes a bug introduced in #12706, where the behavior of the lint has been changed, to avoid suggestions that introduce a move. The motivation in the commit message is quite poor (if the detection for significant drops is not sufficient because it's not transitive, the proper fix would be to make it transitive). However, #12454, the linked issue, provides a good reason for the change — if the value being borrowed is bound to a variable, then moving it will only introduce friction into future refactorings.

Thus #12706 changes the logic so that the lint triggers if the value being borrowed is Copy, or is the result of a function call, simplifying the logic to the point where analysing "is this the only use of this value" isn't necessary.

However, said PR also introduces an undocumented carveout, where referents that themselves are mutable references are treated as Copy, to catch some cases that we do want to lint against. However, that is not sound — it's possible to consume a mutable reference by moving it.

To avoid emitting false suggestions, this PR reintroduces the referent_used_exactly_once logic and runs that check for referents that are themselves mutable references.

Thinking about the code shape of &mut x, where x: &mut T, raises the point that while removing the &mut outright won't work, the extra indirection is still undesirable, and perhaps instead we should suggest reborrowing: &mut *x. That, however, is left as possible future work.

Fixes #12856

changelog: none
@bors bors closed this as completed in c6cc160 Jul 25, 2024
DJMcNab added a commit to DJMcNab/xilem that referenced this issue Jul 26, 2024
This is actually
rust-lang/rust-clippy#12856
But the code was a bit of a smell:
`&&mut PathBuf`
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-false-positive Issue: The lint was triggered on code it shouldn't have L-style Lint: Belongs in the style lint group
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants