Do not suggest borrow that is already there in fully-qualified call

[estebank]

When encountering &str::from("value") do not suggest &&str::from("value").

Fix #132041.

[rustbot]

r? @Nadrieril

rustbot has assigned @Nadrieril.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Nadrieril]

I'm not following the logic: where in this condition are you checking that the expression is already borrowed?

[estebank]

I can add the check for the existing borrow one level up, but I also wanted to avoid suggesting &str::from("") when we have str::from("") as that won't help either.

[Nadrieril]

I see, so the comment below could be saying something like "don't suggest borrowing when this wouldn't fix the problem (because the type is specified by a path instead of inferred)"?

[Nadrieril]

Ah I understand my confusion: I had understood "Do not suggest borrowing when we already do so" as "Do not suggest borrowing when the expression is already borrowed"

[Nadrieril]

Coming back to this: I still don't understand what this branch is catching. Can you rephrase the comment to be more explicit about what the branch means plz?

[estebank]

Hi! Sorry, I had to spend time building context back for myself as well. The whole story:

When trying to find if any trait provides an method or associated function, the resolver needs to keep all trait impls and their obligations in consideration. When evaluating something like Ty::func(), we will inject obligations coming from Ty with the HirId and Span corresponding to that PathSegment, like Sized or any of its where-bound clauses. Because <str as From<&str>::from doesn't exist, we hit that unmet obligation. So far this is the normal behavior.

In suggest_add_reference_to_arg, we try to identify expressions where the obligation isn't met for the found type, but is for the borrow of the found type. So if you have foo(bar) we suggest foo(&mut bar) if appropriate. But that logic kind of assumed that the source of the obligation was an expression, and not a path segment of a type relative path (like Foo::bar, as opposed to type absolute paths like <Foo as Trait>::bar or <Foo>::bar. Because of how rare this case was in our test suite, it seemed like the only situation where the logic I initially added would only ever trigger in a situation like &Foo::bar(), so it didn't seem necessary to make additional checks for the borrow actually being there (which is what was quite confusing of the code as written). The reason this "fixed" the issue was because by having the early return we ensured we didn't suggest the spurious &.

Looking at the changes and situation with fresh eyes, I noticed a couple of cases where we could hit that new logic in item signatures as well, so I added more checks (namely, the expected one that was missing) and added structured suggestions, as we now had all of the spans we needed for that.

[apiraino]

given this comment,

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[estebank]

I noticed there's another case that this PR doesn't account for, which provides an incorrect suggestion:

error[E0277]: the trait bound `S: Trait` is not satisfied
  --> $DIR/dont-suggest-borrowing-existing-borrow.rs:17:13
   |
LL |     let _ = S::foo();
   |             ^ the trait `Trait` is not implemented for `S`
   |
help: consider borrowing here
   |
LL |     let _ = &S::foo();
   |             +
LL |     let _ = &mut S::foo();
   |             ++++

[Nadrieril]

LGTM, r=me unless you want to also fix that other case

[estebank]

@bors r=Nadrieril

@Nadrieril I opened a ticket for that other case. I'll address it later or (even better) someone might snipe it as it is a tricky but not complex case.

[bors]

📌 Commit 7603adc has been approved by Nadrieril

It is now in the queue for this repository.