Overhaul error reporting (enum version) #1144

jswrenn · 2024-04-26T13:27:29Z

This is an implementation of the ideas presented in #1139.

This PR:

introduces the public error module (whose items are also re-exported)
revises zerocopy's APIs to return Results with informative error types, rather than Options

Fundamental Error Types

Generally speaking, zerocopy’s conversions may fail for one of up to three reasons, each reflected in a fundamental error type:

AlignmentError<Src, Dst>: the conversion source was improperly aligned
SizeError<Src, Dst>: the conversion source was of incorrect size
ValidityError<Src, Dst>: the conversion source contained invalid data

These fundamental error types are parameterized with the failed conversion's:

Src: the input value of the conversion, stored within the error; and
Dst: the output type of the conversion, retained as phantom type for error reporting purposes.

Methods that only have one failure mode return that mode's corresponding error type directly. For example, Ref::new_unaligned's only failure mode is a size mismatch. This PR revises its return type from Option<Ref<B, T>> to Result<Ref<B, T>, SizeError<B, T>>.

The input value of the conversion is retrievable from each of these fundamental types with an into_src method.

Compound Error Types

Conversion methods that have either two or three possible failure modes return one of these error types:

CastError<Src, Dst>: the error type of ref-conversions.
TryCastError<Src, Dst>: the error type of fallible ref-conversions.
TryReadError<Src, Dst>: the error type of fallible read-conversions.

The input value of the conversion is retrievable from each of these compound types with an into_src method.

These three types are parameterized aliases for the enum ConvertError<A, S, V>:

pub enum ConvertError<A, S, V> {
    Alignment(A),
    Size(S),
    Validity(V),
}

(This enum is never used directly in zerocopy's public API, and is only referred to via these three parameterized aliases.)

These aliases parameterize their reachable branches of ConvertError with the appropriate fundamental error type, and their unreachable branches with Infallible.

Shared Interfaces

Both the compound and fundamental error types provide a shared set of affordances; they:

provide access to the underlying Src
provide implementations of Debug, PartialEq and Eq
provide identical implementations of Display

User Experience Examples

Accessing the source and treating the error as opaque:

let packet = match Packet::ref_from(source) {
    Ok((packet, suffix)) => {
        source = suffix;
        packet
    },
    Err(e) => {
        tracing::error!("parse error: {}", e);
        source = e.into_src();
    }
};

Accessing the source and reflecting on the error:

let packet = match Packet::try_mut_from_prefix(source) {
    Ok((packet, suffix)) => {
        source = suffix;
        packet
    },
    Err(TryCastError::Alignment(e)) => {
        panic!("bug: {}", e)
    },
    Err(TryCastError::Size(e)) => {
        tracing::trace!("waiting for additional bytes: {}", e);
        source = e.into_src();
        continue; // wait for more bytes
    },
    Err(TryCastError::Validity(e)) => {
        return MyError::InvalidPacket(e);
    },
};

Future Outlook

The stabilization of min_exhaustive_patterns (rust-lang/rust#119612) will allow omitting infallible variants from matches.

Comparison to #1150

Surface Area

This PR introduces seven public types to support error reporting. By contrast, #1150 introduces eleven.

API Complexity

In comparison to #1150, this PR slightly pessimizes access to the underlying byte slice: it provides by-value access via a into_src() method rather than via .src. By contrast, this PR comparatively optimizes access to failure mode: it can be determined by matching directly on the returned error, as opposed to first accessing the failure mode via .err.

Conceptual Complexity

With the possible exception of ConvertError, this PR's types all implement a common, consistent interface: they all provide access to the kind of failure that occurred, access to underlying byte slice, and consistently rich error messages.

By contrast, the capabilities provided by #1150's types vary depending on the type. Access to the source value is provided by Error, AlignmentError, SizeError, ValidityError, CastError, TryCastError and TryReadError, but not by ConvertError, Alignment, Size, or Validity.

The fidelity of the Display implementations vary by type: Error, AlignmentError, SizeError, ValidityError, CastError, TryCastError and TryReadError provide high-fidelity error messages, while ConvertError, Alignment, Size, or Validity provide low fidelity error messages.

Future Work

The return type of try_transmute! (which has not yet been implemented) should be Result<Src, ValidityError<Src, Dst>>.

The type ConvertError is not used directly in any public APIs and is defined in a private module to remove it and its variants (which are also marked #[doc(hidden)] from our SemVer obligations. Before 0.8, we should strive to make these items pub so that we can demonstrate matching on error kind in our documentation and release notes.

Makes progress towards #1139.

src/lib.rs

joshlf · 2024-04-29T17:40:08Z

src/error.rs

+    #[inline]
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str("the conversion failed because the address of the source (")?;
+        f.write_fmt(format_args!("{:p}", self.src.deref()))?;


Leaking the pointer might be considered security sensitive by some users. It can reveal aspects of the program's execution and help with memory corruption exploits (e.g. to help an attacker know where their attack payload will land in memory). We might want to omit this.

jswrenn · 2024-05-02T14:19:11Z

@joshlf, let's take another look at this and #1150 soon. I've added some examples to this PR's description illustrating what it feels like to use this API, and some discussion of why I think it compares favorably to #1150 in both API and cognitive complexity.

jswrenn · 2024-05-02T21:24:02Z

src/error.rs

+mod private {
+    #[cfg(doc)]
+    use super::*;
+
+    /// Zerocopy's generic error type.
+    ///
+    /// Generally speaking, zerocopy's conversions may fail for one of up to three reasons:
+    /// - [`AlignmentError`]: the conversion source was improperly aligned
+    /// - [`SizeError`]: the conversion source was of incorrect size
+    /// - [`ValidityError`]: the conversion source contained invalid data
+    ///
+    /// However, not all conversions produce all errors. For instance,
+    /// [`FromBytes::ref_from`] may fail due to alignment or size issues, but not
+    /// validity issues. This generic error type captures these (im)possibilities
+    /// via parameterization: `A` is parameterized with [`AlignmentError`], `S` is
+    /// parameterized with [`SizeError`], and `V` is parameterized with
+    /// [`Infallible`].
+    ///
+    /// Zerocopy never uses this type directly in its API. Rather, we provide three
+    /// pre-parameterized aliases:
+    /// - [`CastError`]: the error type of ref-conversions.
+    /// - [`TryCastError`]: the error type of fallible ref-conversions.
+    /// - [`TryReadError`]: the error type of fallible read-conversions.
+    #[derive(PartialEq, Eq)]
+    pub enum ConvertError<A, S, V> {
+        /// The conversion source was improperly aligned.
+        #[doc(hidden)]
+        Alignment(A),
+        /// The conversion source was of incorrect size.
+        #[doc(hidden)]
+        Size(S),
+        /// The conversion source contained invalid data.
+        #[doc(hidden)]
+        Validity(V),
+    }
+}


@joshlf I've gone ahead and encapsulated the ConvertError in a private module, so the public API consists solely of:

AlignmentError<Src, Dst>: the conversion source was improperly aligned

SizeError<Src, Dst>: the conversion source was of incorrect size

ValidityError<Src, Dst>: the conversion source contained invalid data

CastError<Src, Dst>: the compound error type of ref-conversions

TryCastError<Src, Dst>: the compound error type of fallible ref-conversions

TryReadError<Src, Dst>: the compound error type of fallible read-conversions

I could go further and put it in a private struct, too, but that doesn't buy us anything as far as SemVer is concerned — it just makes all of our matchs in this PR more verbose.

joshlf · 2024-05-03T09:54:31Z

src/error.rs

+//! - [`CastError`]: the error type of ref-conversions.
+//! - [`TryCastError`]: the error type of fallible ref-conversions.
+//! - [`TryReadError`]: the error type of fallible read-conversions.


Suggested change

//! - [`CastError`]: the error type of ref-conversions.

//! - [`TryCastError`]: the error type of fallible ref-conversions.

//! - [`TryReadError`]: the error type of fallible read-conversions.

//! - [`CastError`]: the error type of reference conversions

//! - [`TryCastError`]: the error type of fallible reference conversions

//! - [`TryReadError`]: the error type of fallible read conversions

Just a stylistic preference; take it or leave it. IIUC, you're using "ref" to be consistent w/ method names, but IMO that makes sense specifically because method names are often abbreviated for code style reasons, which is a justification that doesn't apply for prose. Note, for example, that our FromBytes method doc comments use the noun "reference" even for methods with ref in the name. (Btw if you're going to accept this suggestion, the same applies elsewhere in this PR.)

(Removed periods to be consistent with the preceding bulleted list, and since these aren't full sentences)

joshlf · 2024-05-03T09:55:34Z

src/error.rs

+#[cfg(doc)]
+use crate::{FromBytes, Ref, TryFromBytes};
+
+mod private {


Comment explaining why this is private and linking to the relevant tracking issue?

joshlf · 2024-05-03T09:55:49Z

src/error.rs

+    /// - [`CastError`]: the error type of ref-conversions.
+    /// - [`TryCastError`]: the error type of fallible ref-conversions.
+    /// - [`TryReadError`]: the error type of fallible read-conversions.


Same here as above

joshlf · 2024-05-03T10:01:49Z

src/error.rs

Can you add tests for the Display impls (mostly so we have a place where we can write out how they're formatted - especially so we can see a diff if we change the format in the future)?

joshlf · 2024-05-03T10:02:40Z

src/lib.rs

@@ -281,6 +281,11 @@ mod macros;

 pub mod byteorder;
 mod deprecated;
+// Thid module is `pub` so that zerocopy's error types and error handling


Suggested change

// Thid module is `pub` so that zerocopy's error types and error handling

// This module is `pub` so that zerocopy's error types and error handling

joshlf · 2024-05-03T10:05:06Z

src/pointer/ptr.rs

+        I: Invariants<Validity = Initialized>,
+        T: Immutable,
+    {
+        /// Casts this pointer-to-initialized into a pointer-to-bytes.


Safety precondition?

joshlf · 2024-05-03T10:10:31Z

src/lib.rs

+                        .map_src(|src| {
+                            // SAFETY: The provided `size`, below, is computed from
+                            // `bytes.len()`. By contract on `try_into_valid` we can
+                            // count on `src` being equal to `candidate`. Since
+                            // `candidate` was derived from the entire extent of
+                            // `bytes`, the size of `bytes` is exactly equal to the
+                            // size of `src`.
+                            let bytes = unsafe { src.as_bytes_with_size(size) };
+                            bytes.as_mut()
+                        })


It feels sketchy to me that we have to make this implementation so much messier (esp with introducing new unsafe) than it was before just to support this change to our API. I wonder if there's something we can do to refactor this so it stays reasonable?

Perhaps; I'll poke at this a bit more. This is all done to work around our as_bytes method having a Sized bound. We'll be able to remove that bound once std::mem::size_of_val_raw is stabilized, but until then, the workaround is to stash the size while we have it. I don't think it's likely we'll be able to avoid the new unsafe here.

joshlf · 2024-05-03T10:15:25Z

src/pointer/ptr.rs

+        /// Unsafe code may rely on this method's returned `ValidityError`
+        /// containing `self`.


Suggested change

/// Unsafe code may rely on this method's returned `ValidityError`

/// containing `self`.

/// On error, unsafe code may rely on this method's returned `ValidityError`

/// containing `self`.

joshlf · 2024-05-03T10:16:00Z

src/pointer/ptr.rs

@@ -1081,7 +1105,7 @@ mod _casts {
    /// alignment of `[u8]` is 1.
    impl<'a, I> Ptr<'a, [u8], I>
    where
-        I: Invariants<Validity = Valid>,
+        I: Invariants<Alignment = Aligned, Validity = Valid>,


Removed. This is a holdover from some earlier experimentation.

This PR overhauls error reporting, replacing our `Option` returns with `Result`s. The `Err` type of these results provide both the cause of the failure, and access to the underlying source of the conversion. Makes progress towards #1139.

joshlf · 2024-05-03T18:40:52Z

src/lib.rs

+        match Ref::new(bytes) {
+            Ok(dst) => Ok(dst),
+            Err(CastError::Size(e)) => Err(e),
+            Err(CastError::Alignment(_)) => unreachable!(),
+            Err(CastError::Validity(i)) => match i {},
+        }


We're doing this a lot; any way we can factor this out somehow?

jswrenn commented Apr 26, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

jswrenn force-pushed the errors branch 7 times, most recently from 940a3cb to 2d2c41f Compare April 27, 2024 16:52

joshlf reviewed Apr 29, 2024

View reviewed changes

joshlf mentioned this pull request Apr 29, 2024

Overhaul error reporting (struct version) #1150

Closed

jswrenn force-pushed the errors branch 2 times, most recently from f3c7f8f to aedf979 Compare April 29, 2024 19:55

jswrenn changed the title ~~Overhaul error reporting~~ Overhaul error reporting (enum version) Apr 29, 2024

jswrenn force-pushed the errors branch 8 times, most recently from 4dd7102 to 24037ed Compare May 2, 2024 13:59

jswrenn force-pushed the errors branch from 24037ed to 151540c Compare May 2, 2024 21:17

jswrenn commented May 2, 2024

View reviewed changes

jswrenn force-pushed the errors branch from 151540c to 318209c Compare May 2, 2024 21:36

jswrenn mentioned this pull request May 2, 2024

Error Handling in 0.8 #1139

Closed

3 tasks

jswrenn force-pushed the errors branch 2 times, most recently from 5fdf9b1 to fcaf824 Compare May 2, 2024 21:43

jswrenn marked this pull request as ready for review May 2, 2024 21:44

joshlf requested changes May 3, 2024

View reviewed changes

jswrenn force-pushed the errors branch from fcaf824 to d45389b Compare May 3, 2024 14:22

Overhaul error reporting

b77c8a6

This PR overhauls error reporting, replacing our `Option` returns with `Result`s. The `Err` type of these results provide both the cause of the failure, and access to the underlying source of the conversion. Makes progress towards #1139.

jswrenn force-pushed the errors branch from d45389b to b77c8a6 Compare May 3, 2024 18:13

joshlf requested changes May 3, 2024

View reviewed changes

joshlf approved these changes May 3, 2024

View reviewed changes

joshlf added this pull request to the merge queue May 3, 2024

Merged via the queue into main with commit 5f2478d May 3, 2024
210 checks passed

joshlf deleted the errors branch May 3, 2024 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul error reporting (enum version) #1144

Overhaul error reporting (enum version) #1144

jswrenn commented Apr 26, 2024 •

edited

Loading

joshlf Apr 29, 2024

jswrenn Apr 29, 2024

jswrenn Apr 29, 2024

jswrenn commented May 2, 2024

jswrenn May 2, 2024 •

edited

Loading

joshlf May 3, 2024

jswrenn May 3, 2024

joshlf May 3, 2024

jswrenn May 3, 2024

joshlf May 3, 2024

jswrenn May 3, 2024

joshlf May 3, 2024

jswrenn May 3, 2024

joshlf May 3, 2024

jswrenn May 3, 2024

joshlf May 3, 2024

jswrenn May 3, 2024

joshlf May 3, 2024

jswrenn May 3, 2024

joshlf May 3, 2024

jswrenn May 3, 2024

joshlf May 3, 2024

jswrenn May 3, 2024

joshlf May 3, 2024

	// Thid module is `pub` so that zerocopy's error types and error handling
	// This module is `pub` so that zerocopy's error types and error handling

		/// Unsafe code may rely on this method's returned `ValidityError`
		/// containing `self`.

Overhaul error reporting (enum version) #1144

Overhaul error reporting (enum version) #1144

Conversation

jswrenn commented Apr 26, 2024 • edited Loading

Fundamental Error Types

Compound Error Types

Shared Interfaces

User Experience Examples

Future Outlook

Comparison to #1150

Surface Area

API Complexity

Conceptual Complexity

Future Work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jswrenn commented May 2, 2024

jswrenn May 2, 2024 • edited Loading

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

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

jswrenn commented Apr 26, 2024 •

edited

Loading

jswrenn May 2, 2024 •

edited

Loading