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

fix/feat: Change the existing Add<...> impls #130

Merged
merged 2 commits into from
Jun 27, 2022
Merged

Conversation

ParkMyCar
Copy link
Owner

Implementing Add<CompactString> for String breaks String + &String, as long as compact_str is used somewhere in your current crate. Specifically this code won't compile:

use compact_str;

fn main() {
  println!("{}", "Hello".to_string() + &(" World".to_string()));
}

This isn't good! This has to do with Rust's current trait resolver and is a known issue rust-lang/rust#77143

To fix this, we remove impl Add<CompactString> for String. You can still add CompactString to String, you just need to take a reference to it, then the impl Add<&str> for String kicks in.

We also remove a lot of impl Add<...> for CompactString, leaving just impl Add<&str> for CompactString, which allows us to add more string types than we currently can, e.g. &Box<str> and &Cow<str>. This is because the compiler auto derefs these types to &str and uses that impl

  * removed Add<CompactString> for String, because it broke adding some types to String
  * remove all Add<T> for CompactString types, besides Add<&str> for CompactString, which actually allows us to add _more_ string types
@ParkMyCar ParkMyCar force-pushed the fix/remove_add_impls branch from 354c5f3 to 7d723e5 Compare June 27, 2022 02:41
@ParkMyCar ParkMyCar merged commit 10bc647 into main Jun 27, 2022
@ParkMyCar ParkMyCar deleted the fix/remove_add_impls branch June 27, 2022 14:27
ParkMyCar added a commit that referenced this pull request Jun 28, 2022
    * removed Add<CompactString> for String, because it broke adding some types to String
    * remove all Add<T> for CompactString types, besides Add<&str> for CompactString, which actually allows us to add _more_ string types
    * Update .gitignore

  Backport of #130
ParkMyCar added a commit that referenced this pull request Jun 28, 2022
    * removed Add<CompactString> for String, because it broke adding some types to String
    * remove all Add<T> for CompactString types, besides Add<&str> for CompactString, which actually allows us to add _more_ string types
    * Update .gitignore

  Backport of #130
@ParkMyCar
Copy link
Owner Author

Note: these Add<...> impls were first available on v0.4.0. This fix has been back ported to:

  • v0.4.0 -> v0.4.1
  • v0.5.0 -> v0.5.1

Technically this was a breaking change because you can no longer add a String to a CompactString, but to fix it all you need to do is take a reference, e.g. make it &String. IMO this small change is worth it to prevent the bug, where importing compact_str can break other String + operations, even if they don't use CompactString at all themselves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant