Duplicate implementations with same name

[mitsuhiko]

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.

[mitsuhiko]

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

[BurntSushi]

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.

[mitsuhiko]

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.

[withoutboats]

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.

[withoutboats]

Here is the most minimal reproduction: https://play.rust-lang.org/?gist=cba966edf46fa867f820dd46628ba083&version=stable&mode=debug

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

[mitsuhiko]

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.

[withoutboats]

@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.

[withoutboats]

I've opened a Rust issue to track fixing the underlying bug this has exposed: rust-lang/rust/issues/51402

[mitsuhiko]

@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.

[withoutboats]

@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.

[withoutboats]

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:

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 🎉

[mitsuhiko]

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.

[withoutboats]

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!

[mitsuhiko]

Also when it's removed on the trait, then it no longer exists on fn x<F: Fail>(f: &F) :-/

[withoutboats]

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.

[mitsuhiko]

I still think it makes sense to keep the impl Fail version and remove it from the trait itself.

[withoutboats]

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.

[mitsuhiko]

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.

[mitsuhiko]

The main effect of that would be that some calls that previously permitted foo.causes() would turn into Fail::causes(foo) and similar.

[mitsuhiko]

Another option would be to just rename the methods and deprecate the old ones.

• causes() -> iter_causes()
• root_cause() -> find_root_cause()