Makes `IonError` variants wrap opaque types #584

zslayton · 2023-06-30T20:14:19Z

This PR refactors the IonError enum variants to each wrap an opaque struct type. The wrapped types each offer methods and trait implementations that we can expand upon in the future without breaking changes.

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

codecov · 2023-06-30T20:24:32Z

Codecov Report

Patch coverage: 46.34% and project coverage change: -0.13 ⚠️

Comparison is base (779a950) 83.30% compared to head (7c5718a) 83.18%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #584      +/-   ##
==========================================
- Coverage   83.30%   83.18%   -0.13%     
==========================================
  Files         104      110       +6     
  Lines       19705    19764      +59     
  Branches    19705    19764      +59     
==========================================
+ Hits        16416    16441      +25     
- Misses       1739     1765      +26     
- Partials     1550     1558       +8

Impacted Files	Coverage Δ
src/binary/binary_writer.rs	`64.67% <0.00%> (ø)`
src/binary/raw_binary_writer.rs	`85.17% <0.00%> (-0.17%)`	⬇️
src/binary/type_code.rs	`28.12% <0.00%> (-0.91%)`	⬇️
src/element/builders.rs	`96.84% <ø> (ø)`
src/element/element_stream_reader.rs	`71.93% <0.00%> (-0.37%)`	⬇️
src/element/element_stream_writer.rs	`85.47% <0.00%> (-0.32%)`	⬇️
src/element/writer.rs	`73.33% <ø> (+1.59%)`	⬆️
src/lazy/binary/lazy_reader.rs	`66.66% <0.00%> (ø)`
src/lazy/binary/raw/lazy_raw_reader.rs	`76.40% <0.00%> (ø)`
src/lazy/binary/system/lazy_sequence.rs	`74.54% <ø> (ø)`
... and 47 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

zslayton

🗺️ PR tour

zslayton · 2023-07-03T13:48:27Z

src/binary/decimal.rs

@@ -10,10 +10,9 @@ use crate::binary::raw_binary_writer::MAX_INLINE_LENGTH;
 use crate::binary::var_int::VarInt;
 use crate::binary::var_uint::VarUInt;
 use crate::ion_data::IonEq;
-use crate::result::IonResult;
+use crate::result::{encoding_error_raw, IonResult};


🗺️ I have a TODO to limit the visibility of types to a single prescribed path within the crate. For the moment, IonResult can be found in both the crate root (crate::IonResult) and in result (crate::result::IonResult). This is true of other types as well.

zslayton · 2023-07-03T13:59:38Z

src/data_source.rs

            // TODO: IonResult should have a distinct `IncompleteData` error case
            //       https://github.com/amazon-ion/ion-rust/issues/299
            {
                decoding_error("Unexpected end of stream.")
            }
-            Err(io_error) => Err(IonError::IoError { source: io_error }),
+            Err(io_error) => Err(io_error.into()),


🗺️ The new opaque struct error types (defined further down) can implement traits like Into, making for nicer code.

zslayton · 2023-07-03T14:01:45Z

src/data_source.rs

@@ -261,9 +261,8 @@ mod tests {
    }
 }

-/// Types that implement this trait can be converted into an implementation of [io::BufRead],
-/// allowing users to build a [Reader](crate::reader::Reader) from a variety of types that might not


🗺️ This doc comment referred to Reader, which is feature gated. If you attempted to build the docs without "experimental-reader" enabled, it would fail because Reader was not public.

zslayton · 2023-07-03T14:03:24Z

src/element/mod.rs

-    /// maximize encoding efficiency. To reuse a writer and have greater control over resource
-    /// management, see [`Element::write_to`].


🗺️ This doc comment referred to Element::write_to, which is feature gated. If you attempted to build the docs without "experimental-writer" enabled, it would fail because write_to was not available.

This change hides that sentence when write_to is not available. The content of the comment is otherwise unchanged.

zslayton · 2023-07-03T14:04:03Z

src/element/mod.rs

@@ -663,66 +670,82 @@ impl Element {
        }
    }

-    /// Serializes this [`Element`] as binary Ion, returning the output as a `Vec<u8>`.
-    ///
-    /// This is a convenience method; it is less efficient than [`Element::write_to`] because:


🗺️ This doc comment referred to write_to, which is feature gated. If you attempted to build the docs without "experimental-writer" enabled, it would fail because write_to was not available.

This change hides that section when write_to is not available. The content of the comment is otherwise unchanged.

zslayton · 2023-07-03T16:53:16Z

src/text/non_blocking/raw_text_reader.rs

-            Err(IonError::Incomplete {
-                position:
-                    Position {
-                        line_column: Some((line, column)),
-                        ..
-                    },
-                ..
-            }) => {
-                assert_eq!(line, 2);
-                assert_eq!(column, 0);
+            Err(IonError::Incomplete(e)) => {
+                assert_eq!(e.position().line(), Some(2));
+                assert_eq!(e.position().column(), Some(0));


🗺️ This is a good example of the updated application code for inspecting an IonError that has cropped up.

zslayton · 2023-07-03T16:54:41Z

src/types/decimal.rs

@@ -371,16 +376,17 @@ impl TryFrom<f64> for Decimal {

        fn try_convert(coefficient: f64, exponent: i64) -> Result<Decimal, IonError> {
            // prefer a compact representation for the coefficient; fallback to bigint
-            let coefficient: Coefficient = if !coefficient.trunc().is_zero()


🗺️ This section is a formatting-only change.

zslayton · 2023-07-03T16:55:01Z

src/types/timestamp.rs

        })?;
        if self.precision == Precision::Month {
            return Ok(datetime);
        }

        // If precision >= Day, the day must be set.
        let day = self.day.expect("missing day");
-        datetime = datetime


🗺️ This section is a formatting-only change.

zslayton · 2023-07-03T16:55:34Z

tests/ion_hash_tests.rs

@@ -119,7 +120,7 @@ fn ion_hash_tests() -> IonHashTestResult<()> {
 }

 fn test_file(file_name: &str) -> IonHashTestResult<()> {
-    let data = read(file_name).map_err(|source| ion_rs::IonError::IoError { source })?;
+    let data = read(file_name).map_err(IonError::from)?;


🗺️ Another example of trait impls making the code a bit nicer.

zslayton · 2023-07-03T16:57:55Z

tests/ion_hash_tests.rs

@@ -224,7 +225,7 @@ fn expected_hash(struct_: &Struct) -> IonResult<Vec<u8>> {
    let identity = if let Some(identity) = struct_.get("identity") {
        identity.as_sequence().expect("`identity` should be a sexp")
    } else {
-        illegal_operation("only identity tests are implemented")?
+        todo!("only identity tests are implemented")


🗺️ ion-hash-tests was constructing its own IonResults to describe failure modes that didn't come from Ion itself. The error constructor methods are now limited to crate-visibility. Clients like ion-hash-tests should use their own error type.

In ion-hash-tests' case, it was bubbling up the error result and panicking later on. I've changed those failures to an immediate panic. The end result is the same, and it removes the dependency on now-private functionality.

zslayton · 2023-07-03T17:00:40Z

src/result/decoding_error.rs

🗺️ We still need to add doc comments for these new types. I have opened #589 to track that.

jobarr-amzn · 2023-07-03T17:05:29Z

src/binary/binary_writer.rs

-                    return illegal_operation(format!(
+                    return IonResult::illegal_operation(format!(


Good change!

jobarr-amzn · 2023-07-03T18:01:42Z

src/element/element_stream_writer.rs

+                    return IonResult::illegal_operation(
+                        "Values inside a struct must have a field name.",
+                    );


Generally speaking an error message without context is less helpful than one that does have context. It might be helpful to provide some indication here of which value does not have a field name. I'm not suggesting that we fix it in this PR, but what sort of additional book-keeping or capabilities might we need to provide context in cases like this?

I created #590 to track/discuss.

Perhaps it would be good to link to #590 in places like this, so that it's easier to come back to later?

On the other hand, if we had a more universal context description ability then we'd likely do some general pass when the capability was added.

Agreed on all counts. (This particular case is a bit of an outlier--it shouldn't happen since it's serializing a fully-formed Element. Not setting a field name would be user error when driving an IonWriter.)

jobarr-amzn · 2023-07-03T18:24:29Z

src/result/mod.rs

+// Crate-visible convenience methods for constructing error variants and wrapping them in the
+// appropriate type: IonResult<T> or IonError. This is a trait so these methods can be added to
+// `IonResult<T>`, which is just a type alias for `Result<T, IonError>`, whose implementation
+// does not live in this crate.
+pub(crate) trait IonFailure {
+    fn incomplete(label: &'static str, position: impl Into<Position>) -> Self;
+    fn decoding_error<S: Into<String>>(description: S) -> Self;
+    fn encoding_error<S: Into<String>>(description: S) -> Self;
+    fn illegal_operation<S: Into<String>>(operation: S) -> Self;
+}


It does not include an io method because the only way we ever construct IonError::Io is by converting a std::io::IoError with the ? operator.

Could this stand to be a comment?

zslayton added 9 commits June 30, 2023 14:14

Made IonError::IoError wrap an opaque struct

6ebf589

Made result a directory

36bd620

Moved IoError to its own file

aca6db1

Moved Position to its own file

d20db8f

Refactored Incomplete and Position

573900f

Made EncodingError an opaque type

7c210cc

Made DecodingError an opaque type

80936d1

Made IllegalOperation an opaque type

dcb4cf4

clippy fix

778664a

zslayton added 7 commits June 30, 2023 16:26

Temporarily allow broken doc links

ff77016

Fixes doc links for builds without experimental-writer.

7c669b4

Removed erroneous file.

2397a1f

Switches doc comment to usual format.

50bee50

IonError now derives PartialEq; no custom impl

c264966

Removes Error from IonError variant names.

99cebec

Adds IonFailure trait

8b84a21

zslayton mentioned this pull request Jul 3, 2023

Add doc comments to the error types #589

Open

Removed unused function.

2fa18ed

zslayton commented Jul 3, 2023

View reviewed changes

zslayton marked this pull request as ready for review July 3, 2023 17:00

zslayton requested review from nirosys, desaikd and popematt July 3, 2023 17:01

jobarr-amzn approved these changes Jul 3, 2023

View reviewed changes

Added comment explaning why IonFailure::io is missing

7c5718a

desaikd approved these changes Jul 3, 2023

View reviewed changes

zslayton merged commit c0fd7fd into main Jul 3, 2023
20 of 21 checks passed

zslayton deleted the opaque-errors branch July 3, 2023 18:51

zslayton mentioned this pull request Jul 3, 2023

Add an IonError::Application variant #583

Closed

popematt mentioned this pull request Jul 3, 2023

Clean up IonSchemaError and IonSchemaResult constructor functions amazon-ion/ion-schema-rust#196

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makes `IonError` variants wrap opaque types #584

Makes `IonError` variants wrap opaque types #584

zslayton commented Jun 30, 2023

codecov bot commented Jun 30, 2023 •

edited

Loading

zslayton left a comment

zslayton Jul 3, 2023

zslayton Jul 3, 2023

zslayton Jul 3, 2023

zslayton Jul 3, 2023

zslayton Jul 3, 2023

zslayton Jul 3, 2023

zslayton Jul 3, 2023

zslayton Jul 3, 2023

zslayton Jul 3, 2023

zslayton Jul 3, 2023

zslayton Jul 3, 2023

jobarr-amzn Jul 3, 2023

jobarr-amzn Jul 3, 2023

zslayton Jul 3, 2023

jobarr-amzn Jul 3, 2023

		/// maximize encoding efficiency. To reuse a writer and have greater control over resource
		/// management, see [`Element::write_to`].

		return illegal_operation(format!(
		return IonResult::illegal_operation(format!(

Makes IonError variants wrap opaque types #584

Makes IonError variants wrap opaque types #584

Conversation

zslayton commented Jun 30, 2023

codecov bot commented Jun 30, 2023 • edited Loading

Codecov Report

zslayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Makes `IonError` variants wrap opaque types #584

Makes `IonError` variants wrap opaque types #584

codecov bot commented Jun 30, 2023 •

edited

Loading