Adding impl Add<char> for String breaks code (adding an impl leads to breakage due to deref coercions)

[LukasKalbertodt]

I just prepared a PR to fix #38784. I wanted to add Add<char> and AddAssign<char> impls for String. However, this breaks the following code:

let a = String::from("a");
let b = String::from("b");
a + &b;

(Playground)

Apparently, without Add<char>, deref coercion is used to turn &String into &str. With the additional impl, deref coercion is no more and thus it results in:

error[E0277]: cannot add `&std::string::String` to `std::string::String`
[...]
     = help: the trait `std::ops::Add<&std::string::String>` is not implemented for `std::string::String`

This is unexpected, because RFC 1105 says that adding an implementing for a non-fundamental trait is a minor change and thus should not require a major version bump. But if I were to submit a PR which adds Add<char> for String, it would probably not be merged, because it breaks a bunch of crates. a + &b for a and b being String is widely used.

I would have expected that either deref coercion wouldn't happen in cases where it influences the trait selection (in order to avoid this breakage) or that deref coercion would still happen if there is more than one impl. Since the former is not possible anymore, I think the latter is the solution to this problem.

The third option (I can see) would be to change RFC 1105 and amend a section about this special case.

[hanna-kruppe]

This is unexpected, because RFC 1105 says that adding an implementing for a non-fundamental trait is a minor change and thus should not require a major version bump. But if I were to submit a PR which adds Add for String, it would probably not be merged, because it breaks a bunch of crates. a + &b for a and b being String is widely used.

This is a misreading of the RFC. Practically all changes have the potential to stop existing code from compiling. The section that says implementing a non-fundamental trait is a minor change even explicitly spells out that "technically implementing any existing trait is a breaking change".

What the RFC defines as "minor change" doesn't imply no breakage is possible, it's a middle ground between freezing the entire standard library forever and breaking the stability promise. It achives this balance by the policy 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) or other annotation (like disabling a lint)

In this case the local disambiguration is not relying on Deref coercion (or doing other changes that force the Deref coercion to kick on even if the impl is added, e.g. let tmp: &str = &b; a + tmp;).

The RFC goes on to say the following, which addresses your point about breaking a bunch of crates in practice:

Although this general policy allows some (very limited) breakage in minor releases, it is not a license to make these changes blindly.

Whether a given minor-but-breaking change is accepted is decided by the relevant team, based on considerations such as: how widespread the breakage is, how desirable the change is, how easily the broken crates can be updated, etc.

[eddyb]

error[E0277]: cannot add &std::string::String to std::string::String

I think part of the solution here should be implementing Add<&String> as well.

[LukasKalbertodt]

@rkruppe Sure, I'm fully aware that technically, everything can be a breaking change. And sorry if my initial comment sounded somewhat aggressive -- that was not intended!

It's just that to me this was really unexpected. Maybe it's just me, but I wouldn't have guessed that adding such a harmless looking impl might cause breakage in harmless looking code.

@eddyb

I think part of the solution here should be implementing Add<&String> as well.

This works for the a + &b problem, but won't work for a + &&b cases. And yes, those do exist. I noticed this problem because a part of the compiler stopped compiling because of my change (librustc_target/spec/mod.rs:1086 @ 5fdcd3a, but I don't think the specific code is really important). And in that code, a &&String is added to a String. And we obviously don't want to add impls like Add<&&&&&String>.

---

I just don't understand why deref coercion is not done in this case. Is there a good reason why?

I thought that maybe deref coercion could lead to ambiguity when multiple impls are available. But for one, some kind of ambiguity already exists which is resolved by using the first type that works (meaning, the working result with the fewest *).

Then I thought: maybe when there two impls which expect &str, &String and &String, &str. If we then pass &String, &String, then there might be an ambiguity, because it's not clear on which parameter the deref coercion should happen. But as it turns out, the compiler doesn't complain in this case, so deref coercion is actually done in the multiple impl case... sometimes.

So yeah again: why doesn't it happen for Add::add(String, &String)? And is there a good reason for that?

[jonhoo]

This happens outside the standard library too. For example, in jonhoo/fantoccini#196, a user reported that their code stopped compiling simply by virtue of adding a use fantoccini; to their file. This can also be replicated in a single crate: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1e56e21ffebcbdb9f84dd2da34fc7e80

[UnkwUsr]

This happens outside the standard library too.

This can also be replicated in a single crate: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1e56e21ffebcbdb9f84dd2da34fc7e80

@jonhoo Do you know if there is tracking issue for this behavior? Possibility of library to break the code that uses this library sounds very bad.

I've found mentioning of something related in https://std-dev-guide.rust-lang.org/breaking-changes/new-trait-impls.html#deref-coercion-breaks-when-a-new-impl-is-introduced, but this is std library dev guide, and they mentioning it as precaution in developing std library, not any regular crate.

[jonhoo]

@UnkwUsr Unfortunately no, I don't know of one.