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

False positive with trivially_copy_pass_by_ref #2961

Closed
XAMPPRocky opened this issue Jul 25, 2018 · 5 comments · Fixed by #8639
Closed

False positive with trivially_copy_pass_by_ref #2961

XAMPPRocky opened this issue Jul 25, 2018 · 5 comments · Fixed by #8639
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 T-async-await Type: Issues related to async/await

Comments

@XAMPPRocky
Copy link
Member

The following code falsely assumes that value can be converted from reference to value when the value is returned as a reference by the function.

https://github.com/CraneStation/cranelift/blob/01de6842b1a3230db645a00f4ecf0176b5fdc65f/lib/codegen/src/regalloc/virtregs.rs#L100-L106

ref_slice

https://github.com/CraneStation/cranelift/blob/fb4acf8b5aaf63a96b9364d9b6f8d373fd8913c8/lib/codegen/src/ref_slice.rs#L12-L14

@flip1995
Copy link
Member

See #2951. It's partially fixed and the bug you're describing is (will be) in the known problems section of the lint. Still a bug though, that needs to be fixed somtimes.

I don't see a problem with the second link you posted?

@flip1995 flip1995 added the C-bug Category: Clippy is not doing the correct thing label Jul 25, 2018
@XAMPPRocky
Copy link
Member Author

@flip1995 There's no problem with the second line of code, I just wanted to provide the context of what the signature of ref_slice which uses value was.

@mati865
Copy link
Contributor

mati865 commented Sep 5, 2019

False positive in Rust code:
https://github.com/rust-lang/rust/blob/master/src/libcore/fmt/mod.rs#L271-L273

    fn show_usize(x: &usize, f: &mut Formatter<'_>) -> Result {
        Display::fmt(x, f)
    }

The error:

warning: this argument (8 byte) is passed by reference, but would be more efficient if passed by value (limit: 8 byte)
   --> src/libcore/fmt/mod.rs:271:22
    |
271 |     fn show_usize(x: &usize, f: &mut Formatter<'_>) -> Result {
    |                      ^^^^^^ help: consider passing by value instead: `usize`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref

Display::fmt definition:
https://github.com/rust-lang/rust/blob/a24f636e60a5da57ab641d800ac5952bbde98b65/src/libcore/fmt/mod.rs#L631

fn fmt(&self, f: &mut Formatter<'_>) -> Result;

Quoting @eddyb from Discord:

is this tripping a lint?
if so it should look at the lifetime and check if the lifetime is in the return type
because then it probably matters for the argument to be by-ref

@eddyb
Copy link
Member

eddyb commented Sep 30, 2019

@mati865 In that case, the x: &usize doesn't actually have any lifetime relationships (between arguments and return type), so what I was saying is inapplicable.

What's happening is that we need that signature elsewhere (because we turn that function into a pointer). It should #[allow] the lint (which is fair given how much of a hack show_usize actually is).

@RobbieClarken
Copy link
Contributor

Possibly related false positive when a function is async (playground):

pub async fn foo(x: &u8) -> &u8 { x }

This code fails trivially_copy_pass_by_ref but only if the function is async.

@flip1995 flip1995 added the T-async-await Type: Issues related to async/await label Mar 23, 2020
@phansch phansch added the I-false-positive Issue: The lint was triggered on code it shouldn't have label Dec 21, 2020
@bors bors closed this as completed in 373bb57 Jun 28, 2022
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 T-async-await Type: Issues related to async/await
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants