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

Can’t move out of BString using into methods #84

Open
csnover opened this issue Mar 16, 2021 · 8 comments
Open

Can’t move out of BString using into methods #84

csnover opened this issue Mar 16, 2021 · 8 comments
Labels
question Further information is requested

Comments

@csnover
Copy link

csnover commented Mar 16, 2021

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`
}
@BurntSushi
Copy link
Owner

BurntSushi commented Mar 16, 2021

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);
}

@csnover csnover changed the title Can’t move out of BString Can’t move out of BString using into methods Mar 16, 2021
@csnover
Copy link
Author

csnover commented Mar 16, 2021

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.

@BurntSushi
Copy link
Owner

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 BurntSushi added the question Further information is requested label Mar 16, 2021
@csnover
Copy link
Author

csnover commented Mar 16, 2021

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 rust-lang/rfcs#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 rust-lang/rust#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.

@BurntSushi
Copy link
Owner

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.

@pravic
Copy link

pravic commented Mar 22, 2024

Also stumbled upon this.

This is a highly misleading error, I wish we had either a working BString::into_string or at least a better error message (if that's possible).

@BurntSushi
Copy link
Owner

The "better error message" part of that isn't really something bstr has control over. That's more of a rustc thing.

@Caellian
Copy link

Caellian commented Oct 21, 2024

Thanks for clarifying.

How about having a supplement trait extended by ByteVec and implemented for all the types which are currently broken due to deref issue via a macro?

Would require some experimenting, but could work? I'm assuming rustc will pick shorter implementation path.

If that doesn't work, a macro could address maintainability issues for directly implemented items:

macro_rules! intos {
    ($($mod: tt)?) => {
        /// implemeted in [`test`] macro.
        $($mod)? fn test() {
            println!("hello")
        }
    };
}
struct BString;
impl BString {
    intos!(pub);
}
trait VecExt {
    intos!();
}
impl VecExt for Vec<u8> {}

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

No branches or pull requests

4 participants