GGist / bip-rs

BitTorrent Infrastructure Project In Rust
Apache License 2.0
296 stars 33 forks source link

Lifetimes and construction of BencodeMut trees #122

Closed the8472 closed 7 years ago

the8472 commented 7 years ago

Since BencodeMut takes slices with a lifetime for strings and dictionary keys that means all the slices must outlive the dictionary, which means in turn that constructing trees of dictionaries with data-dependent nesting depth such as in BEP52 file trees can become quite tedious.

I think it would make sense to let them take std::borrow::Cow<'a, [u8]> instead so either Vec<u8> or &'a [u8] can be passed.

Unless there's some pattern that I am missing that would make this easier than my current approach.

GGist commented 7 years ago

@the8472 I suppose it really depends on what the source format of the data is when attempting to create the tree. An example I have where we just have an array of (length, path), which assumes very little, is as such:

extern crate bip_bencode;

use std::path::Path;
use bip_bencode::{BencodeMut, BMutAccess};

fn main() {
    let mut metainfo_files = Vec::new();
    metainfo_files.push((500, Path::new("asd/asd/asd.txt").to_path_buf()));
    metainfo_files.push((500, Path::new("asd/asd.txt").to_path_buf()));
    metainfo_files.push((500, Path::new("asd.txt").to_path_buf()));

    let mut root = BencodeMut::new_dict();

    for &(length, ref path) in metainfo_files.iter() {
        let file_name = path.iter().rev().next().unwrap().to_str().unwrap().as_bytes();

        // Setup the inner most file node
        let mut ben_inner = BencodeMut::new_dict();
        {
            let ben_inner_access = ben_inner.dict_mut().unwrap();
            ben_inner_access.insert(b"length", BencodeMut::new_int(length));
        }

        // Setup the file node wrapper
        let mut ben_inner_file = BencodeMut::new_dict();
        {
            let ben_inner_file_access = ben_inner_file.dict_mut().unwrap();
            ben_inner_file_access.insert(b"", ben_inner);
        }

        // Do a create if not exist iteration starting from the root node
        let mut node_access = root.dict_mut().unwrap();
        for element in path.iter().take(path.iter().count() - 1) {
            let element_str = element.to_str().unwrap().as_bytes();

            if let Some(mut element_access) = node_access.lookup_mut(element_str) {
                node_access = element_access.dict_mut().unwrap();
            } else {
                node_access.insert(element_str, BencodeMut::new_dict());

                node_access = node_access.lookup_mut(element_str).unwrap().dict_mut().unwrap();
            }
        }

        // Insert the file at the current node
        node_access.insert(file_name, ben_inner_file);
    }
}

In this example, I actually didn't have a problem with the lifetime of the dictionary keys per se.

What I did find impossible to do though was during the node_access iteration, where going from the outer most dictionary to the inner most one (for the purposes of visiting the correct directory nodes), was not possible. The only real way to do this currently would be to have your data in a format such that files sharing a common prefix were iterated over together, so the bencode tree could be created from the inside out, without doing any diving back in to any nodes after the fact to mutate them.

So I definitely think this needs to be remedied, so we can allow users to dive back in to a structure and mutate it. However, I am curious as to your use case for possibly owned keys to be passed in to BencodeMut. The only use case I can see is if you are planning on storing the owned BencodeMut structure (which is definitely valid), instead of just creating it and encoding the structure, relinquishing ownership back to the source data.

the8472 commented 7 years ago

However, I am curious as to your use case for possibly owned keys to be passed in to BencodeMut.

The usecase is not so much storing owned keys in itself but alleviating the need to allocate everything up-front, which can be difficult when computing values during tree construction. You can't mutate the vecs and reference them at the same time.

But I think it might be possible to solve this with an arena allocator, I'll have to experiment with that.

the8472 commented 7 years ago

Looks like my problem is solvable using an arena allocator passed to a recursive function with the appropriate lifetime annotations after all.

GGist commented 7 years ago

@the8472 I decided to move forward with your suggestion, which makes the ergonomics of BencodeMut in certain situations a lot better. Thanks!