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

smartstring breaks std::string::String? #7

Open
miquels opened this issue Jul 8, 2020 · 10 comments
Open

smartstring breaks std::string::String? #7

miquels opened this issue Jul 8, 2020 · 10 comments

Comments

@miquels
Copy link

miquels commented Jul 8, 2020

smartstring 0.2.3.
rust 1.44.1

this fails:

$ cat main.rs
use smartstring;

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

in a weird way:

$ cargo run
   Compiling smartstring-test v0.1.0 (/home/mikevs/devel/rust/smartstring-test)
warning: unused import: `smartstring`
 --> src/main.rs:1:5
  |
1 | use smartstring;
  |     ^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

error[E0277]: cannot add `&std::string::String` to `std::string::String`
 --> src/main.rs:4:40
  |
4 |     println!("{}", "Hello".to_string() + &(" World".to_string()));
  |                                        ^ no implementation for `std::string::String + &std::string::String`
  |
  = help: the trait `std::ops::Add<&std::string::String>` is not implemented for `std::string::String`

error: aborting due to previous error; 1 warning emitted

For more information about this error, try `rustc --explain E0277`.
error: could not compile `smartstring-test`.

To learn more, run the command again with --verbose.
@miquels
Copy link
Author

miquels commented Jul 9, 2020

It looks like things aren't dereffing to &str anymore because of something in smartstring.

This compiles just fine:

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

But this normally compiles and now fails to as soon as I use smartstring;.

error[E0277]: cannot add `&&str` to `std::string::String`
 --> src/main.rs:6:40
  |
6 |     println!("{}", "Hello".to_string() + &" World");
  |                                        ^ no implementation for `std::string::String + &&str`
  |
  = help: the trait `std::ops::Add<&&str>` is not implemented for `std::string::String`

miquels added a commit to miquels/nntp-rs that referenced this issue Jul 9, 2020
Note that right now `smartstring` influences String/&string
references to deref to &str. So "String + &String" fails, while
"String + (&String).as_str()" works.
See bodil/smartstring#7
@djc
Copy link
Contributor

djc commented Sep 24, 2020

I filed rust-lang/rust#77143 with a simplified version of this, since it seems like a compiler bug.

@miquels
Copy link
Author

miquels commented Sep 25, 2020

So it's expected behaviour. That's a bit of a papercut. Needs documentation then, I guess.

@bodil
Copy link
Owner

bodil commented Sep 25, 2020

"Expected behaviour" doesn't mean desirable behaviour, but Rust's type system has its limitations currently, I guess we're all still waiting patiently for Chalk to happen.

I'd still like to explore options to get around this before writing it off as expected behaviour, though.

@Erutuon
Copy link

Erutuon commented Dec 13, 2021

I ran into this problem and had the hunch that it was related to Add implementations. So I wrote a test:

#[test]
fn can_add_std_string_ref_to_std_string() {
    let _ = "Hello, ".to_string() + &"world!".to_string();
}

Then I commented out each of the Add implementations in turn. Turns out that commenting out impl<Mode: SmartStringMode> Add<SmartString<Mode>> for String fixes the problem. Not sure why that implementation would break deref inference or whatever the compiler is doing. But that's a pretty easy fix.

SmartString currently has a lot more Add implementations than std::string::String. The String + SmartString impl is more than std::string::String can do: there's no Add<String> for String; you have to add a & on the right side.

If all of the Add implementations are deleted except for impl<Mode: SmartStringMode> Add<&'_ str> for SmartString<Mode>, SmartString has equal powers of addition with std::string::String:

#[test]
fn can_add_ref_to_std_string() {
    let _ = String::from("Hello, ") + &SmartString::<Compact>::from("world!");
}

#[test]
fn can_add_str_to_smart_string() {
    let _ = SmartString::<Compact>::from("Hello, ") + "world!";
}

#[test]
fn can_add_std_string_ref_to_smart_string() {
    let _ = SmartString::<Compact>::from("Hello, ") + &String::from("world!");
}

Meaning you always have to add & to the right side of the addition, so this won't compile anymore, just like String + String won't compile:

#[test]
fn can_add_to_std_string() {
    let _ = String::from("Hello, ") + SmartString::<Compact>::from("world!");
}

(It's currently enabled to compile by impl<Mode: SmartStringMode> Add<SmartString<Mode>> for String, the implementation that broke String + &String.)

Actually, in addition to the breakage in the initial post, there are two implementations that break SmartString + &String, if the explicit implementation of SmartString + &String is commented out: impl<Mode: SmartStringMode> Add<Self> for SmartString<Mode> and impl<Mode: SmartStringMode> Add<&'_ Self> for SmartString.

Summary

This was a kind of convoluted post. Basically it looks like implementing some string additions break inference for certain other string additions. SmartString + SmartString or SmartString + &SmartString breaks SmartString + &String; String + SmartString (and, when I write it in lib.rs, String + &SmartString) breaks String + &String. Only implementing SmartString + &str seems to break the fewest additions. It only breaks AnyOwnedString + SmartString. That would bring SmartString up to feature parity with String, which only explicitly implements String + &str but appears to implement String + &String through compiler magic (deref coercion?).

@horazont
Copy link

horazont commented Apr 12, 2022

It's unfortunate that 1.x went out with this issue.

I think the argument of @Erutuon is sound: SmartString does not need more Add<> impls than String itself. There's nothing to optimize in impl Add<SmartString<..>> for String, so it could be deleted, degrading to the usual "you have to take a reference" situation, as with std-String.

Having use smartstring break unrelated code anywhere in the project is kind of a show-stopper for depending on it. This does not just affect files where smartstring is actively used or even imported, it's sufficient to be anywhere in the crate to break stuff.

@ethereumdegen
Copy link

still getting this issue. I installed the Rhai crate and it is breaking parts of my code that works before!! just because im adding strings with +

really sucks

@akprasad
Copy link

akprasad commented Dec 3, 2022

I agree with the comment by @horazont above:

Having use smartstring break unrelated code anywhere in the project is kind of a show-stopper for depending on it.

I'm curious how others are working around this issue. Is there an older version of smartstring that doesn't have this problem? Or, is there a comparable crate (smallstr?) that implements similar functionality?

I apologize if this is the wrong place to ask; I'm a Rust newbie and I'm very excited at the prospect of using this crate.

(Edit: compact_str seems to work well. Mutability, which I need for my use case, was added in version 0.3)

facebook-github-bot pushed a commit to facebookexperimental/allocative that referenced this issue Jan 4, 2023
Summary:
Upsides:

- It seems more actively maintained.
  - Note: on Github issues, they have a couple open reports of fuzz failures
    (from their automation) but those are from a branch, not master.
- It doesn't have this frustrating bug: bodil/smartstring#7

Downsides:

- It doesn't convert String to an inlined value in `From`, but we don't
  actually rely on this (we use small strings mostly when working with directories
  where those elements are produced via an iterator of FileName).

Reviewed By: ndmitchell

Differential Revision: D42313656

fbshipit-source-id: 0708866fced3b32941047755b770244287f6ad94
facebook-github-bot pushed a commit to facebook/buck2 that referenced this issue Jan 4, 2023
Summary:
Upsides:

- It seems more actively maintained.
  - Note: on Github issues, they have a couple open reports of fuzz failures
    (from their automation) but those are from a branch, not master.
- It doesn't have this frustrating bug: bodil/smartstring#7

Downsides:

- It doesn't convert String to an inlined value in `From`, but we don't
  actually rely on this (we use small strings mostly when working with directories
  where those elements are produced via an iterator of FileName).

Reviewed By: ndmitchell

Differential Revision: D42313656

fbshipit-source-id: 0708866fced3b32941047755b770244287f6ad94
facebook-github-bot pushed a commit to facebook/starlark-rust that referenced this issue Jan 4, 2023
Summary:
Upsides:

- It seems more actively maintained.
  - Note: on Github issues, they have a couple open reports of fuzz failures
    (from their automation) but those are from a branch, not master.
- It doesn't have this frustrating bug: bodil/smartstring#7

Downsides:

- It doesn't convert String to an inlined value in `From`, but we don't
  actually rely on this (we use small strings mostly when working with directories
  where those elements are produced via an iterator of FileName).

Reviewed By: ndmitchell

Differential Revision: D42313656

fbshipit-source-id: 0708866fced3b32941047755b770244287f6ad94
@schungx
Copy link

schungx commented Feb 18, 2023

Just a suggestion. Maybe hiding some of the offending Add implementations behind a feature gate that is on by default?

Something like a extra-adds feature. Then library code can simply omit this feature when building.

This will preserve compatibility while allowing people in the know to get around this problem.

@geosxt
Copy link

geosxt commented Apr 16, 2024

Could we expedite this please? This causes anybody who uses Polars to receive this error if they add strings as describe.

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

No branches or pull requests

9 participants