API improvements for usage of owned Elements

[jpschorr]

This PR includes a couple changes intended to make working with owned Elements easier are not require cloning for 'projecting' an element to be replaced by something it contains.

• Exposes OwnedSequenceIterator and modifies its implementation to clone less
• Adds try_into_<x> methods similar to as_<x> methods on Element
  • These allow 'partial' conversions of owned Elements with the 'failure' case returning the original Element

Example simplified usage:

#[derive(Debug)]
enum ElementIterator {
    Single(Element),
    Sequence(OwnedSequenceIterator),
}

...

match elt.try_into_sequence() {
    Err(elt) => ElementIterator::Single(elt),
    Ok(seq) => ElementIterator::Sequence(seq.into_iter()),
}

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[codecov]

Codecov Report

Attention: Patch coverage is 85.71429% with 37 lines in your changes missing coverage. Please review.

Project coverage is 77.81%. Comparing base (d6cb3b4) to head (fab335e).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/element/mod.rs	89.32%	21 Missing and 1 partial ⚠️
src/result/conversion.rs	42.85%	12 Missing ⚠️
src/result/mod.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #858      +/-   ##
==========================================
+ Coverage   77.55%   77.81%   +0.26%     
==========================================
  Files         135      136       +1     
  Lines       33796    33984     +188     
  Branches    33796    33984     +188     
==========================================
+ Hits        26210    26446     +236     
+ Misses       5645     5598      -47     
+ Partials     1941     1940       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[zslayton]

This looks good, thanks! One question about our error types before we merge this.

[zslayton]

I think I was expecting Conversion and ConversionResult to be the same type, something like:

pub type ConversionResult<FromType, ToType> = Result<ToType, ConversionError<FromType>>;

impl <FromType, ToType> From<ConversionResult<FromType, ToType>> for IonResult<ToType> {
  fn from(conversion: ConversionResult<FromType, ToType>) -> Self {
    match conversion {
      Ok(value) => Ok(value),
      Err(conversion_error) => {
        Err(IonError::Conversion(conversion_err.message())) // some text explanation
      }
    }
  }
}

This way the try_into_* methods returning a ConversionResult would benefit from all of the Result methods (including ok() for Option conversion). You could then also use ? with ConvesionResult in contexts that needed to return an IonResult, which would be nice.

Could you explain the motivation for distinct Conversion/ConversionResult types?

[jpschorr]

ConversionError<FromType> can't be put directly into IonError due to the generic.

pub enum IonError {
   ...
    #[error("{0}")]
    Conversion(#[from] ConversionError<FromType>),
   ...
}

So you end up needing two 'error' structs anyway.

I originally had something like:

/// Represents a mismatch during conversion between the expected type and the actual value's type.
#[derive(Clone, Debug, Error, PartialEq)]
#[error("Type conversion error; expected a(n) {output_type}, found a(n) {input_type}")]
pub struct ConversionError {
    input_value: String,
    output_type: String,
}

/// Represents a mismatch during conversion between the expected type and the actual value's type.
#[derive(Clone, Debug, Error, PartialEq)]
#[error("Type conversion error; expected a(n) {output_type}, found a(n) {input_type}")]
pub struct ConversionOperationError<FromType>
where
    FromType: ValueTypeExpectation,
{
    input_value: FromType,
    output_type: String,
}

So, a pair of Errors and a pair of Results. Except the ConversionOperationError/ConversionOperationResult basically never existed for long and wasn't really used as an error; It was either unwrapped for its success or fail element or converted immediately into a ConversionError/ConversionResult. On top of this, I kept confusing myself as to which was which between Conversion<x> and ConversionOperation<x>.

So, I condensed ConversionOperationError&ConversionOperationResult and into Conversion. However, if you find that more surprising and would prefer the pair of Errors/Results or have other structure or naming suggestions, I'm open to change it.

[jpschorr]

I've refactored to a pair of Errors & Results.

[zslayton]

Why does this need to be boxed? Are you concerned about stack size?

[jpschorr]

Without boxing, clippy warns:

warning: the `Err`-variant returned from this function is very large
   --> src/element/mod.rs:492:34
    |
492 |     pub fn try_into_int(self) -> ConversionOperationResult<Element, Int> {
    |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the `Err`-variant is at least 128 bytes
    |
    = help: try reducing the size of `result::conversion::ConversionOperationError<element::Element, types::integer::Int>`, for example by boxing large elements or replacing it with `Box<result::conversion::ConversionOperationError<element::Element, types::integer::Int>>`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err
    = note: `#[warn(clippy::result_large_err)]` on by default