Suggest borrowing if a trait implementation is found for &/&mut <type>

[FabianWolff]

This pull request fixes #84973 by suggesting to borrow if a trait is not implemented for some type T, but it is for &T or &mut T. For instance:

trait Ti {}
impl<T> Ti for &T {}
fn foo<T: Ti>(_: T) {}

trait Tm {}
impl<T> Tm for &mut T {}
fn bar<T: Tm>(_: T) {}

fn main() {
    let a: i32 = 5;
    foo(a);

    let b: Box<i32> = Box::new(42);
    bar(b);
}

gives, on current nightly:

error[E0277]: the trait bound `i32: Ti` is not satisfied
  --> t2.rs:11:9
   |
3  | fn foo<T: Ti>(_: T) {}
   |           -- required by this bound in `foo`
...
11 |     foo(a);
   |         ^ the trait `Ti` is not implemented for `i32`

error[E0277]: the trait bound `Box<i32>: Tm` is not satisfied
  --> t2.rs:14:9
   |
7  | fn bar<T: Tm>(_: T) {}
   |           -- required by this bound in `bar`
...
14 |     bar(b);
   |         ^ the trait `Tm` is not implemented for `Box<i32>`

error: aborting due to 2 previous errors

whereas with my changes, I get:

error[E0277]: the trait bound `i32: Ti` is not satisfied
  --> t2.rs:11:9
   |
3  | fn foo<T: Ti>(_: T) {}
   |           -- required by this bound in `foo`
...
11 |     foo(a);
   |         ^
   |         |
   |         expected an implementor of trait `Ti`
   |         help: consider borrowing here: `&a`

error[E0277]: the trait bound `Box<i32>: Tm` is not satisfied
  --> t2.rs:14:9
   |
7  | fn bar<T: Tm>(_: T) {}
   |           -- required by this bound in `bar`
...
14 |     bar(b);
   |         ^
   |         |
   |         expected an implementor of trait `Tm`
   |         help: consider borrowing mutably here: `&mut b`

error: aborting due to 2 previous errors

In my implementation, I have added a "blacklist" to make these suggestions flexible. In particular, suggesting to borrow can interfere with other suggestions, such as to add another trait bound to a generic argument. I have tried to configure this blacklist to cause the least amount of test case failures, i.e. to model the current behavior as closely as possible (I only had to change one existing test case, and this change was quite clearly an improvement).

[rust-highfive]

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

[jackh726]

I'm not sure how I feel about the blacklist. Can you provide some examples of errors with/without it? (And add some tests for those (with))

Also, these suggestions are only shown if &T: Trait actually holds right? Not just if T: Trait doesn't?

[jackh726]

I think it's better to say "mutably borrowing"

[FabianWolff]

I'm not sure how I feel about the blacklist. Can you provide some examples of errors with/without it? (And add some tests for those (with))

The blacklist doesn't endanger soundness or anything — it just controls whether we issue a suggestion in this particular function or not. Currently, no suggestions are issued at all for the obligation causes I have addressed in this PR, so we don't lose anything by blacklisting a few traits. Without the blacklist, there are many test case failures in src/test/ui/, i.e. the blacklist makes this PR a much less intrusive change.

As an example,

fn foo<T: Copy>(t: T) {}

fn main() {
    foo("".to_string());
}

gives

error[E0277]: the trait bound `String: Copy` is not satisfied
 --> test.rs:4:9
  |
1 | fn foo<T: Copy>(t: T) {}
  |           ---- required by this bound in `foo`
...
4 |     foo("".to_string());
  |         ^^^^^^^^^^^^^^ the trait `Copy` is not implemented for `String`

error: aborting due to previous error

with the blacklist, and

error[E0277]: the trait bound `String: Copy` is not satisfied
 --> test.rs:4:9
  |
1 | fn foo<T: Copy>(t: T) {}
  |           ---- required by this bound in `foo`
...
4 |     foo("".to_string());
  |         ^^^^^^^^^^^^^^
  |         |
  |         expected an implementor of trait `Copy`
  |         help: consider borrowing here: `&"".to_string()`

error: aborting due to previous error

without it.

Also, these suggestions are only shown if &T: Trait actually holds right? Not just if T: Trait doesn't?

Yes, of course. I have added another test case to emphasize this, along with one test case for the blacklisted traits. I have also adjusted the wording of the suggestion.

[jackh726]

Thank you ♥

@bors r+ rollup

[bors]

📌 Commit 572bb13 has been approved by jackh726

[FabianWolff]

@jackh726 Thanks for taking the time to review this PR!

[pickfire]

Can you add a test where both the & and &mut is there? What will be the output?

[FabianWolff]

Can you add a test where both the & and &mut is there? What will be the output?

No, because this pull request has already been merged. The output will be a suggestion to immutably borrow; you can see for yourself on nightly:

trait Tr {}
struct S {}

impl Tr for &S {}
impl Tr for &mut S {}

fn foo<T: Tr>(t: T) {}

fn main() {
    let s = S {};
    foo(s);
}

error[E0277]: the trait bound `S: Tr` is not satisfied
  --> src/main.rs:11:9
   |
7  | fn foo<T: Tr>(t: T) {}
   |           -- required by this bound in `foo`
...
11 |     foo(s);
   |         ^
   |         |
   |         expected an implementor of trait `Tr`
   |         help: consider borrowing here: `&s`

error: aborting due to previous error