P3KI / bendy

A rust library for encoding and decoding bencode with enforced cannonicalization rules.
BSD 3-Clause "New" or "Revised" License
77 stars 14 forks source link

Add `Value` type holding arbitrary decoded bencoded values #33

Closed casey closed 4 years ago

casey commented 4 years ago

Value implements both FromBencode and ToBencode, for conversion to and from owned values.

With the serde feature enabled, Value implements Serialize and Deserialize. Deserialize can be used to deserialize Values that borrow from an input buffer.

I'm not sure if this is something that's appropriate for Bendy, but I needed it for my own use, so I thought I would open a PR. If you'd prefer not to add it, I can easily use it from my own crate instead.

I wrote a program that traverses large collections of .torrent files to extract statistics and other information, and for a few reasons I'm unable to use bendy's Object type. In particular, I need the ability to traverse a value multiple times, but Object requires decoding as you go, so values can't be reused.

Also, for ergonomic reasons, I need to be able to clone values from the middle of a bencoded object, which doesn't seem to be possible with `Object.

It might be somewhat confusing when someone might want an Object and when they might want a Value, so I added documentation to the Value module explaining the difference.

thequux commented 4 years ago

This is coming along very well! No complaints about the code so far, but I'd rather not merge this until you fix the CI errors. Fortunately, this should be a simple matter of importing Cow and BTreeMap from alloc instead of std. (std actually just re-exports the types from alloc, so the interface will remain exactly the same). Further, because of our extern crate alloc; in lib.rs, you can import types from alloc even when building on std.

casey commented 4 years ago

This is coming along very well! No complaints about the code so far, but I'd rather not merge this until you fix the CI errors. Fortunately, this should be a simple matter of importing Cow and BTreeMap from alloc instead of std. (std actually just re-exports the types from alloc, so the interface will remain exactly the same). Further, because of our extern crate alloc; in lib.rs, you can import types from alloc even when building on std.

Nice, I've never worked in a no-std environment that uses alloc before, so it didn't occur to me that all the std::collections types would be available in alloc. I changed the code to use the collections from alloc.

I rebased onto the changes in the serde-more branch, so I had to force push. Is there a better way to do that which won't cause you to lose the state of the review? I could have done a merge commit, but the history would get pretty gross, and I'm not sure if that would preserve the review state.

I'm running into an issue with the optional dependency on serde_bytes. I'm trying to make it an optional dependency conditional on the serde dependency, but it isn't working, even though I think I have the syntax right.

I can get it work if I create a feature serde_support = ["serde_bytes", "serde"], but it's common to have a feature named serde to enable serde support, and it would be slightly annoying if users had to remember that the feature is called serde_support.

I tried to rename the serde crate to serde_rs, so the feature can be serde = ["serde_bytes", "serde_rs"], but that causes issues with serde_derive macros not being able to find the serde crate.

Do you know if there's a way to get this to work?

casey commented 4 years ago

Here's a minimal example showing the issue withe renaming: https://github.com/casey/optional-dependency/edit/master/src/lib.rs

casey commented 4 years ago

I went ahead and updated the branch with a diff that creates a serde_support feature that depends on the serde and serde_bytes feature. It's not ideal to have that be the feature name that users need to remember, but until namespaced features lands I think it's the best solution. (https://github.com/rust-lang/cargo/issues/5565)

0ndorio commented 4 years ago

I went ahead and updated the branch with a diff that creates a serde_support feature that depends on the serde and serde_bytes feature. It's not ideal to have that be the feature name that users need to remember, but until namespaced features lands I think it's the best solution. (rust-lang/cargo#5565)

Another potential workaround is the #[serde(crate = "$name")] attribute. As serde_derive by default injects an extern crate serde as _serde this can be used to replace this behavior with a custom crate name. So in your example it would be #[sere(crate = "serde_")].

Downside: You would need to add the attribute to every struct you want to derive the serde traits, but as this only affects the internal test cases this might work out.

@thequux What do you think about this?

casey commented 4 years ago

@0ndorio Ahh, that's much better. It's annoying to have to add a bunch of annotations, but it's probably better than users having to remember a nonstandard name for the features.

thequux commented 4 years ago

This looks good, but I've apparently created some spurious merge conflicts with how I merged #32 . I'll go ahead and merge this one manually as well.

thequux commented 4 years ago

Once again, it seems like github can't manually mark a PR as merged :-/. Your changes made it in though.