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

Byron · 2015-02-28T06:28:08Z

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

The text was updated successfully, but these errors were encountered:

huonw · 2016-01-08T06:40:12Z

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 · 2016-01-14T21:17:39Z

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 · 2016-01-14T21:18:22Z

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

eddyb · 2016-01-14T21:34:26Z

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 · 2016-01-14T21:39:13Z

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 · 2016-01-14T22:15:41Z

triage: P-high

nikomatsakis · 2016-01-14T22:17:40Z

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 · 2016-01-19T13:55:28Z

This is still allowed with either rule, right?

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

aturon · 2016-01-19T17:19:57Z

@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 · 2016-01-19T17:20:46Z

@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 · 2016-01-19T17:29:54Z

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 · 2016-01-19T17:31:55Z

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

aturon · 2016-01-19T22:56:57Z

@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 · 2016-01-19T22:57:55Z

@aturon d'oh :)

aturon · 2016-01-19T23:02:04Z

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 · 2016-01-20T01:55:32Z

@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 · 2016-01-20T20:25:17Z

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 · 2016-01-22T20:52:12Z

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 · 2016-01-22T20:52:46Z

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.

impl blocks. For example, the following is now correctly illegal: ```rust struct Foo; impl Foo { fn id() {} } impl Foo { fn id() {} } ``` "Overlapping" here is determined the same way it is for traits (and in fact shares the same code path): roughly, there must be some way of substituting any generic types to unify the impls, such that none of the `where` clauses are provably unsatisfiable under such a unification. Closes rust-lang#22889

@nikomatsakis

Forbid items with the same name from appearing in overlapping inherent impl blocks For example, the following is now correctly illegal: ```rust struct Foo; impl Foo { fn id() {} } impl Foo { fn id() {} } ``` "Overlapping" here is determined the same way it is for traits (and in fact shares the same code path): roughly, there must be some way of substituting any generic types to unify the impls, such that none of the `where` clauses are provably unsatisfiable under such a unification. Along the way, this PR also introduces an `ImplHeader` abstraction (the first commit) that makes it easier to work with impls abstractly (without caring whether they are trait or inherent impl blocks); see the first commit. Closes #22889 r? @nikomatsakis

steveklabnik added the A-trait-system Area: Trait system label Mar 4, 2015

huonw added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 8, 2016

rust-highfive added the I-nominated label Jan 8, 2016

huonw changed the title ~~Traits and type-aliases: previously valid code now fails to compile~~ Duplicate inherent static methods can be defined in separate impl blocks Jan 8, 2016

nikomatsakis added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jan 14, 2016

aturon self-assigned this Jan 14, 2016

nikomatsakis unassigned aturon Jan 14, 2016

rust-highfive added P-high High priority and removed I-nominated labels Jan 14, 2016

aturon self-assigned this Jan 19, 2016

aturon mentioned this issue Feb 26, 2016

Forbid items with the same name from appearing in overlapping inherent impl blocks #31925

Merged

bors closed this as completed in #31925 Mar 12, 2016

shepmaster mentioned this issue Mar 25, 2016

Duplicate methods defined in multiple inherent impls do not report as duplicates #32491

Closed

nikomatsakis mentioned this issue Oct 3, 2016

type items do not work with enums #26264

Closed

apasel422 mentioned this issue Jan 10, 2017

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

Closed

petrochenkov mentioned this issue Jan 24, 2017

Tracking issue for overlapping_inherent_impls compatibility lint #36889

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Byron commented Feb 28, 2015

huonw commented Jan 8, 2016

nikomatsakis commented Jan 14, 2016

nikomatsakis commented Jan 14, 2016

eddyb commented Jan 14, 2016

nikomatsakis commented Jan 14, 2016

nikomatsakis commented Jan 14, 2016

nikomatsakis commented Jan 14, 2016

bluss commented Jan 19, 2016

aturon commented Jan 19, 2016

aturon commented Jan 19, 2016

bluss commented Jan 19, 2016

aturon commented Jan 19, 2016

aturon commented Jan 19, 2016

nikomatsakis commented Jan 19, 2016

aturon commented Jan 19, 2016

nikomatsakis commented Jan 20, 2016

nikomatsakis commented Jan 20, 2016

aturon commented Jan 22, 2016

aturon commented Jan 22, 2016

Duplicate inherent static methods can be defined in separate impl blocks #22889

Duplicate inherent static methods can be defined in separate impl blocks #22889

Comments

Byron commented Feb 28, 2015

Meta

huonw commented Jan 8, 2016

nikomatsakis commented Jan 14, 2016

nikomatsakis commented Jan 14, 2016

eddyb commented Jan 14, 2016

nikomatsakis commented Jan 14, 2016

nikomatsakis commented Jan 14, 2016

nikomatsakis commented Jan 14, 2016

bluss commented Jan 19, 2016

aturon commented Jan 19, 2016

aturon commented Jan 19, 2016

bluss commented Jan 19, 2016

aturon commented Jan 19, 2016

aturon commented Jan 19, 2016

nikomatsakis commented Jan 19, 2016

aturon commented Jan 19, 2016

nikomatsakis commented Jan 20, 2016

nikomatsakis commented Jan 20, 2016

aturon commented Jan 22, 2016

aturon commented Jan 22, 2016

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

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