Rust-clippy: trivially_copy_pass_by_ref does not consider pointers

[antonok-edm]

This is a similar issue to https://github.com/rust-lang/rust-clippy/issues/2946.

Recently it was discovered that a change recommended by the trivially_copy_pass_by_ref lint had started causing undefined behavior in Rust-SDL2: https://github.com/Rust-SDL2/rust-sdl2/pull/1020.

This is the problematic change:

 impl Point {
     // ...
-    pub fn raw(&self) -> *const sys::SDL_Point {
+    pub fn raw(self) -> *const sys::SDL_Point {
         self.raw
     }
 }

Point is a simple wrapper around an owned sys::SDL_Point, which itself is just a simple FFI-compatible data structure. Since Point implements Copy, the trivially_copy_pass_by_ref lint was recommended on Point::raw - even though making that change causes Point::raw to return a pointer to an immediately-dropped copy of one of its own fields.

Here's a minimal reproduction of the issue. The Point::raw method correctly produces a consistent pointer to the internal RawPoint, whereas the Point::raw_linted method produces a different pointer on each call.

#![forbid(clippy::pedantic)]

#[derive(Clone, Copy)]
struct RawPoint {
    pub x: u8,
}

#[derive(Clone, Copy)]
struct Point {
    pub raw: RawPoint,
}

impl Point {
    pub fn raw(&self) -> *const RawPoint {
        &self.raw
    }

    pub fn raw_linted(self) -> *const RawPoint {
        &self.raw
    }
}

fn main() {
    let p = Point { raw: RawPoint { x: 10 } };

    // This passes
    assert_eq!(p.raw(), p.raw());
    // This fails
    assert_eq!(p.raw_linted(), p.raw_linted());
}

This would be a bit more challenging to decide than the solution in https://github.com/rust-lang/rust-clippy/pull/2951. Pointers could be present as arbitrarily nested fields of a returned struct, which would not itself be a pointer. Further, pointers could be manipulated by long-lasting side effects (e.g. sent over a channel) during a function without actually ever being returned. Arguably it is the responsibility of the author of the unsafe dereference site to guarantee that the pointer remains valid, but it's still unreasonable to _recommend_ this change in these cases.

Meta

• cargo clippy -V: clippy 0.0.212 (346aec9 2020-07-11)
• rustc -Vv:
  rustc 1.46.0-nightly (346aec9b0 2020-07-11) binary: rustc commit-hash: 346aec9b02f3c74f3fce97fd6bda24709d220e49 commit-date: 2020-07-11 host: x86_64-unknown-linux-gnu release: 1.46.0-nightly LLVM version: 10.0

[mikerite]

Just to make this clear. The fix is to check if the return type contains a pointer on any level and avoid linting if it does. Right?

[antonok-edm]

@mikerite That should work in most(?) cases, including the original one. But there's still a lot of complexity when considering the potential for something like this, where the pointer is returned _indirectly_ through some arbitrary amount of structural nesting:

struct PointerWrapper(*const RawPoint);

impl Point {
    fn raw_wrapped(&self) -> PointerWrapper {
        PointerWrapper(&self.raw)
    }
}

Or even this, where the pointer is not returned at all, rather it's used elsewhere by a side-effect:

impl Point {
    fn send_raw(&self, channel: &std::sync::mpsc::Sender) {
        channel.send(&self.raw).unwrap();
    }
}

[danielrh]

An alternative is to remove or disable the linter altogether until a safe approach can be determined?