feat!: new semantic for bit-shifts

acvm-repo/brillig_vm/src/arithmetic.rs

@@ -5,16 +5,18 @@ use std::ops::{BitAnd, BitOr, BitXor, Shl, Shr};
 use acir::AcirField;
 use acir::brillig::{BinaryFieldOp, BinaryIntOp, BitSize, IntegerBitSize};
 use num_bigint::BigUint;
-use num_traits::{CheckedDiv, WrappingAdd, WrappingMul, WrappingSub, Zero};
+use num_traits::{CheckedDiv, ToPrimitive, WrappingAdd, WrappingMul, WrappingSub, Zero};
 
 use crate::memory::{MemoryTypeError, MemoryValue};
 
-#[derive(Debug, thiserror::Error)]
+#[derive(Debug, PartialEq, thiserror::Error)]
 pub(crate) enum BrilligArithmeticError {
     #[error("Bit size for lhs {lhs_bit_size} does not match op bit size {op_bit_size}")]
     MismatchedLhsBitSize { lhs_bit_size: u32, op_bit_size: u32 },
     #[error("Bit size for rhs {rhs_bit_size} does not match op bit size {op_bit_size}")]
     MismatchedRhsBitSize { rhs_bit_size: u32, op_bit_size: u32 },
+    #[error("Attempted to shift by {shift_size} bits on a type of bit size {bit_size}")]
+    BitshiftOverflow { bit_size: u32, shift_size: u32 },
     #[error("Attempted to divide by zero")]
     DivisionByZero,
 }
@@ -163,43 +165,66 @@ pub(crate) fn evaluate_binary_int_op<F: AcirField>(
             }
         }
 
-        BinaryIntOp::Shl | BinaryIntOp::Shr => {
-            let rhs = rhs.expect_u8().map_err(|error| match error {
-                MemoryTypeError::MismatchedBitSize { value_bit_size, expected_bit_size } => {
-                    BrilligArithmeticError::MismatchedRhsBitSize {
-                        rhs_bit_size: value_bit_size,
-                        op_bit_size: expected_bit_size,
-                    }
-                }
-                _ => unreachable!("MemoryTypeError NotInteger is only produced by to_u128"),
-            })?;
-
-            match (lhs, bit_size) {
-                (MemoryValue::U1(lhs), IntegerBitSize::U1) => {
-                    let result = if rhs == 0 { lhs } else { false };
-                    Ok(MemoryValue::U1(result))
+        BinaryIntOp::Shl | BinaryIntOp::Shr => match (lhs, rhs, bit_size) {
+            (MemoryValue::U1(lhs), MemoryValue::U1(rhs), IntegerBitSize::U1) => {
+                if rhs {
+                    Err(BrilligArithmeticError::BitshiftOverflow { bit_size: 1, shift_size: 1 })
+                } else {
+                    Ok(MemoryValue::U1(lhs))
                 }
-                (MemoryValue::U8(lhs), IntegerBitSize::U8) => {
+            }
+            (MemoryValue::U8(lhs), MemoryValue::U8(rhs), IntegerBitSize::U8) => {
+                if rhs < 8 {
                     Ok(MemoryValue::U8(evaluate_binary_int_op_shifts(op, lhs, rhs)))
+                } else {
+                    Err(BrilligArithmeticError::BitshiftOverflow {
+                        bit_size: 8,
+                        shift_size: rhs as u32,
+                    })
                 }
-                (MemoryValue::U16(lhs), IntegerBitSize::U16) => {
+            }
+            (MemoryValue::U16(lhs), MemoryValue::U16(rhs), IntegerBitSize::U16) => {
+                if rhs < 16 {
                     Ok(MemoryValue::U16(evaluate_binary_int_op_shifts(op, lhs, rhs)))
+                } else {
+                    Err(BrilligArithmeticError::BitshiftOverflow {
+                        bit_size: 8,
+                        shift_size: rhs as u32,
+                    })
                 }
-                (MemoryValue::U32(lhs), IntegerBitSize::U32) => {
+            }
+            (MemoryValue::U32(lhs), MemoryValue::U32(rhs), IntegerBitSize::U32) => {
+                if rhs < 32 {
                     Ok(MemoryValue::U32(evaluate_binary_int_op_shifts(op, lhs, rhs)))
+                } else {
+                    Err(BrilligArithmeticError::BitshiftOverflow { bit_size: 8, shift_size: rhs })
                 }
-                (MemoryValue::U64(lhs), IntegerBitSize::U64) => {
+            }
+            (MemoryValue::U64(lhs), MemoryValue::U64(rhs), IntegerBitSize::U64) => {
+                if rhs < 64 {
                     Ok(MemoryValue::U64(evaluate_binary_int_op_shifts(op, lhs, rhs)))
+                } else {
+                    Err(BrilligArithmeticError::BitshiftOverflow {
+                        bit_size: 8,
+                        shift_size: rhs as u32,
+                    })
                 }
-                (MemoryValue::U128(lhs), IntegerBitSize::U128) => {
+            }
+            (MemoryValue::U128(lhs), MemoryValue::U128(rhs), IntegerBitSize::U128) => {
+                if rhs < 128 {
                     Ok(MemoryValue::U128(evaluate_binary_int_op_shifts(op, lhs, rhs)))
+                } else {
+                    Err(BrilligArithmeticError::BitshiftOverflow {
+                        bit_size: 8,
+                        shift_size: rhs as u32,
+                    })
                 }
-                _ => Err(BrilligArithmeticError::MismatchedLhsBitSize {
-                    lhs_bit_size: lhs.bit_size().to_u32::<F>(),
-                    op_bit_size: bit_size.into(),
-                }),
             }
-        }
+            _ => Err(BrilligArithmeticError::MismatchedLhsBitSize {
+                lhs_bit_size: lhs.bit_size().to_u32::<F>(),
+                op_bit_size: bit_size.into(),
+            }),
+        },
     }
 }
 
@@ -255,19 +280,19 @@ fn evaluate_binary_int_op_cmp<T: Ord + PartialEq>(op: &BinaryIntOp, lhs: T, rhs:
 ///
 /// # Panics
 /// If an unsupported operator is provided (i.e., not Shl or Shr).
-fn evaluate_binary_int_op_shifts<T: From<u8> + Zero + Shl<Output = T> + Shr<Output = T>>(
+fn evaluate_binary_int_op_shifts<T: ToPrimitive + Zero + Shl<Output = T> + Shr<Output = T>>(
     op: &BinaryIntOp,
     lhs: T,
-    rhs: u8,
+    rhs: T,
 ) -> T {
     match op {
         BinaryIntOp::Shl => {
-            let rhs_usize: usize = rhs as usize;
-            if rhs_usize >= 8 * size_of::<T>() { T::zero() } else { lhs << rhs.into() }
+            let rhs_usize: usize = rhs.to_usize().expect("Could not convert rhs to usize");
+            if rhs_usize >= 8 * size_of::<T>() { T::zero() } else { lhs << rhs }
         }
         BinaryIntOp::Shr => {
-            let rhs_usize: usize = rhs as usize;
-            if rhs_usize >= 8 * size_of::<T>() { T::zero() } else { lhs >> rhs.into() }
+            let rhs_usize: usize = rhs.to_usize().expect("Could not convert rhs to usize");
+            if rhs_usize >= 8 * size_of::<T>() { T::zero() } else { lhs >> rhs }
         }
         _ => unreachable!("Operator not handled by this function: {op:?}"),
     }
@@ -432,4 +457,44 @@ mod tests {
 
         evaluate_int_ops(test_ops, BinaryIntOp::Div, bit_size);
     }
+
+    #[test]
+    fn shl_test() {
+        let bit_size = IntegerBitSize::U8;
+
+        let test_ops =
+            vec![TestParams { a: 1, b: 7, result: 128 }, TestParams { a: 5, b: 7, result: 128 }];
+
+        evaluate_int_ops(test_ops, BinaryIntOp::Shl, bit_size);
+
+        assert_eq!(
+            evaluate_binary_int_op(
+                &BinaryIntOp::Shl,
+                MemoryValue::<FieldElement>::U8(1u8),
+                MemoryValue::<FieldElement>::U8(8u8),
+                IntegerBitSize::U8
+            ),
+            Err(BrilligArithmeticError::BitshiftOverflow { bit_size: 8, shift_size: 8 })
+        );
+    }
+
+    #[test]
+    fn shr_test() {
+        let bit_size = IntegerBitSize::U8;
+
+        let test_ops =
+            vec![TestParams { a: 1, b: 0, result: 1 }, TestParams { a: 5, b: 1, result: 2 }];
+
+        evaluate_int_ops(test_ops, BinaryIntOp::Shr, bit_size);
+
+        assert_eq!(
+            evaluate_binary_int_op(
+                &BinaryIntOp::Shr,
+                MemoryValue::<FieldElement>::U8(1u8),
+                MemoryValue::<FieldElement>::U8(8u8),
+                IntegerBitSize::U8
+            ),
+            Err(BrilligArithmeticError::BitshiftOverflow { bit_size: 8, shift_size: 8 })
+        );
+    }
 }

compiler/noirc_evaluator/src/ssa/interpreter/errors.rs

@@ -115,8 +115,6 @@ pub enum InternalError {
         "Invalid bit size of `{bit_size}` given to truncate, maximum size allowed for unsigned values is {MAX_UNSIGNED_BIT_SIZE}"
     )]
     InvalidUnsignedTruncateBitSize { bit_size: u32 },
-    #[error("Rhs of `{operator}` should be a u8 but found `{rhs_id} = {rhs}`")]
-    RhsOfBitShiftShouldBeU8 { operator: &'static str, rhs_id: ValueId, rhs: String },
     #[error(
         "Expected {expected_type} value in {instruction} but instead found `{value_id} = {value}`"
     )]

compiler/noirc_evaluator/src/ssa/interpreter/mod.rs

@@ -1297,34 +1297,62 @@ impl<W: Write> Interpreter<'_, W> {
                 apply_int_binop!(lhs, rhs, binary, std::ops::BitXor::bitxor)
             }
             BinaryOp::Shl => {
-                let Some(rhs) = rhs.as_u8() else {
-                    let rhs = rhs.to_string();
-                    return Err(internal(InternalError::RhsOfBitShiftShouldBeU8 {
-                        operator: "<<",
-                        rhs_id,
-                        rhs,
-                    }));
-                };
-
-                let rhs = rhs as u32;
                 use NumericValue::*;
-                match lhs {
-                    Field(_) => {
+                let instruction =
+                    format!("`{}` ({lhs} << {rhs})", display_binary(binary, self.dfg()));
+                let overflow = InterpreterError::Overflow { operator: BinaryOp::Shl, instruction };
+                match (lhs, rhs) {
+                    (Field(_), _) | (_, Field(_)) => {
                         return Err(internal(InternalError::UnsupportedOperatorForType {
                             operator: "<<",
                             typ: "Field",
                         }));
                     }
-                    U1(value) => U1(if rhs == 0 { value } else { false }),
-                    U8(value) => U8(value.checked_shl(rhs).unwrap_or(0)),
-                    U16(value) => U16(value.checked_shl(rhs).unwrap_or(0)),
-                    U32(value) => U32(value.checked_shl(rhs).unwrap_or(0)),
-                    U64(value) => U64(value.checked_shl(rhs).unwrap_or(0)),
-                    U128(value) => U128(value.checked_shl(rhs).unwrap_or(0)),
-                    I8(value) => I8(value.checked_shl(rhs).unwrap_or(0)),
-                    I16(value) => I16(value.checked_shl(rhs).unwrap_or(0)),
-                    I32(value) => I32(value.checked_shl(rhs).unwrap_or(0)),
-                    I64(value) => I64(value.checked_shl(rhs).unwrap_or(0)),
+                    (U1(lhs_value), U1(rhs_value)) => {
+                        U1(if !rhs_value { lhs_value } else { false })
+                    }
+                    (U8(lhs_value), U8(rhs_value)) => {
+                        U8(lhs_value.checked_shl(rhs_value.into()).unwrap_or(0))
+                    }
+                    (U16(lhs_value), U16(rhs_value)) => {
+                        U16(lhs_value.checked_shl(rhs_value.into()).unwrap_or(0))
+                    }
+                    (U32(lhs_value), U32(rhs_value)) => {
+                        U32(lhs_value.checked_shl(rhs_value).unwrap_or(0))
+                    }
+                    (U64(lhs_value), U64(rhs_value)) => {
+                        let rhs_value: u32 = rhs_value.try_into().map_err(|_| overflow)?;
+                        U64(lhs_value.checked_shl(rhs_value).unwrap_or(0))
+                    }
+                    (U128(lhs_value), U128(rhs_value)) => {
+                        let rhs_value: u32 = rhs_value.try_into().map_err(|_| overflow)?;
+                        U128(lhs_value.checked_shl(rhs_value).unwrap_or(0))
+                    }
+                    (I8(lhs_value), I8(rhs_value)) => {
+                        let rhs_value: u32 = rhs_value.try_into().map_err(|_| overflow)?;
+                        I8(lhs_value.checked_shl(rhs_value).unwrap_or(0))
+                    }
+                    (I16(lhs_value), I16(rhs_value)) => {
+                        let rhs_value: u32 = rhs_value.try_into().map_err(|_| overflow)?;
+                        I16(lhs_value.checked_shl(rhs_value).unwrap_or(0))
+                    }
+                    (I32(lhs_value), I32(rhs_value)) => {
+                        let rhs_value: u32 = rhs_value.try_into().map_err(|_| overflow)?;
+                        I32(lhs_value.checked_shl(rhs_value).unwrap_or(0))
+                    }
+                    (I64(lhs_value), I64(rhs_value)) => {
+                        let rhs_value: u32 = rhs_value.try_into().map_err(|_| overflow)?;
+                        I64(lhs_value.checked_shl(rhs_value).unwrap_or(0))
+                    }
+                    _ => {
+                        return Err(internal(InternalError::MismatchedTypesInBinaryOperator {
+                            lhs: lhs.to_string(),
+                            rhs: rhs.to_string(),
+                            operator: binary.operator,
+                            lhs_id: binary.lhs,
+                            rhs_id: binary.rhs,
+                        }));
+                    }
                 }
             }
             BinaryOp::Shr => {
@@ -1335,35 +1363,63 @@ impl<W: Write> Interpreter<'_, W> {
                         NumericValue::zero(lhs.get_type())
                     }
                 };
+                let instruction =
+                    format!("`{}` ({lhs} >> {rhs})", display_binary(binary, self.dfg()));
+                let overflow = InterpreterError::Overflow { operator: BinaryOp::Shr, instruction };
 
-                let Some(rhs) = rhs.as_u8() else {
-                    let rhs = rhs.to_string();
-                    return Err(internal(InternalError::RhsOfBitShiftShouldBeU8 {
-                        operator: ">>",
-                        rhs_id,
-                        rhs,
-                    }));
-                };
-
-                let rhs = rhs as u32;
                 use NumericValue::*;
-                match lhs {
-                    Field(_) => {
+                match (lhs, rhs) {
+                    (Field(_), _) | (_, Field(_)) => {
                         return Err(internal(InternalError::UnsupportedOperatorForType {
-                            operator: ">>",
+                            operator: "<<",
                             typ: "Field",
                         }));
                     }
-                    U1(value) => U1(if rhs == 0 { value } else { false }),
-                    U8(value) => value.checked_shr(rhs).map(U8).unwrap_or_else(fallback),
-                    U16(value) => value.checked_shr(rhs).map(U16).unwrap_or_else(fallback),
-                    U32(value) => value.checked_shr(rhs).map(U32).unwrap_or_else(fallback),
-                    U64(value) => value.checked_shr(rhs).map(U64).unwrap_or_else(fallback),
-                    U128(value) => value.checked_shr(rhs).map(U128).unwrap_or_else(fallback),
-                    I8(value) => value.checked_shr(rhs).map(I8).unwrap_or_else(fallback),
-                    I16(value) => value.checked_shr(rhs).map(I16).unwrap_or_else(fallback),
-                    I32(value) => value.checked_shr(rhs).map(I32).unwrap_or_else(fallback),
-                    I64(value) => value.checked_shr(rhs).map(I64).unwrap_or_else(fallback),
+                    (U1(lhs_value), U1(rhs_value)) => {
+                        U1(if !rhs_value { lhs_value } else { false })
+                    }
+                    (U8(lhs_value), U8(rhs_value)) => {
+                        lhs_value.checked_shr(rhs_value.into()).map(U8).unwrap_or_else(fallback)
+                    }
+                    (U16(lhs_value), U16(rhs_value)) => {
+                        lhs_value.checked_shr(rhs_value.into()).map(U16).unwrap_or_else(fallback)
+                    }
+                    (U32(lhs_value), U32(rhs_value)) => {
+                        lhs_value.checked_shr(rhs_value).map(U32).unwrap_or_else(fallback)
+                    }
+                    (U64(lhs_value), U64(rhs_value)) => {
+                        let rhs_value: u32 = rhs_value.try_into().map_err(|_| overflow)?;
+                        lhs_value.checked_shr(rhs_value).map(U64).unwrap_or_else(fallback)
+                    }
+                    (U128(lhs_value), U128(rhs_value)) => {
+                        let rhs_value: u32 = rhs_value.try_into().map_err(|_| overflow)?;
+                        lhs_value.checked_shr(rhs_value).map(U128).unwrap_or_else(fallback)
+                    }
+                    (I8(lhs_value), I8(rhs_value)) => {
+                        let rhs_value: u32 = rhs_value.try_into().map_err(|_| overflow)?;
+                        lhs_value.checked_shr(rhs_value).map(I8).unwrap_or_else(fallback)
+                    }
+                    (I16(lhs_value), I16(rhs_value)) => {
+                        let rhs_value: u32 = rhs_value.try_into().map_err(|_| overflow)?;
+                        lhs_value.checked_shr(rhs_value).map(I16).unwrap_or_else(fallback)
+                    }
+                    (I32(lhs_value), I32(rhs_value)) => {
+                        let rhs_value: u32 = rhs_value.try_into().map_err(|_| overflow)?;
+                        lhs_value.checked_shr(rhs_value).map(I32).unwrap_or_else(fallback)
+                    }
+                    (I64(lhs_value), I64(rhs_value)) => {
+                        let rhs_value: u32 = rhs_value.try_into().map_err(|_| overflow)?;
+                        lhs_value.checked_shr(rhs_value).map(I64).unwrap_or_else(fallback)
+                    }
+                    _ => {
+                        return Err(internal(InternalError::MismatchedTypesInBinaryOperator {
+                            lhs: lhs.to_string(),
+                            rhs: rhs.to_string(),
+                            operator: binary.operator,
+                            lhs_id: binary.lhs,
+                            rhs_id: binary.rhs,
+                        }));
+                    }
                 }
             }
         };
@@ -1472,18 +1528,18 @@ impl<W: Write> Interpreter<'_, W> {
             BinaryOp::Or => lhs | rhs,
             BinaryOp::Xor => lhs ^ rhs,
             BinaryOp::Shl => {
-                return Err(internal(InternalError::RhsOfBitShiftShouldBeU8 {
-                    operator: "<<",
-                    rhs_id,
-                    rhs: format!("u1 {}", rhs as u8),
-                }));
+                if rhs {
+                    false
+                } else {
+                    lhs
+                }
             }
             BinaryOp::Shr => {
-                return Err(internal(InternalError::RhsOfBitShiftShouldBeU8 {
-                    operator: ">>",
-                    rhs_id,
-                    rhs: format!("u1 {}", rhs as u8),
-                }));
+                if rhs {
+                    false
+                } else {
+                    lhs
+                }
             }
         };
         Ok(Value::Numeric(NumericValue::U1(result)))