Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possibly-incorrect coherence error with blanket impl bounded by a trait with type param #28881

Closed
huonw opened this issue Oct 7, 2015 · 8 comments
Labels
A-traits Area: Trait system T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@huonw
Copy link
Member

huonw commented Oct 7, 2015

This fails to compile:

trait Base<X> {}

trait Foo<X> {}
impl<X, T: Base<X>> Foo<X> for T {}

struct Bar<T>(T);
impl<T> Foo<T> for Bar<T> {}

fn main() {}
<anon>:4:1: 4:36 error: conflicting implementations for trait `Foo` [E0119]
<anon>:4 impl<X, T: Base<X>> Foo<X> for T {}
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<anon>:4:1: 4:36 help: see the detailed explanation for E0119
<anon>:7:1: 7:29 note: note conflicting implementation here
<anon>:7 impl<T> Foo<T> for Bar<T> {}
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
error: aborting due to previous error

Which is possibly a violation of the coherence RFC.

Notably, removing the parameter of Base compiles fine:

trait Base {}

trait Foo<X> {}
impl<X, T: Base> Foo<X> for T {}

struct Bar<T>(T);
impl<T> Foo<T> for Bar<T> {}

fn main() {}
@huonw huonw added A-traits Area: Trait system T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 7, 2015
@aturon
Copy link
Member

aturon commented Oct 7, 2015

cc me

@nikomatsakis
Copy link
Contributor

I believe the problem is that the negative reasoning you need is: Bar<T>: !Foo<T>, but we do not know this to be true. For example, we are afraid that some downstream crate might define:

struct Baz { }
impl Foo<Baz> for Bar<Baz> { }
impl Base for Baz { }

I believe this does, in fact, pass the current orphan rules (at least if we adopted Covered First). Based on the table at the end, it seems to fall under the Cow<MyVec<T>,T> scenario. (However, I know I have also thought that we might be able to tighten up coherence to take the orphan rules into account, and I think I was thinking of cases like this.) So I guess I have to double check just what is going on here in that respect.

@huonw
Copy link
Member Author

huonw commented Oct 7, 2015

Ah, well. It's a little unfortunate if it is correctly rejected: this sort of blanket impl is useful in cases like IntoIterator having a blanket impl for all Iterators (i.e. IntoIterator acts as a converter, and having an automatic noop conversion grants flexibility/usability), but with type parameters.

(The concrete use-case I was thinking of was allowing all InPlaces to be a Placer.)

@arielb1
Copy link
Contributor

arielb1 commented Oct 7, 2015

@nikomatsakis

The issue here is rather that you can impl Base<MyLocalType> for Baz<MyLocalType> - this passes the orphan check easily, as there are no generics and MyLocalType is local.

@nikomatsakis
Copy link
Contributor

@arielb1 right, sorry, that's what I was trying to say, but I think I got a bit confused glancing up at the example text, since I saw @huonw's second example. Thanks for the correction.

@pnkfelix
Copy link
Member

pnkfelix commented Jan 3, 2017

triage: From reading the last two comments from ariel and niko, this seems like it is expected behavior, and could be closed?

@nikomatsakis
Copy link
Contributor

@pnkfelix that seems right. I'd probably like to make a list of coherence failures though -- i.e., that we might use to validate negative reasoning designs and other things like that. Not sure where to put such a list, but it seems like this would be on it.

@nikomatsakis
Copy link
Contributor

I made an etherpad to collect these sorts of examples: https://public.etherpad-mozilla.org/p/rust-coherence-room-for-improvement

After discussing in @rust-lang/lang mtg we agreed this is expected behavior, hence closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-traits Area: Trait system T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants