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

Issue:
Non-panicking `capnp::struct_list::Reader::get`
Hello!

Currently `capnp::struct_list::Reader::get` can panic if the index is out of bounds because of an assertion at https://docs.capnproto-rust.org/src/capnp/struct_list.rs.html#84

This is rather unexpected as the documentation doesn't say so and the usual convention for `get` method of collections is to return an `Option<T>` for in invalid index/key/etc.

Would it make sense to (1) change the prototype of `capnp::struct_list::Reader::get` to return an option and maybe (2) implement the `Index` trait with the old panicking behavior as done for slices in std?

I am willing to provide the PR, I am just unsure of what non backward compatible change are deemed acceptable.



Discussion / hints:
We can't use `std::ops::Index`, because its `index()` method returns a reference, which we generally can't do because we may need to swap bytes of the value to account for endianness (in the case of `primitive_list`).

We have an `IndexMove` trait for this purpose: https://github.com/capnproto/capnproto-rust/blob/3f31a36058d45e3812147c6de62d451b7bfd7973/capnp/src/traits.rs#L106-L108

There have been proposals for adding something like it upstream: https://github.com/rust-lang/rfcs/issues/997



> We can't use `std::ops::Index`, because its `index()` method returns a reference, which we generally can't do because we may need to swap bytes of the value to account for endianness (in the case of `primitive_list`).

Right, that's unfortunate. That being said, I am more interested in having a non-panicking version of get methods.

Fix patch:
diff --git a/capnp/src/any_pointer_list.rs b/capnp/src/any_pointer_list.rs
index 62eb574ce..0009ef53d 100644
--- a/capnp/src/any_pointer_list.rs
+++ b/capnp/src/any_pointer_list.rs
@@ -56,6 +56,14 @@ impl <'a> Reader<'a> {
         assert!(index <  self.len());
         crate::any_pointer::Reader::new(self.reader.get_pointer_element(index))
     }
+
+    pub fn try_get(self, index : u32) -> Option<crate::any_pointer::Reader<'a>> {
+        if index <  self.len() {
+            Some(crate::any_pointer::Reader::new(self.reader.get_pointer_element(index)))
+        } else {
+            None
+        }
+    }
 }
 
 impl <'a> IndexMove<u32, Result<crate::any_pointer::Reader<'a>>> for Reader<'a>{
@@ -92,6 +100,14 @@ impl <'a> Builder<'a> {
         crate::any_pointer::Builder::new(self.builder.get_pointer_element(index))
     }
 
+    pub fn try_get(self, index : u32) -> Option<crate::any_pointer::Builder<'a>> {
+        if index <  self.len() {
+            Some(crate::any_pointer::Builder::new(self.builder.get_pointer_element(index)))
+        } else {
+            None
+        }
+    }
+
     pub fn reborrow<'b>(&'b mut self) -> Builder<'b> {
         Builder {builder: self.builder.reborrow()}
     }
diff --git a/capnp/src/capability_list.rs b/capnp/src/capability_list.rs
index ae1ba6b4a..9ed05bbd9 100644
--- a/capnp/src/capability_list.rs
+++ b/capnp/src/capability_list.rs
@@ -78,6 +78,14 @@ impl <'a, T> Reader<'a, T> where T: FromClientHook {
         assert!(index < self.len());
         Ok(FromClientHook::new(self.reader.get_pointer_element(index).get_capability()?))
     }
+
+    pub fn try_get(self, index: u32) -> Option<Result<T>> {
+        if index < self.len() {
+            Some(self.reader.get_pointer_element(index).get_capability().map(FromClientHook::new))
+        } else {
+            None
+        }
+    }
 }
 
 impl <'a, T>  IndexMove<u32, Result<T>> for Reader<'a, T> where T: FromClientHook {
@@ -133,6 +141,14 @@ impl <'a, T> Builder<'a, T> where T: FromClientHook {
         assert!(index < self.len());
         Ok(FromClientHook::new(self.builder.get_pointer_element(index).get_capability()?))
     }
+
+    pub fn try_get(self, index: u32) -> Option<Result<T>> {
+        if index < self.len() {
+            Some(self.builder.get_pointer_element(index).get_capability().map(FromClientHook::new))
+        } else {
+            None
+        }
+    }
 }
 
 impl <'a, T> crate::traits::SetPointerBuilder for Reader<'a, T>
diff --git a/capnp/src/data_list.rs b/capnp/src/data_list.rs
index 406a90d4e..fef8a8958 100644
--- a/capnp/src/data_list.rs
+++ b/capnp/src/data_list.rs
@@ -68,6 +68,14 @@ impl <'a> Reader<'a> {
         assert!(index <  self.len());
         self.reader.get_pointer_element(index).get_data(None)
     }
+
+    pub fn try_get(self, index : u32) -> Option<Result<crate::data::Reader<'a>>> {
+        if index <  self.len() {
+            Some(self.reader.get_pointer_element(index).get_data(None))
+        } else {
+            None
+        }
+    }
 }
 
 impl <'a> crate::traits::IntoInternalListReader<'a> for Reader<'a> {
@@ -121,6 +129,14 @@ impl <'a> Builder<'a> {
         assert!(index < self.len());
         self.builder.get_pointer_element(index).get_data(None)
     }
+
+    pub fn try_get(self, index: u32) -> Option<Result<crate::data::Builder<'a>>> {
+        if index < self.len() {
+            Some(self.builder.get_pointer_element(index).get_data(None))
+        } else {
+            None
+        }
+    }
 }
 
 
diff --git a/capnp/src/enum_list.rs b/capnp/src/enum_list.rs
index a144a4327..29f736f6e 100644
--- a/capnp/src/enum_list.rs
+++ b/capnp/src/enum_list.rs
@@ -73,6 +73,15 @@ impl <'a, T : FromU16> Reader<'a, T> {
         let result: u16 = PrimitiveElement::get(&self.reader, index);
         FromU16::from_u16(result)
     }
+
+    pub fn try_get(&self, index: u32) -> Option<::core::result::Result<T, NotInSchema>> {
+        if index < self.len() {
+            let result: u16 = PrimitiveElement::get(&self.reader, index);
+            Some(FromU16::from_u16(result))
+        } else {
+            None
+        }
+    }
 }
 
 impl <'a, T> crate::traits::IntoInternalListReader<'a> for Reader<'a, T> where T: PrimitiveElement {
@@ -117,6 +126,15 @@ impl <'a, T : ToU16 + FromU16>  Builder<'a, T> {
         FromU16::from_u16(result)
     }
 
+    pub fn try_get(&self, index: u32) -> Option<::core::result::Result<T, NotInSchema>> {
+        if index < self.len() {
+            let result: u16 = PrimitiveElement::get_from_builder(&self.builder, index);
+            Some(FromU16::from_u16(result))
+        } else {
+            None
+        }
+    }
+
     pub fn reborrow<'b>(&'b self) -> Builder<'b, T> {
         Builder { .. *self }
     }
diff --git a/capnp/src/list_list.rs b/capnp/src/list_list.rs
index e0de98957..5ff187f33 100644
--- a/capnp/src/list_list.rs
+++ b/capnp/src/list_list.rs
@@ -74,6 +74,14 @@ impl <'a, T> Reader<'a, T> where T: for<'b> crate::traits::Owned<'b> {
         assert!(index <  self.len());
         FromPointerReader::get_from_pointer(&self.reader.get_pointer_element(index), None)
     }
+
+    pub fn try_get(self, index: u32) -> Option<Result<<T as crate::traits::Owned<'a>>::Reader>> {
+        if index <  self.len() {
+            Some(FromPointerReader::get_from_pointer(&self.reader.get_pointer_element(index), None))
+        } else {
+            None
+        }
+    }
 }
 
 impl <'a, T> crate::traits::IntoInternalListReader<'a> for Reader<'a, T> where T: for<'b> crate::traits::Owned<'b> {
@@ -128,6 +136,14 @@ impl <'a, T> Builder<'a, T> where T: for<'b> crate::traits::Owned<'b> {
         FromPointerBuilder::get_from_pointer(self.builder.get_pointer_element(index), None)
     }
 
+    pub fn try_get(self, index: u32) -> Option<Result<<T as crate::traits::Owned<'a>>::Builder>> {
+        if index < self.len() {
+            Some(FromPointerBuilder::get_from_pointer(self.builder.get_pointer_element(index), None))
+        } else {
+            None
+        }
+    }
+
     pub fn set<'b>(&self, index: u32, value: <T as crate::traits::Owned<'b>>::Reader) -> Result<()>
         where <T as crate::traits::Owned<'b>>::Reader: crate::traits::IntoInternalListReader<'b>
     {
diff --git a/capnp/src/primitive_list.rs b/capnp/src/primitive_list.rs
index 1a128d8e8..8f19d4307 100644
--- a/capnp/src/primitive_list.rs
+++ b/capnp/src/primitive_list.rs
@@ -72,6 +72,14 @@ impl <'a, T: PrimitiveElement> Reader<'a, T> {
         PrimitiveElement::get(&self.reader, index)
     }
 
+    pub fn try_get(&self, index: u32) -> Option<T> {
+        if index < self.len() {
+            Some(PrimitiveElement::get(&self.reader, index))
+        } else {
+            None
+        }
+    }
+
     #[cfg(all(target_endian = "little"))]
     /// Returns something if the slice is as expected in memory.
     pub fn as_slice(&self) -> Option<&[T]> {
@@ -143,6 +151,14 @@ impl <'a, T : PrimitiveElement> Builder<'a, T> {
         PrimitiveElement::get_from_builder(&self.builder, index)
     }
 
+    pub fn try_get(&self, index: u32) -> Option<T> {
+        if index < self.len() {
+            Some(PrimitiveElement::get_from_builder(&self.builder, index))
+        } else {
+            None
+        }
+    }
+
     pub fn reborrow<'b>(&'b self) -> Builder<'b, T> {
         Builder { .. *self }
     }
diff --git a/capnp/src/struct_list.rs b/capnp/src/struct_list.rs
index 464ab2dca..bde768562 100644
--- a/capnp/src/struct_list.rs
+++ b/capnp/src/struct_list.rs
@@ -84,6 +84,14 @@ impl <'a, T> Reader<'a, T> where T: for<'b> crate::traits::OwnedStruct<'b> {
         assert!(index < self.len());
         FromStructReader::new(self.reader.get_struct_element(index))
     }
+
+    pub fn try_get(self, index: u32) -> Option<<T as crate::traits::OwnedStruct<'a>>::Reader> {
+        if index < self.len() {
+            Some(FromStructReader::new(self.reader.get_struct_element(index)))
+        } else {
+            None
+        }
+    }
 }
 
 impl <'a, T> crate::traits::IntoInternalListReader<'a> for Reader<'a, T> where T: for<'b> crate::traits::OwnedStruct<'b> {
@@ -151,6 +159,14 @@ impl <'a, T> Builder<'a, T> where T: for<'b> crate::traits::OwnedStruct<'b> {
         assert!(index < self.len());
         FromStructBuilder::new(self.builder.get_struct_element(index))
     }
+
+    pub fn try_get(self, index: u32) -> Option<<T as crate::traits::OwnedStruct<'a>>::Builder> {
+        if index < self.len() {
+            Some(FromStructBuilder::new(self.builder.get_struct_element(index)))
+        } else {
+            None
+        }
+    }
 }
 
 impl <'a, T> crate::traits::SetPointerBuilder for Reader<'a, T>
diff --git a/capnp/src/text_list.rs b/capnp/src/text_list.rs
index e3e10d980..719472bb4 100644
--- a/capnp/src/text_list.rs
+++ b/capnp/src/text_list.rs
@@ -68,6 +68,14 @@ impl <'a> Reader<'a> {
         assert!(index <  self.len());
         self.reader.get_pointer_element(index).get_text(None)
     }
+
+    pub fn try_get(self, index : u32) -> Option<Result<crate::text::Reader<'a>>> {
+        if index <  self.len() {
+            Some(self.reader.get_pointer_element(index).get_text(None))
+        } else {
+            None
+        }
+    }
 }
 
 impl <'a> crate::traits::IntoInternalListReader<'a> for Reader<'a> {
@@ -117,8 +125,17 @@ impl <'a> FromPointerBuilder<'a> for Builder<'a> {
 
 impl <'a> Builder<'a> {
     pub fn get(self, index: u32) -> Result<crate::text::Builder<'a>> {
+        assert!(index <  self.len());
         self.builder.get_pointer_element(index).get_text(None)
     }
+
+    pub fn try_get(self, index: u32) -> Option<Result<crate::text::Builder<'a>>> {
+        if index < self.len() {
+            Some(self.builder.get_pointer_element(index).get_text(None))
+        } else {
+            None
+        }
+    }
 }
 
 impl <'a> crate::traits::SetPointerBuilder for Reader<'a> {


Test patch: