Correctly detect signed/unsigned integer overflows/underflows

compiler/noirc_evaluator/src/ssa/ir/types.rs

@@ -31,12 +31,31 @@ impl NumericType {
 
     /// Returns true if the given Field value is within the numeric limits
     /// for the current NumericType.
-    pub(crate) fn value_is_within_limits(self, field: FieldElement) -> bool {
+    ///
+    /// Note that if `negative` is true, it's expected that the field value holds the negative value
+    /// already casted to the range of `self` (for example, `-1` for a `i8` or `u8` type is
+    /// represted as `255`).
+    pub(crate) fn value_is_within_limits(self, field: FieldElement, negative: bool) -> bool {
         match self {
-            NumericType::Signed { bit_size } | NumericType::Unsigned { bit_size } => {
+            NumericType::Unsigned { bit_size } => {
+                if negative {
+                    return false;
+                }
                 let max = 2u128.pow(bit_size) - 1;
                 field <= max.into()
             }
+            NumericType::Signed { bit_size } => {
+                if negative {
+                    let base = 1_u128 << bit_size;
+                    // Recover the original value (a similar operation is done in compiler/noirc_frontend/src/monomorphization/mod.rs)
+                    let field = FieldElement::from(base) - field;
+                    let max = 2u128.pow(bit_size - 1);
+                    field <= max.into()
+                } else {
+                    let max = 2u128.pow(bit_size - 1) - 1;
+                    field <= max.into()
+                }
+            }
             NumericType::NativeField => true,
         }
     }
@@ -210,3 +229,36 @@ impl std::fmt::Display for NumericType {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_u8_is_within_limits() {
+        let u8 = NumericType::Unsigned { bit_size: 8 };
+        assert!(!u8.value_is_within_limits(
+            FieldElement::from(256_i128) - FieldElement::from(1_i128),
+            true
+        ));
+        assert!(u8.value_is_within_limits(FieldElement::from(0_i128), false));
+        assert!(u8.value_is_within_limits(FieldElement::from(255_i128), false));
+        assert!(!u8.value_is_within_limits(FieldElement::from(256_i128), false));
+    }
+
+    #[test]
+    fn test_i8_is_within_limits() {
+        let i8 = NumericType::Signed { bit_size: 8 };
+        assert!(!i8.value_is_within_limits(
+            FieldElement::from(256_i128) - FieldElement::from(129_i128),
+            true
+        ));
+        assert!(i8.value_is_within_limits(
+            FieldElement::from(256_i128) - FieldElement::from(128_i128),
+            true
+        ));
+        assert!(i8.value_is_within_limits(FieldElement::from(0_i128), false));
+        assert!(i8.value_is_within_limits(FieldElement::from(127_i128), false));
+        assert!(!i8.value_is_within_limits(FieldElement::from(128_i128), false));
+    }
+}

compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs

@@ -266,12 +266,13 @@ impl<'a> FunctionContext<'a> {
     pub(super) fn checked_numeric_constant(
         &mut self,
         value: impl Into<FieldElement>,
+        negative: bool,
         typ: Type,
     ) -> Result<ValueId, RuntimeError> {
         let value = value.into();
 
         if let Type::Numeric(typ) = typ {
-            if !typ.value_is_within_limits(value) {
+            if !typ.value_is_within_limits(value, negative) {
                 let call_stack = self.builder.get_call_stack();
                 return Err(RuntimeError::IntegerOutOfBounds { value, typ, call_stack });
             }

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

@@ -219,10 +219,10 @@ impl<'a> FunctionContext<'a> {
                     _ => unreachable!("ICE: unexpected slice literal type, got {}", array.typ),
                 })
             }
-            ast::Literal::Integer(value, typ, location) => {
+            ast::Literal::Integer(value, negative, typ, location) => {
                 self.builder.set_location(*location);
                 let typ = Self::convert_non_tuple_type(typ);
-                self.checked_numeric_constant(*value, typ).map(Into::into)
+                self.checked_numeric_constant(*value, *negative, typ).map(Into::into)
             }
             ast::Literal::Bool(value) => {
                 // Don't need to call checked_numeric_constant here since `value` can only be true or false

compiler/noirc_frontend/src/monomorphization/ast.rs

@@ -93,7 +93,7 @@ pub struct For {
 pub enum Literal {
     Array(ArrayLiteral),
     Slice(ArrayLiteral),
-    Integer(FieldElement, Type, Location),
+    Integer(FieldElement, /*sign*/ bool, Type, Location), // false for positive integer and true for negative
     Bool(bool),
     Unit,
     Str(String),

compiler/noirc_frontend/src/monomorphization/mod.rs

@@ -420,16 +420,16 @@ impl<'interner> Monomorphizer<'interner> {
 
                 if sign {
                     match typ {
-                        ast::Type::Field => Literal(Integer(-value, typ, location)),
+                        ast::Type::Field => Literal(Integer(-value, sign, typ, location)),
                         ast::Type::Integer(_, bit_size) => {
                             let bit_size: u32 = bit_size.into();
                             let base = 1_u128 << bit_size;
-                            Literal(Integer(FieldElement::from(base) - value, typ, location))
+                            Literal(Integer(FieldElement::from(base) - value, sign, typ, location))
                         }
                         _ => unreachable!("Integer literal must be numeric"),
                     }
                 } else {
-                    Literal(Integer(value, typ, location))
+                    Literal(Integer(value, sign, typ, location))
                 }
             }
             HirExpression::Literal(HirLiteral::Array(array)) => match array {
@@ -911,7 +911,7 @@ impl<'interner> Monomorphizer<'interner> {
                 let value = FieldElement::from(value as u128);
                 let location = self.interner.id_location(expr_id);
                 let typ = Self::convert_type(&typ, ident.location)?;
-                ast::Expression::Literal(ast::Literal::Integer(value, typ, location))
+                ast::Expression::Literal(ast::Literal::Integer(value, false, typ, location))
             }
         };
 
@@ -1268,7 +1268,9 @@ impl<'interner> Monomorphizer<'interner> {
                         let bits = (FieldElement::max_num_bits() as u128).into();
                         let typ =
                             ast::Type::Integer(Signedness::Unsigned, IntegerBitSize::SixtyFour);
-                        Some(ast::Expression::Literal(ast::Literal::Integer(bits, typ, location)))
+                        Some(ast::Expression::Literal(ast::Literal::Integer(
+                            bits, false, typ, location,
+                        )))
                     }
                     "zeroed" => {
                         let location = self.interner.expr_location(expr_id);
@@ -1308,7 +1310,12 @@ impl<'interner> Monomorphizer<'interner> {
         let int_type = Type::Integer(crate::ast::Signedness::Unsigned, arr_elem_bits);
 
         let bytes_as_expr = vecmap(bytes, |byte| {
-            Expression::Literal(Literal::Integer((byte as u128).into(), int_type.clone(), location))
+            Expression::Literal(Literal::Integer(
+                (byte as u128).into(),
+                false,
+                int_type.clone(),
+                location,
+            ))
         });
 
         let typ = Type::Slice(Box::new(int_type));
@@ -1595,7 +1602,7 @@ impl<'interner> Monomorphizer<'interner> {
         match typ {
             ast::Type::Field | ast::Type::Integer(..) => {
                 let typ = typ.clone();
-                ast::Expression::Literal(ast::Literal::Integer(0_u128.into(), typ, location))
+                ast::Expression::Literal(ast::Literal::Integer(0_u128.into(), false, typ, location))
             }
             ast::Type::Bool => ast::Expression::Literal(ast::Literal::Bool(false)),
             ast::Type::Unit => ast::Expression::Literal(ast::Literal::Unit),
@@ -1750,7 +1757,8 @@ impl<'interner> Monomorphizer<'interner> {
                 let operator =
                     if matches!(operator.kind, Less | Greater) { Equal } else { NotEqual };
 
-                let int_value = ast::Literal::Integer(ordering_value, ast::Type::Field, location);
+                let int_value =
+                    ast::Literal::Integer(ordering_value, false, ast::Type::Field, location);
                 let rhs = Box::new(ast::Expression::Literal(int_value));
                 let lhs = Box::new(ast::Expression::ExtractTupleField(Box::new(result), 0));
 

compiler/noirc_frontend/src/monomorphization/printer.rs

@@ -102,7 +102,7 @@ impl AstPrinter {
                 self.print_comma_separated(&array.contents, f)?;
                 write!(f, "]")
             }
-            super::ast::Literal::Integer(x, _, _) => x.fmt(f),
+            super::ast::Literal::Integer(x, _, _, _) => x.fmt(f),
             super::ast::Literal::Bool(x) => x.fmt(f),
             super::ast::Literal::Str(s) => s.fmt(f),
             super::ast::Literal::FmtStr(s, _, _) => {

test_programs/compile_failure/integer_literal_overflow/Nargo.toml → test_programs/compile_failure/signed_integer_literal_overflow/Nargo.toml

@@ -1,5 +1,5 @@
 [package]
-name = "integer_literal_overflow"
+name = "signed_integer_literal_overflow"
 type = "bin"
 authors = [""]
 [dependencies]

test_programs/compile_failure/signed_integer_literal_overflow/src/main.nr

@@ -0,0 +1,5 @@
+fn main() {
+    foo(128)
+}
+
+fn foo(_x: i8) {}

test_programs/compile_failure/signed_integer_literal_underflow/Nargo.toml

@@ -0,0 +1,5 @@
+[package]
+name = "signed_integer_literal_underflow"
+type = "bin"
+authors = [""]
+[dependencies]

test_programs/compile_failure/signed_integer_literal_underflow/src/main.nr

@@ -0,0 +1,5 @@
+fn main() {
+    foo(-129)
+}
+
+fn foo(_x: i8) {}

test_programs/compile_failure/unsigned_integer_literal_overflow/Nargo.toml

@@ -0,0 +1,5 @@
+[package]
+name = "unsigned_integer_literal_overflow"
+type = "bin"
+authors = [""]
+[dependencies]

test_programs/compile_failure/unsigned_integer_literal_underflow/Nargo.toml

@@ -0,0 +1,5 @@
+[package]
+name = "unsigned_integer_literal_underflow"
+type = "bin"
+authors = [""]
+[dependencies]

test_programs/compile_failure/unsigned_integer_literal_underflow/src/main.nr

@@ -0,0 +1,5 @@
+fn main() {
+    foo(-1)
+}
+
+fn foo(_x: u8) {}