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

Inherent impl blocks on trait objects don't check for coherence or overlapping impls until called #38967

Closed
QuietMisdreavus opened this issue Jan 10, 2017 · 6 comments
Labels
A-traits Area: Trait system P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@QuietMisdreavus
Copy link
Member

Consider the following snippet:

trait A {}

trait B {}

trait C<T> {}

impl <T: A> C<T> { fn asdf(&self) { println!("asdf"); } }

impl <T: B> C<T> { fn asdf(&self) { println!("asdf!!"); } }

struct Z {}
impl A for Z {}
impl B for Z {}

struct W {}
impl C<Z> for W {}

fn main() {
    let asdf = Box::new(W {}) as Box<C<Z>>;
    
    asdf.asdf();
}

When asdf.asdf() is called, which impl should be used? Is it possible to select one impl or the other?

When discussing this in #rust, @mbrubeck also found that impls on trait objects don't check for overlap at all:

trait C<T> {}

impl<T> C<T> { fn asdf(&self) {} }
impl<T> C<T> { fn asdf(&self) {} }

struct W {}
impl<T> C<T> for W {}

fn main() {
    let b = Box::new(W {}) as Box<C<()>>;
    b.asdf();
}

In both of these examples, taking out the call to asdf() makes the sample compile, meaning that it's only upon method resolution that this conflict is found. The former can be ruled out as a coherence issue, and the latter is directly an overlapping impl, that wouldn't be allowed on a regular struct/enum.

cc #36889, per talchas in #rust

@mbrubeck
Copy link
Contributor

Here's a minimal test case. The following should trigger the overlapping_inherent_impls lint but does not:

trait C {}
impl C { fn f() {} }
impl C { fn f() {} }

@apasel422
Copy link
Contributor

CC #22889

@arielb1 arielb1 added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 11, 2017
@nikomatsakis
Copy link
Contributor

Hmm, seems like a bug, for sure. For example, this code https://is.gd/YSKjuW using structs does error.

cc @aturon, who wrote the lint I think.

@nikomatsakis
Copy link
Contributor

triage: P-medium

@rust-highfive rust-highfive added P-medium Medium priority and removed I-nominated labels Jan 12, 2017
@nikomatsakis nikomatsakis added the A-traits Area: Trait system label Jan 12, 2017
@nikomatsakis
Copy link
Contributor

The source of the bug looks pretty simple. I'll open a PR.

@nikomatsakis
Copy link
Contributor

Pending fix in #39143

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jan 20, 2017
check inherent impls of traits for overlap as well

Simple oversight. Fixes rust-lang#38967.

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

No branches or pull requests

6 participants