Lint againt the use of ref

[Luro02]

The ref keyword seems to be causing some confusion
https://users.rust-lang.org/t/ref-keyword-versus/18818/26
rust-lang/rust-by-example#390
https://www.reddit.com/r/rust/comments/2tq33x/ref_keyword/

There was also some effort in removing it from compiler suggestions (rust-lang/rust#52423).

As far as I know, you can replace any use of it, by borrowing the variable, that it is matched against.

For example

if let Some(ref x) = Some("Hello") {
    println!("{}", x);
}

if let Some(x) = &Some("Hello") {
    println!("{}", x);
}

or

match Some("Hello") {
    Some(ref x) =>  {
        println!("{}", x);
   }
   None => ()
}

match &Some("Hello") {
    Some(x) =>  {
        println!("{}", x);
   }
   None => ()
}

Some real world uses

https://github.com/TedDriggs/darling/blob/3c3d9a8224a164a5b8604a784381849caa32f034/core/src/error/kind.rs#L63
https://github.com/TedDriggs/darling/blob/b16d131a011f8b1f6cf99113d78edd4b119b5d97/core/src/codegen/variant_data.rs#L72
https://github.com/rust-random/rand/blob/249ebfc4352d8f3e1ebcb2a884f047b31ba32981/rand_distr/src/gamma.rs#L156

---

Edit(2020-04-11):

Relevant discussion on the rust internals forum:

https://internals.rust-lang.org/t/is-ref-more-powerful-than-match-ergonomics/12111

[basil-cow]

I think you can't replace all uses of it, since you might want to move out some parts of the struct and work with others in place (in release you can probably move out all of them, but debug performance is important too). So it's a probably a restriction lint

[Luro02]

@Areredify
I always thought, that transferring ownership is as cheap as borrowing? By "you might want to move out some parts of the struct and work with others in place" you meant something like this?:

#[derive(Debug)]
enum Example {
    Element1(String),
    Element2(String),
    Element3(String),
}

fn main() {
    let example = Example::Element1("String".to_string());
    
    match example {
        Example::Element1(ref data) => println!("{}", data),
        Example::Element2(data) => println!("{}", data),
        Example::Element3(_) => {}
    }
    
    println!("Example: {:?}", example);
}

   Compiling playground v0.0.1 (/playground)
error[E0382]: borrow of moved value: `example`
  --> src/main.rs:17:31
   |
13 |         Example::Element2(data) => println!("{}", data),
   |                           ---- value moved here
...
17 |     println!("Example: {:?}", example);
   |                               ^^^^^^^ value borrowed here after partial move
   |
   = note: move occurs because value has type `std::string::String`, which does not implement the `Copy` trait

error: aborting due to previous error

For more information about this error, try `rustc --explain E0382`.
error: could not compile `playground`.

playground

but with this example I wonder why somebody would do this, when they could just do

match example {
    Example::Element1(data) => println!("{}", data),
    Example::Element2(data) => println!("{}", data),
    Example::Element3(_) => {}
}

It has the same effect and makes the code more readable. Of course in this example the readability improvement can be neglected, but for example this piece of code would really benefit from it.

Is the performance impact that large? It looks like some kind of mirco optimization to me, that has little to no benefit and only hurts readability (similar to vec![] and Vec::new(), where one could argue, that all those expansions of the vec![] macro would hurt the performance.)

[basil-cow]

It's not the same, because when you are moving out of the struct, you gotta copy your value onto the stack, and the value could be fairly large and the code could be on the hot path. In the release mode in most cases llvm would probably elide the copy, since the value will die after the match anyway. I think we should lint if all of the match arms are by ref (and if maybe if some of them are copy types matched by value).

[flip1995]

We already have a similar lint: match_ref_pats

Linting all occurrences of ref could maybe be implemented as a restriction lint, but building a suggestion how to change the code to not use ref may be impossible.

So maybe we could extend the match_ref_pats lint to also lint if lets.

[Luro02]

It would be nice to have both lints :)

This should be linted against as a warning;

if let ErrorKind::Multiple(ref items) = *self

---

I think I should split this up into separate issues, maybe;

• if_let_ref_pats (warn by default, for the above case)
• ref_pat (pedantic)

(I will do this later today or tomorrow)

[Luro02]

opened a new issue #5038

[camsteffen]

FWIW, I personally prefer to not borrow the matched value and I like to use ref, because it is more flexible in the general case. It allows me to copy inner values that are copy-able, and take a reference of values that are not. It also prevents me from accidentally creating a reference of a reference.