diff --git a/noir/.gitrepo b/noir/.gitrepo index a1508c3c154..b2b95fbf96f 100644 --- a/noir/.gitrepo +++ b/noir/.gitrepo @@ -6,7 +6,7 @@ [subrepo] remote = https://github.com/noir-lang/noir branch = aztec-packages - commit = 7f22446f3940f8ef7ccae785a4b29835f2e869fe - parent = fd1f619f443916c172b6311aa71a84153145ef4d + commit = 0f38b229f5ed6024e5a0ffe8fd502264b282d3aa + parent = e28a6bf14cee1f7bade14337c74ecdcba1350899 method = merge cmdver = 0.4.6 diff --git a/noir/compiler/noirc_driver/tests/stdlib_warnings.rs b/noir/compiler/noirc_driver/tests/stdlib_warnings.rs new file mode 100644 index 00000000000..e9153ec2f99 --- /dev/null +++ b/noir/compiler/noirc_driver/tests/stdlib_warnings.rs @@ -0,0 +1,27 @@ +use std::path::Path; + +use noirc_driver::{file_manager_with_stdlib, prepare_crate, ErrorsAndWarnings}; +use noirc_frontend::hir::Context; + +#[test] +fn stdlib_does_not_produce_constant_warnings() -> Result<(), ErrorsAndWarnings> { + // We use a minimal source file so that if stdlib produces warnings then we can expect these warnings to _always_ + // be emitted. + let source = "fn main() {}"; + + let root = Path::new(""); + let file_name = Path::new("main.nr"); + let mut file_manager = file_manager_with_stdlib(root); + file_manager.add_file_with_source(file_name, source.to_owned()).expect( + "Adding source buffer to file manager should never fail when file manager is empty", + ); + + let mut context = Context::new(file_manager); + let root_crate_id = prepare_crate(&mut context, file_name); + + let ((), warnings) = noirc_driver::check_crate(&mut context, root_crate_id, false, false)?; + + assert_eq!(warnings, Vec::new(), "stdlib is producing warnings"); + + Ok(()) +} diff --git a/noir/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/noir/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 2871f149b41..35782ea85ae 100644 --- a/noir/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/noir/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -293,8 +293,9 @@ impl FunctionBuilder { if let Some(rhs_constant) = self.current_function.dfg.get_numeric_constant(rhs) { // Happy case is that we know precisely by how many bits the the integer will // increase: lhs_bit_size + rhs - let (rhs_bit_size_pow_2, overflows) = - 2_u128.overflowing_pow(rhs_constant.to_u128() as u32); + let bit_shift_size = rhs_constant.to_u128() as u32; + + let (rhs_bit_size_pow_2, overflows) = 2_u128.overflowing_pow(bit_shift_size); if overflows { assert!(bit_size < 128, "ICE - shift left with big integers are not supported"); if bit_size < 128 { @@ -303,7 +304,10 @@ impl FunctionBuilder { } } let pow = self.numeric_constant(FieldElement::from(rhs_bit_size_pow_2), typ); - (bit_size + (rhs_constant.to_u128() as u32), pow) + + let max_lhs_bits = self.current_function.dfg.get_value_max_num_bits(lhs); + + (max_lhs_bits + bit_shift_size, pow) } else { // we use a predicate to nullify the result in case of overflow let bit_size_var = diff --git a/noir/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/noir/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 9942a48a38a..870b5e602f1 100644 --- a/noir/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/noir/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -337,6 +337,25 @@ impl DataFlowGraph { self.values[value].get_type().clone() } + /// Returns the maximum possible number of bits that `value` can potentially be. + /// + /// Should `value` be a numeric constant then this function will return the exact number of bits required, + /// otherwise it will return the minimum number of bits based on type information. + pub(crate) fn get_value_max_num_bits(&self, value: ValueId) -> u32 { + match self[value] { + Value::Instruction { instruction, .. } => { + if let Instruction::Cast(original_value, _) = self[instruction] { + self.type_of_value(original_value).bit_size() + } else { + self.type_of_value(value).bit_size() + } + } + + Value::NumericConstant { constant, .. } => constant.num_bits(), + _ => self.type_of_value(value).bit_size(), + } + } + /// True if the type of this value is Type::Reference. /// Using this method over type_of_value avoids cloning the value's type. pub(crate) fn value_is_reference(&self, value: ValueId) -> bool { diff --git a/noir/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/noir/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index b34b667c31a..f1a2154d3a8 100644 --- a/noir/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/noir/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -335,29 +335,71 @@ impl<'a> FunctionContext<'a> { } } Type::Numeric(NumericType::Unsigned { bit_size }) => { - let op_name = match operator { - BinaryOpKind::Add => "add", - BinaryOpKind::Subtract => "subtract", - BinaryOpKind::Multiply => "multiply", - BinaryOpKind::ShiftLeft => "left shift", - _ => unreachable!("operator {} should not overflow", operator), - }; + let dfg = &self.builder.current_function.dfg; - if operator == BinaryOpKind::Multiply && bit_size == 1 { - result - } else if operator == BinaryOpKind::ShiftLeft - || operator == BinaryOpKind::ShiftRight - { - self.check_shift_overflow(result, rhs, bit_size, location, false) - } else { - let message = format!("attempt to {} with overflow", op_name); - self.builder.set_location(location).insert_range_check( - result, - bit_size, - Some(message), - ); - result + let max_lhs_bits = self.builder.current_function.dfg.get_value_max_num_bits(lhs); + let max_rhs_bits = self.builder.current_function.dfg.get_value_max_num_bits(rhs); + + match operator { + BinaryOpKind::Add => { + if std::cmp::max(max_lhs_bits, max_rhs_bits) < bit_size { + // `lhs` and `rhs` have both been casted up from smaller types and so cannot overflow. + return result; + } + + let message = "attempt to add with overflow".to_string(); + self.builder.set_location(location).insert_range_check( + result, + bit_size, + Some(message), + ); + } + BinaryOpKind::Subtract => { + if dfg.is_constant(lhs) && max_lhs_bits > max_rhs_bits { + // `lhs` is a fixed constant and `rhs` is restricted such that `lhs - rhs > 0` + // Note strict inequality as `rhs > lhs` while `max_lhs_bits == max_rhs_bits` is possible. + return result; + } + + let message = "attempt to subtract with overflow".to_string(); + self.builder.set_location(location).insert_range_check( + result, + bit_size, + Some(message), + ); + } + BinaryOpKind::Multiply => { + if bit_size == 1 || max_lhs_bits + max_rhs_bits <= bit_size { + // Either performing boolean multiplication (which cannot overflow), + // or `lhs` and `rhs` have both been casted up from smaller types and so cannot overflow. + return result; + } + + let message = "attempt to multiply with overflow".to_string(); + self.builder.set_location(location).insert_range_check( + result, + bit_size, + Some(message), + ); + } + BinaryOpKind::ShiftLeft => { + if let Some(rhs_const) = dfg.get_numeric_constant(rhs) { + let bit_shift_size = rhs_const.to_u128() as u32; + + if max_lhs_bits + bit_shift_size <= bit_size { + // `lhs` has been casted up from a smaller type such that shifting it by a constant + // `rhs` is known not to exceed the maximum bit size. + return result; + } + } + + self.check_shift_overflow(result, rhs, bit_size, location, false); + } + + _ => unreachable!("operator {} should not overflow", operator), } + + result } _ => result, }