Duplicate inherent static methods can be defined in separate `impl` blocks

[Byron]

This code I used to compile fine with a version of Rust that was about 9 days old. All the comments in the code have been there before, now the code fails to compile at the lines assert_eq!(Foo::id(), "FOO"); and assert_eq!(Bar::id(), "BAR");:

#[test]
fn type_aliases() {
    struct Foo {
        a: u32,
    };
    impl Foo {
        fn bark(&self) {
            println!("Wuff {}", self.a);
        }

        fn id() -> &'static str {
            "FOO"
        }
    }
    assert_eq!(Foo::id(), "FOO");
    let f = Foo { a: 1 };
    f.bark();

    type Bar = Foo;
    // assert_eq!(!Bar::id(), "FOO");
    // error: unresolved name `Bar::id`
    // tests/lang.rs:628     assert_eq!(!Bar::id(), "FOO");
    let b = Bar { a: 2 };
    b.bark(); // this works though

    impl Bar {
        // This would add a similarly named implementation, that is difficult to call
        // due to ambiguity.
        // Interestingly, it also affects Foo, as well as Bar !!
        // fn bark(&self) {
        //     println!("Grrrr {}", self.a);
        // }

        fn id() -> &'static str {
            "BAR"
        }   
    }
    // b.bark(); // or f.bark();
    // error: multiple applicable methods in scope [E0034]
    // tests/lang.rs:625     f.bark();
    //                         ^~~~~~
    // tests/lang.rs:615:9: 617:10 note: candidate #1 is defined in an impl for the type `type_aliases::Foo`
    // tests/lang.rs:615         fn bark(&self) {
    // tests/lang.rs:616             println!("Wuff {}", self.a);
    // tests/lang.rs:617         }
    // tests/lang.rs:637:9: 639:10 note: candidate #2 is defined in an impl for the type `type_aliases::Foo`
    // tests/lang.rs:637         fn bark(&self) {
    // tests/lang.rs:638             println!("Grrrr {}", self.a);
    assert_eq!(Bar::id(), "BAR");
}

When compiling this now, I get this error, which seems wrong as Bar affects Foo:

tests/lang.rs:636:16: 636:23 error: multiple applicable methods in scope [E0034]
tests/lang.rs:636     assert_eq!(Foo::id(), "FOO");
                                 ^~~~~~~
<std macros>:1:1: 9:39 note: in expansion of assert_eq!
tests/lang.rs:636:5: 636:34 note: expansion site
tests/lang.rs:632:9: 634:10 note: candidate #1 is defined in an impl for the type `type_aliases::Foo`
tests/lang.rs:632         fn id() -> &'static str {
tests/lang.rs:633             "FOO"
tests/lang.rs:634         }
tests/lang.rs:655:9: 657:10 note: candidate #2 is defined in an impl for the type `type_aliases::Foo`
tests/lang.rs:655         fn id() -> &'static str {
tests/lang.rs:656             "BAR"
tests/lang.rs:657         }
<std macros>:5:10: 5:20 error: the type of this value must be known in this context
<std macros>:5 if ! ( ( * left_val == * right_val ) && ( * right_val == * left_val ) ) {
                        ^~~~~~~~~~
<std macros>:1:1: 9:39 note: in expansion of assert_eq!
tests/lang.rs:636:5: 636:34 note: expansion site
<std macros>:5:24: 5:35 error: the type of this value must be known in this context
<std macros>:5 if ! ( ( * left_val == * right_val ) && ( * right_val == * left_val ) ) {
                                      ^~~~~~~~~~~
<std macros>:1:1: 9:39 note: in expansion of assert_eq!
tests/lang.rs:636:5: 636:34 note: expansion site
tests/lang.rs:670:16: 670:23 error: multiple applicable methods in scope [E0034]
tests/lang.rs:670     assert_eq!(Bar::id(), "BAR");
                                 ^~~~~~~
<std macros>:1:1: 9:39 note: in expansion of assert_eq!
tests/lang.rs:670:5: 670:34 note: expansion site
tests/lang.rs:632:9: 634:10 note: candidate #1 is defined in an impl for the type `type_aliases::Foo`
tests/lang.rs:632         fn id() -> &'static str {
tests/lang.rs:633             "FOO"
tests/lang.rs:634         }
tests/lang.rs:655:9: 657:10 note: candidate #2 is defined in an impl for the type `type_aliases::Foo`
tests/lang.rs:655         fn id() -> &'static str {
tests/lang.rs:656             "BAR"
tests/lang.rs:657         }
<std macros>:5:10: 5:20 error: the type of this value must be known in this context
<std macros>:5 if ! ( ( * left_val == * right_val ) && ( * right_val == * left_val ) ) {
                        ^~~~~~~~~~
<std macros>:1:1: 9:39 note: in expansion of assert_eq!
tests/lang.rs:670:5: 670:34 note: expansion site
<std macros>:5:24: 5:35 error: the type of this value must be known in this context
<std macros>:5 if ! ( ( * left_val == * right_val ) && ( * right_val == * left_val ) ) {
                                      ^~~~~~~~~~~
<std macros>:1:1: 9:39 note: in expansion of assert_eq!
tests/lang.rs:670:5: 670:34 note: expansion site

Even if this is now desired, I think the compiler message could be improved - it looks similar for the two candidates, and if it wasn't for the code shown below it, I would look like it means the same thing.

Meta

$ rustc --verbose --version
rustc 1.0.0-nightly (4db0b3246 2015-02-25) (built 2015-02-25)
binary: rustc
commit-hash: 4db0b32467535d718d6474de7ae8d1007d900818
commit-date: 2015-02-25
build-date: 2015-02-25
host: x86_64-apple-darwin
release: 1.0.0-nightly

[huonw]

triage: I-nominated

I think this is fundamentally caused by the following compiling, which seems fairly bad:

struct Foo;
impl Foo {
    fn id() {}
}
impl Foo {
    fn id() {}
}
fn main() {}

Putting both id's in the same impl fails to compile, as does trying to call Foo::id().

[nikomatsakis]

So this came up in the context of the specialization RFC rust-lang/rfcs#1210. Initially that RFC tried to rationalize inherent impls as being from an anonymous trait, but that language was pulled. Nonetheless, I (and @aturon) think that this behavior is basically a bug and we should deprecate overlapping inherent impls of this kind. The question is what the precise rules OUGHT to be.

Some possibilities:

• cannot have two inherent impls with the same fn, period.
• cannot have two inherent impls with the same fn unless coherence judges them to be disjoint (using the overlap checker rules).

[nikomatsakis]

The latter is obviously more backwards compatible, so it'd be good to try and get some crater results.

[eddyb]

While there may be interactions with specialization, inherent methods are more flexible, so even the disjoint rule can be non-trivial, if we ever want argument-type-based-lookup.

At least it wouldn't affect existing code with callable methods, as the current lookup implementation would result in ambiguity even if the signatures are disjoint (but Self isn't).

[nikomatsakis]

At least it wouldn't affect existing code with callable methods, as the current lookup implementation would result in ambiguity even if the signatures are disjoint (but Self isn't).

Yeah, good point. Hopefully impact would be minimal. Still, it's not so hard to have types that are disjoint in practice, but not disjoint enough for the coherence checker -- particularly given the conservative rules imposed by RFC 1023.

[nikomatsakis]

triage: P-high

[nikomatsakis]

In @rust-lang/lang meeting decided to try the more conservative rules and do a crater run to try and estimate the impact. Seems good to start with the simplest rules, if we can get away with it, since we can always allow more later once we know how specialization pans out.

[bluss]

This is still allowed with either rule, right?

impl Foo<i32> {
   fn abc(&self) { }
}
impl Foo<u32> {
   fn abc(&self) { }
}

[aturon]

@bluss No, that's only allowed by the second rule. The first rule essentially says that for any data type, a given method name can appear in at most one of its inherent impl blocks.

[aturon]

@bluss If you know offhand of crates where this more conservative rule would cause significant breakage, pointers would be appreciated; that could save the time of making a branch and doing a crater run.

[bluss]

The interpretation of "same function" I found most reasonable.. I realized after asking the question,
is they are not the same function in the sense that one is fn(&Foo<i32>) and the other is fn(&Foo<u32>).

Anyway, ndarray definitely uses something like that example, but I'm not sure if the breakage is significant or not; it's a stable crate, very few users so far, and a work around is possible. I've definitely thought about this design opportunity in other cases, but I don't remember any other use.

[aturon]

@bluss Sorry, to try to be crystal clear: when @nikomatsakis says "same fn" he means "function with the same name".

[aturon]

@nikomatsakis The more conservative rule prevents bootstrapping, due to stable impls of downcast like:

impl Box<Any> {
    pub fn downcast<T: Any>(self) -> Result<Box<T>, Box<Any>> { ... }
}
impl Box<Any + Send> {
    pub fn downcast<T: Any>(self) -> Result<Box<T>, Box<Any + Send>> { ... }
}

These impls are currently needed due to the lack of upcasting in dispatch. We could see how much breakage comes out of dropping the second impl...

[nikomatsakis]

@aturon d'oh :)

[aturon]

So, one issue with the less conservative rule: what requirements, if any, do we impose about the relevant signatures? For the Box example, the fn signatures varied, though in a simple way.

The specialization RFC laid out thoughts on how to "infer" implicit traits based on inherent impls -- I believe the only requirement is matching arity.

[nikomatsakis]

@aturon ah yes, I remember this algorithm now -- so do you believe that the only requirement we would have to enforce for future compat is to insist that the self types are non-overlapping and arity is the same?

[nikomatsakis]

I was just looking at some compiler code that relies on this pattern of inherent impls in a way that is not really intended to act as specialization would act. For example:

impl Binder<TraitPredicate> { ... }
impl Binder<LifetimePredicate> { ... }

These two impls are really disjoint sets of methods with nothing in common. As it happens, I doubt that they have overlapping names, but if they did, there isn't necessarily a reason to expect that those methods would have the same arity (or purpose). In other words, they would be puns, not the same method specialized to different types.

I'm not sure what to make of this. Just pointing it out.

[aturon]

OK, @nikomatsakis and I discussed this some on IRC, and the consensus between us is that we do not need to impose any constraints on signatures. That is, the rule today should be: you can write whatever you like in inherent impl blocks, but if two blocks overlap on their Self type, they cannot contain items with the same name.

That rules out things like:

struct Foo;
impl Foo {
    fn id() {}
}
impl Foo {
    fn id() {}
}

struct Bar<T>(T);
impl<T> Bar<T> {
    fn bar(&self) {}
}
impl Bar<i32> {
    fn bar(&self) {}
}

but allows things like:

impl Box<Any> {
    pub fn downcast<T: Any>(self) -> Result<Box<T>, Box<Any>> { ... }
}
impl Box<Any + Send> {
    pub fn downcast<T: Any>(self) -> Result<Box<T>, Box<Any + Send>> { ... }
}

impl Foo<i32> {
   fn abc(&self) { }
}
impl Foo<u32> {
   fn abc(&self, flag: bool) { }
}

I believe that if we want to allow specialization of inherent impls in the future, we can make it work with this approach. It'd be mildly more subtle than a version that requires matching arities everywhere, but that's a pretty arbitrary constraint anyway.

[aturon]

Oh, and the point is that this approach also has minimal breakage, and the things it breaks (cases of overlap) really shouldn't be happening today.