Suggest `self` instead of `&self` for `: Copy` types

[nyurik]

Summary

When type implements Copy trait, especially when its size is small (e.g. an enum without variables), it would make sense to suggest changing signature to pass-by-value.

Lint Name

trivially_copy_pass_by_ref

Reproducer

I tried this code with pedantic clippy:

#[derive(Clone, Copy)]
pub enum Format {
    Gif,
    Jpeg,
    Json,
    Mvt,
    Png,
    Webp,
}

pub fn main() {
    let v = Format::Gif;
    let _ = v.use_ref();
    let _ = v.use_self();
    let _ = v.use_ref();
    let _ = v.use_self();
}

impl Format {
    // This is NOT optimal because of the reference
    #[inline(never)]
    pub fn use_ref(&self) -> bool {
           match *self {
            Self::Png | Self::Jpeg | Self::Gif | Self::Webp => true,
            Self::Mvt | Self::Json => false,
        }
    }

    // This function should not generate a warning
    #[inline(never)]
    pub fn use_self(self) -> bool {
        match self {
            Self::Png | Self::Jpeg | Self::Gif | Self::Webp => true,
            Self::Mvt | Self::Json => false,
        }
    }
}

I expected to see this happen:
A suggestion to use self instead of &self

Instead, this happened:
no warnings

Assembly

https://rust.godbolt.org/z/xxf6eqcax

example::Format::use_self:
        and     dil, -2
        cmp     dil, 2
        setne   al
        ret

example::Format::use_ref:
        movzx   eax, byte ptr [rdi]   # <-- memory access is slower than register access
        and     al, 6
        cmp     al, 2
        setne   al
        ret

Version

rustc 1.75.0 (82e1608df 2023-12-21)
binary: rustc
commit-hash: 82e1608dfa6e0b5569232559e3d385fea5a93112
commit-date: 2023-12-21
host: x86_64-unknown-linux-gnu
release: 1.75.0
LLVM version: 17.0.6

[ARandomDev99]

@rustbot label +A-lint

[nicklimmm]

Found an existing pedantic lint called trivially_copy_pass_by_ref. It is suppressed by default (see clippy configuration doc here).

rust-clippy/clippy_lints/src/pass_by_ref_or_value.rs

Lines 205 to 216 in c9a43b1

	span_lint_and_sugg(
	cx,
	TRIVIALLY_COPY_PASS_BY_REF,
	input.span,
	&format!(
	"this argument ({size} byte) is passed by reference, but would be more efficient if passed by value (limit: {} byte)",
	self.ref_min_size
	),
	"consider passing by value instead",
	value_type,
	Applicability::Unspecified,
	);

To enable, add the following to clippy.toml:

avoid-breaking-exported-api = false

[nyurik]

Oh, thanks, I was not aware of that setting (I did mention trivially_copy_pass_by_ref is the original message). I think it raises a serious issue because most people would declare functions as pub from the start, thus never catching these issues even when the API is not yet "public". I wonder if it might be better to remove this setting? After all, a public API can always have an explicit allow, whereas hiding it behind an obscure flag means 99% of the ppl will never notice this...

[PartiallyUntyped]

I found this writeup the other day, and while I agree that certain things should be passed by value, could we perhaps limit this by struct size or at least benchmark it to see whether it is worth?

Maybe the above writeup is completely irrelevant for rustc but better err on the side of caution, especially given that this can break a lot of code.

Edit:

There's another consideration here, that is alignment, if we have such a lint, it should consider how things are padded for alignment, and their size, if we decide to check by size, otherwise it can create many false positives or even hinder performance.

@rustbot label +S-needs-discussion

[nyurik]

@partiallytyped you bring up valid points, and they should be addressed, but I think they are orthogonal to the issue here - which is that public API is never validated by the lint before it gets published.

So even if I did want to address the issue, I am most likely to never be warned especially when implementing a library code -- because more code may be public from the start. Also, it seems Cargo does not (yet) support global settings, and instead rely on unstable clippy.toml file (per project), the chances of this lint to ever catch public things are next to none.

[PartiallyUntyped]

I don't disagree, I am just expressing that it may not be that straightforward, buuut perhaps if the struct already derives copy, then it is fine?

[sffc]

We have many types in ICU4X called FooBorrowed<'a> which are basically thin, Copy wrappers over one or more references. It would be useful to have a link that could alert us when one of these has a method that would be better written with self instead of &self.

[Manishearth]

@sffc It appears that trivially_copy_pass_by_ref already serves this need? Is there a need to distinguish between self and other parameters for this?