Skip to content

trivially_copy_pass_by_ref does not consider pointers #5953

Closed
@antonok-edm

Description

@antonok-edm

This is a similar issue to #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: Rust-SDL2/rust-sdl2#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 #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
    

Metadata

Metadata

Assignees

No one assigned

    Labels

    C-bugCategory: Clippy is not doing the correct thingE-help-wantedCall for participation: Help is requested to fix this issue.E-mediumCall for participation: Medium difficulty level problem and requires some initial experience.I-false-positiveIssue: The lint was triggered on code it shouldn't have

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions