Repository: capnproto/capnproto-rust (language: rust)

Issue:
Mandatory value accessors
Hello and thank you for your work!

I am working on a project that uses a capnproto schema where most of the values are mandatory when reading a message.
I got bitten multiple times by the fact that a `reader.get_field()` method for a `field @0 :Text` member will return an empty string when `field` is not specified in the message.
Indeed I am now using `reader.has_field()` with the following pattern:
```Rust
let key = if reader.has_key() {
    reader.get_key()?
} else {
    return Err(::capnp::Error::failed("Missing `key` parameter".to_owned()));
};
```

This is rather cumbersome to do everywhere and I wonder if the API could be improved.
Would it make sense to add a new error type that could be called `MissingValue` (I don't care much about the actual name) and add a new `try_get_<name>` methods in readers to retrieve a value if it exist and fail otherwise without ever returning a default value if one hasn't been specified in the schema.

The sounds fine in principle but `get_<name>` reader methods already return a `Result`which means both methods would have the same prototype, only proper documentation would differentiate them:
* `get_<name>` methods will return a value, and sometime a default one for types that have an implicitly defined value like `Text` or integer fields
*  `try_get_<name>` methods will only return a value when it is present in the message

Thank you for taking the time to read me. Maybe my solution is totally off but the ergonomic issues are real though :)


Discussion / hints:
If we consider that capnpproto-rust errors are "low-level" errors maybe instead of adding a new error type that is mostly about application implementation, we could add `opt_get_<name` methods that would return an Option?
Some other cases where this is would be useful are [get_data_field][1] and [get_bool_field][1].

Imagine the case where a schema has an added data or boolean field.  If the data is not present those routines return `Zero::zero` and `false` respectively.

In these cases it may be desirable for the application code to detect and handle the case where the data is not present.

[1]: https://github.com/capnproto/capnproto-rust/blob/d390ede04717b5961da8b5e9ab90dbeace32197f/capnp/src/private/layout.rs#L2854-L2867
[2]: https://github.com/capnproto/capnproto-rust/blob/d390ede04717b5961da8b5e9ab90dbeace32197f/capnp/src/private/layout.rs#L2869-L2880
My preferred way to do this would be to add an annotation in [rust.capnp](https://github.com/capnproto/capnproto-rust/blob/d390ede04717b5961da8b5e9ab90dbeace32197f/capnpc/rust.capnp) to allow individual fields to opt in to getting wrapped in `std::Option` in the generated code.
@dwrensha we are hoping to switch to capnproto at my work.  Since the capnproto format is extensible, we want to add fields to our data-structures over time. We are excited to have this feature.

I am still coming up to speed with this codebase. Could you give some pointers on how to get started implementing what you have in mind?

Fix patch:
diff --git a/capnp/src/private/layout.rs b/capnp/src/private/layout.rs
index 7f5fbffaf..b7f1c1e05 100644
--- a/capnp/src/private/layout.rs
+++ b/capnp/src/private/layout.rs
@@ -3479,6 +3479,15 @@ impl<'a> StructReader<'a> {
         }
     }
 
+    #[inline]
+    pub fn is_pointer_field_null(&self, ptr_index: WirePointerCount) -> bool {
+        if ptr_index < self.pointer_count as WirePointerCount {
+            unsafe { (*self.pointers.add(ptr_index)).is_null() }
+        } else {
+            true
+        }
+    }
+
     pub fn total_size(&self) -> Result<MessageSize> {
         let mut result = MessageSize {
             word_count: u64::from(wire_helpers::round_bits_up_to_words(u64::from(
diff --git a/capnpc/rust.capnp b/capnpc/rust.capnp
index d375199f7..4636baa3a 100644
--- a/capnpc/rust.capnp
+++ b/capnpc/rust.capnp
@@ -26,3 +26,26 @@ annotation parentModule @0xabee386cd1450364 (file) :Text;
 #      }
 #    }
 #  }
+
+annotation option @0xabfef22c4ee1964e (field) :Void;
+# Make the generated getters return Option<T> instead of T. Supported on
+# pointer types (e.g. structs, lists, and blobs).
+#
+# Capnp pointer types are nullable. Normally get_field will return the default
+# value if the field isn't set. With this annotation you get Some(...) when
+# the field is set and None when it isn't.
+#
+# Given
+#
+#     struct Test {
+#         field @0 :Text $Rust.option;
+#     }
+#
+# you get getters like so
+#
+#     assert_eq!(struct_with.get_field(), Some("foo"));
+#     assert_eq!(struct_without.get_field(), None));
+#
+# The setters are unchanged to match the Rust convention.
+#
+# Note: Support for this annotation on interfaces isn't implemented yet.
diff --git a/capnpc/src/codegen.rs b/capnpc/src/codegen.rs
index c3fe0e23c..2db5dfdb0 100644
--- a/capnpc/src/codegen.rs
+++ b/capnpc/src/codegen.rs
@@ -489,6 +489,7 @@ fn module_name(camel_case: &str) -> String {
 // Annotation IDs, as defined in rust.capnp.
 const NAME_ANNOTATION_ID: u64 = 0xc2fe4c6d100166d0;
 const PARENT_MODULE_ANNOTATION_ID: u64 = 0xabee386cd1450364;
+const OPTION_ANNOTATION_ID: u64 = 0xabfef22c4ee1964e;
 
 fn name_annotation_value(annotation: schema_capnp::annotation::Reader) -> capnp::Result<&str> {
     if let schema_capnp::value::Text(t) = annotation.get_value()?.which()? {
@@ -553,6 +554,32 @@ fn capnp_name_to_rust_name(capnp_name: &str, name_kind: NameKind) -> String {
     }
 }
 
+fn is_option_field(field: schema_capnp::field::Reader) -> capnp::Result<bool> {
+    use capnp::schema_capnp::*;
+
+    let enabled = field
+        .get_annotations()?
+        .iter()
+        .any(|a| a.get_id() == OPTION_ANNOTATION_ID);
+
+    if enabled {
+        let supported = match field.which()? {
+            field::Which::Group(_) => false,
+            field::Which::Slot(field) => {
+                let ty = field.get_type()?;
+                ty.is_pointer()? && !matches!(ty.which()?, type_::Interface(_))
+            }
+        };
+        if !supported {
+            return Err(capnp::Error::failed(
+                "$Rust.option annotation only supported on pointer fields (support for optional interfaces isn't implemented yet)".to_string(),
+            ));
+        }
+    }
+
+    Ok(enabled)
+}
+
 fn prim_default(value: &schema_capnp::value::Reader) -> ::capnp::Result<Option<String>> {
     use capnp::schema_capnp::value;
     match value.which()? {
@@ -665,37 +692,46 @@ pub fn getter_text(
                 offset: usize,
                 default: T,
                 zero: T,
-            ) -> FormattedText {
+            ) -> String {
                 if default == zero {
-                    Line(format!("self.{member}.get_data_field::<{typ}>({offset})"))
+                    format!("self.{member}.get_data_field::<{typ}>({offset})")
                 } else {
-                    Line(format!(
-                        "self.{member}.get_data_field_mask::<{typ}>({offset}, {default})"
-                    ))
+                    format!("self.{member}.get_data_field_mask::<{typ}>({offset}, {default})")
                 }
             }
 
             let raw_type = reg_field.get_type()?;
-            let typ = raw_type.type_string(ctx, module)?;
+            let inner_type = raw_type.type_string(ctx, module)?;
             let default_value = reg_field.get_default_value()?;
             let default = default_value.which()?;
             let default_name = format!(
                 "DEFAULT_{}",
                 snake_to_upper_case(&camel_to_snake_case(get_field_name(*field)?))
             );
+            let should_get_option = is_option_field(*field)?;
 
-            let mut result_type = match raw_type.which()? {
-                type_::Enum(_) => fmt!(ctx, "::core::result::Result<{typ},{capnp}::NotInSchema>"),
-                type_::AnyPointer(_) if !raw_type.is_parameter()? => typ.clone(),
-                type_::Interface(_) => {
+            let typ = if should_get_option {
+                format!("Option<{}>", inner_type)
+            } else {
+                inner_type
+            };
+
+            let (is_fallible, mut result_type) = match raw_type.which()? {
+                type_::Enum(_) => (
+                    true,
+                    fmt!(ctx, "::core::result::Result<{typ},{capnp}::NotInSchema>"),
+                ),
+                type_::AnyPointer(_) if !raw_type.is_parameter()? => (false, typ.clone()),
+                type_::Interface(_) => (
+                    true,
                     fmt!(
                         ctx,
                         "{capnp}::Result<{}>",
                         raw_type.type_string(ctx, Leaf::Client)?
-                    )
-                }
-                _ if raw_type.is_prim()? => typ.clone(),
-                _ => fmt!(ctx, "{capnp}::Result<{typ}>"),
+                    ),
+                ),
+                _ if raw_type.is_prim()? => (false, typ.clone()),
+                _ => (true, fmt!(ctx, "{capnp}::Result<{typ}>")),
             };
 
             if is_fn {
@@ -706,19 +742,19 @@ pub fn getter_text(
                 }
             }
 
-            let getter_code = match (raw_type.which()?, default) {
+            let getter_fragment = match (raw_type.which()?, default) {
                 (type_::Void(()), value::Void(())) => {
                     if is_fn {
-                        Line("".to_string())
+                        "".to_string()
                     } else {
-                        Line("()".to_string())
+                        "()".to_string()
                     }
-                },
+                }
                 (type_::Bool(()), value::Bool(b)) => {
                     if b {
-                        Line(format!("self.{member}.get_bool_field_mask({offset}, true)"))
+                        format!("self.{member}.get_bool_field_mask({offset}, true)")
                     } else {
-                        Line(format!("self.{member}.get_bool_field({offset})"))
+                        format!("self.{member}.get_bool_field({offset})")
                     }
                 }
                 (type_::Int8(()), value::Int8(i)) => primitive_case(&typ, &member, offset, i, 0),
@@ -726,60 +762,94 @@ pub fn getter_text(
                 (type_::Int32(()), value::Int32(i)) => primitive_case(&typ, &member, offset, i, 0),
                 (type_::Int64(()), value::Int64(i)) => primitive_case(&typ, &member, offset, i, 0),
                 (type_::Uint8(()), value::Uint8(i)) => primitive_case(&typ, &member, offset, i, 0),
-                (type_::Uint16(()), value::Uint16(i)) => primitive_case(&typ, &member, offset, i, 0),
-                (type_::Uint32(()), value::Uint32(i)) => primitive_case(&typ, &member, offset, i, 0),
-                (type_::Uint64(()), value::Uint64(i)) => primitive_case(&typ, &member, offset, i, 0),
-                (type_::Float32(()), value::Float32(f)) =>
-                    primitive_case(&typ, &member, offset, f.to_bits(), 0),
-                (type_::Float64(()), value::Float64(f)) =>
-                    primitive_case(&typ, &member, offset, f.to_bits(), 0),
+                (type_::Uint16(()), value::Uint16(i)) => {
+                    primitive_case(&typ, &member, offset, i, 0)
+                }
+                (type_::Uint32(()), value::Uint32(i)) => {
+                    primitive_case(&typ, &member, offset, i, 0)
+                }
+                (type_::Uint64(()), value::Uint64(i)) => {
+                    primitive_case(&typ, &member, offset, i, 0)
+                }
+                (type_::Float32(()), value::Float32(f)) => {
+                    primitive_case(&typ, &member, offset, f.to_bits(), 0)
+                }
+                (type_::Float64(()), value::Float64(f)) => {
+                    primitive_case(&typ, &member, offset, f.to_bits(), 0)
+                }
                 (type_::Enum(_), value::Enum(d)) => {
                     if d == 0 {
-                        Line(format!("::core::convert::TryInto::try_into(self.{member}.get_data_field::<u16>({offset}))"))
+                        format!("::core::convert::TryInto::try_into(self.{member}.get_data_field::<u16>({offset}))")
                     } else {
-                        Line(
-                            format!(
-                                "::core::convert::TryInto::try_into(self.{member}.get_data_field_mask::<u16>({offset}, {d}))"))
+                        format!(
+                                "::core::convert::TryInto::try_into(self.{member}.get_data_field_mask::<u16>({offset}, {d}))")
                     }
                 }
 
-                (type_::Text(()), value::Text(_)) |
-                (type_::Data(()), value::Data(_)) |
-                (type_::List(_), value::List(_)) |
-                (type_::Struct(_), value::Struct(_)) => {
+                (type_::Text(()), value::Text(_))
+                | (type_::Data(()), value::Data(_))
+                | (type_::List(_), value::List(_))
+                | (type_::Struct(_), value::Struct(_)) => {
                     let default = if reg_field.get_had_explicit_default() {
                         default_decl = Some(crate::pointer_constants::word_array_declaration(
                             ctx,
                             &default_name,
                             ::capnp::raw::get_struct_pointer_section(default_value).get(0),
-                            crate::pointer_constants::WordArrayDeclarationOptions {public: true})?);
-                        format!("Some(&_private::{default_name}[..])")
+                            crate::pointer_constants::WordArrayDeclarationOptions { public: true },
+                        )?);
+                        format!("::core::option::Option::Some(&_private::{default_name}[..])")
                     } else {
                         "::core::option::Option::None".to_string()
                     };
 
                     if is_reader {
-                        Line(fmt!(ctx,
-                            "{capnp}::traits::FromPointerReader::get_from_pointer(&self.{member}.get_pointer_field({offset}), {default})"))
+                        fmt!(ctx,
+                            "{capnp}::traits::FromPointerReader::get_from_pointer(&self.{member}.get_pointer_field({offset}), {default})")
                     } else {
-                        Line(fmt!(ctx,"{capnp}::traits::FromPointerBuilder::get_from_pointer(self.{member}.get_pointer_field({offset}), {default})"))
+                        fmt!(ctx,"{capnp}::traits::FromPointerBuilder::get_from_pointer(self.{member}.get_pointer_field({offset}), {default})")
                     }
                 }
 
                 (type_::Interface(_), value::Interface(_)) => {
-                    Line(fmt!(ctx,"match self.{member}.get_pointer_field({offset}).get_capability() {{ ::core::result::Result::Ok(c) => ::core::result::Result::Ok({capnp}::capability::FromClientHook::new(c)), ::core::result::Result::Err(e) => ::core::result::Result::Err(e)}}"))
+                    fmt!(ctx,"match self.{member}.get_pointer_field({offset}).get_capability() {{ ::core::result::Result::Ok(c) => ::core::result::Result::Ok({capnp}::capability::FromClientHook::new(c)), ::core::result::Result::Err(e) => ::core::result::Result::Err(e)}}")
                 }
                 (type_::AnyPointer(_), value::AnyPointer(_)) => {
                     if !raw_type.is_parameter()? {
-                        Line(fmt!(ctx,"{capnp}::any_pointer::{module_string}::new(self.{member}.get_pointer_field({offset}))"))
+                        fmt!(ctx,"{capnp}::any_pointer::{module_string}::new(self.{member}.get_pointer_field({offset}))")
                     } else if is_reader {
-                        Line(fmt!(ctx,"{capnp}::traits::FromPointerReader::get_from_pointer(&self.{member}.get_pointer_field({offset}), ::core::option::Option::None)"))
+                        fmt!(ctx,"{capnp}::traits::FromPointerReader::get_from_pointer(&self.{member}.get_pointer_field({offset}), ::core::option::Option::None)")
                     } else {
-                        Line(fmt!(ctx,"{capnp}::traits::FromPointerBuilder::get_from_pointer(self.{member}.get_pointer_field({offset}), ::core::option::Option::None)"))
+                        fmt!(ctx,"{capnp}::traits::FromPointerBuilder::get_from_pointer(self.{member}.get_pointer_field({offset}), ::core::option::Option::None)")
                     }
                 }
                 _ => return Err(Error::failed("default value was of wrong type".to_string())),
             };
+
+            let getter_code = if should_get_option {
+                Branch(vec![
+                    Line(format!(
+                        "if self.{member}.is_pointer_field_null({offset}) {{"
+                    )),
+                    Indent(Box::new(Line(
+                        if is_fallible {
+                            "core::result::Result::Ok(core::option::Option::None)"
+                        } else {
+                            "::core::option::Option::None"
+                        }
+                        .to_string(),
+                    ))),
+                    Line("} else {".to_string()),
+                    Indent(Box::new(Line(if is_fallible {
+                        format!("{getter_fragment}.map(::core::option::Option::Some)")
+                    } else {
+                        format!("::core::option::Option::Some({getter_fragment})")
+                    }))),
+                    Line("}".to_string()),
+                ])
+            } else {
+                Line(getter_fragment)
+            };
+
             Ok((result_type, getter_code, default_decl))
         }
     }
@@ -2454,7 +2524,7 @@ fn generate_node(
                 )));
 
                 client_impl_interior.push(Indent(Box::new(Line(format!(
-                    "self.client.new_call(_private::TYPE_ID, {ordinal}, None)"
+                    "self.client.new_call(_private::TYPE_ID, {ordinal}, ::core::option::Option::None)"
                 )))));
                 client_impl_interior.push(Line("}".to_string()));
 
diff --git a/capnpc/src/codegen_types.rs b/capnpc/src/codegen_types.rs
index 1d9ca0af3..a8790fa4e 100644
--- a/capnpc/src/codegen_types.rs
+++ b/capnpc/src/codegen_types.rs
@@ -97,6 +97,7 @@ pub trait RustNodeInfo {
 // this is a collection of helpers acting on a "Type" (someplace where a Type is used, not defined)
 pub trait RustTypeInfo {
     fn is_prim(&self) -> Result<bool, Error>;
+    fn is_pointer(&self) -> Result<bool, Error>;
     fn is_parameter(&self) -> Result<bool, Error>;
     fn is_branded(&self) -> Result<bool, Error>;
     fn type_string(&self, ctx: &GeneratorContext, module: Leaf) -> Result<String, Error>;
@@ -330,6 +331,19 @@ impl<'a> RustTypeInfo for type_::Reader<'a> {
             _ => Ok(false),
         }
     }
+
+    #[inline(always)]
+    fn is_pointer(&self) -> Result<bool, Error> {
+        Ok(matches!(
+            self.which()?,
+            type_::Text(())
+                | type_::Data(())
+                | type_::List(_)
+                | type_::Struct(_)
+                | type_::Interface(_)
+                | type_::AnyPointer(_)
+        ))
+    }
 }
 
 ///


Test patch:
diff --git a/capnpc/test/test.capnp b/capnpc/test/test.capnp
index 6c9ec6a11..70513b2cf 100644
--- a/capnpc/test/test.capnp
+++ b/capnpc/test/test.capnp
@@ -446,6 +446,20 @@ struct TestNewUnionVersion {
   }
 }
 
+struct TestFieldGetOption {
+  text @0 :Text $Rust.option;
+  data @1 :Data $Rust.option;
+  list @2 :List(UInt8) $Rust.option;
+  emptyStruct @3 :EmptyStruct $Rust.option;
+  simpleStruct @4 :SimpleStruct $Rust.option;
+  any @5 :AnyPointer $Rust.option;
+
+  struct EmptyStruct {}
+  struct SimpleStruct {
+    field @0 :Text $Rust.option;
+  }
+}
+
 struct TestGenerics(Foo, Bar) {
   foo @0 :Foo;
   bar @1 :Bar;
diff --git a/capnpc/test/test.rs b/capnpc/test/test.rs
index 0e49afd1c..b052a5251 100644
--- a/capnpc/test/test.rs
+++ b/capnpc/test/test.rs
@@ -82,6 +82,22 @@ mod tests {
     use crate::test_util::{init_test_message, CheckTestMessage};
     use capnp::message::ReaderOptions;
     use capnp::message::{self, TypedBuilder, TypedReader};
+    use capnp::{primitive_list, text};
+
+    // like the unstable std::assert_matches::assert_matches but doesn't
+    // require $left implement Debug
+    macro_rules! assert_matches {
+        ($left:expr, $pattern:pat_param) => {
+            match $left {
+                $pattern => {}
+                _ => panic!(
+                    "assertion failed: {:?} does not match {:?}",
+                    stringify!($left),
+                    stringify!($pattern),
+                ),
+            }
+        };
+    }
 
     #[test]
     fn test_prim_list() {
@@ -723,6 +739,89 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_field_get_option() -> capnp::Result<()> {
+        use crate::test_capnp::test_field_get_option as subject;
+
+        let mut message_set = message::Builder::new_default();
+        let mut message_unset = message::Builder::new_default();
+
+        let mut test_set = message_set.init_root::<subject::Builder<'_>>();
+        let mut test_unset = message_unset.init_root::<subject::Builder<'_>>();
+
+        // Check setters
+
+        test_set.set_text("foo");
+        test_set.set_data(&[42]);
+        {
+            let mut b = test_set.reborrow().init_list(3);
+            b.set(0, 1);
+            b.set(1, 2);
+            b.set(2, 3);
+        }
+        test_set.reborrow().init_empty_struct();
+        test_set.reborrow().init_simple_struct().set_field("buzz");
+        {
+            let mut b = test_set.reborrow().init_any();
+            b.set_as("dyn")?;
+        }
+
+        // Check builder getters
+
+        assert_matches!(test_set.reborrow().get_text()?, Some(text::Builder { .. }));
+        assert!(test_unset.reborrow().get_text()?.is_none());
+
+        assert_matches!(test_set.reborrow().get_data()?, Some(&mut [..]));
+        assert!(test_unset.reborrow().get_data()?.is_none());
+
+        assert_matches!(
+            test_set.reborrow().get_list()?,
+            Some(primitive_list::Builder { .. })
+        );
+        assert!(test_unset.reborrow().get_list()?.is_none());
+
+        assert_matches!(
+            test_set.reborrow().get_empty_struct()?,
+            Some(subject::empty_struct::Builder { .. })
+        );
+        assert!(test_unset.reborrow().get_empty_struct()?.is_none());
+
+        assert_matches!(
+            test_set.reborrow().get_simple_struct()?,
+            Some(subject::simple_struct::Builder { .. })
+        );
+        assert!(test_unset.reborrow().get_simple_struct()?.is_none());
+
+        // Check reader getters
+
+        let set_reader = test_set.into_reader();
+        let unset_reader = test_unset.into_reader();
+
+        assert!(unset_reader.get_text()?.is_none());
+        assert_eq!(set_reader.get_text()?, Some("foo"));
+
+        assert!(unset_reader.get_data()?.is_none());
+        assert_eq!(set_reader.get_data()?, Some(&[42][..]));
+
+        assert!(unset_reader.get_list()?.is_none());
+        let r = set_reader.get_list()?.expect("is some");
+        assert_eq!(r.get(0), 1);
+        assert_eq!(r.get(1), 2);
+        assert_eq!(r.get(2), 3);
+
+        assert!(unset_reader.get_empty_struct()?.is_none());
+        assert!(set_reader.get_empty_struct()?.is_some());
+
+        assert!(unset_reader.get_simple_struct()?.is_none());
+        let r = set_reader.get_simple_struct()?.expect("is some");
+        assert_eq!(r.get_field()?, Some("buzz"));
+
+        assert!(unset_reader.get_any().is_none());
+        assert!(set_reader.get_any().is_some());
+
+        Ok(())
+    }
+
     #[test]
     fn test_generic_one_parameter() {
         use crate::test_capnp::brand_once;