Repository: rust
Issue: Compiler suggests restricting bound to only blanket impl for trait instead of just the trait
Labels: C-bug,A-suggestion-diagnostics

As with all diagnostics bugs, the title of this issue is almost completely unhelpful in understanding what I'm talking about, so let me show you some code.

(If you want to cut to the chase [here's a playground that showcases the whole bug](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=b2ec330573cedee08e79773ae2872164)).

## Bits and pieces

This code is incorrect:

```rust
impl<S, M> Service for MagicService<S, M>
where
    S: Service,
    M: Magician<()> + Clone + 'static,
{
    type Output = S::Output;
    type Promise = MagicPromise<S::Promise, M>;

    fn call(&self) -> Self::Promise {
        MagicPromise {
            magician: self.magician.clone(),
            promise: self.service.call(),
        }
    }
}
```

The bound on `M` should be `Magician<S::Output>`, but instead it's just `Magician<()>`.

As a result, `M` does *not* implement `Magician<S::Output>`, which causes `MagicPromise<S::Promise, M>` to *not* implement `Promise`, because its `impl Promise` has this bound:

```rust
impl<P, M> Promise for MagicPromise<P, M>
where
    P: Promise,
    M: Magician<P::Output>, // 👈 that bound
{
    type Output = <P as Promise>::Output;

    fn poll(&self) -> Self::Output {
        todo!()
    }
}
```

Which then violates that bound in `Service`:

```rust
trait Service {
    type Output;
    type Promise: Promise<Output = Self::Output>; // 👈 that bound

    fn call(&self) -> Self::Promise;
}
```

Which causes the whole thing to come crashing down.

## The full picture

Here's a full reproducible example (and [here's the same playground link as before](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=b2ec330573cedee08e79773ae2872164)):

```rust
/// A magician performs on a `T`, by immutable reference
trait Magician<T> {
    fn perform(&self, res: &T);
}

/// All `Fn` closures are magicians.
///
/// 💡 This is what gets the compiler confused. If we comment out this
/// implementation, the diagnostic is no longer wrong.
impl<F, T> Magician<T> for F
where
    F: Fn(&T),
{
    fn perform(&self, res: &T) {
        (self)(res)
    }
}

/// Simple version of `Future`
trait Promise {
    type Output;

    fn poll(&self) -> Self::Output;
}

/// Simple version of tower's `Service`
trait Service {
    type Output;
    type Promise: Promise<Output = Self::Output>;

    fn call(&self) -> Self::Promise;
}

/// A promise that would call `magician` on its result (if it wasn't a stub)
struct MagicPromise<P, M> {
    magician: M,
    promise: P,
}

/// I lied - a MagicPromise is only a promise is `P` is a Promise and
/// `M` is a magician that works on this promise's output.
impl<P, M> Promise for MagicPromise<P, M>
where
    P: Promise,
    M: Magician<P::Output>,
{
    type Output = <P as Promise>::Output;

    fn poll(&self) -> Self::Output {
        todo!() // don't need to implement it to show the bug
    }
}

// A MagicService takes a Service and lets a magician perform on top of it!
struct MagicService<S, M> {
    service: S,
    magician: M,
}

// Here comes the problematic diagnostic

impl<S, M> Service for MagicService<S, M>
where
    S: Service,
    // ⛔ This is an incorrect bound, and triggers an incorrect diagnostic
    M: Magician<()> + Clone + 'static,
    // ✅ This is the correct bound, the code compiles with it
    // M: Magician<S::Output> + Clone + 'static,
{
    type Output = S::Output;
    type Promise = MagicPromise<S::Promise, M>;

    fn call(&self) -> Self::Promise {
        MagicPromise {
            magician: self.magician.clone(),
            promise: self.service.call(),
        }
    }
}

fn main() {
    println!("https://giphy.com/gifs/12NUbkX6p4xOO4");
}
```

If we compile this with Rust 1.48, we get the following diagnostics:

![image](https://user-images.githubusercontent.com/7998310/100920720-ce62dd80-34db-11eb-9280-1490bba3a660.png)

I can sorta get what the compiler is getting at. Right now, the *only* type that implements `Magician<<S as Service>::Output>` is indeed `Fn<(&<S As Service>::Output)>`.

But we're defining a generic type, so there could possibly (in another crate..) be another type that does implement `Magician<<S as Service>::Output>` - the bound that we, in fact, want here.

If we comment out the blanket `impl Magician<T> for Fn(&T)`, we get this diagnostic instead:

![image](https://user-images.githubusercontent.com/7998310/100920945-24378580-34dc-11eb-93bf-8ba8c38dcb80.png)

Which is what we want! ie. the compiler is not trying to be extra helpful.

## What about nightly?

If we try the original code in Nightly (with the blanket impl), we get much of the same *but* there's an additional bit of information about where the bound comes from:

![image](https://user-images.githubusercontent.com/7998310/100921106-63fe6d00-34dc-11eb-9599-2164df01b847.png)

...and this is what finally allowed me to fix my bug, in this instance. Although, as you can imagine, the types were several orders of magnitude larger, so it was still not a fun time.

## What should it do?

The suggestion may actually make sense in some scenarios? I think this particular diagnostic is just overshooting a tiny bit. The main issue that bothers me is that it's hiding the real issue.

Honestly the diagnostic when there is no blanket impl for `Fn` is fairly solid on its own:

```raw
error[E0277]: the trait bound `M: Magician<<S as Service>::Output>` is not satisfied
  --> src\main.rs:71:5
   |
29 |     type Promise: Promise<Output = Self::Output>;
   |                           --------------------- required by this bound in `Service::Promise`
...
71 |     type Promise = MagicPromise<S::Promise, M>;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Magician<<S as Service>::Output>` is not implemented for `M`
   |
   = note: required because of the requirements on the impl of `Promise` for `MagicPromise<<S as Service>::Promise, M>`     
help: consider further restricting this bound
   |
66 |     M: Magician<()> + Clone + 'static + Magician<<S as Service>::Output>,
   |                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

It might be possible to combine the two by saying "btw if `M` was just an `Fn(&T)` then the bound would also be satisfied - but everything's already generic so I would probably go for *just* the "no blanket impl" diagnostic.

### Meta

`rustc --version --verbose`:

```
rustc 1.48.0 (7eac88abb 2020-11-16)
binary: rustc
commit-hash: 7eac88abb2e57e752f3302f02be5f3ce3d7adfb4
commit-date: 2020-11-16
host: x86_64-pc-windows-msvc
release: 1.48.0
LLVM version: 11.0
```

(but I witnessed it on `x86_64-unknown-linux-gnu` first)

`cargo +nightly rustc -- --version --verbose`:

```
   Compiling trait-repro v0.1.0 (C:\Users\amos\work\trait-repro)
rustc 1.50.0-nightly (da3846948 2020-11-21)
binary: rustc
commit-hash: da384694807172f0ca40eca2e49a11688aba6e93
commit-date: 2020-11-21
host: x86_64-pc-windows-msvc
release: 1.50.0-nightly
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
```