Passing strings & ergonomics

help
1

If you're writing a method that will take in a string that will be stored as an owned string, but you'll end up having quite a few callers with string literals -- how would you model the function parameter?

Are there others (with any merit) beyond these:

#[derive(Default)]
struct MyStruct {
  s: String
}

impl MyStruct {
  fn set_owned(&mut self, s: String) {
    self.s = s;
  }
  fn set_ref(&mut self, s: &str) {
    self.s = s.to_owned();
  }
  fn set_asref(&mut self, s: impl AsRef<str>) {
    self.s = s.as_ref().to_owned();
  }
  fn to_str(&mut self, s: impl ToString) {
    self.s = s.to_string();
  }
}

fn main() {
  let mut ms = MyStruct::default();
  
  // Pro: Passing (owned) String is very natural and doesn't allocate
  //      superflous strings
  // Con: Requires explicit conversion at call site for refs and literals
  ms.set_owned("hello".into());
  let owned = String::from("hello");
  ms.set_owned(owned);

  // Pro: Passing ref/literals is very natural
  // Con: If owned string is of no use, it could have been reused rather than
  //      allocating a new one.
  ms.set_ref("hello");
  let owned = String::from("hello");
  ms.set_ref(&owned);
  
  // Pro: Ergonomic for both ref/literals and owned
  // Con: Adds overhead in callee for owned case
  ms.set_asref("hello");
  let owned = String::from("hello");
  ms.set_asref(owned);

  // Pro: Ergonomic for both ref/literals and owned
  //      Accepts anything that is ToString (which turns out to be a lot)
  // Con: Hidden superfluous clone for owned case. Adds overhead in callee
  //      for owned case.
  ms.to_str("hello");
  let owned = String::from("hello");
  ms.to_str(owned);
  ms.to_str(42);
}

I used to use AsRef<str> a lot a few years ago, but someone pointed out that if you're using AsRef<str> to create an owned String inside the function, then you haven't understood ToString, so I started using that instead. However, once I started using clippy at maximum nag mode, I started getting warnings about impl ToString (it wants it to be a reference, but this is less useful from an ergonomic perspective).

After a quick and very unscientific look at some of the third party crates that I use, it looks like the preferred way is to simply use &str and accepting the extra cost of allocating a new string in the function (for the cases where the caller has an owned string that it could hand over). Maybe simpler is better?

2

impl Into<String> is an option that accepts both &str and String, and only allocates for the &str case. It does have some rough edges though, like not accepting &String.

3

Since you want to store a String value, the impl ToString variant is all you need. If a String value is passed, the passed value is simply moved.

Which clippy lint is firing? And can you post an example of the code that causes it?

4

It requires pedantic lints, and the lint warning is this:

 2  warning: this argument is passed by value, but not consumed in the function body
   --> src/main.rs:16:27
    |
 16 |   fn to_str(&mut self, s: impl ToString) {
    |                           ^^^^^^^^^^^^^ help: consider taking a reference instead: `&impl ToString`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value
    = note: `-W clippy::needless-pass-by-value` implied by `-W clippy::pedantic`
    = help: to override `-W clippy::pedantic` add `#[allow(clippy::needless_pass_by_value)]`

The code I posted does trigger it (along with a few other things). I understand why it triggers, but for impl ToString I don't think it should warn because - like I wrote earlier - its suggestion makes impl ToString less useful.

5

I was wrong, ToString will always allocate a new String since it takes &self. (I incorrectly thought there was compiler magic for ToString.) The clippy lint is telling us something important, since the intention is to consume a passed String value!

Use the solution from @sfackler instead.

6

TBH, if you're storing a String I would just say to tell people to call .into() or .to_owned() or String::from or .to_string() themselves. Its fine.

If it's really true that you want to optimize for things like string literals, then consider Cow<'static, str> instead of storing a String, for example. Or maybe make an enum of common values so those don't need to be strings at all.