Changing public api by adding Impl

1

Until now I was under the impression that adding implementations should never change the public api of a library (Except some special cases like Send Sync Drop).

Some time ago however I came along this example which seems to prove me wrong and I'm wondering if this is on purpose or if it was just missed while designing a language feature, and if so, if it might get "fixed" in a future language edition.

trait Trait<T>{
    fn do_something(&self){}
}
struct Bar<T>(T);

impl Default for Bar<u32> {
    fn default() -> Self {
        Bar(0)
    }
}

impl Default for Bar<u64> {
    fn default() -> Self {
        Bar(0)
    }
}

struct Foo;

impl<X> Trait<Bar<X>> for Foo
where
    Bar<X>: Default,
{
    fn do_something(&self) {}
}

fn main() {
    let foo = Foo;
    foo.do_something();
}

playground

Removing the second impl Default block fixes the compile error. (This also happens if Trait, Bar, Foo are imported from another crate.

2

Simplified:

trait Marker {}
impl Marker for u32 {}
impl Marker for u64 {}

impl<X> Trait<X> for Foo where X: Marker ...
1 Like
3

Under RFC 1105 (API Evolution), such changes are considered “minor” breaking changes because they can be fixed by local changes to the calling code to remove disambiguity:

That means that any breakage in a minor release must be very "shallow": it must always be possible to locally fix the problem through some kind of disambiguation that could have been done in advance (by using more explicit forms)

For example, the ambiguous foo.do_something() could be fixed by changing it to an unambigous form like:

Trait::<Bar<u32>>::do_something(&foo)

The RFC notes that any new trait impl is potentially a minor breaking change. However, it also cautions that:

All that said, it is incumbent on library authors to ensure that such "minor" changes are in fact minor in practice: if a conflict like t.foo() is likely to arise at all often in downstream code, it would be advisable to explore a different choice of names.

3 Likes
4

RFC 2451 ammended RFC 1105 to remove that statement and to note that blanket trait implementations are major breaking changes:

As such, RFC #1105 is amended to remove the statement that implementing a non-fundamental trait is a minor breaking change, and states that adding any blanket impl for an existing trait is a major breaking change, using the definition of blanket impl given above.

(The text of RFC 1105 was never updated, but should be. There were also ammendments to RFC 1023, but I haven't checked if those were applied to the text or not.)

1 Like
5

In my opinion, RFCs do not work well as living documents, and shouldn't be treated as such. Particularly in this case, this RFC does not age well as the language changes and new edge cases are discovered. A copy of most of the rules have been moved to the Cargo book here, where new entries can be more easily added, and there is automated tooling to ensure the before/after examples continue to work as expected.

6

I guess it's a larger decision for some team or teams to decide on, but there is a precedent (other RFCs have had their text updated by later RFCs). At the minimum I feel a pointer to other RFCs could (and should) be added to the affected RFCs, IETF style. Then in situations like this thread, there's much less depending on people who happen to know showing up.

2 Likes
7

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.