Encode infallible alignment errors in types #1718

joshlf · 2024-09-21T17:16:07Z

Permit callers to prove at compile time that alignment errors are unreachable for unaligned destination types. This permits them to infallibly ignore this error condition.

joshlf · 2024-09-21T17:16:40Z

Still need to bikeshed naming and how to write the doc comment, but otherwise it's done.

jswrenn

This is really elegant! I particularly like the error-ization of validate_aligned_to.

src/util/mod.rs

src/error.rs

jswrenn

A few #[inline(always)] nits, but then I'm happy to merge this.

I wonder if this is sufficiently elegant that we should remove the unaligned methods from Ref!

jswrenn · 2024-09-21T18:21:48Z

src/error.rs

@@ -626,6 +683,17 @@ impl<Src, Dst: ?Sized> CastError<Src, Dst> {
    }
 }

+impl<Src, Dst: ?Sized + Unaligned> Into<SizeError<Src, Dst>> for CastError<Src, Dst> {
+    fn into(self) -> SizeError<Src, Dst> {


Slap an #[inline(always)] on this — we want the effects of unreachable_unchecked to propagate up the call chain.

jswrenn · 2024-09-21T18:22:16Z

src/error.rs

@@ -255,6 +290,28 @@ impl<Src, Dst: ?Sized> AlignmentError<Src, Dst> {
    }
 }

+impl<Src, Dst: ?Sized + Unaligned> AlignmentError<Src, Dst> {
+    fn into_infallible(self) -> Infallible {


Slap an #[inline(always)] on this — we want the effects of unreachable_unchecked to propagate up the call chain.

jswrenn · 2024-09-21T18:26:50Z

src/error.rs

+    }
+}
+
+impl<Src, Dst: ?Sized + Unaligned, S, V> Into<ConvertError<Infallible, S, V>>


Why are we implementing Into here instead of From?

jswrenn · 2024-09-21T18:37:27Z

src/error.rs

@@ -255,6 +290,28 @@ impl<Src, Dst: ?Sized> AlignmentError<Src, Dst> {
    }
 }

+impl<Src, Dst: ?Sized + Unaligned> AlignmentError<Src, Dst> {
+    fn into_infallible(self) -> Infallible {


For consistency, should this be an implementation of From? Then we could have a consistent messaging that calling .into() at all levels makes impossible error branches Infallible.

src/error.rs

codecov-commenter · 2024-09-21T18:52:20Z

Codecov Report

Attention: Patch coverage is 53.19149% with 22 lines in your changes missing coverage. Please review.

Project coverage is 88.35%. Comparing base (553996e) to head (761d909).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/error.rs	42.42%	19 Missing ⚠️
src/ref.rs	50.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1718      +/-   ##
==========================================
- Coverage   88.79%   88.35%   -0.45%     
==========================================
  Files          16       16              
  Lines        5846     5889      +43     
==========================================
+ Hits         5191     5203      +12     
- Misses        655      686      +31

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

src/error.rs

Permit callers to prove at compile time that alignment errors are unreachable for unaligned destination types. This permits them to infallibly ignore this error condition.

joshlf · 2024-09-21T19:24:12Z

I wonder if this is sufficiently elegant that we should remove the unaligned methods from Ref!

Probably, but let's test it in Fuchsia first.

jswrenn · 2024-09-21T20:12:05Z

src/error.rs

+    /// Infallibly discards the alignment error from this `ConvertError` since
+    /// `Dst` is unaligned.
+    ///
+    /// Since [`Dst: Unaligned`], it is impossible to encounter an alignment
+    /// error. This method permits discarding that alignment error infallibly
+    /// and replacing it with [`Infallible`].
+    ///
+    /// [`Dst: Unaligned`]: crate::Unaligned
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use core::convert::Infallible;
+    /// use zerocopy::*;
+    /// # use zerocopy_derive::*;
+    ///
+    /// #[derive(TryFromBytes, KnownLayout, Unaligned, Immutable)]
+    /// #[repr(C, packed)]
+    /// struct Bools {
+    ///     one: bool,
+    ///     two: bool,
+    ///     many: [bool],
+    /// }
+    ///
+    /// impl Bools {
+    ///     fn parse(bytes: &[u8]) -> Result<&Bools, UnalignedTryCastError<&[u8], Bools>> {
+    ///         // Since `Bools: Unaligned`, we can infallibly discard
+    ///         // the alignment error.
+    ///         Bools::try_ref_from_bytes(bytes).map_err(Into::into)
+    ///     }
+    /// }
+    /// ```


I don't think users are primed to expect bespoke documentation on trait impls. Let's move this documentation to the error module docs.

jswrenn · 2024-09-21T20:12:35Z

src/error.rs

+impl<Src, Dst: ?Sized + Unaligned, S, V> From<ConvertError<AlignmentError<Src, Dst>, S, V>>
+    for ConvertError<Infallible, S, V>


I think we should make this a general From conversion of A, S, and V:

Suggested change

impl<Src, Dst: ?Sized + Unaligned, S, V> From<ConvertError<AlignmentError<Src, Dst>, S, V>>

for ConvertError<Infallible, S, V>

impl<Src, Dst: ?Sized + Unaligned, A, S, V, NewA, NewS, NewV>

From<ConvertError<A, S, V>>

for

ConvertError<NewA, NewS, NewV>

where

NewA: From<A>,

NewS: From<S>,

NewV: From<V>,

jswrenn requested changes Sep 21, 2024

View reviewed changes

src/util/mod.rs Show resolved Hide resolved

src/error.rs Outdated Show resolved Hide resolved

joshlf force-pushed the infallible-alignment-error branch from 1b8c3f7 to e9e640d Compare September 21, 2024 18:00

joshlf commented Sep 21, 2024

View reviewed changes

src/error.rs Show resolved Hide resolved

jswrenn requested changes Sep 21, 2024

View reviewed changes

jswrenn reviewed Sep 21, 2024

View reviewed changes

joshlf force-pushed the infallible-alignment-error branch from e9e640d to df8a716 Compare September 21, 2024 18:44

joshlf commented Sep 21, 2024

View reviewed changes

src/error.rs Show resolved Hide resolved

src/error.rs Show resolved Hide resolved

joshlf mentioned this pull request Sep 21, 2024

Test infallible alignment errors in Fuchsia, then remove Ref unaligned constructors #1720

Open

joshlf force-pushed the infallible-alignment-error branch from df8a716 to 22e78b9 Compare September 21, 2024 19:00

joshlf commented Sep 21, 2024

View reviewed changes

src/error.rs Outdated Show resolved Hide resolved

joshlf mentioned this pull request Sep 21, 2024

Make infallible alignment errors more discoverable #1721

Open

Encode infallible alignment errors in types

761d909

Permit callers to prove at compile time that alignment errors are unreachable for unaligned destination types. This permits them to infallibly ignore this error condition.

joshlf force-pushed the infallible-alignment-error branch from 22e78b9 to 761d909 Compare September 21, 2024 19:22

joshlf marked this pull request as ready for review September 21, 2024 19:25

joshlf enabled auto-merge September 21, 2024 19:25

jswrenn requested changes Sep 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encode infallible alignment errors in types #1718

Encode infallible alignment errors in types #1718

joshlf commented Sep 21, 2024

joshlf commented Sep 21, 2024

jswrenn left a comment

jswrenn left a comment

jswrenn Sep 21, 2024

joshlf Sep 21, 2024

jswrenn Sep 21, 2024

joshlf Sep 21, 2024

jswrenn Sep 21, 2024

joshlf Sep 21, 2024

jswrenn Sep 21, 2024

joshlf Sep 21, 2024

codecov-commenter commented Sep 21, 2024 •

edited

Loading

joshlf commented Sep 21, 2024

jswrenn Sep 21, 2024

jswrenn Sep 21, 2024 •

edited

Loading

		impl<Src, Dst: ?Sized + Unaligned, S, V> From<ConvertError<AlignmentError<Src, Dst>, S, V>>
		for ConvertError<Infallible, S, V>

-impl<Src, Dst: ?Sized + Unaligned, S, V> From<ConvertError<AlignmentError<Src, Dst>, S, V>>
-    for ConvertError<Infallible, S, V>
+impl<Src, Dst: ?Sized + Unaligned, A, S, V, NewA, NewS, NewV>
+    From<ConvertError<A, S, V>>
+for
+    ConvertError<NewA, NewS, NewV>
+where
+    NewA: From<A>,
+    NewS: From<S>,
+    NewV: From<V>,

Encode infallible alignment errors in types #1718

Are you sure you want to change the base?

Encode infallible alignment errors in types #1718

Conversation

joshlf commented Sep 21, 2024

joshlf commented Sep 21, 2024

jswrenn left a comment

Choose a reason for hiding this comment

jswrenn 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

codecov-commenter commented Sep 21, 2024 • edited Loading

Codecov Report

joshlf commented Sep 21, 2024

Choose a reason for hiding this comment

jswrenn Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Sep 21, 2024 •

edited

Loading

jswrenn Sep 21, 2024 •

edited

Loading