Repository: rust-lang-deprecated/failure (language: rust)

Issue:
Duplicate implementations with same name
I keep running into confusing issues because a few methods are implemented twice. For instance there is `Fail::causes` as default impl and then on `impl Fail`. This is making for ridiculous situations where I have this:

```
note: candidate #1 is defined in an impl for the type `failure::Fail + 'static`
note: candidate #2 is defined in the trait `failure::Fail`
```

But I think rust has no syntax to disambiguate the call. The only proposed thing is `Fail::causes(x)` which is picking the `Self: Sized` one which is the one I do not want. For the other impl there does not seem to be a syntax? In any case this is confusing and I wonder why we need these two implementations at all.


Discussion / hints:
I think this is a bug in the failure api and we should do something before stabilizing this. Not sure what yet but this shows the issue in isolation quite well: http://play.rust-lang.org/?gist=ae67b7566aca902a3e2dcf4c80533d7c&version=stable&mode=debug
They both appear to have the same implementation as well? I agree that this looks like a bug to me unless I'm missing something.
The main difference is that one can in theory be overridden by traits but I don't think that's useful. The only one that really is callable in the environments I encountered it is the `impl Fail` one because it does not require Sized. I wonder if it would be possible to just remove it from the trait.
Why isn't the inherent method overriding the trait method?

I think this might've been a mistake originally, but the advantage of it (if it worked properly) is that you could call the method without importing the `Fail` trait.
Here is the most minimal reproduction: https://play.rust-lang.org/?gist=cba966edf46fa867f820dd46628ba083&version=stable&mode=debug

```rust
trait Foo {
    fn f(&self) { }
}

impl Foo {
    fn f(&self) { }
}

impl Foo for i32 {}

fn main() {
    let x: &Foo = &42i32;
    x.f();
}
```

This is surprising! I expect this to call the inherent method on the `dyn Foo` type, because my understanding was that inherent methods override trait methods (unless there are unfortunate, weird, deref coercions involved as in the example @mitsuhiko cites in the linked Rust issue). I definitely don't expect there to be an ambiguity like this, and it appears that its from special handling of trait object types.

EDIT: Expanded example to show that this works as expected for non trait object types: https://play.rust-lang.org/?gist=bea9908fa056c5620b939f2a487ba726&version=stable&mode=debug
> I think this might've been a mistake originally, but the advantage of it (if it worked properly) is that you could call the method without importing the Fail trait.

So what we only that method stays and the one on the trait itself goes away? That also removes the Sized restriction.

The current behavior is really problematic.
@mitsuhiko I agree the current behavior is problematic. I think the right *immediate* solution is to remove the `where Self: Sized` bound on the trait method (which AFAICT is a complete mistake, since the method is object safe) and remove the method on the trait object type.

However, it looks like method resolution for trait object types is also not behaving correctly, and that's a compiler bug that should be fixed, after which this method could (conceivably at least) be added back to the trait object without causing any error.
I've opened a Rust issue to track fixing the underlying bug this has exposed: rust-lang/rust/issues/51402
@withoutboats yeah. it looks like the removal of the bound would be very helpful here. I do not think anyone implements either of those methods so it should be fairly safe to do. This however will mean that 0.1 also breaks backwards compat given the current crate setup.
@mitsuhiko Urgh, you're right.

The only breakage is if anyone is calling `causes` on `&Fail` *without* importing the `Fail` trait. I'd love to have a set up to do a "crater run" on all the crates on crates.io that depend on failure.
According to Niko, the issue isn't that inherent methods aren't getting precedent, but trait methods are treated as inherent methods on the trait object type. Surprising, possibly a mistake, but pretty much impossible to change now. Fortunately for us, I think that means it is backward compatible to remove the causes from `impl Fail` if we just remove the `where Self: Sized` bound from the other one.

To wit:

```rust
mod foo {
    pub trait Foo {
        fn f(&self) { }
    }
    impl Foo for i32 { }
    
    pub fn foo(x: &i32) -> &Foo { x }
}

fn main() {
    let x = foo::foo(&0);
    x.f();
}
```

https://play.rust-lang.org/?gist=6440c2939ee8960b9c5928541531dd23&version=stable&mode=debug

So I believe that removing the bound and the inherent method would fix this bug without being a breaking change :tada:
It does not look like we can remove the `where Self: Sized` bound. It causes the default impls to not work:

```
165 |         find_root_cause(self)
    |                         ^^^^ `Self` does not have a constant size known at compile-time
```

However it's possible to just remove the trait methods and leave them on `impl Fail`.
Oh right, I did have a good reason for the current set up after all (I just didn't know method resolution would behave so poorly). This is frustrating!
Also when it's removed on the trait, then it no longer exists on `fn x<F: Fail>(f: &F)` :-/
Yes, you'd have to explicitly cast that into `&Fail` before calling it. Another way this bug could be fixed is that these shouldn't be ambiguous with the `where Self: Sized` bound, because `&Fail` is not `Sized`, and so it doesn't meet the bounds. However, I suspect that the ambiguity may arise in name resolution before that kind of information is available to guide decision making.

This is a place where trait objects just aren't working correctly and we need some design work to have an improved solution.
I still think it makes sense to keep the `impl Fail` version and remove it from the trait itself.
What if we implemented `Fail for &'a Fail` instead?

It would be very unusual in my opinion to have this only on the trait object type. I think most people make this work using an impl like this.
I am currently thinking of just removing the duplicates on the trait itself and break the ecosystem slightly. I do not see any reasonable workarounds.
The main effect of that would be that some calls that previously permitted `foo.causes()` would turn into `Fail::causes(foo)` and similar.
Another option would be to just rename the methods and deprecate the old ones.

* `causes()` -> `iter_causes()`
* `root_cause()` -> `find_root_cause()`

Fix patch:
diff --git a/src/lib.rs b/src/lib.rs
index eb932ab..f6eaa30 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -160,8 +160,8 @@ pub trait Fail: Display + Debug + Send + Sync + 'static {
         Compat { error: self }
     }
 
-    /// Returns a iterator over the causes of this `Fail` with itself
-    /// as the first item and the `root_cause` as the final item.
+    #[doc(hidden)]
+    #[deprecated(since = "0.1.2", note = "please use the 'iter_causes()' method instead")]
     fn causes(&self) -> Causes
     where
         Self: Sized,
@@ -169,11 +169,8 @@ pub trait Fail: Display + Debug + Send + Sync + 'static {
         Causes { fail: Some(self) }
     }
 
-    /// Returns the "root cause" of this `Fail` - the last value in the
-    /// cause chain which does not return an underlying `cause`.
-    ///
-    /// If this type does not have a cause, `self` is returned, because
-    /// it is its own root cause.
+    #[doc(hidden)]
+    #[deprecated(since = "0.1.2", note = "please use the 'find_root_cause()' method instead")]
     fn root_cause(&self) -> &Fail
     where
         Self: Sized,
@@ -216,12 +213,27 @@ impl Fail {
     ///
     /// If this type does not have a cause, `self` is returned, because
     /// it is its own root cause.
-    pub fn root_cause(&self) -> &Fail {
+    ///
+    /// This is equivalent to iterating over `iter_causes()` and taking
+    /// the last item.
+    pub fn find_root_cause(&self) -> &Fail {
         find_root_cause(self)
     }
 
     /// Returns a iterator over the causes of this `Fail` with itself
     /// as the first item and the `root_cause` as the final item.
+    pub fn iter_causes(&self) -> Causes {
+        Causes { fail: Some(self) }
+    }
+
+    /// Deprecated alias to `find_root_cause`.
+    #[deprecated(since = "0.1.2", note = "please use the 'find_root_cause()' method instead")]
+    pub fn root_cause(&self) -> &Fail {
+        find_root_cause(self)
+    }
+
+    /// Deprecated alias to `iter_causes`.
+    #[deprecated(since = "0.1.2", note = "please use the 'iter_causes()' method instead")]
     pub fn causes(&self) -> Causes {
         Causes { fail: Some(self) }
     }


Test patch: