`url::form_urlencoded::serialize` is moving iterated key-value pairs

[Byron]

Currently serialize() is difficult to use for all users who don't want to drain vectors of string slices, but prefer keep iteration strictly read only.
Maybe, in the servo project it's uncommon to do that, but at least I am making my case and believe read-only operation should be the default.

Example

url::form_urlencoded::serialize([("client_id", "bogus"),
                                 ("scope", "A B C"].iter().map(|&(a,b)| (a, b)));

To my mind, the .map() call should not be required.

Maybe my code is the exception though, and not the rule, but when implementing protocols one will usually have a pre-determined set of key-value pairs.

[SimonSapin]

This part of the API has not gotten a lot of use or thought yet. What specific input type do you think would work better here?

[Byron]

I think if the signature would be changed to ...

// note the `&` in front of the tuple
// disclaimer: don't know if it actually compiles
pub fn serialize<'a, I>(pairs: I) -> String where I: Iterator<Item = &'a (&'a str, &'a str)> {

... then we should be good for my case.
While we are talking about it, it might be worth removing serialized_owned from the public API as well, just because it's unintuitive that it all of the sudden is that specific. Just making it generic might be worth doing alternatively.

[SimonSapin]

This signature works for you specific use case, but is less flexible: the outer & means you have to have existing tuples of &str somewhere, to reference them.

I’m starting to thing an iterator is not so great for this. What do you think of a method you call multiple times instead?

let mut s = String::new();
{
    let mut query = url::from_urlencoded::Serializer::new(&mut s);
    query.add_pair("client_id", "bogus");
    query.add_pair("scope", "A B C");
}

[Byron]

The builder patterns seems like 'too much' in this case. It would require the serializer to keep its own, dynamically allocated state, which is less efficient than it should be. It's my believe that an API should not enforce unnecessary copies or allocations. Ideally, it's made so that it works out of the box for 80% of the use-cases, but can be adapted with a little effort (like .iter().map(|&(a,b)| (a, b)) in my case .. which is of course part of the 80% of the code ;)).

Here are my thoughts as code:

    fn pair_transformer<'a, I>(pairs: I) -> String
        where I: Iterator<Item=&'a (&'a str, &'a str)> {
        for pair in pairs {
            let &(a, b) = pair;
            // do something
        }
        String::new()
    }

    fn consuming_pair_transformer<'a, I>(pairs: I) -> String
        where I: Iterator<Item=(&'a str, &'a str)> {
        for pair in pairs {
            let (a, b) = pair;
            // do something
        }
        String::new()
    }

    // This works, natively
    pair_transformer([("a", "b")].iter());

    // This one too
    pair_transformer(vec![("a", "b")].iter());

    struct Pair<A: Copy, B: Copy> {
        first: A,
        second: B
    }

    impl<A: Copy, B: Copy> Pair<A, B> {
        fn from_tuple(t: (A, B)) -> Pair<A, B> {
            Pair {
                first: t.0,
                second: t.1
            }
        }

        fn as_tuple(&self) -> (A, B) {
            (self.first, self.second)
        }
    }

    // Consuming custom type instances
    let pairs = [Pair::from_tuple(("a", "b"))];
    // pair_transformer(pairs.iter().map(|p| &p.as_tuple()));
        //     tests/lang.rs:845:44: 845:56 error: borrowed value does not live long enough
        // tests/lang.rs:845     pair_transformer(pairs.iter().map(|p| &p.as_tuple()));
        //                                                              ^~~~~~~~~~~~
        // tests/lang.rs:845:22: 845:57 note: reference must be valid for the method call at 845:21...
        // tests/lang.rs:845     pair_transformer(pairs.iter().map(|p| &p.as_tuple()));
        //                                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        // tests/lang.rs:845:43: 845:56 note: ...but borrowed value is only valid for the block at 845:42
        // tests/lang.rs:845     pair_transformer(pairs.iter().map(|p| &p.as_tuple()));
        //                                                             ^~~~~~~~~~~~~
    // This works now ... 
    consuming_pair_transformer(pairs.iter().map(|p| p.as_tuple()));

When looking at the example above, one might be inclined to think that serialize_owned() might want to be semantically similar to consuming_pair_transformer(...), to support the case of generated values as well.

Of course it would be better to have only one function for all of it ... I will see if I can find something useful ... .
If not, i'd clearly go for two versions of the functions, but would prefer a review of which one should be the default ... is it tuned for generated values like in the second example, or for arrays and vectors ?

[SimonSapin]

The builder patterns seems like 'too much' in this case. It would require the serializer to keep its own, dynamically allocated state, which is less efficient than it should be.

I don’t think this is the case. In my previous message, the internal state of Serializer is just a boolean (whether the next call needs a & separator), which can definitely be stack-allocated.

[Byron]

Ah, so it's build on the fly. Please discard my previous comment about it needing dynamic allocation. The remaining issue I take that it is a very powerful pattern for something that effectively supports add_pair(...) only, in this example.

[SimonSapin]

serialize_owned can be safely disregarded. I added it as a convenience that I imagined would be nice to have, but that’s it.

That aside, I don’t understand your point in the last two comments.

[Byron]

Maybe there is no point - in short I just liked the conciseness of the current implementation. An iterator over pairs seems like the simplest solution.

By the way: I find this example extremely interesting as it is as simple as it can get, but can teach a lot about API design in Rust.

Here we are basically talking about: How to deal with borrowed or owned values generically ?. The serialize() function is read-only, and doesn't actually care about whether the value it reads is moved or borrowed. The case were ownership is transferred to serialize() only seems interesting for generated values.

[SimonSapin]

Yes, good API design is hard :)

How to deal with borrowed or owned values generically ?

For just one value we have pretty good conventions: take a borrow if you don’t need to take ownership, and let deref coercions do their magic. The difficulty here is dealing with a sequence of pairs of borrowed values.

[Byron]

Do you think a Pair trait could help ? It's surprisingly absent from std, and might be able to hide ownership.

[SimonSapin]

I think anything called Pair is over-engineered when we have tuples.

[Byron]

I agree: tuples are much more elegant. But would a trait be able to hide ownership ? My feeling says: it could.

[SimonSapin]

I’m skeptical, but what would this trait look like?

[Byron]

I am currently trying to put one together, still working on making the compiler happy (as you might have figured by now, I am a Rust-greenling :)!)

[Byron]

This is how far I got - now it's telling me the following:

tests/lang.rs:902:5: 902:21 error: the trait `pair_trait_for_iteration::Pair<'_, &core::str::Str, &core::str::Str>` is not implemented for the type `&(&str, &str)` [E0277]
tests/lang.rs:902     pair_transformer([("a", "b")].iter());
                      ^~~~~~~~~~~~~~~~

Here is the code

    trait Pair<'a, A, B> {
        fn first_ref(&'a self) -> &'a A;
        fn second_ref(&'a self) -> &'a B;
    };

    struct PairOwned<A, B> {
        first: A,
        second: B,
    }

    // Only implemented for the cases we are interested in ...
    impl<'a, A, B> Pair<'a, A, B> for PairOwned<&'a A,&'a B> {
        fn first_ref(&'a self) -> &'a A {
            self.first
        }
        fn second_ref(&'a self) -> &'a B {
            self.second
        }
    }

    impl<'a, A, B> Pair<'a, A, B> for &'a(&'a A, &'a B) {
        fn first_ref(&'a self) -> &'a A {
            self.0
        }
        fn second_ref(&'a self) -> &'a B {
            self.1
        }
    }

    fn pair_transformer<'a, I, T>(pairs: I) -> String
        where   T: Pair<'a, &'a Str, &'a Str> + 'a,
                I: Iterator<Item=T> {
        let mut s = String::new();
        for pair in pairs {
            s = s
                + pair.first_ref().as_slice()
                + pair.second_ref().as_slice();
        }
        s
    }

    pair_transformer([("a", "b")].iter());

I was hoping that & will just be included in T, which would hide ownership effectively, and allow the function to work in any case, provided Pair is implemented for all the involved types. The latter is easy for two-tuples and two-arrays.

Posted the question on Stackoverflow.