bstr icon indicating copy to clipboard operation
bstr copied to clipboard

Can’t move out of BString using `into` methods

Open csnover opened this issue 3 years ago • 7 comments

Because ByteVec methods operate through deref on BString, it is impossible to use the ones with move semantics (into_string etc.) to move out of a BString, which is confusing.

use bstr::{BString, ByteVec};

fn main() {
    let x = BString::from("hello world");
    x.into_string_lossy(); // cannot move out of dereference of `BString`
}

csnover avatar Mar 16 '21 05:03 csnover

This is a limitation of Deref. There is no DerefMove. There's nothing I can do about it, sorry. An alternative would be to define inherent methods on BString, but I'm not sure I want to go down that route.

But I note that strictly speaking, the title of your issue is wrong. You can of course move out of a BString:

use bstr::BString;

fn main() {
    let x = BString::from("foo");
    let y = <Vec<u8>>::from(x);
    println!("{:?}", y);
}

BurntSushi avatar Mar 16 '21 10:03 BurntSushi

There's nothing I can do about it, sorry.

Can you not implement the into methods on BString and forward them to the inner bytes?

impl BString {
    pub fn into_string_lossy(self) -> String {
        self.bytes.into_string_lossy()
    }
}

I note that strictly speaking, the title of your issue is wrong.

OK, I updated the title to not be wrong.

csnover avatar Mar 16 '21 16:03 csnover

Can you not implement the into methods on BString and forward them to the inner bytes?

If you quoted the rest of that paragraph...

An alternative would be to define inherent methods on BString, but I'm not sure I want to go down that route.

To elaborate:

  • I would like to keep the types simple and small wrappers.
  • The maintenance overhead of copying every method that takes self by value is annoying.
  • Users looking at the docs for BString may very well be confused by the fact that has a subset of ByteVec methods as inherent methods but not others. It's probably a confusion that's easily cleared up with docs or what not, but it's still a cost IMO.

I'm not saying "no," but those are my thoughts currently.

BurntSushi avatar Mar 16 '21 16:03 BurntSushi

If you quoted the rest of that paragraph...

Ugh, I’m sorry about that. I’m having one of those days.

I agree that it is crappy for maintenance to have to reimplement methods (this is one of my own top pain points writing newtypes). Hopefully https://github.com/rust-lang/rfcs/issues/997 eventually eliminates the need to do it, though I don’t see any indication that that’s coming soon. At least implementing forwarding methods is simple enough to do by macro, so boilerplate would be minimal, and the docs on the methods could be similarly rote (“Consumes the inner Vec by forwarding to [`ByteVec::<into_whatever>`].” or something).

To your point about user confusion, as a user I would say it is probably more confusing to have a BString type which owns its contents but can’t be moved-from in the way the API suggests should be possible (via the auto-derefed into methods). Part of this is that Rust’s diagnostic here is confusing; there is an open issue about that at https://github.com/rust-lang/rust/issues/45515.

Ergomonically, I feel like it is not great for a consumer to have to e.g. Vec::<u8>::from(foo).into_string_lossy(). It’s also not an obvious solution (at least it was surprising to me), and so I suspect some people—especially those who are newer to Rust—will end up doing something bad like (*x).clone() to escape a seemingly impossible situation. Since I was already writing a wrapper myself, I just ended up replacing BString with Vec<u8> instead.

If you really feel like reimplementing the into methods on BString is a wrong solution, maybe a compromise would be to just add a method that gives a clear way to unwrap it?:

pub fn into_vec(self) -> Vec<u8> {
    self.bytes
}

Thanks for replying so quickly, and apologies again for my brain dysfunction.

csnover avatar Mar 16 '21 17:03 csnover

Those are all fair points. I suppose if we could do this will minimal fuss using a macro, then I might be on board with it.

Adding more explicit constructors and eliminators to the APIs of BStr and BString are also something I'd be amenable to, independent of your specific issue.

BurntSushi avatar Mar 16 '21 17:03 BurntSushi