Improve zero discriminant detection for enums

zerocopy-derive/src/lib.rs

@@ -38,7 +38,8 @@ use {
     quote::quote,
     syn::{
         parse_quote, parse_quote_spanned, Data, DataEnum, DataStruct, DataUnion, DeriveInput,
-        Error, Expr, ExprLit, GenericParam, Ident, Lit, Path, Type, WherePredicate,
+        Error, Expr, ExprLit, ExprUnary, GenericParam, Ident, Lit, Path, Type, UnOp,
+        WherePredicate,
     },
 };
 
@@ -634,6 +635,84 @@ fn derive_from_zeros_struct(ast: &DeriveInput, strct: &DataStruct) -> proc_macro
 // - all of its variants are fieldless
 // - one of the variants has a discriminant of `0`
 
+// Returns `Ok(index)` if variant `index` of the enum has a discriminant of
+// zero. If `Err(bool)` is returned, the boolean is true if the enum has unknown
+// discriminants (e.g. discriminants set to const expressions which we can't
+// evaluate in a proc macro). If the enum has unknown discriminants, then it
+// might have a zero variant that we just can't detect.
+fn find_zero_variant(enm: &DataEnum) -> Result<usize, bool> {
+    // Discriminants can be anywhere in the range [i128::MIN, u128::MAX] because
+    // the discriminant type may be signed or unsigned. Since we only care about
+    // tracking the discriminant when it's less than or equal to zero, we can
+    // avoid u128 -> i128 conversions and bounds checking by making the "next
+    // discriminant" value implicitly negative.
+    // Technically 64 bits is enough, but 128 is better for future compatibility
+    // with https://github.com/rust-lang/rust/issues/56071
+    let mut next_negative_discriminant = Some(0);
+
+    // Sometimes we encounter explicit discriminants that we can't know the
+    // value of (e.g. a constant expression that requires evaluation). These
+    // could evaluate to zero or a negative number, but we can't assume that
+    // they do (no false positives allowed!). So we treat them like strictly-
+    // positive values that can't result in any zero variants, and track whether
+    // we've encountered any unknown discriminants.
+    let mut has_unknown_discriminants = false;
+
+    for (i, v) in enm.variants.iter().enumerate() {
+        match v.discriminant.as_ref() {
+            // Implicit discriminant
+            None => {
+                match next_negative_discriminant.as_mut() {
+                    Some(0) => return Ok(i),
+                    // n is nonzero so subtraction is always safe
+                    Some(n) => *n -= 1,
+                    None => (),
+                }
+            }
+            // Explicit positive discriminant
+            Some((_, Expr::Lit(ExprLit { lit: Lit::Int(int), .. }))) => {
+                match int.base10_parse::<u128>().ok() {
+                    Some(0) => return Ok(i),
+                    Some(_) => next_negative_discriminant = None,
+                    None => {
+                        // Numbers should never fail to parse, but just in case:
+                        has_unknown_discriminants = true;
+                        next_negative_discriminant = None;
+                    }
+                }
+            }
+            // Explicit negative discriminant
+            Some((_, Expr::Unary(ExprUnary { op: UnOp::Neg(_), expr, .. }))) => match &**expr {
+                Expr::Lit(ExprLit { lit: Lit::Int(int), .. }) => {
+                    match int.base10_parse::<u128>().ok() {
+                        Some(0) => return Ok(i),
+                        // x is nonzero so subtraction is always safe
+                        Some(x) => next_negative_discriminant = Some(x - 1),
+                        None => {
+                            // Numbers should never fail to parse, but just in
+                            // case:
+                            has_unknown_discriminants = true;
+                            next_negative_discriminant = None;
+                        }
+                    }
+                }
+                // Unknown negative discriminant (e.g. const repr)
+                _ => {
+                    has_unknown_discriminants = true;
+                    next_negative_discriminant = None;
+                }
+            },
+            // Unknown discriminant (e.g. const expr)
+            _ => {
+                has_unknown_discriminants = true;
+                next_negative_discriminant = None;
+            }
+        }
+    }
+
+    Err(has_unknown_discriminants)
+}
+
 fn derive_from_zeros_enum(ast: &DeriveInput, enm: &DataEnum) -> proc_macro2::TokenStream {
     if !enm.is_fieldless() {
         return Error::new_spanned(ast, "only field-less enums can implement FromZeros")
@@ -644,25 +723,23 @@ fn derive_from_zeros_enum(ast: &DeriveInput, enm: &DataEnum) -> proc_macro2::Tok
     // the allowed ones.
     try_or_print!(ENUM_FROM_ZEROS_INTO_BYTES_CFG.validate_reprs(ast));
 
-    let has_explicit_zero_discriminant =
-        enm.variants.iter().filter_map(|v| v.discriminant.as_ref()).any(|(_, e)| {
-            if let Expr::Lit(ExprLit { lit: Lit::Int(i), .. }) = e {
-                i.base10_parse::<usize>().ok() == Some(0)
-            } else {
-                false
-            }
-        });
-    // If the first variant of an enum does not specify its discriminant, it is set to zero:
-    // https://doc.rust-lang.org/reference/items/enumerations.html#custom-discriminant-values-for-fieldless-enumerations
-    let has_implicit_zero_discriminant =
-        enm.variants.iter().next().map(|v| v.discriminant.is_none()) == Some(true);
-
-    if !has_explicit_zero_discriminant && !has_implicit_zero_discriminant {
-        return Error::new_spanned(
-            ast,
-            "FromZeros only supported on enums with a variant that has a discriminant of `0`",
-        )
-        .to_compile_error();
+    if let Err(has_unknown_variants) = find_zero_variant(enm) {
+        if has_unknown_variants {
+            return Error::new_spanned(
+                ast,
+                "FromZeros only supported on enums with a variant that has a discriminant of `0`\n\
+                help: This enum has discriminants which are not literal integers. One of those may \
+                define or imply which variant has a discriminant of zero. Use a literal integer to \
+                define or imply the variant with a discriminant of zero.",
+            )
+            .to_compile_error();
+        } else {
+            return Error::new_spanned(
+                ast,
+                "FromZeros only supported on enums with a variant that has a discriminant of `0`",
+            )
+            .to_compile_error();
+        }
     }
 
     impl_block(ast, enm, Trait::FromZeros, FieldBounds::ALL_SELF, SelfBounds::None, None, None)

zerocopy-derive/tests/enum_from_zeros.rs

@@ -36,3 +36,17 @@ enum Baz {
 }
 
 util_assert_impl_all!(Baz: imp::FromZeros);
+
+#[derive(imp::FromZeros)]
+#[repr(i8)]
+enum ImplicitNonFirstVariantIsZero {
+    A = -1,
+    B,
+}
+
+#[derive(imp::FromZeros)]
+#[repr(u64)]
+enum LargeDiscriminant {
+    A = 0xFFFF_FFFF_FFFF_FFFF,
+    B = 0x0000_0000_0000_0000,
+}

zerocopy-derive/tests/ui-msrv/enum.stderr

@@ -89,124 +89,145 @@ error: must have a non-align #[repr(...)] attribute in order to guarantee this t
    |
    = note: this error originates in the derive macro `FromZeros` (in Nightly builds, run with -Z macro-backtrace for more info)
 
+error: FromZeros only supported on enums with a variant that has a discriminant of `0`
+   --> tests/ui-msrv/enum.rs:102:1
+    |
+102 | / #[repr(u8)]
+103 | | enum FromZeros4 {
+104 | |     A = 1,
+105 | |     B = 2,
+106 | | }
+    | |_^
+
+error: FromZeros only supported on enums with a variant that has a discriminant of `0`
+help: This enum has discriminants which are not literal integers. One of those may define or imply which variant has a discriminant of zero. Use a literal integer to define or imply the variant with a discriminant of zero.
+   --> tests/ui-msrv/enum.rs:111:1
+    |
+111 | / #[repr(i8)]
+112 | | enum FromZeros5 {
+113 | |     A = NEGATIVE_ONE,
+114 | |     B,
+115 | | }
+    | |_^
+
 error: FromBytes requires repr of "u8", "u16", "i8", or "i16"
-   --> tests/ui-msrv/enum.rs:106:8
+   --> tests/ui-msrv/enum.rs:122:8
     |
-106 | #[repr(C)]
+122 | #[repr(C)]
     |        ^
 
 error: FromBytes requires repr of "u8", "u16", "i8", or "i16"
-   --> tests/ui-msrv/enum.rs:112:8
+   --> tests/ui-msrv/enum.rs:128:8
     |
-112 | #[repr(usize)]
+128 | #[repr(usize)]
     |        ^^^^^
 
 error: FromBytes requires repr of "u8", "u16", "i8", or "i16"
-   --> tests/ui-msrv/enum.rs:118:8
+   --> tests/ui-msrv/enum.rs:134:8
     |
-118 | #[repr(isize)]
+134 | #[repr(isize)]
     |        ^^^^^
 
 error: FromBytes requires repr of "u8", "u16", "i8", or "i16"
-   --> tests/ui-msrv/enum.rs:124:8
+   --> tests/ui-msrv/enum.rs:140:8
     |
-124 | #[repr(u32)]
+140 | #[repr(u32)]
     |        ^^^
 
 error: FromBytes requires repr of "u8", "u16", "i8", or "i16"
-   --> tests/ui-msrv/enum.rs:130:8
+   --> tests/ui-msrv/enum.rs:146:8
     |
-130 | #[repr(i32)]
+146 | #[repr(i32)]
     |        ^^^
 
 error: FromBytes requires repr of "u8", "u16", "i8", or "i16"
-   --> tests/ui-msrv/enum.rs:136:8
+   --> tests/ui-msrv/enum.rs:152:8
     |
-136 | #[repr(u64)]
+152 | #[repr(u64)]
     |        ^^^
 
 error: FromBytes requires repr of "u8", "u16", "i8", or "i16"
-   --> tests/ui-msrv/enum.rs:142:8
+   --> tests/ui-msrv/enum.rs:158:8
     |
-142 | #[repr(i64)]
+158 | #[repr(i64)]
     |        ^^^
 
 error: Unaligned requires repr of "u8" or "i8", and no alignment (i.e., repr(align(N > 1)))
-   --> tests/ui-msrv/enum.rs:152:8
+   --> tests/ui-msrv/enum.rs:168:8
     |
-152 | #[repr(C)]
+168 | #[repr(C)]
     |        ^
 
 error: Unaligned requires repr of "u8" or "i8", and no alignment (i.e., repr(align(N > 1)))
-   --> tests/ui-msrv/enum.rs:158:8
+   --> tests/ui-msrv/enum.rs:174:8
     |
-158 | #[repr(u16)]
+174 | #[repr(u16)]
     |        ^^^
 
 error: Unaligned requires repr of "u8" or "i8", and no alignment (i.e., repr(align(N > 1)))
-   --> tests/ui-msrv/enum.rs:164:8
+   --> tests/ui-msrv/enum.rs:180:8
     |
-164 | #[repr(i16)]
+180 | #[repr(i16)]
     |        ^^^
 
 error: Unaligned requires repr of "u8" or "i8", and no alignment (i.e., repr(align(N > 1)))
-   --> tests/ui-msrv/enum.rs:170:8
+   --> tests/ui-msrv/enum.rs:186:8
     |
-170 | #[repr(u32)]
+186 | #[repr(u32)]
     |        ^^^
 
 error: Unaligned requires repr of "u8" or "i8", and no alignment (i.e., repr(align(N > 1)))
-   --> tests/ui-msrv/enum.rs:176:8
+   --> tests/ui-msrv/enum.rs:192:8
     |
-176 | #[repr(i32)]
+192 | #[repr(i32)]
     |        ^^^
 
 error: Unaligned requires repr of "u8" or "i8", and no alignment (i.e., repr(align(N > 1)))
-   --> tests/ui-msrv/enum.rs:182:8
+   --> tests/ui-msrv/enum.rs:198:8
     |
-182 | #[repr(u64)]
+198 | #[repr(u64)]
     |        ^^^
 
 error: Unaligned requires repr of "u8" or "i8", and no alignment (i.e., repr(align(N > 1)))
-   --> tests/ui-msrv/enum.rs:188:8
+   --> tests/ui-msrv/enum.rs:204:8
     |
-188 | #[repr(i64)]
+204 | #[repr(i64)]
     |        ^^^
 
 error: Unaligned requires repr of "u8" or "i8", and no alignment (i.e., repr(align(N > 1)))
-   --> tests/ui-msrv/enum.rs:194:8
+   --> tests/ui-msrv/enum.rs:210:8
     |
-194 | #[repr(usize)]
+210 | #[repr(usize)]
     |        ^^^^^
 
 error: Unaligned requires repr of "u8" or "i8", and no alignment (i.e., repr(align(N > 1)))
-   --> tests/ui-msrv/enum.rs:200:8
+   --> tests/ui-msrv/enum.rs:216:8
     |
-200 | #[repr(isize)]
+216 | #[repr(isize)]
     |        ^^^^^
 
 error: cannot derive Unaligned with repr(align(N > 1))
-   --> tests/ui-msrv/enum.rs:206:12
+   --> tests/ui-msrv/enum.rs:222:12
     |
-206 | #[repr(u8, align(2))]
+222 | #[repr(u8, align(2))]
     |            ^^^^^^^^
 
 error: cannot derive Unaligned with repr(align(N > 1))
-   --> tests/ui-msrv/enum.rs:212:12
+   --> tests/ui-msrv/enum.rs:228:12
     |
-212 | #[repr(i8, align(2))]
+228 | #[repr(i8, align(2))]
     |            ^^^^^^^^
 
 error: cannot derive Unaligned with repr(align(N > 1))
-   --> tests/ui-msrv/enum.rs:218:18
+   --> tests/ui-msrv/enum.rs:234:18
     |
-218 | #[repr(align(1), align(2))]
+234 | #[repr(align(1), align(2))]
     |                  ^^^^^^^^
 
 error: cannot derive Unaligned with repr(align(N > 1))
-   --> tests/ui-msrv/enum.rs:224:8
+   --> tests/ui-msrv/enum.rs:240:8
     |
-224 | #[repr(align(2), align(4))]
+240 | #[repr(align(2), align(4))]
     |        ^^^^^^^^
 
 error[E0565]: meta item in `repr` must be an identifier

zerocopy-derive/tests/ui-nightly/enum.rs

@@ -98,6 +98,22 @@ enum FromZeros3 {
     B,
 }
 
+#[derive(FromZeros)]
+#[repr(u8)]
+enum FromZeros4 {
+    A = 1,
+    B = 2,
+}
+
+const NEGATIVE_ONE: i8 = -1;
+
+#[derive(FromZeros)]
+#[repr(i8)]
+enum FromZeros5 {
+    A = NEGATIVE_ONE,
+    B,
+}
+
 //
 // FromBytes errors
 //