Add more tests for io::Error packing, and fix some comments that weren't quite accurate anymore

Author: thomcc

library/std/src/io/error.rs

@@ -76,6 +76,9 @@ impl fmt::Debug for Error {
     }
 }
 
+// Only derive debug in tests, to make sure it
+// doesn't accidentally get printed.
+#[cfg_attr(test, derive(Debug))]
 enum ErrorData<C> {
     Os(i32),
     Simple(ErrorKind),
@@ -98,6 +101,7 @@ enum ErrorData<C> {
 // if `error/repr_bitpacked.rs` is in use — for the unpacked repr it doesn't
 // matter at all)
 #[repr(align(4))]
+#[derive(Debug)]
 pub(crate) struct SimpleMessage {
     kind: ErrorKind,
     message: &'static str,

library/std/src/io/error/repr_bitpacked.rs

@@ -6,9 +6,9 @@
 //! a more clever manner than `rustc`'s default layout algorithm would).
 //!
 //! Conceptually, it stores the same data as the "unpacked" equivalent we use on
-//! other targets. Specifically, you can imagine it as an optimized following
-//! data (which is equivalent to what's stored by `repr_unpacked::Repr`, e.g.
-//! `super::ErrorData<Box<Custom>>`):
+//! other targets. Specifically, you can imagine it as an optimized version of
+//! the following enum (which is roughly equivalent to what's stored by
+//! `repr_unpacked::Repr`, e.g. `super::ErrorData<Box<Custom>>`):
 //!
 //! ```ignore (exposition-only)
 //! enum ErrorData {
@@ -135,7 +135,16 @@ impl Repr {
         // (rather than `ptr::wrapping_add`), but it's unclear this would give
         // any benefit, so we just use `wrapping_add` instead.
         let tagged = p.wrapping_add(TAG_CUSTOM).cast::<()>();
-        // Safety: the above safety comment also means the result can't be null.
+        // Safety: `TAG_CUSTOM + p` is the same as `TAG_CUSTOM | p`,
+        // because `p`'s alignment means it isn't allowed to have any of the
+        // `TAG_BITS` set (you can verify that addition and bitwise-or are the
+        // same when the operands have no bits in common using a truth table).
+        //
+        // Then, `TAG_CUSTOM | p` is not zero, as that would require
+        // `TAG_CUSTOM` and `p` both be zero, and neither is (as `p` came from a
+        // box, and `TAG_CUSTOM` just... isn't zero -- it's `0b01`). Therefore,
+        // `TAG_CUSTOM + p` isn't zero and so `tagged` can't be, and the
+        // `new_unchecked` is safe.
         let res = Self(unsafe { NonNull::new_unchecked(tagged) });
         // quickly smoke-check we encoded the right thing (This generally will
         // only run in libstd's tests, unless the user uses -Zbuild-std)
@@ -342,12 +351,25 @@ static_assert!(@usize_eq: size_of::<NonNull<()>>(), size_of::<usize>());
 static_assert!(@usize_eq: size_of::<&'static SimpleMessage>(), 8);
 static_assert!(@usize_eq: size_of::<Box<Custom>>(), 8);
 
-// And they must have >= 4 byte alignment.
-static_assert!(align_of::<SimpleMessage>() >= 4);
-static_assert!(align_of::<Custom>() >= 4);
+static_assert!((TAG_MASK + 1).is_power_of_two());
+// And they must have sufficient alignment.
+static_assert!(align_of::<SimpleMessage>() >= TAG_MASK + 1);
+static_assert!(align_of::<Custom>() >= TAG_MASK + 1);
+
+static_assert!(@usize_eq: (TAG_MASK & TAG_SIMPLE_MESSAGE), TAG_SIMPLE_MESSAGE);
+static_assert!(@usize_eq: (TAG_MASK & TAG_CUSTOM), TAG_CUSTOM);
+static_assert!(@usize_eq: (TAG_MASK & TAG_OS), TAG_OS);
+static_assert!(@usize_eq: (TAG_MASK & TAG_SIMPLE), TAG_SIMPLE);
 
-// This is obviously true (`TAG_CUSTOM` is `0b01`), but our implementation of
-// `Repr::new_custom` and such would be wrong if it were not, so we check.
+// This is obviously true (`TAG_CUSTOM` is `0b01`), but in `Repr::new_custom` we
+// offset a pointer by this value, and expect it to both be within the same
+// object, and to not wrap around the address space. See the comment in that
+// function for further details.
+//
+// Actually, at the moment we use `ptr::wrapping_add`, not `ptr::add`, so this
+// check isn't needed for that one, although the assertion that we don't
+// actually wrap around in that wrapping_add does simplify the safety reasoning
+// elsewhere considerably.
 static_assert!(size_of::<Custom>() >= TAG_CUSTOM);
 
 // These two store a payload which is allowed to be zero, so they must be

library/std/src/io/error/tests.rs

@@ -1,4 +1,5 @@
-use super::{const_io_error, Custom, Error, ErrorKind, Repr};
+use super::{const_io_error, Custom, Error, ErrorData, ErrorKind, Repr};
+use crate::assert_matches::assert_matches;
 use crate::error;
 use crate::fmt;
 use crate::mem::size_of;
@@ -69,16 +70,74 @@ fn test_const() {
 }
 
 #[test]
-fn test_error_packing() {
+fn test_os_packing() {
     for code in -20i32..20i32 {
         let e = Error::from_raw_os_error(code);
         assert_eq!(e.raw_os_error(), Some(code));
+        assert_matches!(
+            e.repr.data(),
+            ErrorData::Os(c) if c == code,
+        );
     }
+}
+
+#[test]
+fn test_errorkind_packing() {
     assert_eq!(Error::from(ErrorKind::NotFound).kind(), ErrorKind::NotFound);
+    assert_eq!(Error::from(ErrorKind::PermissionDenied).kind(), ErrorKind::PermissionDenied);
     assert_eq!(Error::from(ErrorKind::Uncategorized).kind(), ErrorKind::Uncategorized);
-    assert_eq!(Error::from(ErrorKind::NotFound).kind(), ErrorKind::NotFound);
-    assert_eq!(Error::from(ErrorKind::Uncategorized).kind(), ErrorKind::Uncategorized);
-    let dunno = const_io_error!(ErrorKind::Uncategorized, "dunno");
-    assert_eq!(dunno.kind(), ErrorKind::Uncategorized);
-    assert!(format!("{:?}", dunno).contains("dunno"))
+    // Check that the innards look like like what we want.
+    assert_matches!(
+        Error::from(ErrorKind::OutOfMemory).repr.data(),
+        ErrorData::Simple(ErrorKind::OutOfMemory),
+    );
+}
+
+#[test]
+fn test_simple_message_packing() {
+    use super::{ErrorKind::*, SimpleMessage};
+    macro_rules! check_simple_msg {
+        ($err:expr, $kind:ident, $msg:literal) => {{
+            let e = &$err;
+            // Check that the public api is right.
+            assert_eq!(e.kind(), $kind);
+            assert!(format!("{:?}", e).contains($msg));
+            // and we got what we expected
+            assert_matches!(
+                e.repr.data(),
+                ErrorData::SimpleMessage(SimpleMessage { kind: $kind, message: $msg })
+            );
+        }};
+    }
+
+    let not_static = const_io_error!(Uncategorized, "not a constant!");
+    check_simple_msg!(not_static, Uncategorized, "not a constant!");
+
+    const CONST: Error = const_io_error!(NotFound, "definitely a constant!");
+    check_simple_msg!(CONST, NotFound, "definitely a constant!");
+
+    static STATIC: Error = const_io_error!(BrokenPipe, "a constant, sort of!");
+    check_simple_msg!(STATIC, BrokenPipe, "a constant, sort of!");
+}
+
+#[derive(Debug, PartialEq)]
+struct Bojji(bool);
+impl error::Error for Bojji {}
+impl fmt::Display for Bojji {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "ah! {:?}", self)
+    }
+}
+
+#[test]
+fn test_custom_error_packing() {
+    use super::Custom;
+    let test = Error::new(ErrorKind::Uncategorized, Bojji(true));
+    assert_matches!(
+        test.repr.data(),
+        ErrorData::Custom(Custom {
+            kind: ErrorKind::Uncategorized,
+            error,
+        }) if error.downcast_ref::<Bojji>().as_deref() == Some(&Bojji(true)),
+    );
 }