Change list get methods to return an Option<T>

[marmeladema]

Hi @dwrensha

I have opened this as a draft because its a work-in-progress and I have a few questions but I would like to start the discussion:

1. I have changed (some) get list reader methods to return an Option<T> and introduced a get_unchecked to mirror std slice API. This is a breaking change but as far as I know, the API is not considered stable at this point? I think that's the right call because its less confusing since it adhere to idiomatic rust practices but if the change is considered to severe, maybe adding a checked_get method instead would be more acceptable? A bonus point from this work is that in places where the index is already known to be in range, we can use get_unchecked directly and avoid some useless bound checks.

2. There are some get methods that already return a Result<T> but usually the index access is done prior to the call that returns the Result. In such case I believe the correct return type would be Option<Result<T>>. Do you agree?

3. What about get builder methods, should they be changed too for coherency?

Fixes #251

[dwrensha]

I suppose we could closely mirror the std API with:

• get() returns an Option<T>
• unsafe get_unchecked() returns a T and does no bounds checking
• index() returns a T (and panics on out-of-bounds)

Existing get() calls would mostly need to migrate to index(). This makes things slightly more verbose, but it's not as bad as requiring .get().unwrap().

I would want to roll this out as a multi-step process, where first we add index() and deprecate the current get(), and then on the next "major" version bump we change the type of get().

This would cause some disruption to downstream users, but maybe it's worth it in the long run?

[dwrensha]

2. There are some get methods that already return a Result<T> but usually the index access is done prior to the call that returns the Result. In such case I believe the correct return type would be Option<Result<T>>. Do you agree?

Makes sense to me.

3. What about get builder methods, should they be changed too for coherency?

Yes, I think exactly the same concerns about out-of-bounds and panicking apply there too.

[dwrensha]

Alternatively, we could keep the current get() methods as-is, and add new methods

• fn try_get(self) -> Option<T>
• unsafe fn get_unchecked(self) -> T

This would have the advantage of not disrupting any current users. Appending a try_ to mean "can return None" is consistent with naming elsewhere in capnproto-rust, e.g.

capnproto-rust/capnp/src/serialize.rs

Lines 266 to 288 in ec84c7a

	/// Reads a serialized message from a stream with the provided options.
	///
	/// For optimal performance, `read` should be a buffered reader type.
	pub fn read_message<R>(mut read: R, options: message::ReaderOptions) -> Result<message::Reader<OwnedSegments>>
	where R: Read {
	let owned_segments_builder = match read_segment_table(&mut read, options)? {
	Some(b) => b,
	None => return Err(Error::failed("Premature end of file".to_string())),
	};
	read_segments(&mut read, owned_segments_builder.into_owned_segments(), options)
	}
	/// Like `read_message()`, but returns None instead of an error if there are zero bytes left in
	/// `read`. This is useful for reading a stream containing an unknown number of messages -- you
	/// call this function until it returns None.
	pub fn try_read_message<R>(mut read: R, options: message::ReaderOptions) -> Result<Option<message::Reader<OwnedSegments>>>
	where R: Read {
	let owned_segments_builder = match read_segment_table(&mut read, options)? {
	Some(b) => b,
	None => return Ok(None),
	};
	Ok(Some(read_segments(&mut read, owned_segments_builder.into_owned_segments(), options)?))
	}

I'm not too worried about get() in capnp meaning something slightly different from get() in std::collections, especially if we document it.

[marmeladema]

@dwrensha what do you prefer? Both approaches seem good to me. My main desire is to have a get-like function that does not panic. I personally think that introducing try_get is probably the easiest. No disruption for users and we can still decide later on if we want to change get itself.

[marmeladema]

Opened #285 for the try_get approach

[dwrensha]

Yeah try_get sounds good to me. We can rename the methods later if that seems like the appropriate thing.

[dwrensha]

Added some documentation: da7a9f7

[dwrensha]

Closing in favor of #285. Please open a new pull request if you want to add get_unchecked() methods.