Fix needless_borrow false positive

[smoelius]

The PR fixes the false positive exposed by @BusyJay's example in: #9111 (comment)

The current approach is described in #9674 (comment) and #9674 (comment).

The original approach appears below.

---

The proposed fix is to flag only "simple" trait implementations involving references, a concept
that I introduce next.

Intuitively, a trait implementation is "simple" if all it does is dereference and apply the trait
implementation of a type named by a type parameter. AsRef provides a good example of a simple
implementation: https://doc.rust-lang.org/std/convert/trait.AsRef.html#impl-AsRef%3CU%3E-for-%26T

We can make this idea more precise as follows. Given a trait implementation, first determine
whether the implementation is "used defined." If so, then examine its nested obligations.
Consider the implementation simple if-and-only-if:

• there is at least one nested obligation for the same trait
• for each type X in the nested obligation's substitution, either X is the same as that of
  the original obligation's substitution, or the original type is &X

For example, the following implementation from @BusyJay's example is "complex" (i.e., not simple)
because it produces no nested obligations:

impl<'a> Extend<&'a u8> for A { ... }

On the other hand, the following slightly modified implementation is simple, because it produces
a nested obligation for Extend<X>:

impl<'a, X> Extend<&'a X> for A where A: Extend<X> { ... }

How does flagging only simple implementations help? One way of interpreting the false positive in
@BusyJay's example is that it separates a reference from a concrete type. Doing so turns a
successful type inference into a failing one. By flagging only simple implementations, we
separate references from type variables only, thereby eliminating this class of false positives.

Note that Deref is a special case, as the obligations generated for it already involve the
underlying type.

r? @Jarcho (Sorry to keep pinging you with needless_borrow stuff. But my impression is no one knows this code better than you.)

changelog: fix needless_borrow false positive

[Jarcho]

Requiring nested obligations is overly restrictive here. The problem for the given example comes from switching which impl is being used, essentially causing a different function to be called.

In the example, extend has thjree different generic parameters. Self and A from the Extend trait; and T from the function itself. I think the problem comes from seeing the predicate T: IntoIterator<Item = A>, which causes the Item type from the new T to be propagated to A. This shouldn't happen as A was introduced in the surrounding context, not the current function.

The fix would be to check that the target of the type propagation is introduced by the current function. Should be as easy as checking if the parameter index is greater than the number of parameters in the parent generics list.

[smoelius]

The problem for the given example comes from switching which impl is being used, essentially causing a different function to be called.

I agree with this 100%.

In the example, extend has thjree different generic parameters. Self and A from the Extend trait; and T from the function itself. I think the problem comes from seeing the predicate T: IntoIterator<Item = A>, which causes the Item type from the new T to be propagated to A. This shouldn't happen as A was introduced in the surrounding context, not the current function.

The fix would be to check that the target of the type propagation is introduced by the current function. Should be as easy as checking if the parameter index is greater than the number of parameters in the parent generics list.

I may be misunderstanding your solution, but wouldn't that allow examples like this to get through?

#[derive(Clone, Copy)]
struct A;

trait Foo {
    fn bar(self);
}

impl Foo for &A {
    fn bar(self) {
        println!("okay");
    }
}

impl Foo for A {
    fn bar(self) {
        panic!();
    }
}

fn call_bar(foo: impl Foo) {
    foo.bar();
}

fn main() {
    call_bar(&A);
}

[Jarcho]

That is a pretty pathological case. It requires writing a trait impl for some type and it's reference which do different things. This is a thing which is already surprising. Also note you can do the same thing even with a nested obligation.

trait Tr { fn foo() }
impl Tr for u8 { fn foo() { panic!() } }
impl<'a, T: 'a + Tr> Tr for &'a T { fn foo() { call_some_other_fn() } }

This issue is the reason I had you add the exception for Any.

[smoelius]

That is a pretty pathological case.

I agree with you it's pathological. But, to be honest, I would consider any false positive that doesn't involve nested obligations to be pathological. Would you disagree?

For example, if you run lintcheck with the configuration from 332b5b3, you get the same 22 "the borrowed expression implements the required traits" warnings with and without the nested obligation check. In other words, the nested obligation check does not eliminate any of the warnings seen in those crates.

[Jarcho]

The lack of nested obligations will come into play when the impl for the reference isn't generic. e.g.

trait Tr {}
impl Tr for u8 {}
impl<'a> Tr &'a u8 {}

Here the impl on the reference has no nested obligation, but will still most likely be equivalent to the non-reference impl. This is also the only way to implement this for foreign traits on local types.

[smoelius]

I'm not convinced, but I'll adjust the PR to implement your fix.

Thank you for your patient explanation.

[smoelius]

I think the most recent commit implements your idea in #9674 (comment). Please let me know if I misunderstood.

[Jarcho]

That looks like your blocking any T: Trait predicate where T is from the parent generics list. This will block any function in the trait trait T: Bound {}.

The predicates that need to be blocked are U: Trait<Assoc = T> and U::Assoc: Trait<Assoc = T> where U is a type introduced on the current function, and T is a type introduced by the parent context.

[smoelius]

Is this more like what you had in mind?

[Jarcho]

That looks like it. Thank you.

@bors r+

[bors]

📌 Commit 83771c5 has been approved by Jarcho

It is now in the queue for this repository.

[bors]

⌛ Testing commit 83771c5 with merge 40af5be...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 40af5be to master...