Transferring ownership of fields

[icorderi]

Say type T has a field u of type U and field v of type V.
T and U are both ::protobuf::Message.

I have the following function:

fn foo(t: T) -> Result<U> {
    // look into t.v and return Err based on same value
    // return Ok(t.u) 
}

Even though it seems like I have ownership over t, I can't deconstruct a Message and pass on ownership of the field u.

I recommend adding something similar to the following two extra functions per field

    // same as existing
    pub fn mut_field(&'a mut self) -> &'a mut T {
        if self.status.is_none() {
            self.field.set_default();
        };
        self.field.as_mut().unwrap()
    }

    // same as existing
    pub fn get_field(&'a self) -> &'a T {
        self.field.as_ref().unwrap_or_else(|| T::default_instance())
    }

    // new, the idea is to take the field away from the enclosing message
    // ownership is transferred to the caller
    pub fn take_field(&mut self) -> T {
        // perhaps we should leave a tombstone behind 
        // the current impl of take refill the value with default
        self.field.take().unwrap() 
    }

    // the original message's life ends here, 
    // ownership of the field is passed on
    pub fn unwrap_field(self) -> T {
        self.field.unwrap()
    }

With the new semantics I can now implement my method by:

fn foo(t: T) -> Result<U> {
    let v = t.take_v();
    if v.w { Err("Error...") }
    else { Ok ( t.unwrap_u() ) }
}

I don't know if this is the correct way of solving the use case, but it makes it possible to implement.

Another option is...

A possible breaking change given that this needs to be implemented for each field is to have a single get_field that returns Field<T> and have that wrapper trait surface take(), unwrap(), as_ref(), and as_mut() (perhaps clone() if T : Clone). I know this is a subset of SingularField and SingularPtrField backing the internal Message.

Honestly, I've seen all those functions being used throughout the std and other crates, I'm not sure if it's worth bringing it up with the Rust devs but having 4 unique traits that represent each of those operations might clean up a lot of code and allow for certain generic macro sugaring. If I'm not mistaken, Deref and DerefMut cover the signatures for as_ref and as_mut as operator overloads which is what is currently available with get_field and get_mut_field. I'm not sure if the borrow module is more applicable.

[icorderi]

Here is a possible patch addressing this. It passes all tests.

[stepancheg]

I added one take_xxx operation.

Added take_xxx does not crash if field is unset, but rather returns a new object. I think it is more convenient.

Another difference is that take_xxx is also generated for repeated fields, but not generated for primitive types.

unwrap_xxx could be useful, but I'm concerned about generated code size. Less useful operations like unwrap_xxx could be later turned on by .proto file options, similarly to how gogo-protobuf does.

Exposing SinglularPtrField instead of xxx_yyy functions is an interesting idea. I need to think about it.

[icorderi]

Well my generated file has 14712 lines (GitHub has issues rendering it and caps at 13105 lines). I'm not really that worried about file size.

My concerns are with run-time performance, which is why the take_xxx semantics are for ownership transferring with no copying. The current implementation is a clone_xxx and has certain performance implications expected from a clone.

When doing dozens to hundreds of thousands of operations per second this things start to add up. Unnecessary copying is a problem, specially when hidden behind interfaces you don't have control over.

unwrap_xxx is a case where you want to get the inner data and forget about the envelop. It might not seem useful with flat simple .proto constructions, but with large .proto with deep chains it becomes a necessity. Remember proto messages both represent structs and union cases, there is no distinction between the two. take_xxx and unwrap_xxx are extremly helpful when dealing with union types inside a proto.

Custom .proto per language is not always possible

Unfortunately, modifying .proto file definitions to augment them for specific languages is a nice feature but not a practical one. The .proto files are in general shared across language barriers, having to maintain a custom copy of the .proto for each language that access it is not practical and defeats the purpose of having a .proto that can be used to exchange data between systems written in different languages (not to mention in some cases it crosses corporate boundaries).

Refactor suggestion

I would be more than happy to help abstract the fields into a Field, OptionalField and RepeatableField traits and having those traits expose to_owned, unwrap, as_ref, as_mut, clone and whatever else I'm missing. This could very well be implemented inside the protobuf library, having the specific code for each primitive type and std:: type as well as the default implementation over Message.
Each custom type generated by codegen.rs would only need to add #[deriving(Field)]
Hence, reducing greatly the size of the generated output and giving the caller of the library more control over what is going on.
This also allows users of the library to extend the behavior of fields.

[stepancheg]

To be clear, take_xxx solves performance issue?

[stepancheg]

Options in .proto

About options to customize file generation. Patching .proto file during code generation is a recommendation of protobuf authors.

I don't like it personally, but protoc does not pass any options to plugin.

Refactor suggestion

I don't understand, how #[deriving(Field)] should work, and what generated code would look like. Code snippet might help.

[icorderi]

I expected certain features from the macro that are simply not there yet. I tried creating an example and run into both a limitation on the current macro system that prevents us from doing that, and another bug preventing us to do accomplish the first macro manually with multiple calls to a smaller macro (plus some auto generated code).

I run into a few more setbacks when returning a custom type that wraps the object.
The current state of operator overloading is not complete, and I'm not certain if get/set will be turned into traits and allow overloading. Without that it gets very verbose and creates extremely long chains of method calls.

let mut me = Person::new();
me.mut_age().set(27u);
me.mut_name().as_mut().mut_first().set("John".to_string());
me.mut_name().as_mut().mut_last().set("John".to_string());

If we had a built-in trait abstracting assignment we could change that to something closer to:

let mut me = Person::new();
me.age = 27u;
me.name.first = "John".to_string();
me.name.last = "John".to_string();

I'll keep tabs on the state of the ops module.

Regarding the take_xxx it improves performance when it is not copying. If it copies then its a problem.

TL;DR; Perhaps we can revisit the architecture later when the macro system and the ops module mature a bit. I would like to insist though on a non-copy take_xxx(&mut self) -> T and an unwrap_xxx(self) -> T

[stepancheg]

take_xxx does not copy. What's hurry for unwrap_xxx?

[manio]

This issue was active 11 years ago, but anyway, you made my day 🥇
#761