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.

Eh, without the generalization recommendation (which is impossible), I'm okay having these docs here.

src/error.rs

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

Closed

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

Closed

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

jswrenn approved these changes Sep 21, 2024

View reviewed changes

joshlf added this pull request to the merge queue Sep 21, 2024

Merged via the queue into main with commit c6b9554 Sep 21, 2024
86 checks passed

joshlf deleted the infallible-alignment-error branch September 21, 2024 20:47

Encode infallible alignment errors in types #1718

Encode infallible alignment errors in types #1718

Uh oh!

Conversation

joshlf commented Sep 21, 2024

Uh oh!

joshlf commented Sep 21, 2024

Uh oh!

jswrenn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jswrenn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

joshlf commented Sep 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Sep 21, 2024 •

edited

Loading