From 2247814f951f5d33257cd123a3bdcba857c9b167 Mon Sep 17 00:00:00 2001 From: guipublic <47281315+guipublic@users.noreply.github.com> Date: Tue, 5 Nov 2024 10:31:24 +0100 Subject: [PATCH 1/4] fix: right shift is not a regular division (#6400) --- .../src/brillig/brillig_gen/brillig_block.rs | 156 +++++++----------- .../noirc_evaluator/src/brillig/brillig_ir.rs | 82 +++++++++ .../src/ssa/ir/instruction/binary.rs | 10 +- .../src/ssa/opt/remove_bit_shifts.rs | 33 ++-- .../bit_shifts_comptime/src/main.nr | 4 + .../bit_shifts_runtime/Prover.toml | 1 + .../bit_shifts_runtime/src/main.nr | 4 +- 7 files changed, 175 insertions(+), 115 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 23c802e3a14..0e9ebd8900d 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -2,6 +2,7 @@ use crate::brillig::brillig_ir::artifact::Label; use crate::brillig::brillig_ir::brillig_variable::{ type_to_heap_value_type, BrilligArray, BrilligVariable, SingleAddrVariable, }; + use crate::brillig::brillig_ir::registers::Stack; use crate::brillig::brillig_ir::{ BrilligBinaryOp, BrilligContext, ReservedRegisters, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, @@ -1202,7 +1203,7 @@ impl<'block> BrilligBlock<'block> { let brillig_binary_op = match binary.operator { BinaryOp::Div => { if is_signed { - self.convert_signed_division(left, right, result_variable); + self.brillig_context.convert_signed_division(left, right, result_variable); return; } else if is_field { BrilligBinaryOp::FieldDiv @@ -1234,7 +1235,14 @@ impl<'block> BrilligBlock<'block> { BinaryOp::Or => BrilligBinaryOp::Or, BinaryOp::Xor => BrilligBinaryOp::Xor, BinaryOp::Shl => BrilligBinaryOp::Shl, - BinaryOp::Shr => BrilligBinaryOp::Shr, + BinaryOp::Shr => { + if is_signed { + self.convert_signed_shr(left, right, result_variable); + return; + } else { + BrilligBinaryOp::Shr + } + } }; self.brillig_context.binary_instruction(left, right, result_variable, brillig_binary_op); @@ -1250,98 +1258,6 @@ impl<'block> BrilligBlock<'block> { ); } - /// Splits a two's complement signed integer in the sign bit and the absolute value. - /// For example, -6 i8 (11111010) is split to 00000110 (6, absolute value) and 1 (is_negative). - fn absolute_value( - &mut self, - num: SingleAddrVariable, - absolute_value: SingleAddrVariable, - result_is_negative: SingleAddrVariable, - ) { - let max_positive = self - .brillig_context - .make_constant_instruction(((1_u128 << (num.bit_size - 1)) - 1).into(), num.bit_size); - - // Compute if num is negative - self.brillig_context.binary_instruction( - max_positive, - num, - result_is_negative, - BrilligBinaryOp::LessThan, - ); - - // Two's complement of num - let zero = self.brillig_context.make_constant_instruction(0_usize.into(), num.bit_size); - let twos_complement = - SingleAddrVariable::new(self.brillig_context.allocate_register(), num.bit_size); - self.brillig_context.binary_instruction(zero, num, twos_complement, BrilligBinaryOp::Sub); - - // absolute_value = result_is_negative ? twos_complement : num - self.brillig_context.codegen_branch(result_is_negative.address, |ctx, is_negative| { - if is_negative { - ctx.mov_instruction(absolute_value.address, twos_complement.address); - } else { - ctx.mov_instruction(absolute_value.address, num.address); - } - }); - - self.brillig_context.deallocate_single_addr(zero); - self.brillig_context.deallocate_single_addr(max_positive); - self.brillig_context.deallocate_single_addr(twos_complement); - } - - fn convert_signed_division( - &mut self, - left: SingleAddrVariable, - right: SingleAddrVariable, - result: SingleAddrVariable, - ) { - let left_is_negative = SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); - let left_abs_value = - SingleAddrVariable::new(self.brillig_context.allocate_register(), left.bit_size); - - let right_is_negative = - SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); - let right_abs_value = - SingleAddrVariable::new(self.brillig_context.allocate_register(), right.bit_size); - - let result_is_negative = - SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); - - // Compute both absolute values - self.absolute_value(left, left_abs_value, left_is_negative); - self.absolute_value(right, right_abs_value, right_is_negative); - - // Perform the division on the absolute values - self.brillig_context.binary_instruction( - left_abs_value, - right_abs_value, - result, - BrilligBinaryOp::UnsignedDiv, - ); - - // Compute result sign - self.brillig_context.binary_instruction( - left_is_negative, - right_is_negative, - result_is_negative, - BrilligBinaryOp::Xor, - ); - - // If result has to be negative, perform two's complement - self.brillig_context.codegen_if(result_is_negative.address, |ctx| { - let zero = ctx.make_constant_instruction(0_usize.into(), result.bit_size); - ctx.binary_instruction(zero, result, result, BrilligBinaryOp::Sub); - ctx.deallocate_single_addr(zero); - }); - - self.brillig_context.deallocate_single_addr(left_is_negative); - self.brillig_context.deallocate_single_addr(left_abs_value); - self.brillig_context.deallocate_single_addr(right_is_negative); - self.brillig_context.deallocate_single_addr(right_abs_value); - self.brillig_context.deallocate_single_addr(result_is_negative); - } - fn convert_signed_modulo( &mut self, left: SingleAddrVariable, @@ -1354,7 +1270,7 @@ impl<'block> BrilligBlock<'block> { SingleAddrVariable::new(self.brillig_context.allocate_register(), left.bit_size); // i = left / right - self.convert_signed_division(left, right, scratch_var_i); + self.brillig_context.convert_signed_division(left, right, scratch_var_i); // j = i * right self.brillig_context.binary_instruction( @@ -1401,6 +1317,56 @@ impl<'block> BrilligBlock<'block> { self.brillig_context.deallocate_single_addr(bias); } + fn convert_signed_shr( + &mut self, + left: SingleAddrVariable, + right: SingleAddrVariable, + result: SingleAddrVariable, + ) { + // Check if left is negative + let left_is_negative = SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); + let max_positive = self + .brillig_context + .make_constant_instruction(((1_u128 << (left.bit_size - 1)) - 1).into(), left.bit_size); + self.brillig_context.binary_instruction( + max_positive, + left, + left_is_negative, + BrilligBinaryOp::LessThan, + ); + + self.brillig_context.codegen_branch(left_is_negative.address, |ctx, is_negative| { + if is_negative { + let one = ctx.make_constant_instruction(1_u128.into(), left.bit_size); + + // computes 2^right + let two = ctx.make_constant_instruction(2_u128.into(), left.bit_size); + let two_pow = ctx.make_constant_instruction(1_u128.into(), left.bit_size); + let right_u32 = SingleAddrVariable::new(ctx.allocate_register(), 32); + ctx.cast(right_u32, right); + let pow_body = |ctx: &mut BrilligContext<_, _>, _: SingleAddrVariable| { + ctx.binary_instruction(two_pow, two, two_pow, BrilligBinaryOp::Mul); + }; + ctx.codegen_for_loop(None, right_u32.address, None, pow_body); + + // Right shift using division on 1-complement + ctx.binary_instruction(left, one, result, BrilligBinaryOp::Add); + ctx.convert_signed_division(result, two_pow, result); + ctx.binary_instruction(result, one, result, BrilligBinaryOp::Sub); + + // Clean-up + ctx.deallocate_single_addr(one); + ctx.deallocate_single_addr(two); + ctx.deallocate_single_addr(two_pow); + ctx.deallocate_single_addr(right_u32); + } else { + ctx.binary_instruction(left, right, result, BrilligBinaryOp::Shr); + } + }); + + self.brillig_context.deallocate_single_addr(left_is_negative); + } + #[allow(clippy::too_many_arguments)] fn add_overflow_check( &mut self, diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index 3633700a795..66d51b2accc 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -25,6 +25,7 @@ mod entry_point; mod instructions; use artifact::Label; +use brillig_variable::SingleAddrVariable; pub(crate) use instructions::BrilligBinaryOp; use registers::{RegisterAllocator, ScratchSpace}; @@ -109,6 +110,87 @@ impl BrilligContext { can_call_procedures: true, } } + + /// Splits a two's complement signed integer in the sign bit and the absolute value. + /// For example, -6 i8 (11111010) is split to 00000110 (6, absolute value) and 1 (is_negative). + pub(crate) fn absolute_value( + &mut self, + num: SingleAddrVariable, + absolute_value: SingleAddrVariable, + result_is_negative: SingleAddrVariable, + ) { + let max_positive = self + .make_constant_instruction(((1_u128 << (num.bit_size - 1)) - 1).into(), num.bit_size); + + // Compute if num is negative + self.binary_instruction(max_positive, num, result_is_negative, BrilligBinaryOp::LessThan); + + // Two's complement of num + let zero = self.make_constant_instruction(0_usize.into(), num.bit_size); + let twos_complement = SingleAddrVariable::new(self.allocate_register(), num.bit_size); + self.binary_instruction(zero, num, twos_complement, BrilligBinaryOp::Sub); + + // absolute_value = result_is_negative ? twos_complement : num + self.codegen_branch(result_is_negative.address, |ctx, is_negative| { + if is_negative { + ctx.mov_instruction(absolute_value.address, twos_complement.address); + } else { + ctx.mov_instruction(absolute_value.address, num.address); + } + }); + + self.deallocate_single_addr(zero); + self.deallocate_single_addr(max_positive); + self.deallocate_single_addr(twos_complement); + } + + pub(crate) fn convert_signed_division( + &mut self, + left: SingleAddrVariable, + right: SingleAddrVariable, + result: SingleAddrVariable, + ) { + let left_is_negative = SingleAddrVariable::new(self.allocate_register(), 1); + let left_abs_value = SingleAddrVariable::new(self.allocate_register(), left.bit_size); + + let right_is_negative = SingleAddrVariable::new(self.allocate_register(), 1); + let right_abs_value = SingleAddrVariable::new(self.allocate_register(), right.bit_size); + + let result_is_negative = SingleAddrVariable::new(self.allocate_register(), 1); + + // Compute both absolute values + self.absolute_value(left, left_abs_value, left_is_negative); + self.absolute_value(right, right_abs_value, right_is_negative); + + // Perform the division on the absolute values + self.binary_instruction( + left_abs_value, + right_abs_value, + result, + BrilligBinaryOp::UnsignedDiv, + ); + + // Compute result sign + self.binary_instruction( + left_is_negative, + right_is_negative, + result_is_negative, + BrilligBinaryOp::Xor, + ); + + // If result has to be negative, perform two's complement + self.codegen_if(result_is_negative.address, |ctx| { + let zero = ctx.make_constant_instruction(0_usize.into(), result.bit_size); + ctx.binary_instruction(zero, result, result, BrilligBinaryOp::Sub); + ctx.deallocate_single_addr(zero); + }); + + self.deallocate_single_addr(left_is_negative); + self.deallocate_single_addr(left_abs_value); + self.deallocate_single_addr(right_is_negative); + self.deallocate_single_addr(right_abs_value); + self.deallocate_single_addr(result_is_negative); + } } /// Special brillig context to codegen compiler intrinsic shared procedures diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs index 03262be0a06..487370488b9 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs @@ -281,15 +281,7 @@ impl Binary { let zero = dfg.make_constant(FieldElement::zero(), operand_type); return SimplifyResult::SimplifiedTo(zero); } - - // `two_pow_rhs` is limited to be at most `2 ^ {operand_bitsize - 1}` so it fits in `operand_type`. - let two_pow_rhs = FieldElement::from(2u128).pow(&rhs_const); - let two_pow_rhs = dfg.make_constant(two_pow_rhs, operand_type); - return SimplifyResult::SimplifiedToInstruction(Instruction::binary( - BinaryOp::Div, - self.lhs, - two_pow_rhs, - )); + return SimplifyResult::None; } } }; diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs index 6f3f2fa14b7..cdbb1043232 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs @@ -145,6 +145,8 @@ impl Context<'_> { } /// Insert ssa instructions which computes lhs >> rhs by doing lhs/2^rhs + /// For negative signed integers, we do the division on the 1-complement representation of lhs, + /// before converting back the result to the 2-complement representation. pub(crate) fn insert_shift_right( &mut self, lhs: ValueId, @@ -153,16 +155,27 @@ impl Context<'_> { ) -> ValueId { let lhs_typ = self.function.dfg.type_of_value(lhs); let base = self.field_constant(FieldElement::from(2_u128)); - // we can safely cast to unsigned because overflow_checks prevent bit-shift with a negative value - let rhs_unsigned = self.insert_cast(rhs, Type::unsigned(bit_size)); - let pow = self.pow(base, rhs_unsigned); - // We need at least one more bit for the case where rhs == bit_size - let div_type = Type::unsigned(bit_size + 1); - let casted_lhs = self.insert_cast(lhs, div_type.clone()); - let casted_pow = self.insert_cast(pow, div_type); - let div_result = self.insert_binary(casted_lhs, BinaryOp::Div, casted_pow); - // We have to cast back to the original type - self.insert_cast(div_result, lhs_typ) + let pow = self.pow(base, rhs); + if lhs_typ.is_unsigned() { + // unsigned right bit shift is just a normal division + self.insert_binary(lhs, BinaryOp::Div, pow) + } else { + // Get the sign of the operand; positive signed operand will just do a division as well + let zero = self.numeric_constant(FieldElement::zero(), Type::signed(bit_size)); + let lhs_sign = self.insert_binary(lhs, BinaryOp::Lt, zero); + let lhs_sign_as_field = self.insert_cast(lhs_sign, Type::field()); + let lhs_as_field = self.insert_cast(lhs, Type::field()); + // For negative numbers, convert to 1-complement using wrapping addition of a + 1 + let one_complement = self.insert_binary(lhs_sign_as_field, BinaryOp::Add, lhs_as_field); + let one_complement = self.insert_truncate(one_complement, bit_size, bit_size + 1); + let one_complement = self.insert_cast(one_complement, Type::signed(bit_size)); + // Performs the division on the 1-complement (or the operand if positive) + let shifted_complement = self.insert_binary(one_complement, BinaryOp::Div, pow); + // Convert back to 2-complement representation if operand is negative + let lhs_sign_as_int = self.insert_cast(lhs_sign, lhs_typ); + let shifted = self.insert_binary(shifted_complement, BinaryOp::Sub, lhs_sign_as_int); + self.insert_truncate(shifted, bit_size, bit_size + 1) + } } /// Computes lhs^rhs via square&multiply, using the bits decomposition of rhs diff --git a/test_programs/execution_success/bit_shifts_comptime/src/main.nr b/test_programs/execution_success/bit_shifts_comptime/src/main.nr index 6d9736b6abb..a11dae1c716 100644 --- a/test_programs/execution_success/bit_shifts_comptime/src/main.nr +++ b/test_programs/execution_success/bit_shifts_comptime/src/main.nr @@ -15,6 +15,10 @@ fn main(x: u64) { assert(x << 63 == 0); assert_eq((1 as u64) << 32, 0x0100000000); + + //regression for 6201 + let a: i16 = -769; + assert_eq(a >> 3, -97); } fn regression_2250() { diff --git a/test_programs/execution_success/bit_shifts_runtime/Prover.toml b/test_programs/execution_success/bit_shifts_runtime/Prover.toml index 67bf6a6a234..7b56d36c9d5 100644 --- a/test_programs/execution_success/bit_shifts_runtime/Prover.toml +++ b/test_programs/execution_success/bit_shifts_runtime/Prover.toml @@ -1,2 +1,3 @@ x = 64 y = 1 +z = "-769" diff --git a/test_programs/execution_success/bit_shifts_runtime/src/main.nr b/test_programs/execution_success/bit_shifts_runtime/src/main.nr index 059bbe84dac..370bb699048 100644 --- a/test_programs/execution_success/bit_shifts_runtime/src/main.nr +++ b/test_programs/execution_success/bit_shifts_runtime/src/main.nr @@ -1,4 +1,4 @@ -fn main(x: u64, y: u8) { +fn main(x: u64, y: u8, z: i16) { // runtime shifts on compile-time known values assert(64 << y == 128); assert(64 >> y == 32); @@ -17,4 +17,6 @@ fn main(x: u64, y: u8) { assert(a << y == -2); assert(x >> (x as u8) == 0); + + assert_eq(z >> 3, -97); } From 66f15caba8466501256a98cee289c49376b27097 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 5 Nov 2024 12:43:16 +0000 Subject: [PATCH 2/4] fix(ssa): Resolve value IDs in terminator before comparing to array (#6448) --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 5 +++-- compiler/noirc_evaluator/src/ssa/opt/array_set.rs | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index d79916a9e11..2be9ffa9afa 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -74,8 +74,9 @@ pub(crate) struct DataFlowGraph { blocks: DenseMap, /// Debugging information about which `ValueId`s have had their underlying `Value` substituted - /// for that of another. This information is purely used for printing the SSA, and has no - /// material effect on the SSA itself. + /// for that of another. In theory this information is purely used for printing the SSA, + /// and has no material effect on the SSA itself, however in practice the IDs can get out of + /// sync and may need this resolution before they can be compared. #[serde(skip)] replaced_value_ids: HashMap, diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs index 7035345436e..7d9694d4872 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs @@ -105,13 +105,15 @@ impl<'f> Context<'f> { // If the array comes from a load we may potentially being mutating an array at a reference // that is loaded from by other values. let terminator = self.dfg[block_id].unwrap_terminator(); + // If we are in a return block we are not concerned about the array potentially being mutated again. let is_return_block = matches!(terminator, TerminatorInstruction::Return { .. }); // We also want to check that the array is not part of the terminator arguments, as this means it is used again. let mut array_in_terminator = false; terminator.for_each_value(|value| { - if value == array { + // The terminator can contain original IDs, while the SSA has replaced the array value IDs; we need to resolve to compare. + if !array_in_terminator && self.dfg.resolve(value) == array { array_in_terminator = true; } }); From 76f04236f8501bf8abe7df7bef01aa16012d1f70 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 5 Nov 2024 10:12:03 -0300 Subject: [PATCH 3/4] chore: split path and import lookups (#6430) Co-authored-by: Akosh Farkash Co-authored-by: Tom French --- compiler/noirc_frontend/src/ast/statement.rs | 5 - compiler/noirc_frontend/src/elaborator/mod.rs | 8 +- .../src/elaborator/path_resolution.rs | 347 +++++++++ .../noirc_frontend/src/elaborator/patterns.rs | 4 +- .../noirc_frontend/src/elaborator/scope.rs | 101 +-- .../noirc_frontend/src/elaborator/types.rs | 7 +- .../src/hir/def_collector/dc_crate.rs | 103 +-- .../src/hir/def_collector/dc_mod.rs | 1 - .../src/hir/resolution/import.rs | 706 ++++++------------ .../noirc_frontend/src/hir/resolution/mod.rs | 1 - .../src/hir/resolution/path_resolver.rs | 102 --- .../src/hir/resolution/visibility.rs | 46 +- compiler/noirc_frontend/src/locations.rs | 59 +- compiler/noirc_frontend/src/tests/imports.rs | 27 + tooling/lsp/src/attribute_reference_finder.rs | 44 +- tooling/lsp/src/requests/completion.rs | 15 +- tooling/lsp/src/requests/goto_definition.rs | 1 - tooling/lsp/src/requests/hover.rs | 1 - tooling/lsp/src/visibility.rs | 27 +- 19 files changed, 715 insertions(+), 890 deletions(-) create mode 100644 compiler/noirc_frontend/src/elaborator/path_resolution.rs delete mode 100644 compiler/noirc_frontend/src/hir/resolution/path_resolver.rs diff --git a/compiler/noirc_frontend/src/ast/statement.rs b/compiler/noirc_frontend/src/ast/statement.rs index 49f585894d9..7c173fbb900 100644 --- a/compiler/noirc_frontend/src/ast/statement.rs +++ b/compiler/noirc_frontend/src/ast/statement.rs @@ -445,11 +445,6 @@ impl Path { self.span } - pub fn first_segment(&self) -> PathSegment { - assert!(!self.segments.is_empty()); - self.segments.first().unwrap().clone() - } - pub fn last_segment(&self) -> PathSegment { assert!(!self.segments.is_empty()); self.segments.last().unwrap().clone() diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 5af435f9201..b58d552d866 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -4,9 +4,8 @@ use std::{ }; use crate::{ - ast::ItemVisibility, hir::resolution::import::PathResolutionItem, - hir_def::traits::ResolvedTraitBound, usage_tracker::UsageTracker, StructField, StructType, - TypeBindings, + ast::ItemVisibility, hir_def::traits::ResolvedTraitBound, usage_tracker::UsageTracker, + StructField, StructType, TypeBindings, }; use crate::{ ast::{ @@ -24,7 +23,6 @@ use crate::{ def_map::{DefMaps, ModuleData}, def_map::{LocalModuleId, ModuleId, MAIN_FUNCTION}, resolution::errors::ResolverError, - resolution::import::PathResolution, scope::ScopeForest as GenericScopeForest, type_check::{generics::TraitGenerics, TypeCheckError}, Context, @@ -47,6 +45,7 @@ use crate::{ mod comptime; mod expressions; mod lints; +mod path_resolution; mod patterns; mod scope; mod statements; @@ -58,6 +57,7 @@ mod unquote; use fm::FileId; use iter_extended::vecmap; use noirc_errors::{Location, Span, Spanned}; +use path_resolution::{PathResolution, PathResolutionItem}; use types::bind_ordered_generics; use self::traits::check_trait_impl_method_matches_declaration; diff --git a/compiler/noirc_frontend/src/elaborator/path_resolution.rs b/compiler/noirc_frontend/src/elaborator/path_resolution.rs new file mode 100644 index 00000000000..c4e536d2d5b --- /dev/null +++ b/compiler/noirc_frontend/src/elaborator/path_resolution.rs @@ -0,0 +1,347 @@ +use noirc_errors::{Location, Span}; + +use crate::ast::{Path, PathKind, UnresolvedType}; +use crate::hir::def_map::{ModuleDefId, ModuleId}; +use crate::hir::resolution::import::{resolve_path_kind, PathResolutionError}; + +use crate::hir::resolution::errors::ResolverError; +use crate::hir::resolution::visibility::item_in_module_is_visible; + +use crate::locations::ReferencesTracker; +use crate::node_interner::{FuncId, GlobalId, StructId, TraitId, TypeAliasId}; +use crate::Type; + +use super::types::SELF_TYPE_NAME; +use super::Elaborator; + +#[derive(Debug)] +pub(crate) struct PathResolution { + pub(crate) item: PathResolutionItem, + pub(crate) errors: Vec, +} + +/// All possible items that result from resolving a Path. +/// Note that this item doesn't include the last turbofish in a Path, +/// only intermediate ones, if any. +#[derive(Debug, Clone)] +pub enum PathResolutionItem { + Module(ModuleId), + Struct(StructId), + TypeAlias(TypeAliasId), + Trait(TraitId), + Global(GlobalId), + ModuleFunction(FuncId), + StructFunction(StructId, Option, FuncId), + TypeAliasFunction(TypeAliasId, Option, FuncId), + TraitFunction(TraitId, Option, FuncId), +} + +impl PathResolutionItem { + pub fn function_id(&self) -> Option { + match self { + PathResolutionItem::ModuleFunction(func_id) + | PathResolutionItem::StructFunction(_, _, func_id) + | PathResolutionItem::TypeAliasFunction(_, _, func_id) + | PathResolutionItem::TraitFunction(_, _, func_id) => Some(*func_id), + _ => None, + } + } + + pub fn module_id(&self) -> Option { + match self { + Self::Module(module_id) => Some(*module_id), + _ => None, + } + } + + pub fn description(&self) -> &'static str { + match self { + PathResolutionItem::Module(..) => "module", + PathResolutionItem::Struct(..) => "type", + PathResolutionItem::TypeAlias(..) => "type alias", + PathResolutionItem::Trait(..) => "trait", + PathResolutionItem::Global(..) => "global", + PathResolutionItem::ModuleFunction(..) + | PathResolutionItem::StructFunction(..) + | PathResolutionItem::TypeAliasFunction(..) + | PathResolutionItem::TraitFunction(..) => "function", + } + } +} + +#[derive(Debug, Clone)] +pub struct Turbofish { + pub generics: Vec, + pub span: Span, +} + +/// Any item that can appear before the last segment in a path. +#[derive(Debug)] +enum IntermediatePathResolutionItem { + Module(ModuleId), + Struct(StructId, Option), + TypeAlias(TypeAliasId, Option), + Trait(TraitId, Option), +} + +pub(crate) type PathResolutionResult = Result; + +impl<'context> Elaborator<'context> { + pub(super) fn resolve_path_or_error( + &mut self, + path: Path, + ) -> Result { + let path_resolution = self.resolve_path(path)?; + + for error in path_resolution.errors { + self.push_err(error); + } + + Ok(path_resolution.item) + } + + /// Resolves a path in the current module. + /// If the referenced name can't be found, `Err` will be returned. If it can be found, `Ok` + /// will be returned with a potential list of errors if, for example, one of the segments + /// is not accessible from the current module (e.g. because it's private). + pub(super) fn resolve_path(&mut self, mut path: Path) -> PathResolutionResult { + let mut module_id = self.module_id(); + + if path.kind == PathKind::Plain && path.first_name() == SELF_TYPE_NAME { + if let Some(Type::Struct(struct_type, _)) = &self.self_type { + let struct_type = struct_type.borrow(); + if path.segments.len() == 1 { + return Ok(PathResolution { + item: PathResolutionItem::Struct(struct_type.id), + errors: Vec::new(), + }); + } + + module_id = struct_type.id.module_id(); + path.segments.remove(0); + } + } + + self.resolve_path_in_module(path, module_id) + } + + /// Resolves a path in `current_module`. + /// `importing_module` is the module where the lookup originally started. + fn resolve_path_in_module( + &mut self, + path: Path, + importing_module: ModuleId, + ) -> PathResolutionResult { + let references_tracker = if self.interner.is_in_lsp_mode() { + Some(ReferencesTracker::new(self.interner, self.file)) + } else { + None + }; + let (path, module_id, _) = + resolve_path_kind(path, importing_module, self.def_maps, references_tracker)?; + self.resolve_name_in_module(path, module_id, importing_module) + } + + /// Resolves a Path assuming we are inside `starting_module`. + /// `importing_module` is the module where the lookup originally started. + fn resolve_name_in_module( + &mut self, + path: Path, + starting_module: ModuleId, + importing_module: ModuleId, + ) -> PathResolutionResult { + // There is a possibility that the import path is empty. In that case, early return. + if path.segments.is_empty() { + return Ok(PathResolution { + item: PathResolutionItem::Module(starting_module), + errors: Vec::new(), + }); + } + + let plain_or_crate = matches!(path.kind, PathKind::Plain | PathKind::Crate); + + // The current module and module ID as we resolve path segments + let mut current_module_id = starting_module; + let mut current_module = self.get_module(starting_module); + + let mut intermediate_item = IntermediatePathResolutionItem::Module(current_module_id); + + let first_segment = + &path.segments.first().expect("ice: could not fetch first segment").ident; + let mut current_ns = current_module.find_name(first_segment); + if current_ns.is_none() { + return Err(PathResolutionError::Unresolved(first_segment.clone())); + } + + self.usage_tracker.mark_as_referenced(current_module_id, first_segment); + + let mut errors = Vec::new(); + for (index, (last_segment, current_segment)) in + path.segments.iter().zip(path.segments.iter().skip(1)).enumerate() + { + let last_ident = &last_segment.ident; + let current_ident = ¤t_segment.ident; + let last_segment_generics = &last_segment.generics; + + let (typ, visibility) = match current_ns.types { + None => return Err(PathResolutionError::Unresolved(last_ident.clone())), + Some((typ, visibility, _)) => (typ, visibility), + }; + + let location = Location::new(last_segment.span, self.file); + self.interner.add_module_def_id_reference( + typ, + location, + last_segment.ident.is_self_type_name(), + ); + + (current_module_id, intermediate_item) = match typ { + ModuleDefId::ModuleId(id) => { + if last_segment_generics.is_some() { + errors.push(PathResolutionError::TurbofishNotAllowedOnItem { + item: format!("module `{last_ident}`"), + span: last_segment.turbofish_span(), + }); + } + + (id, IntermediatePathResolutionItem::Module(id)) + } + ModuleDefId::TypeId(id) => ( + id.module_id(), + IntermediatePathResolutionItem::Struct( + id, + last_segment_generics.as_ref().map(|generics| Turbofish { + generics: generics.clone(), + span: last_segment.turbofish_span(), + }), + ), + ), + ModuleDefId::TypeAliasId(id) => { + let type_alias = self.interner.get_type_alias(id); + let type_alias = type_alias.borrow(); + + let module_id = match &type_alias.typ { + Type::Struct(struct_id, _generics) => struct_id.borrow().id.module_id(), + Type::Error => { + return Err(PathResolutionError::Unresolved(last_ident.clone())); + } + _ => { + // For now we only allow type aliases that point to structs. + // The more general case is captured here: https://github.com/noir-lang/noir/issues/6398 + panic!("Type alias in path not pointing to struct not yet supported") + } + }; + + ( + module_id, + IntermediatePathResolutionItem::TypeAlias( + id, + last_segment_generics.as_ref().map(|generics| Turbofish { + generics: generics.clone(), + span: last_segment.turbofish_span(), + }), + ), + ) + } + ModuleDefId::TraitId(id) => ( + id.0, + IntermediatePathResolutionItem::Trait( + id, + last_segment_generics.as_ref().map(|generics| Turbofish { + generics: generics.clone(), + span: last_segment.turbofish_span(), + }), + ), + ), + ModuleDefId::FunctionId(_) => panic!("functions cannot be in the type namespace"), + ModuleDefId::GlobalId(_) => panic!("globals cannot be in the type namespace"), + }; + + // If the path is plain or crate, the first segment will always refer to + // something that's visible from the current module. + if !((plain_or_crate && index == 0) + || item_in_module_is_visible( + self.def_maps, + importing_module, + current_module_id, + visibility, + )) + { + errors.push(PathResolutionError::Private(last_ident.clone())); + } + + current_module = self.get_module(current_module_id); + + // Check if namespace + let found_ns = current_module.find_name(current_ident); + if found_ns.is_none() { + return Err(PathResolutionError::Unresolved(current_ident.clone())); + } + + self.usage_tracker.mark_as_referenced(current_module_id, current_ident); + + current_ns = found_ns; + } + + let (module_def_id, visibility, _) = + current_ns.values.or(current_ns.types).expect("Found empty namespace"); + + let name = path.last_ident(); + let is_self_type = name.is_self_type_name(); + let location = Location::new(name.span(), self.file); + self.interner.add_module_def_id_reference(module_def_id, location, is_self_type); + + let item = merge_intermediate_path_resolution_item_with_module_def_id( + intermediate_item, + module_def_id, + ); + + if !(self.self_type_module_id() == Some(current_module_id) + || item_in_module_is_visible( + self.def_maps, + importing_module, + current_module_id, + visibility, + )) + { + errors.push(PathResolutionError::Private(name.clone())); + } + + Ok(PathResolution { item, errors }) + } + + fn self_type_module_id(&self) -> Option { + if let Some(Type::Struct(struct_type, _)) = &self.self_type { + Some(struct_type.borrow().id.module_id()) + } else { + None + } + } +} + +fn merge_intermediate_path_resolution_item_with_module_def_id( + intermediate_item: IntermediatePathResolutionItem, + module_def_id: ModuleDefId, +) -> PathResolutionItem { + match module_def_id { + ModuleDefId::ModuleId(module_id) => PathResolutionItem::Module(module_id), + ModuleDefId::TypeId(struct_id) => PathResolutionItem::Struct(struct_id), + ModuleDefId::TypeAliasId(type_alias_id) => PathResolutionItem::TypeAlias(type_alias_id), + ModuleDefId::TraitId(trait_id) => PathResolutionItem::Trait(trait_id), + ModuleDefId::GlobalId(global_id) => PathResolutionItem::Global(global_id), + ModuleDefId::FunctionId(func_id) => match intermediate_item { + IntermediatePathResolutionItem::Module(_) => { + PathResolutionItem::ModuleFunction(func_id) + } + IntermediatePathResolutionItem::Struct(struct_id, generics) => { + PathResolutionItem::StructFunction(struct_id, generics, func_id) + } + IntermediatePathResolutionItem::TypeAlias(alias_id, generics) => { + PathResolutionItem::TypeAliasFunction(alias_id, generics, func_id) + } + IntermediatePathResolutionItem::Trait(trait_id, generics) => { + PathResolutionItem::TraitFunction(trait_id, generics, func_id) + } + }, + } +} diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 9e60adcbc6f..3928362db11 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -9,7 +9,7 @@ use crate::{ }, hir::{ def_collector::dc_crate::CompilationError, - resolution::{errors::ResolverError, import::PathResolutionItem}, + resolution::errors::ResolverError, type_check::{Source, TypeCheckError}, }, hir_def::{ @@ -20,7 +20,7 @@ use crate::{ Kind, Shared, StructType, Type, TypeAlias, TypeBindings, }; -use super::{Elaborator, ResolverMeta}; +use super::{path_resolution::PathResolutionItem, Elaborator, ResolverMeta}; impl<'context> Elaborator<'context> { pub(super) fn elaborate_pattern( diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs index 33e97c1df22..fe01e3cb7f3 100644 --- a/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/compiler/noirc_frontend/src/elaborator/scope.rs @@ -1,9 +1,8 @@ -use noirc_errors::{Location, Spanned}; +use noirc_errors::Spanned; -use crate::ast::{Ident, Path, PathKind, ERROR_IDENT}; +use crate::ast::{Ident, Path, ERROR_IDENT}; use crate::hir::def_map::{LocalModuleId, ModuleId}; -use crate::hir::resolution::import::{PathResolution, PathResolutionItem, PathResolutionResult}; -use crate::hir::resolution::path_resolver::{PathResolver, StandardPathResolver}; + use crate::hir::scope::{Scope as GenericScope, ScopeTree as GenericScopeTree}; use crate::{ hir::resolution::errors::ResolverError, @@ -16,6 +15,7 @@ use crate::{ }; use crate::{Type, TypeAlias}; +use super::path_resolution::PathResolutionItem; use super::types::SELF_TYPE_NAME; use super::{Elaborator, ResolverMeta}; @@ -37,99 +37,6 @@ impl<'context> Elaborator<'context> { current_module } - pub(super) fn resolve_path_or_error( - &mut self, - path: Path, - ) -> Result { - let path_resolution = self.resolve_path(path)?; - - for error in path_resolution.errors { - self.push_err(error); - } - - Ok(path_resolution.item) - } - - pub(super) fn resolve_path(&mut self, path: Path) -> PathResolutionResult { - let mut module_id = self.module_id(); - let mut path = path; - - if path.kind == PathKind::Plain && path.first_name() == SELF_TYPE_NAME { - if let Some(Type::Struct(struct_type, _)) = &self.self_type { - let struct_type = struct_type.borrow(); - if path.segments.len() == 1 { - return Ok(PathResolution { - item: PathResolutionItem::Struct(struct_type.id), - errors: Vec::new(), - }); - } - - module_id = struct_type.id.module_id(); - path = Path { - segments: path.segments[1..].to_vec(), - kind: PathKind::Plain, - span: path.span(), - }; - } - } - - self.resolve_path_in_module(path, module_id) - } - - fn resolve_path_in_module(&mut self, path: Path, module_id: ModuleId) -> PathResolutionResult { - let self_type_module_id = if let Some(Type::Struct(struct_type, _)) = &self.self_type { - Some(struct_type.borrow().id.module_id()) - } else { - None - }; - - let resolver = StandardPathResolver::new(module_id, self_type_module_id); - - if !self.interner.lsp_mode { - return resolver.resolve( - self.interner, - self.def_maps, - path, - self.usage_tracker, - &mut None, - ); - } - - let last_segment = path.last_ident(); - let location = Location::new(last_segment.span(), self.file); - let is_self_type_name = last_segment.is_self_type_name(); - - let mut references: Vec<_> = Vec::new(); - let path_resolution = resolver.resolve( - self.interner, - self.def_maps, - path.clone(), - self.usage_tracker, - &mut Some(&mut references), - ); - - for (referenced, segment) in references.iter().zip(path.segments) { - self.interner.add_reference( - *referenced, - Location::new(segment.ident.span(), self.file), - segment.ident.is_self_type_name(), - ); - } - - let path_resolution = match path_resolution { - Ok(path_resolution) => path_resolution, - Err(err) => return Err(err), - }; - - self.interner.add_path_resolution_kind_reference( - path_resolution.item.clone(), - location, - is_self_type_name, - ); - - Ok(path_resolution) - } - pub(super) fn get_struct(&self, type_id: StructId) -> Shared { self.interner.get_struct(type_id) } diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index c8a16a6cd9b..ce60f8329fd 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -14,10 +14,7 @@ use crate::{ hir::{ comptime::{Interpreter, Value}, def_collector::dc_crate::CompilationError, - resolution::{ - errors::ResolverError, - import::{PathResolutionError, PathResolutionItem}, - }, + resolution::{errors::ResolverError, import::PathResolutionError}, type_check::{ generics::{Generic, TraitGenerics}, NoMatchingImplFoundError, Source, TypeCheckError, @@ -41,7 +38,7 @@ use crate::{ UnificationError, }; -use super::{lints, Elaborator}; +use super::{lints, path_resolution::PathResolutionItem, Elaborator}; pub const SELF_TYPE_NAME: &str = "Self"; pub const WILDCARD_TYPE: &str = "_"; diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 9917207d217..51e62599b05 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -5,13 +5,13 @@ use crate::graph::CrateId; use crate::hir::comptime::InterpreterError; use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}; use crate::hir::resolution::errors::ResolverError; -use crate::hir::resolution::path_resolver; use crate::hir::type_check::TypeCheckError; +use crate::locations::ReferencesTracker; use crate::token::SecondaryAttribute; use crate::usage_tracker::UnusedItem; use crate::{Generics, Type}; -use crate::hir::resolution::import::{resolve_import, ImportDirective, PathResolution}; +use crate::hir::resolution::import::{resolve_import, ImportDirective}; use crate::hir::Context; use crate::ast::Expression; @@ -347,45 +347,23 @@ impl DefCollector { // Resolve unresolved imports collected from the crate, one by one. for collected_import in std::mem::take(&mut def_collector.imports) { - let module_id = collected_import.module_id; - let resolved_import = if context.def_interner.lsp_mode { - let mut references: Vec = Vec::new(); - let resolved_import = resolve_import( - crate_id, - &collected_import, - &context.def_interner, - &context.def_maps, - &mut context.usage_tracker, - &mut Some(&mut references), - ); - - let current_def_map = context.def_maps.get(&crate_id).unwrap(); - let file_id = current_def_map.file_id(module_id); + let local_module_id = collected_import.module_id; + let module_id = ModuleId { krate: crate_id, local_id: local_module_id }; + let current_def_map = context.def_maps.get(&crate_id).unwrap(); + let file_id = current_def_map.file_id(local_module_id); + + let resolved_import = resolve_import( + collected_import.path.clone(), + module_id, + &context.def_maps, + &mut context.usage_tracker, + Some(ReferencesTracker::new(&mut context.def_interner, file_id)), + ); - for (referenced, segment) in references.iter().zip(&collected_import.path.segments) - { - context.def_interner.add_reference( - *referenced, - Location::new(segment.ident.span(), file_id), - false, - ); - } - - resolved_import - } else { - resolve_import( - crate_id, - &collected_import, - &context.def_interner, - &context.def_maps, - &mut context.usage_tracker, - &mut None, - ) - }; match resolved_import { Ok(resolved_import) => { let current_def_map = context.def_maps.get_mut(&crate_id).unwrap(); - let file_id = current_def_map.file_id(module_id); + let file_id = current_def_map.file_id(local_module_id); let has_path_resolution_error = !resolved_import.errors.is_empty(); for error in resolved_import.errors { @@ -396,11 +374,11 @@ impl DefCollector { } // Populate module namespaces according to the imports used - let name = resolved_import.name; + let name = collected_import.name(); let visibility = collected_import.visibility; - let is_prelude = resolved_import.is_prelude; + let is_prelude = collected_import.is_prelude; for (module_def_id, item_visibility, _) in - resolved_import.resolved_namespace.iter_items() + resolved_import.namespace.iter_items() { if item_visibility < visibility { errors.push(( @@ -414,25 +392,26 @@ impl DefCollector { } let visibility = visibility.min(item_visibility); - let result = current_def_map.modules[resolved_import.module_scope.0] - .import(name.clone(), visibility, module_def_id, is_prelude); + let result = current_def_map.modules[local_module_id.0].import( + name.clone(), + visibility, + module_def_id, + is_prelude, + ); // If we error on path resolution don't also say it's unused (in case it ends up being unused) if !has_path_resolution_error { - let module_id = ModuleId { - krate: crate_id, - local_id: resolved_import.module_scope, - }; + let defining_module = + ModuleId { krate: crate_id, local_id: local_module_id }; + context.usage_tracker.add_unused_item( - module_id, + defining_module, name.clone(), UnusedItem::Import, visibility, ); if visibility != ItemVisibility::Private { - let local_id = resolved_import.module_scope; - let defining_module = ModuleId { krate: crate_id, local_id }; context.def_interner.register_name_for_auto_import( name.to_string(), module_def_id, @@ -442,14 +421,6 @@ impl DefCollector { } } - let last_segment = collected_import.path.last_ident(); - - add_import_reference( - module_def_id, - &last_segment, - &mut context.def_interner, - file_id, - ); if let Some(ref alias) = collected_import.alias { add_import_reference( module_def_id, @@ -558,17 +529,18 @@ fn inject_prelude( span: Span::default(), }; - if let Ok(PathResolution { item, errors }) = path_resolver::resolve_path( - &context.def_interner, - &context.def_maps, - ModuleId { krate: crate_id, local_id: crate_root }, - None, + if let Ok(resolved_import) = resolve_import( path, + ModuleId { krate: crate_id, local_id: crate_root }, + &context.def_maps, &mut context.usage_tracker, - &mut None, + None, // references tracker ) { - assert!(errors.is_empty(), "Tried to add private item to prelude"); - let module_id = item.module_id().expect("std::prelude should be a module"); + assert!(resolved_import.errors.is_empty(), "Tried to add private item to prelude"); + + let (module_def_id, _, _) = + resolved_import.namespace.types.expect("couldn't resolve std::prelude"); + let module_id = module_def_id.as_module().expect("std::prelude should be a module"); let prelude = context.module(module_id).scope().names(); for path in prelude { @@ -580,7 +552,6 @@ fn inject_prelude( ImportDirective { visibility: ItemVisibility::Private, module_id: crate_root, - self_type_module_id: None, path: Path { segments, kind: PathKind::Plain, span: Span::default() }, alias: None, is_prelude: true, diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 94c091f9791..2d15d4927c9 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -81,7 +81,6 @@ pub fn collect_defs( collector.def_collector.imports.push(ImportDirective { visibility: import.visibility, module_id: collector.module_id, - self_type_module_id: None, path: import.path, alias: import.alias, is_prelude: false, diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index 7afb056cbaf..376b85bfbd9 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -4,110 +4,37 @@ use thiserror::Error; use crate::graph::CrateId; use crate::hir::def_collector::dc_crate::CompilationError; -use crate::node_interner::{ - FuncId, GlobalId, NodeInterner, ReferenceId, StructId, TraitId, TypeAliasId, -}; +use crate::locations::ReferencesTracker; use crate::usage_tracker::UsageTracker; -use crate::Type; use std::collections::BTreeMap; -use crate::ast::{Ident, ItemVisibility, Path, PathKind, PathSegment, UnresolvedType}; -use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleDefId, ModuleId, PerNs}; +use crate::ast::{Ident, ItemVisibility, Path, PathKind}; +use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleData, ModuleDefId, ModuleId, PerNs}; use super::errors::ResolverError; -use super::visibility::can_reference_module_id; +use super::visibility::item_in_module_is_visible; #[derive(Debug, Clone)] pub struct ImportDirective { pub visibility: ItemVisibility, pub module_id: LocalModuleId, - pub self_type_module_id: Option, pub path: Path, pub alias: Option, pub is_prelude: bool, } -struct NamespaceResolution { - module_id: ModuleId, - item: PathResolutionItem, - namespace: PerNs, - errors: Vec, -} - -type NamespaceResolutionResult = Result; - -#[derive(Debug)] -pub struct PathResolution { - pub item: PathResolutionItem, - pub errors: Vec, -} - -/// All possible items that result from resolving a Path. -/// Note that this item doesn't include the last turbofish in a Path, -/// only intermediate ones, if any. -#[derive(Debug, Clone)] -pub enum PathResolutionItem { - Module(ModuleId), - Struct(StructId), - TypeAlias(TypeAliasId), - Trait(TraitId), - Global(GlobalId), - ModuleFunction(FuncId), - StructFunction(StructId, Option, FuncId), - TypeAliasFunction(TypeAliasId, Option, FuncId), - TraitFunction(TraitId, Option, FuncId), -} - -impl PathResolutionItem { - pub fn function_id(&self) -> Option { - match self { - PathResolutionItem::ModuleFunction(func_id) - | PathResolutionItem::StructFunction(_, _, func_id) - | PathResolutionItem::TypeAliasFunction(_, _, func_id) - | PathResolutionItem::TraitFunction(_, _, func_id) => Some(*func_id), - _ => None, - } - } - - pub fn module_id(&self) -> Option { - match self { - Self::Module(module_id) => Some(*module_id), - _ => None, - } - } - - pub fn description(&self) -> &'static str { - match self { - PathResolutionItem::Module(..) => "module", - PathResolutionItem::Struct(..) => "type", - PathResolutionItem::TypeAlias(..) => "type alias", - PathResolutionItem::Trait(..) => "trait", - PathResolutionItem::Global(..) => "global", - PathResolutionItem::ModuleFunction(..) - | PathResolutionItem::StructFunction(..) - | PathResolutionItem::TypeAliasFunction(..) - | PathResolutionItem::TraitFunction(..) => "function", +impl ImportDirective { + /// Returns the name that's brought into scope: either the alias or the last segment of the path + pub fn name(&self) -> Ident { + match &self.alias { + None => self.path.last_ident(), + Some(ident) => ident.clone(), } } } -#[derive(Debug, Clone)] -pub struct Turbofish { - pub generics: Vec, - pub span: Span, -} - -/// Any item that can appear before the last segment in a path. -#[derive(Debug)] -enum IntermediatePathResolutionItem { - Module(ModuleId), - Struct(StructId, Option), - TypeAlias(TypeAliasId, Option), - Trait(TraitId, Option), -} - -pub(crate) type PathResolutionResult = Result; +type ImportResolutionResult = Result; #[derive(Debug, Clone, PartialEq, Eq, Error)] pub enum PathResolutionError { @@ -119,19 +46,15 @@ pub enum PathResolutionError { NoSuper(Span), #[error("turbofish (`::<_>`) not allowed on {item}")] TurbofishNotAllowedOnItem { item: String, span: Span }, + #[error("{ident} is a {kind}, not a module")] + NotAModule { ident: Ident, kind: &'static str }, } #[derive(Debug)] pub struct ResolvedImport { - // name of the namespace, either last path segment or an alias - pub name: Ident, // The symbol which we have resolved to - pub resolved_namespace: PerNs, - // The item which we have resolved to - pub item: PathResolutionItem, + pub namespace: PerNs, // The module which we must add the resolved namespace to - pub module_scope: LocalModuleId, - pub is_prelude: bool, pub errors: Vec, } @@ -159,452 +82,255 @@ impl<'a> From<&'a PathResolutionError> for CustomDiagnostic { PathResolutionError::TurbofishNotAllowedOnItem { item: _, span } => { CustomDiagnostic::simple_error(error.to_string(), String::new(), *span) } + PathResolutionError::NotAModule { ident, kind: _ } => { + CustomDiagnostic::simple_error(error.to_string(), String::new(), ident.span()) + } } } } +/// Resolves a Path in a `use` statement, assuming it's located in `importing_module`. +/// +/// If the imported name can't be found, `Err` will be returned. If it can be found, `Ok` +/// will be returned with a potential list of errors if, for example, one of the segments +/// is not accessible from the importing module (e.g. because it's private). pub fn resolve_import( - crate_id: CrateId, - import_directive: &ImportDirective, - interner: &NodeInterner, + path: Path, + importing_module: ModuleId, def_maps: &BTreeMap, usage_tracker: &mut UsageTracker, - path_references: &mut Option<&mut Vec>, -) -> Result { - let module_scope = import_directive.module_id; - let NamespaceResolution { - module_id: resolved_module, - item, - namespace: resolved_namespace, - mut errors, - } = resolve_path_to_ns( - import_directive, - crate_id, - crate_id, - interner, - def_maps, - usage_tracker, - path_references, - )?; - - let name = resolve_path_name(import_directive); - - let visibility = resolved_namespace - .values - .or(resolved_namespace.types) - .map(|(_, visibility, _)| visibility) - .expect("Found empty namespace"); - - if !(import_directive.self_type_module_id == Some(resolved_module) - || can_reference_module_id( - def_maps, - crate_id, - import_directive.module_id, - resolved_module, - visibility, - )) - { - errors.push(PathResolutionError::Private(name.clone())); - } - - Ok(ResolvedImport { - name, - resolved_namespace, - item, - module_scope, - is_prelude: import_directive.is_prelude, - errors, - }) + references_tracker: Option, +) -> ImportResolutionResult { + let (path, module_id, references_tracker) = + resolve_path_kind(path, importing_module, def_maps, references_tracker)?; + let mut solver = + ImportSolver::new(importing_module, def_maps, usage_tracker, references_tracker); + solver.resolve_name_in_module(path, module_id) } -fn resolve_path_to_ns( - import_directive: &ImportDirective, - crate_id: CrateId, - importing_crate: CrateId, - interner: &NodeInterner, +/// Given a Path and a ModuleId it's being used in, this function returns a plain Path +/// and a ModuleId where that plain Path should be resolved. That is, this method will +/// resolve the Path kind and translate it to a plain path. +/// +/// The third value in the tuple is a reference tracker that must be passed to this +/// method, which is used in case the path kind is `dep`: the segment after `dep` +/// will be linked to the root module of the external dependency. +pub fn resolve_path_kind<'r>( + path: Path, + importing_module: ModuleId, def_maps: &BTreeMap, - usage_tracker: &mut UsageTracker, - path_references: &mut Option<&mut Vec>, -) -> NamespaceResolutionResult { - let import_path = &import_directive.path.segments; - - match import_directive.path.kind { - crate::ast::PathKind::Crate => { - // Resolve from the root of the crate - resolve_path_from_crate_root( - crate_id, - importing_crate, - import_path, - interner, - def_maps, - usage_tracker, - path_references, - ) + references_tracker: Option>, +) -> Result<(Path, ModuleId, Option>), PathResolutionError> { + let mut solver = + PathResolutionTargetResolver { importing_module, def_maps, references_tracker }; + let (path, module_id) = solver.resolve(path)?; + Ok((path, module_id, solver.references_tracker)) +} + +struct PathResolutionTargetResolver<'def_maps, 'references_tracker> { + importing_module: ModuleId, + def_maps: &'def_maps BTreeMap, + references_tracker: Option>, +} + +impl<'def_maps, 'references_tracker> PathResolutionTargetResolver<'def_maps, 'references_tracker> { + fn resolve(&mut self, path: Path) -> Result<(Path, ModuleId), PathResolutionError> { + match path.kind { + PathKind::Crate => self.resolve_crate_path(path), + PathKind::Plain => self.resolve_plain_path(path, self.importing_module), + PathKind::Dep => self.resolve_dep_path(path), + PathKind::Super => self.resolve_super_path(path), } - crate::ast::PathKind::Plain => { - // There is a possibility that the import path is empty - // In that case, early return - if import_path.is_empty() { - return resolve_name_in_module( - crate_id, - importing_crate, - import_path, - import_directive.module_id, - interner, - def_maps, - true, // plain or crate - usage_tracker, - path_references, - ); - } + } - let def_map = &def_maps[&crate_id]; - let current_mod_id = ModuleId { krate: crate_id, local_id: import_directive.module_id }; - let current_mod = &def_map.modules[current_mod_id.local_id.0]; - let first_segment = - &import_path.first().expect("ice: could not fetch first segment").ident; - if current_mod.find_name(first_segment).is_none() { - // Resolve externally when first segment is unresolved - return resolve_external_dep( - crate_id, - // def_map, - import_directive, - interner, - def_maps, - usage_tracker, - path_references, - importing_crate, - ); - } + fn resolve_crate_path(&mut self, path: Path) -> Result<(Path, ModuleId), PathResolutionError> { + let root_module = self.def_maps[&self.importing_module.krate].root; + let current_module = ModuleId { krate: self.importing_module.krate, local_id: root_module }; + Ok((path, current_module)) + } - resolve_name_in_module( - crate_id, - importing_crate, - import_path, - import_directive.module_id, - interner, - def_maps, - true, // plain or crate - usage_tracker, - path_references, - ) + fn resolve_plain_path( + &mut self, + path: Path, + current_module: ModuleId, + ) -> Result<(Path, ModuleId), PathResolutionError> { + // There is a possibility that the import path is empty. In that case, early return. + // This happens on import statements such as `use crate` or `use std`. + if path.segments.is_empty() { + return Ok((path, current_module)); } - crate::ast::PathKind::Dep => resolve_external_dep( - crate_id, - import_directive, - interner, - def_maps, - usage_tracker, - path_references, - importing_crate, - ), - - crate::ast::PathKind::Super => { - if let Some(parent_module_id) = - def_maps[&crate_id].modules[import_directive.module_id.0].parent - { - resolve_name_in_module( - crate_id, - importing_crate, - import_path, - parent_module_id, - interner, - def_maps, - false, // plain or crate - usage_tracker, - path_references, - ) - } else { - let span_start = import_directive.path.span().start(); - let span = Span::from(span_start..span_start + 5); // 5 == "super".len() - Err(PathResolutionError::NoSuper(span)) - } + let first_segment = + &path.segments.first().expect("ice: could not fetch first segment").ident; + if get_module(self.def_maps, current_module).find_name(first_segment).is_none() { + // Resolve externally when first segment is unresolved + return self.resolve_dep_path(path); + } + + Ok((path, current_module)) + } + + fn resolve_dep_path( + &mut self, + mut path: Path, + ) -> Result<(Path, ModuleId), PathResolutionError> { + // Use extern_prelude to get the dep + let current_def_map = &self.def_maps[&self.importing_module.krate]; + + // Fetch the root module from the prelude + let crate_name = &path.segments.first().unwrap().ident; + let dep_module = current_def_map + .extern_prelude + .get(&crate_name.0.contents) + .ok_or_else(|| PathResolutionError::Unresolved(crate_name.to_owned()))?; + + if let Some(references_tracker) = &mut self.references_tracker { + let span = crate_name.span(); + references_tracker.add_reference(ModuleDefId::ModuleId(*dep_module), span, false); } + + // Now the path can be solved starting from the second segment as a plain path + path.kind = PathKind::Plain; + path.segments.remove(0); + + Ok((path, *dep_module)) + } + + fn resolve_super_path(&mut self, path: Path) -> Result<(Path, ModuleId), PathResolutionError> { + let Some(parent_module_id) = get_module(self.def_maps, self.importing_module).parent else { + let span_start = path.span.start(); + let span = Span::from(span_start..span_start + 5); // 5 == "super".len() + return Err(PathResolutionError::NoSuper(span)); + }; + + let current_module = + ModuleId { krate: self.importing_module.krate, local_id: parent_module_id }; + Ok((path, current_module)) } } -fn resolve_path_from_crate_root( - crate_id: CrateId, - importing_crate: CrateId, - import_path: &[PathSegment], - interner: &NodeInterner, - def_maps: &BTreeMap, - usage_tracker: &mut UsageTracker, - path_references: &mut Option<&mut Vec>, -) -> NamespaceResolutionResult { - let starting_mod = def_maps[&crate_id].root; - resolve_name_in_module( - crate_id, - importing_crate, - import_path, - starting_mod, - interner, - def_maps, - true, // plain or crate - usage_tracker, - path_references, - ) +struct ImportSolver<'def_maps, 'usage_tracker, 'references_tracker> { + importing_module: ModuleId, + def_maps: &'def_maps BTreeMap, + usage_tracker: &'usage_tracker mut UsageTracker, + references_tracker: Option>, } -#[allow(clippy::too_many_arguments)] -fn resolve_name_in_module( - krate: CrateId, - importing_crate: CrateId, - import_path: &[PathSegment], - starting_mod: LocalModuleId, - interner: &NodeInterner, - def_maps: &BTreeMap, - plain_or_crate: bool, - usage_tracker: &mut UsageTracker, - path_references: &mut Option<&mut Vec>, -) -> NamespaceResolutionResult { - let def_map = &def_maps[&krate]; - let mut current_mod_id = ModuleId { krate, local_id: starting_mod }; - let mut current_mod = &def_map.modules[current_mod_id.local_id.0]; - - let mut intermediate_item = IntermediatePathResolutionItem::Module(current_mod_id); - - // There is a possibility that the import path is empty - // In that case, early return - if import_path.is_empty() { - return Ok(NamespaceResolution { - module_id: current_mod_id, - item: PathResolutionItem::Module(current_mod_id), - namespace: PerNs::types(current_mod_id.into()), - errors: Vec::new(), - }); +impl<'def_maps, 'usage_tracker, 'references_tracker> + ImportSolver<'def_maps, 'usage_tracker, 'references_tracker> +{ + fn new( + importing_module: ModuleId, + def_maps: &'def_maps BTreeMap, + usage_tracker: &'usage_tracker mut UsageTracker, + references_tracker: Option>, + ) -> Self { + Self { importing_module, def_maps, usage_tracker, references_tracker } } - let first_segment = &import_path.first().expect("ice: could not fetch first segment").ident; - let mut current_ns = current_mod.find_name(first_segment); - if current_ns.is_none() { - return Err(PathResolutionError::Unresolved(first_segment.clone())); - } + fn resolve_name_in_module( + &mut self, + path: Path, + starting_module: ModuleId, + ) -> ImportResolutionResult { + // There is a possibility that the import path is empty. In that case, early return. + if path.segments.is_empty() { + return Ok(ResolvedImport { + namespace: PerNs::types(starting_module.into()), + errors: Vec::new(), + }); + } - usage_tracker.mark_as_referenced(current_mod_id, first_segment); + let plain_or_crate = matches!(path.kind, PathKind::Plain | PathKind::Crate); - let mut errors = Vec::new(); - for (index, (last_segment, current_segment)) in - import_path.iter().zip(import_path.iter().skip(1)).enumerate() - { - let last_ident = &last_segment.ident; - let current_ident = ¤t_segment.ident; - let last_segment_generics = &last_segment.generics; + // The current module and module ID as we resolve path segments + let mut current_module_id = starting_module; + let mut current_module = get_module(self.def_maps, starting_module); - let (typ, visibility) = match current_ns.types { - None => return Err(PathResolutionError::Unresolved(last_ident.clone())), - Some((typ, visibility, _)) => (typ, visibility), - }; + let first_segment = + &path.segments.first().expect("ice: could not fetch first segment").ident; + let mut current_ns = current_module.find_name(first_segment); + if current_ns.is_none() { + return Err(PathResolutionError::Unresolved(first_segment.clone())); + } - // In the type namespace, only Mod can be used in a path. - (current_mod_id, intermediate_item) = match typ { - ModuleDefId::ModuleId(id) => { - if let Some(path_references) = path_references { - path_references.push(ReferenceId::Module(id)); - } + self.usage_tracker.mark_as_referenced(current_module_id, first_segment); - if last_segment_generics.is_some() { - errors.push(PathResolutionError::TurbofishNotAllowedOnItem { - item: format!("module `{last_ident}`"), - span: last_segment.turbofish_span(), + let mut errors = Vec::new(); + for (index, (last_segment, current_segment)) in + path.segments.iter().zip(path.segments.iter().skip(1)).enumerate() + { + let last_ident = &last_segment.ident; + let current_ident = ¤t_segment.ident; + + let (typ, visibility) = match current_ns.types { + None => return Err(PathResolutionError::Unresolved(last_ident.clone())), + Some((typ, visibility, _)) => (typ, visibility), + }; + + self.add_reference(typ, last_segment.span, last_segment.ident.is_self_type_name()); + + // In the type namespace, only Mod can be used in a path. + current_module_id = match typ { + ModuleDefId::ModuleId(id) => id, + ModuleDefId::TypeId(id) => id.module_id(), + ModuleDefId::TypeAliasId(..) => { + return Err(PathResolutionError::NotAModule { + ident: last_segment.ident.clone(), + kind: "type alias", }); } - - (id, IntermediatePathResolutionItem::Module(id)) + ModuleDefId::TraitId(id) => id.0, + ModuleDefId::FunctionId(_) => panic!("functions cannot be in the type namespace"), + ModuleDefId::GlobalId(_) => panic!("globals cannot be in the type namespace"), + }; + + // If the path is plain or crate, the first segment will always refer to + // something that's visible from the current module. + if !((plain_or_crate && index == 0) + || self.item_in_module_is_visible(current_module_id, visibility)) + { + errors.push(PathResolutionError::Private(last_ident.clone())); } - ModuleDefId::FunctionId(_) => panic!("functions cannot be in the type namespace"), - // TODO: If impls are ever implemented, types can be used in a path - ModuleDefId::TypeId(id) => { - if let Some(path_references) = path_references { - path_references.push(ReferenceId::Struct(id)); - } - ( - id.module_id(), - IntermediatePathResolutionItem::Struct( - id, - last_segment_generics.as_ref().map(|generics| Turbofish { - generics: generics.clone(), - span: last_segment.turbofish_span(), - }), - ), - ) - } - ModuleDefId::TypeAliasId(id) => { - if let Some(path_references) = path_references { - path_references.push(ReferenceId::Alias(id)); - } + current_module = + &self.def_maps[¤t_module_id.krate].modules[current_module_id.local_id.0]; - let type_alias = interner.get_type_alias(id); - let type_alias = type_alias.borrow(); - - let module_id = match &type_alias.typ { - Type::Struct(struct_id, _generics) => struct_id.borrow().id.module_id(), - Type::Error => { - return Err(PathResolutionError::Unresolved(last_ident.clone())); - } - _ => { - // For now we only allow type aliases that point to structs. - // The more general case is captured here: https://github.com/noir-lang/noir/issues/6398 - panic!("Type alias in path not pointing to struct not yet supported") - } - }; - - ( - module_id, - IntermediatePathResolutionItem::TypeAlias( - id, - last_segment_generics.as_ref().map(|generics| Turbofish { - generics: generics.clone(), - span: last_segment.turbofish_span(), - }), - ), - ) + // Check if namespace + let found_ns = current_module.find_name(current_ident); + if found_ns.is_none() { + return Err(PathResolutionError::Unresolved(current_ident.clone())); } - ModuleDefId::TraitId(id) => { - if let Some(path_references) = path_references { - path_references.push(ReferenceId::Trait(id)); - } - ( - id.0, - IntermediatePathResolutionItem::Trait( - id, - last_segment_generics.as_ref().map(|generics| Turbofish { - generics: generics.clone(), - span: last_segment.turbofish_span(), - }), - ), - ) - } - ModuleDefId::GlobalId(_) => panic!("globals cannot be in the type namespace"), - }; + self.usage_tracker.mark_as_referenced(current_module_id, current_ident); - // If the path is plain or crate, the first segment will always refer to - // something that's visible from the current module. - if !((plain_or_crate && index == 0) - || can_reference_module_id( - def_maps, - importing_crate, - starting_mod, - current_mod_id, - visibility, - )) - { - errors.push(PathResolutionError::Private(last_ident.clone())); + current_ns = found_ns; } - current_mod = &def_maps[¤t_mod_id.krate].modules[current_mod_id.local_id.0]; + let (module_def_id, visibility, _) = + current_ns.values.or(current_ns.types).expect("Found empty namespace"); - // Check if namespace - let found_ns = current_mod.find_name(current_ident); + self.add_reference(module_def_id, path.segments.last().unwrap().ident.span(), false); - if found_ns.is_none() { - return Err(PathResolutionError::Unresolved(current_ident.clone())); + if !self.item_in_module_is_visible(current_module_id, visibility) { + errors.push(PathResolutionError::Private(path.last_ident())); } - usage_tracker.mark_as_referenced(current_mod_id, current_ident); - - current_ns = found_ns; + Ok(ResolvedImport { namespace: current_ns, errors }) } - let module_def_id = - current_ns.values.or(current_ns.types).map(|(id, _, _)| id).expect("Found empty namespace"); - - let item = merge_intermediate_path_resolution_item_with_module_def_id( - intermediate_item, - module_def_id, - ); - - Ok(NamespaceResolution { module_id: current_mod_id, item, namespace: current_ns, errors }) -} - -fn resolve_path_name(import_directive: &ImportDirective) -> Ident { - match &import_directive.alias { - None => import_directive.path.last_ident(), - Some(ident) => ident.clone(), + fn add_reference(&mut self, reference_id: ModuleDefId, span: Span, is_self_type_name: bool) { + if let Some(references_tracker) = &mut self.references_tracker { + references_tracker.add_reference(reference_id, span, is_self_type_name); + } } -} -fn resolve_external_dep( - crate_id: CrateId, - directive: &ImportDirective, - interner: &NodeInterner, - def_maps: &BTreeMap, - usage_tracker: &mut UsageTracker, - path_references: &mut Option<&mut Vec>, - importing_crate: CrateId, -) -> NamespaceResolutionResult { - // Use extern_prelude to get the dep - let path = &directive.path.segments; - - let current_def_map = &def_maps[&crate_id]; - - // Fetch the root module from the prelude - let crate_name = &path.first().unwrap().ident; - let dep_module = current_def_map - .extern_prelude - .get(&crate_name.0.contents) - .ok_or_else(|| PathResolutionError::Unresolved(crate_name.to_owned()))?; - - // Create an import directive for the dependency crate - // XXX: This will panic if the path is of the form `use std`. Ideal algorithm will not distinguish between crate and module - // See `singleton_import.nr` test case for a check that such cases are handled elsewhere. - let path_without_crate_name = &path[1..]; - - if let Some(path_references) = path_references { - path_references.push(ReferenceId::Module(*dep_module)); + fn item_in_module_is_visible(&self, module: ModuleId, visibility: ItemVisibility) -> bool { + item_in_module_is_visible(self.def_maps, self.importing_module, module, visibility) } - - let path = Path { - segments: path_without_crate_name.to_vec(), - kind: PathKind::Plain, - span: Span::default(), - }; - let dep_directive = ImportDirective { - visibility: ItemVisibility::Private, - module_id: dep_module.local_id, - self_type_module_id: directive.self_type_module_id, - path, - alias: directive.alias.clone(), - is_prelude: false, - }; - - resolve_path_to_ns( - &dep_directive, - dep_module.krate, - importing_crate, - interner, - def_maps, - usage_tracker, - path_references, - ) } -fn merge_intermediate_path_resolution_item_with_module_def_id( - intermediate_item: IntermediatePathResolutionItem, - module_def_id: ModuleDefId, -) -> PathResolutionItem { - match module_def_id { - ModuleDefId::ModuleId(module_id) => PathResolutionItem::Module(module_id), - ModuleDefId::TypeId(struct_id) => PathResolutionItem::Struct(struct_id), - ModuleDefId::TypeAliasId(type_alias_id) => PathResolutionItem::TypeAlias(type_alias_id), - ModuleDefId::TraitId(trait_id) => PathResolutionItem::Trait(trait_id), - ModuleDefId::GlobalId(global_id) => PathResolutionItem::Global(global_id), - ModuleDefId::FunctionId(func_id) => match intermediate_item { - IntermediatePathResolutionItem::Module(_) => { - PathResolutionItem::ModuleFunction(func_id) - } - IntermediatePathResolutionItem::Struct(struct_id, generics) => { - PathResolutionItem::StructFunction(struct_id, generics, func_id) - } - IntermediatePathResolutionItem::TypeAlias(alias_id, generics) => { - PathResolutionItem::TypeAliasFunction(alias_id, generics, func_id) - } - IntermediatePathResolutionItem::Trait(trait_id, generics) => { - PathResolutionItem::TraitFunction(trait_id, generics, func_id) - } - }, - } +fn get_module(def_maps: &BTreeMap, module: ModuleId) -> &ModuleData { + let message = "A crate should always be present for a given crate id"; + &def_maps.get(&module.krate).expect(message).modules[module.local_id.0] } diff --git a/compiler/noirc_frontend/src/hir/resolution/mod.rs b/compiler/noirc_frontend/src/hir/resolution/mod.rs index 223b88b5c5d..44a0114074e 100644 --- a/compiler/noirc_frontend/src/hir/resolution/mod.rs +++ b/compiler/noirc_frontend/src/hir/resolution/mod.rs @@ -7,5 +7,4 @@ //! will have the same DefinitionId. pub mod errors; pub mod import; -pub mod path_resolver; pub mod visibility; diff --git a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs deleted file mode 100644 index 98d4aee3f5c..00000000000 --- a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs +++ /dev/null @@ -1,102 +0,0 @@ -use super::import::{resolve_import, ImportDirective, PathResolution, PathResolutionResult}; -use crate::ast::{ItemVisibility, Path}; -use crate::node_interner::{NodeInterner, ReferenceId}; -use crate::usage_tracker::UsageTracker; - -use std::collections::BTreeMap; - -use crate::graph::CrateId; -use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}; - -pub trait PathResolver { - /// Resolve the given path returning the resolved ModuleDefId. - /// If `path_references` is `Some`, a `ReferenceId` for each segment in `path` - /// will be resolved and pushed (some entries will be None if they don't refer to - /// a module or type). - fn resolve( - &self, - interner: &NodeInterner, - def_maps: &BTreeMap, - path: Path, - usage_tracker: &mut UsageTracker, - path_references: &mut Option<&mut Vec>, - ) -> PathResolutionResult; - - fn local_module_id(&self) -> LocalModuleId; - - fn module_id(&self) -> ModuleId; -} - -pub struct StandardPathResolver { - // Module that we are resolving the path in - module_id: ModuleId, - // The module of the self type, if any (for example, the ModuleId of a struct) - self_type_module_id: Option, -} - -impl StandardPathResolver { - pub fn new(module_id: ModuleId, self_type_module_id: Option) -> StandardPathResolver { - Self { module_id, self_type_module_id } - } -} - -impl PathResolver for StandardPathResolver { - fn resolve( - &self, - interner: &NodeInterner, - def_maps: &BTreeMap, - path: Path, - usage_tracker: &mut UsageTracker, - path_references: &mut Option<&mut Vec>, - ) -> PathResolutionResult { - resolve_path( - interner, - def_maps, - self.module_id, - self.self_type_module_id, - path, - usage_tracker, - path_references, - ) - } - - fn local_module_id(&self) -> LocalModuleId { - self.module_id.local_id - } - - fn module_id(&self) -> ModuleId { - self.module_id - } -} - -/// Resolve the given path to a function or a type. -/// In the case of a conflict, functions are given priority -pub fn resolve_path( - interner: &NodeInterner, - def_maps: &BTreeMap, - module_id: ModuleId, - self_type_module_id: Option, - path: Path, - usage_tracker: &mut UsageTracker, - path_references: &mut Option<&mut Vec>, -) -> PathResolutionResult { - // lets package up the path into an ImportDirective and resolve it using that - let import = ImportDirective { - visibility: ItemVisibility::Private, - module_id: module_id.local_id, - self_type_module_id, - path, - alias: None, - is_prelude: false, - }; - let resolved_import = resolve_import( - module_id.krate, - &import, - interner, - def_maps, - usage_tracker, - path_references, - )?; - - Ok(PathResolution { item: resolved_import.item, errors: resolved_import.errors }) -} diff --git a/compiler/noirc_frontend/src/hir/resolution/visibility.rs b/compiler/noirc_frontend/src/hir/resolution/visibility.rs index 492f303d2c4..c2fe887fe62 100644 --- a/compiler/noirc_frontend/src/hir/resolution/visibility.rs +++ b/compiler/noirc_frontend/src/hir/resolution/visibility.rs @@ -7,34 +7,43 @@ use std::collections::BTreeMap; use crate::ast::ItemVisibility; use crate::hir::def_map::{CrateDefMap, DefMaps, LocalModuleId, ModuleId}; -// Returns false if the given private function is being called from a non-child module, or -// if the given pub(crate) function is being called from another crate. Otherwise returns true. -pub fn can_reference_module_id( +/// Returns true if an item with the given visibility in the target module +/// is visible from the current module. For example: +/// ```text +/// mod foo { +/// ^^^ <-- target module +/// pub(crate) fn bar() {} +/// ^^^^^^^^^^ <- visibility +/// } +/// ``` +pub fn item_in_module_is_visible( def_maps: &BTreeMap, - importing_crate: CrateId, - current_module: LocalModuleId, + current_module: ModuleId, target_module: ModuleId, visibility: ItemVisibility, ) -> bool { // Note that if the target module is in a different crate from the current module then we will either // return true as the target module is public or return false as it is private without looking at the `CrateDefMap` in either case. - let same_crate = target_module.krate == importing_crate; + let same_crate = target_module.krate == current_module.krate; match visibility { ItemVisibility::Public => true, ItemVisibility::PublicCrate => same_crate, ItemVisibility::Private => { + if !same_crate { + return false; + } + let target_crate_def_map = &def_maps[&target_module.krate]; - same_crate - && (module_descendent_of_target( - target_crate_def_map, - target_module.local_id, - current_module, - ) || module_is_parent_of_struct_module( - target_crate_def_map, - current_module, - target_module.local_id, - )) + module_descendent_of_target( + target_crate_def_map, + target_module.local_id, + current_module.local_id, + ) || module_is_parent_of_struct_module( + target_crate_def_map, + current_module.local_id, + target_module.local_id, + ) } } } @@ -116,10 +125,9 @@ pub fn method_call_is_visible( ItemVisibility::Private => { if object_type.is_primitive() { let func_module = interner.function_module(func_id); - can_reference_module_id( + item_in_module_is_visible( def_maps, - current_module.krate, - current_module.local_id, + current_module, func_module, modifiers.visibility, ) diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index 4dd699251a6..ecae5b19a95 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -1,14 +1,11 @@ use fm::FileId; -use noirc_errors::Location; +use noirc_errors::{Location, Span}; use rangemap::RangeMap; use rustc_hash::FxHashMap as HashMap; use crate::{ ast::{FunctionDefinition, ItemVisibility}, - hir::{ - def_map::{ModuleDefId, ModuleId}, - resolution::import::PathResolutionItem, - }, + hir::def_map::{ModuleDefId, ModuleId}, node_interner::{ DefinitionId, FuncId, GlobalId, NodeInterner, ReferenceId, StructId, TraitId, TypeAliasId, }, @@ -37,6 +34,27 @@ impl LocationIndices { } } +pub struct ReferencesTracker<'a> { + interner: &'a mut NodeInterner, + file_id: FileId, +} + +impl<'a> ReferencesTracker<'a> { + pub fn new(interner: &'a mut NodeInterner, file_id: FileId) -> Self { + Self { interner, file_id } + } + + pub(crate) fn add_reference( + &mut self, + module_def_id: ModuleDefId, + span: Span, + is_self_type: bool, + ) { + let location = Location::new(span, self.file_id); + self.interner.add_module_def_id_reference(module_def_id, location, is_self_type); + } +} + impl NodeInterner { pub fn reference_location(&self, reference: ReferenceId) -> Location { match reference { @@ -102,37 +120,6 @@ impl NodeInterner { }; } - pub(crate) fn add_path_resolution_kind_reference( - &mut self, - kind: PathResolutionItem, - location: Location, - is_self_type: bool, - ) { - match kind { - PathResolutionItem::Module(module_id) => { - self.add_module_reference(module_id, location); - } - PathResolutionItem::Struct(struct_id) => { - self.add_struct_reference(struct_id, location, is_self_type); - } - PathResolutionItem::TypeAlias(type_alias_id) => { - self.add_alias_reference(type_alias_id, location); - } - PathResolutionItem::Trait(trait_id) => { - self.add_trait_reference(trait_id, location, is_self_type); - } - PathResolutionItem::Global(global_id) => { - self.add_global_reference(global_id, location); - } - PathResolutionItem::ModuleFunction(func_id) - | PathResolutionItem::StructFunction(_, _, func_id) - | PathResolutionItem::TypeAliasFunction(_, _, func_id) - | PathResolutionItem::TraitFunction(_, _, func_id) => { - self.add_function_reference(func_id, location); - } - } - } - pub(crate) fn add_module_reference(&mut self, id: ModuleId, location: Location) { self.add_reference(ReferenceId::Module(id), location, false); } diff --git a/compiler/noirc_frontend/src/tests/imports.rs b/compiler/noirc_frontend/src/tests/imports.rs index 1da7f5ce0ce..8598d8296e5 100644 --- a/compiler/noirc_frontend/src/tests/imports.rs +++ b/compiler/noirc_frontend/src/tests/imports.rs @@ -127,3 +127,30 @@ fn warns_on_re_export_of_item_with_less_visibility() { ) )); } + +#[test] +fn errors_if_using_alias_in_import() { + let src = r#" + mod foo { + pub type bar = i32; + } + + use foo::bar::baz; + + fn main() { + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::DefinitionError(DefCollectorErrorKind::PathResolutionError( + PathResolutionError::NotAModule { ident, kind }, + )) = &errors[0].0 + else { + panic!("Expected a 'not a module' error, got {:?}", errors[0].0); + }; + + assert_eq!(ident.to_string(), "bar"); + assert_eq!(*kind, "type alias"); +} diff --git a/tooling/lsp/src/attribute_reference_finder.rs b/tooling/lsp/src/attribute_reference_finder.rs index 660ee8ffdfb..f831f83d492 100644 --- a/tooling/lsp/src/attribute_reference_finder.rs +++ b/tooling/lsp/src/attribute_reference_finder.rs @@ -13,24 +13,22 @@ use noirc_frontend::{ graph::CrateId, hir::{ def_map::{CrateDefMap, LocalModuleId, ModuleId}, - resolution::{ - import::PathResolutionItem, - path_resolver::{PathResolver, StandardPathResolver}, - }, + resolution::import::resolve_import, }, - node_interner::{NodeInterner, ReferenceId}, + node_interner::ReferenceId, parser::{ParsedSubModule, Parser}, token::CustomAttribute, usage_tracker::UsageTracker, ParsedModule, }; +use crate::modules::module_def_id_to_reference_id; + pub(crate) struct AttributeReferenceFinder<'a> { byte_index: usize, /// The module ID in scope. This might change as we traverse the AST /// if we are analyzing something inside an inline module declaration. module_id: ModuleId, - interner: &'a NodeInterner, def_maps: &'a BTreeMap, reference_id: Option, } @@ -41,7 +39,6 @@ impl<'a> AttributeReferenceFinder<'a> { file: FileId, byte_index: usize, krate: CrateId, - interner: &'a NodeInterner, def_maps: &'a BTreeMap, ) -> Self { // Find the module the current file belongs to @@ -54,7 +51,7 @@ impl<'a> AttributeReferenceFinder<'a> { def_map.root() }; let module_id = ModuleId { krate, local_id }; - Self { byte_index, module_id, interner, def_maps, reference_id: None } + Self { byte_index, module_id, def_maps, reference_id: None } } pub(crate) fn find(&mut self, parsed_module: &ParsedModule) -> Option { @@ -102,28 +99,19 @@ impl<'a> Visitor for AttributeReferenceFinder<'a> { return; }; - let resolver = StandardPathResolver::new(self.module_id, None); - let mut usage_tracker = UsageTracker::default(); - let Ok(result) = - resolver.resolve(self.interner, self.def_maps, path, &mut usage_tracker, &mut None) - else { + // The path here must resolve to a function and it's a simple path (can't have turbofish) + // so it can (and must) be solved as an import. + let Ok(Some((module_def_id, _, _))) = resolve_import( + path, + self.module_id, + self.def_maps, + &mut UsageTracker::default(), + None, // references tracker + ) + .map(|result| result.namespace.values) else { return; }; - self.reference_id = Some(path_resolution_item_to_reference_id(result.item)); - } -} - -fn path_resolution_item_to_reference_id(item: PathResolutionItem) -> ReferenceId { - match item { - PathResolutionItem::Module(module_id) => ReferenceId::Module(module_id), - PathResolutionItem::Struct(struct_id) => ReferenceId::Struct(struct_id), - PathResolutionItem::TypeAlias(type_alias_id) => ReferenceId::Alias(type_alias_id), - PathResolutionItem::Trait(trait_id) => ReferenceId::Trait(trait_id), - PathResolutionItem::Global(global_id) => ReferenceId::Global(global_id), - PathResolutionItem::ModuleFunction(func_id) - | PathResolutionItem::StructFunction(_, _, func_id) - | PathResolutionItem::TypeAliasFunction(_, _, func_id) - | PathResolutionItem::TraitFunction(_, _, func_id) => ReferenceId::Function(func_id), + self.reference_id = Some(module_def_id_to_reference_id(module_def_id)); } } diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index 2dd42c740cd..3bfb411ea4d 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -26,7 +26,9 @@ use noirc_frontend::{ graph::{CrateId, Dependency}, hir::{ def_map::{CrateDefMap, LocalModuleId, ModuleDefId, ModuleId}, - resolution::visibility::{method_call_is_visible, struct_member_is_visible}, + resolution::visibility::{ + item_in_module_is_visible, method_call_is_visible, struct_member_is_visible, + }, }, hir_def::traits::Trait, node_interner::{NodeInterner, ReferenceId, StructId}, @@ -38,8 +40,7 @@ use sort_text::underscore_sort_text; use crate::{ requests::to_lsp_location, trait_impl_method_stub_generator::TraitImplMethodStubGenerator, - use_segment_positions::UseSegmentPositions, utils, visibility::item_in_module_is_visible, - LspState, + use_segment_positions::UseSegmentPositions, utils, LspState, }; use super::process_request; @@ -799,10 +800,10 @@ impl<'a> NodeFinder<'a> { let per_ns = module_data.find_name(ident); if let Some((module_def_id, visibility, _)) = per_ns.types { if item_in_module_is_visible( - module_id, + self.def_maps, self.module_id, + module_id, visibility, - self.def_maps, ) { let completion_items = self.module_def_id_completion_items( module_def_id, @@ -820,10 +821,10 @@ impl<'a> NodeFinder<'a> { if let Some((module_def_id, visibility, _)) = per_ns.values { if item_in_module_is_visible( - module_id, + self.def_maps, self.module_id, + module_id, visibility, - self.def_maps, ) { let completion_items = self.module_def_id_completion_items( module_def_id, diff --git a/tooling/lsp/src/requests/goto_definition.rs b/tooling/lsp/src/requests/goto_definition.rs index 9380209da5c..a2443ea165d 100644 --- a/tooling/lsp/src/requests/goto_definition.rs +++ b/tooling/lsp/src/requests/goto_definition.rs @@ -46,7 +46,6 @@ fn on_goto_definition_inner( file_id, byte_index, args.crate_id, - args.interner, args.def_maps, ); finder.find(&parsed_module) diff --git a/tooling/lsp/src/requests/hover.rs b/tooling/lsp/src/requests/hover.rs index eb066b53b78..64974a42133 100644 --- a/tooling/lsp/src/requests/hover.rs +++ b/tooling/lsp/src/requests/hover.rs @@ -47,7 +47,6 @@ pub(crate) fn on_hover_request( file_id, byte_index, args.crate_id, - args.interner, args.def_maps, ); finder.find(&parsed_module) diff --git a/tooling/lsp/src/visibility.rs b/tooling/lsp/src/visibility.rs index 6a21525f069..6724a0ba505 100644 --- a/tooling/lsp/src/visibility.rs +++ b/tooling/lsp/src/visibility.rs @@ -5,36 +5,13 @@ use noirc_frontend::{ graph::CrateId, hir::{ def_map::{CrateDefMap, ModuleDefId, ModuleId}, - resolution::visibility::can_reference_module_id, + resolution::visibility::item_in_module_is_visible, }, node_interner::NodeInterner, }; use crate::modules::get_parent_module; -/// Returns true if an item with the given visibility in the target module -/// is visible from the current module. For example: -/// -/// mod foo { -/// ^^^ <-- target module -/// pub(crate) fn bar() {} -/// ^^^^^^^^^^ <- visibility -/// } -pub(super) fn item_in_module_is_visible( - target_module_id: ModuleId, - current_module_id: ModuleId, - visibility: ItemVisibility, - def_maps: &BTreeMap, -) -> bool { - can_reference_module_id( - def_maps, - current_module_id.krate, - current_module_id.local_id, - target_module_id, - visibility, - ) -} - /// Returns true if the given ModuleDefId is visible from the current module, given its visibility. /// This will in turn check if the ModuleDefId parent modules are visible from the current module. /// If `defining_module` is Some, it will be considered as the parent of the item to check @@ -57,7 +34,7 @@ pub(super) fn module_def_id_is_visible( // Then check if it's visible, and upwards while let Some(module_id) = target_module_id { - if !item_in_module_is_visible(module_id, current_module_id, visibility, def_maps) { + if !item_in_module_is_visible(def_maps, current_module_id, module_id, visibility) { return false; } From 2972db20fc00ed0e43b662092f0d0712421d122f Mon Sep 17 00:00:00 2001 From: Michael J Klein Date: Tue, 5 Nov 2024 08:57:49 -0500 Subject: [PATCH 4/4] feat!: Always Check Arithmetic Generics at Monomorphization (#6329) Co-authored-by: jfecher Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_driver/src/abi_gen.rs | 20 +- compiler/noirc_evaluator/src/ssa.rs | 4 +- .../src/ssa/function_builder/data_bus.rs | 2 +- compiler/noirc_frontend/src/elaborator/mod.rs | 8 + .../noirc_frontend/src/elaborator/types.rs | 29 ++- .../noirc_frontend/src/hir/comptime/errors.rs | 20 +- .../src/hir/comptime/hir_to_display_ast.rs | 1 + .../src/hir/comptime/interpreter.rs | 44 ++-- .../src/hir/comptime/interpreter/builtin.rs | 96 +++++--- .../src/hir/resolution/errors.rs | 13 +- .../src/hir/type_check/errors.rs | 48 +++- compiler/noirc_frontend/src/hir_def/stmt.rs | 2 +- compiler/noirc_frontend/src/hir_def/types.rs | 218 ++++++++++++++---- .../src/hir_def/types/arithmetic.rs | 117 ++++++++-- .../src/monomorphization/errors.rs | 18 +- .../src/monomorphization/mod.rs | 111 +++++++-- compiler/noirc_frontend/src/node_interner.rs | 1 + compiler/noirc_frontend/src/tests.rs | 143 +++++++++++- tooling/lsp/src/requests/completion.rs | 8 + tooling/lsp/src/requests/hover.rs | 1 + tooling/lsp/src/requests/inlay_hint.rs | 1 + .../src/trait_impl_method_stub_generator.rs | 1 + 22 files changed, 745 insertions(+), 161 deletions(-) diff --git a/compiler/noirc_driver/src/abi_gen.rs b/compiler/noirc_driver/src/abi_gen.rs index ad54d0f7d6b..4cd480b883d 100644 --- a/compiler/noirc_driver/src/abi_gen.rs +++ b/compiler/noirc_driver/src/abi_gen.rs @@ -6,6 +6,7 @@ use iter_extended::vecmap; use noirc_abi::{ Abi, AbiErrorType, AbiParameter, AbiReturnType, AbiType, AbiValue, AbiVisibility, Sign, }; +use noirc_errors::Span; use noirc_frontend::ast::{Signedness, Visibility}; use noirc_frontend::TypeBinding; use noirc_frontend::{ @@ -39,10 +40,20 @@ pub(super) fn gen_abi( Abi { parameters, return_type, error_types } } +// Get the Span of the root crate's main function, or else a dummy span if that fails +fn get_main_function_span(context: &Context) -> Span { + if let Some(func_id) = context.get_main_function(context.root_crate_id()) { + context.function_meta(&func_id).location.span + } else { + Span::default() + } +} + fn build_abi_error_type(context: &Context, typ: &Type) -> AbiErrorType { match typ { Type::FmtString(len, item_types) => { - let length = len.evaluate_to_u32().expect("Cannot evaluate fmt length"); + let span = get_main_function_span(context); + let length = len.evaluate_to_u32(span).expect("Cannot evaluate fmt length"); let Type::Tuple(item_types) = item_types.as_ref() else { unreachable!("FmtString items must be a tuple") }; @@ -58,8 +69,9 @@ pub(super) fn abi_type_from_hir_type(context: &Context, typ: &Type) -> AbiType { match typ { Type::FieldElement => AbiType::Field, Type::Array(size, typ) => { + let span = get_main_function_span(context); let length = size - .evaluate_to_u32() + .evaluate_to_u32(span) .expect("Cannot have variable sized arrays as a parameter to main"); let typ = typ.as_ref(); AbiType::Array { length, typ: Box::new(abi_type_from_hir_type(context, typ)) } @@ -86,8 +98,9 @@ pub(super) fn abi_type_from_hir_type(context: &Context, typ: &Type) -> AbiType { } Type::Bool => AbiType::Boolean, Type::String(size) => { + let span = get_main_function_span(context); let size = size - .evaluate_to_u32() + .evaluate_to_u32(span) .expect("Cannot have variable sized strings as a parameter to main"); AbiType::String { length: size } } @@ -102,6 +115,7 @@ pub(super) fn abi_type_from_hir_type(context: &Context, typ: &Type) -> AbiType { AbiType::Struct { fields, path } } Type::Alias(def, args) => abi_type_from_hir_type(context, &def.borrow().get_type(args)), + Type::CheckedCast { to, .. } => abi_type_from_hir_type(context, to), Type::Tuple(fields) => { let fields = vecmap(fields, |typ| abi_type_from_hir_type(context, typ)); AbiType::Tuple { fields } diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index c993ec291dd..8480d2c08cd 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -365,8 +365,8 @@ fn split_public_and_private_inputs( func_sig .0 .iter() - .map(|(_, typ, visibility)| { - let num_field_elements_needed = typ.field_count() as usize; + .map(|(pattern, typ, visibility)| { + let num_field_elements_needed = typ.field_count(&pattern.location()) as usize; let witnesses = input_witnesses[idx..idx + num_field_elements_needed].to_vec(); idx += num_field_elements_needed; (visibility, witnesses) diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/data_bus.rs b/compiler/noirc_evaluator/src/ssa/function_builder/data_bus.rs index de9ae8a24d7..5a62e9c8e9a 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/data_bus.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/data_bus.rs @@ -48,7 +48,7 @@ impl DataBusBuilder { ast::Visibility::CallData(id) => DatabusVisibility::CallData(id), ast::Visibility::ReturnData => DatabusVisibility::ReturnData, }; - let len = param.1.field_count() as usize; + let len = param.1.field_count(¶m.0.location()) as usize; params_is_databus.extend(vec![is_databus; len]); } params_is_databus diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index b58d552d866..fcca9470cfb 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -896,6 +896,10 @@ impl<'context> Elaborator<'context> { Type::Alias(alias_type, generics) => { self.mark_type_as_used(&alias_type.borrow().get_type(generics)); } + Type::CheckedCast { from, to } => { + self.mark_type_as_used(from); + self.mark_type_as_used(to); + } Type::MutableReference(typ) => { self.mark_type_as_used(typ); } @@ -1501,6 +1505,10 @@ impl<'context> Elaborator<'context> { span, ); } + Type::CheckedCast { from, to } => { + self.check_type_is_not_more_private_then_item(name, visibility, from, span); + self.check_type_is_not_more_private_then_item(name, visibility, to, span); + } Type::Function(args, return_type, env, _) => { for arg in args { self.check_type_is_not_more_private_then_item(name, visibility, arg, span); diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index ce60f8329fd..b9c4b506229 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -462,14 +462,26 @@ impl<'context> Elaborator<'context> { }); return Type::Error; } - if let Some(result) = op.function(lhs, rhs, &lhs_kind) { - Type::Constant(result, lhs_kind) - } else { - self.push_err(ResolverError::OverflowInType { lhs, op, rhs, span }); - Type::Error + match op.function(lhs, rhs, &lhs_kind, span) { + Ok(result) => Type::Constant(result, lhs_kind), + Err(err) => { + let err = Box::new(err); + self.push_err(ResolverError::BinaryOpError { + lhs, + op, + rhs, + err, + span, + }); + Type::Error + } } } - (lhs, rhs) => Type::InfixExpr(Box::new(lhs), op, Box::new(rhs)).canonicalize(), + (lhs, rhs) => { + let infix = Type::InfixExpr(Box::new(lhs), op, Box::new(rhs)); + Type::CheckedCast { from: Box::new(infix.clone()), to: Box::new(infix) } + .canonicalize() + } } } UnresolvedTypeExpression::AsTraitPath(path) => { @@ -1738,6 +1750,11 @@ impl<'context> Elaborator<'context> { | Type::Quoted(_) | Type::Forall(_, _) => (), + Type::CheckedCast { from, to } => { + Self::find_numeric_generics_in_type(from, found); + Self::find_numeric_generics_in_type(to, found); + } + Type::TraitAsType(_, _, args) => { for arg in &args.ordered { Self::find_numeric_generics_in_type(arg, found); diff --git a/compiler/noirc_frontend/src/hir/comptime/errors.rs b/compiler/noirc_frontend/src/hir/comptime/errors.rs index dfd328f85f0..198ba91156e 100644 --- a/compiler/noirc_frontend/src/hir/comptime/errors.rs +++ b/compiler/noirc_frontend/src/hir/comptime/errors.rs @@ -3,7 +3,10 @@ use std::rc::Rc; use crate::{ ast::TraitBound, - hir::{def_collector::dc_crate::CompilationError, type_check::NoMatchingImplFoundError}, + hir::{ + def_collector::dc_crate::CompilationError, + type_check::{NoMatchingImplFoundError, TypeCheckError}, + }, parser::ParserError, Type, }; @@ -87,6 +90,7 @@ pub enum InterpreterError { }, NonIntegerArrayLength { typ: Type, + err: Option>, location: Location, }, NonNumericCasted { @@ -228,6 +232,7 @@ pub enum InterpreterError { }, UnknownArrayLength { length: Type, + err: Box, location: Location, }, @@ -435,9 +440,13 @@ impl<'a> From<&'a InterpreterError> for CustomDiagnostic { let secondary = "This is likely a bug".into(); CustomDiagnostic::simple_error(msg, secondary, location.span) } - InterpreterError::NonIntegerArrayLength { typ, location } => { + InterpreterError::NonIntegerArrayLength { typ, err, location } => { let msg = format!("Non-integer array length: `{typ}`"); - let secondary = "Array lengths must be integers".into(); + let secondary = if let Some(err) = err { + format!("Array lengths must be integers, but evaluating `{typ}` resulted in `{err}`") + } else { + "Array lengths must be integers".to_string() + }; CustomDiagnostic::simple_error(msg, secondary, location.span) } InterpreterError::NonNumericCasted { typ, location } => { @@ -640,9 +649,10 @@ impl<'a> From<&'a InterpreterError> for CustomDiagnostic { let msg = format!("`{expression}` is not a valid function body"); CustomDiagnostic::simple_error(msg, String::new(), location.span) } - InterpreterError::UnknownArrayLength { length, location } => { + InterpreterError::UnknownArrayLength { length, err, location } => { let msg = format!("Could not determine array length `{length}`"); - CustomDiagnostic::simple_error(msg, String::new(), location.span) + let secondary = format!("Evaluating the length failed with: `{err}`"); + CustomDiagnostic::simple_error(msg, secondary, location.span) } } } diff --git a/compiler/noirc_frontend/src/hir/comptime/hir_to_display_ast.rs b/compiler/noirc_frontend/src/hir/comptime/hir_to_display_ast.rs index 260c4e3848a..5540a199cec 100644 --- a/compiler/noirc_frontend/src/hir/comptime/hir_to_display_ast.rs +++ b/compiler/noirc_frontend/src/hir/comptime/hir_to_display_ast.rs @@ -340,6 +340,7 @@ impl Type { let name = Path::from_single(name.as_ref().clone(), Span::default()); UnresolvedTypeData::Named(name, GenericTypeArgs::default(), true) } + Type::CheckedCast { to, .. } => to.to_display_ast().typ, Type::Function(args, ret, env, unconstrained) => { let args = vecmap(args, |arg| arg.to_display_ast()); let ret = Box::new(ret.to_display_ast()); diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index 7205242ead9..147dec97d7d 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -585,20 +585,25 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { } DefinitionKind::NumericGeneric(type_variable, numeric_typ) => { let value = match &*type_variable.borrow() { - TypeBinding::Unbound(_, _) => None, + TypeBinding::Unbound(_, _) => { + let typ = self.elaborator.interner.id_type(id); + let location = self.elaborator.interner.expr_location(&id); + Err(InterpreterError::NonIntegerArrayLength { typ, err: None, location }) + } TypeBinding::Bound(binding) => { - binding.evaluate_to_field_element(&Kind::Numeric(numeric_typ.clone())) + let span = self.elaborator.interner.id_location(id).span; + binding + .evaluate_to_field_element(&Kind::Numeric(numeric_typ.clone()), span) + .map_err(|err| { + let typ = Type::TypeVariable(type_variable.clone()); + let err = Some(Box::new(err)); + let location = self.elaborator.interner.expr_location(&id); + InterpreterError::NonIntegerArrayLength { typ, err, location } + }) } - }; + }?; - if let Some(value) = value { - let typ = self.elaborator.interner.id_type(id); - self.evaluate_integer(value, false, id) - } else { - let location = self.elaborator.interner.expr_location(&id); - let typ = Type::TypeVariable(type_variable.clone()); - Err(InterpreterError::NonIntegerArrayLength { typ, location }) - } + self.evaluate_integer(value, false, id) } } } @@ -805,12 +810,17 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { HirArrayLiteral::Repeated { repeated_element, length } => { let element = self.evaluate(repeated_element)?; - if let Some(length) = length.evaluate_to_u32() { - let elements = (0..length).map(|_| element.clone()).collect(); - Ok(Value::Array(elements, typ)) - } else { - let location = self.elaborator.interner.expr_location(&id); - Err(InterpreterError::NonIntegerArrayLength { typ: length, location }) + let span = self.elaborator.interner.id_location(id).span; + match length.evaluate_to_u32(span) { + Ok(length) => { + let elements = (0..length).map(|_| element.clone()).collect(); + Ok(Value::Array(elements, typ)) + } + Err(err) => { + let err = Some(Box::new(err)); + let location = self.elaborator.interner.expr_location(&id); + Err(InterpreterError::NonIntegerArrayLength { typ: length, err, location }) + } } } } diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index d8842215a29..76fcccd573c 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -12,7 +12,7 @@ use builtin_helpers::{ }; use im::Vector; use iter_extended::{try_vecmap, vecmap}; -use noirc_errors::Location; +use noirc_errors::{Location, Span}; use num_bigint::BigUint; use rustc_hash::FxHashMap as HashMap; @@ -230,7 +230,7 @@ impl<'local, 'context> Interpreter<'local, 'context> { "unresolved_type_is_bool" => unresolved_type_is_bool(interner, arguments, location), "unresolved_type_is_field" => unresolved_type_is_field(interner, arguments, location), "unresolved_type_is_unit" => unresolved_type_is_unit(interner, arguments, location), - "zeroed" => zeroed(return_type), + "zeroed" => zeroed(return_type, location.span), _ => { let item = format!("Comptime evaluation for builtin function {name}"); Err(InterpreterError::Unimplemented { item, location }) @@ -710,7 +710,7 @@ fn quoted_as_expr( }, ); - option(return_type, value) + option(return_type, value, location.span) } // fn as_module(quoted: Quoted) -> Option @@ -737,7 +737,7 @@ fn quoted_as_module( module.map(Value::ModuleDefinition) }); - option(return_type, option_value) + option(return_type, option_value, location.span) } // fn as_trait_constraint(quoted: Quoted) -> TraitConstraint @@ -869,11 +869,22 @@ fn type_as_constant( return_type: Type, location: Location, ) -> IResult { - type_as(arguments, return_type, location, |typ| { + type_as_or_err(arguments, return_type, location, |typ| { // Prefer to use `evaluate_to_u32` over matching on `Type::Constant` // since arithmetic generics may be `Type::InfixExpr`s which evaluate to // constants but are not actually the `Type::Constant` variant. - typ.evaluate_to_u32().map(Value::U32) + match typ.evaluate_to_u32(location.span) { + Ok(constant) => Ok(Some(Value::U32(constant))), + Err(err) => { + // Evaluating to a non-constant returns 'None' in user code + if err.is_non_constant_evaluated() { + Ok(None) + } else { + let err = Some(Box::new(err)); + Err(InterpreterError::NonIntegerArrayLength { typ, err, location }) + } + } + } }) } @@ -979,7 +990,6 @@ fn type_as_tuple( }) } -// Helper function for implementing the `type_as_...` functions. fn type_as( arguments: Vec<(Value, Location)>, return_type: Type, @@ -988,13 +998,26 @@ fn type_as( ) -> IResult where F: FnOnce(Type) -> Option, +{ + type_as_or_err(arguments, return_type, location, |x| Ok(f(x))) +} + +// Helper function for implementing the `type_as_...` functions. +fn type_as_or_err( + arguments: Vec<(Value, Location)>, + return_type: Type, + location: Location, + f: F, +) -> IResult +where + F: FnOnce(Type) -> IResult>, { let value = check_one_argument(arguments, location)?; let typ = get_type(value)?.follow_bindings(); - let option_value = f(typ); + let option_value = f(typ)?; - option(return_type, option_value) + option(return_type, option_value, location.span) } // fn type_eq(_first: Type, _second: Type) -> bool @@ -1029,7 +1052,7 @@ fn type_get_trait_impl( _ => None, }; - option(return_type, option_value) + option(return_type, option_value, location.span) } // fn implements(self, constraint: TraitConstraint) -> bool @@ -1150,7 +1173,7 @@ fn typed_expr_as_function_definition( } else { None }; - option(return_type, option_value) + option(return_type, option_value, location.span) } // fn get_type(self) -> Option @@ -1172,7 +1195,7 @@ fn typed_expr_get_type( } else { None }; - option(return_type, option_value) + option(return_type, option_value, location.span) } // fn as_mutable_reference(self) -> Option @@ -1256,16 +1279,16 @@ where let option_value = f(typ); - option(return_type, option_value) + option(return_type, option_value, location.span) } // fn zeroed() -> T -fn zeroed(return_type: Type) -> IResult { +fn zeroed(return_type: Type, span: Span) -> IResult { match return_type { Type::FieldElement => Ok(Value::Field(0u128.into())), Type::Array(length_type, elem) => { - if let Some(length) = length_type.evaluate_to_u32() { - let element = zeroed(elem.as_ref().clone())?; + if let Ok(length) = length_type.evaluate_to_u32(span) { + let element = zeroed(elem.as_ref().clone(), span)?; let array = std::iter::repeat(element).take(length as usize).collect(); Ok(Value::Array(array, Type::Array(length_type, elem))) } else { @@ -1288,7 +1311,7 @@ fn zeroed(return_type: Type) -> IResult { }, Type::Bool => Ok(Value::Bool(false)), Type::String(length_type) => { - if let Some(length) = length_type.evaluate_to_u32() { + if let Ok(length) = length_type.evaluate_to_u32(span) { Ok(Value::String(Rc::new("\0".repeat(length as usize)))) } else { // Assume we can resolve the length later @@ -1296,9 +1319,9 @@ fn zeroed(return_type: Type) -> IResult { } } Type::FmtString(length_type, captures) => { - let length = length_type.evaluate_to_u32(); + let length = length_type.evaluate_to_u32(span); let typ = Type::FmtString(length_type, captures); - if let Some(length) = length { + if let Ok(length) = length { Ok(Value::FormatString(Rc::new("\0".repeat(length as usize)), typ)) } else { // Assume we can resolve the length later @@ -1306,26 +1329,27 @@ fn zeroed(return_type: Type) -> IResult { } } Type::Unit => Ok(Value::Unit), - Type::Tuple(fields) => Ok(Value::Tuple(try_vecmap(fields, zeroed)?)), + Type::Tuple(fields) => Ok(Value::Tuple(try_vecmap(fields, |field| zeroed(field, span))?)), Type::Struct(struct_type, generics) => { let fields = struct_type.borrow().get_fields(&generics); let mut values = HashMap::default(); for (field_name, field_type) in fields { - let field_value = zeroed(field_type)?; + let field_value = zeroed(field_type, span)?; values.insert(Rc::new(field_name), field_value); } let typ = Type::Struct(struct_type, generics); Ok(Value::Struct(values, typ)) } - Type::Alias(alias, generics) => zeroed(alias.borrow().get_type(&generics)), + Type::Alias(alias, generics) => zeroed(alias.borrow().get_type(&generics), span), + Type::CheckedCast { to, .. } => zeroed(*to, span), typ @ Type::Function(..) => { // Using Value::Zeroed here is probably safer than using FuncId::dummy_id() or similar Ok(Value::Zeroed(typ)) } Type::MutableReference(element) => { - let element = zeroed(*element)?; + let element = zeroed(*element, span)?; Ok(Value::Pointer(Shared::new(element), false)) } // Optimistically assume we can resolve this type later or that the value is unused @@ -1389,7 +1413,7 @@ fn expr_as_assert( let option_type = tuple_types.pop().unwrap(); let message = message.map(|msg| Value::expression(msg.kind)); - let message = option(option_type, message).ok()?; + let message = option(option_type, message, location.span).ok()?; Some(Value::Tuple(vec![predicate, message])) } else { @@ -1435,7 +1459,7 @@ fn expr_as_assert_eq( let option_type = tuple_types.pop().unwrap(); let message = message.map(|message| Value::expression(message.kind)); - let message = option(option_type, message).ok()?; + let message = option(option_type, message, location.span).ok()?; Some(Value::Tuple(vec![lhs, rhs, message])) } else { @@ -1611,7 +1635,7 @@ fn expr_as_constructor( None }; - option(return_type, option_value) + option(return_type, option_value, location.span) } // fn as_for(self) -> Option<(Quoted, Expr, Expr)> @@ -1705,6 +1729,7 @@ fn expr_as_if( let alternative = option( alternative_option_type, if_expr.alternative.map(|e| Value::expression(e.kind)), + location.span, ); Some(Value::Tuple(vec![ @@ -1793,7 +1818,7 @@ fn expr_as_lambda( } else { Some(Value::UnresolvedType(typ.typ)) }; - let typ = option(option_unresolved_type.clone(), typ).unwrap(); + let typ = option(option_unresolved_type.clone(), typ, location.span).unwrap(); Value::Tuple(vec![pattern, typ]) }) .collect(); @@ -1812,7 +1837,7 @@ fn expr_as_lambda( Some(return_type) }; let return_type = return_type.map(Value::UnresolvedType); - let return_type = option(option_unresolved_type, return_type).ok()?; + let return_type = option(option_unresolved_type, return_type, location.span).ok()?; let body = Value::expression(lambda.body.kind); @@ -1846,7 +1871,7 @@ fn expr_as_let( Some(Value::UnresolvedType(let_statement.r#type.typ)) }; - let typ = option(option_type, typ).ok()?; + let typ = option(option_type, typ, location.span).ok()?; Some(Value::Tuple(vec![ Value::pattern(let_statement.pattern), @@ -2098,7 +2123,7 @@ where let expr_value = unwrap_expr_value(interner, expr_value); let option_value = f(expr_value); - option(return_type, option_value) + option(return_type, option_value, location.span) } // fn resolve(self, in_function: Option) -> TypedExpr @@ -2764,12 +2789,12 @@ fn trait_def_as_trait_constraint( /// Creates a value that holds an `Option`. /// `option_type` must be a Type referencing the `Option` type. -pub(crate) fn option(option_type: Type, value: Option) -> IResult { +pub(crate) fn option(option_type: Type, value: Option, span: Span) -> IResult { let t = extract_option_generic_type(option_type.clone()); let (is_some, value) = match value { Some(value) => (Value::Bool(true), value), - None => (Value::Bool(false), zeroed(t)?), + None => (Value::Bool(false), zeroed(t, span)?), }; let mut fields = HashMap::default(); @@ -2818,9 +2843,10 @@ fn derive_generators( _ => panic!("ICE: Should only have an array return type"), }; - let Some(num_generators) = size.evaluate_to_u32() else { - return Err(InterpreterError::UnknownArrayLength { length: *size, location }); - }; + let num_generators = size.evaluate_to_u32(location.span).map_err(|err| { + let err = Box::new(err); + InterpreterError::UnknownArrayLength { length: *size, err, location } + })?; let generators = bn254_blackbox_solver::derive_generators( &domain_separator_string, diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index e1e60daff60..a170cb7c71c 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -5,7 +5,7 @@ use thiserror::Error; use crate::{ ast::{Ident, UnsupportedNumericGenericType}, - hir::comptime::InterpreterError, + hir::{comptime::InterpreterError, type_check::TypeCheckError}, parser::ParserError, usage_tracker::UnusedItem, Type, @@ -127,11 +127,12 @@ pub enum ResolverError { NamedTypeArgs { span: Span, item_kind: &'static str }, #[error("Associated constants may only be a field or integer type")] AssociatedConstantsMustBeNumeric { span: Span }, - #[error("Overflow in `{lhs} {op} {rhs}`")] - OverflowInType { + #[error("Computing `{lhs} {op} {rhs}` failed with error {err}")] + BinaryOpError { lhs: FieldElement, op: crate::BinaryTypeOperator, rhs: FieldElement, + err: Box, span: Span, }, #[error("`quote` cannot be used in runtime code")] @@ -534,10 +535,10 @@ impl<'a> From<&'a ResolverError> for Diagnostic { *span, ) } - ResolverError::OverflowInType { lhs, op, rhs, span } => { + ResolverError::BinaryOpError { lhs, op, rhs, err, span } => { Diagnostic::simple_error( - format!("Overflow in `{lhs} {op} {rhs}`"), - "Overflow here".to_string(), + format!("Computing `{lhs} {op} {rhs}` failed with error {err}"), + String::new(), *span, ) } diff --git a/compiler/noirc_frontend/src/hir/type_check/errors.rs b/compiler/noirc_frontend/src/hir/type_check/errors.rs index cb69053b72a..a63601a4280 100644 --- a/compiler/noirc_frontend/src/hir/type_check/errors.rs +++ b/compiler/noirc_frontend/src/hir/type_check/errors.rs @@ -11,7 +11,7 @@ use crate::ast::{ use crate::hir::resolution::errors::ResolverError; use crate::hir_def::expr::HirBinaryOp; use crate::hir_def::traits::TraitConstraint; -use crate::hir_def::types::Type; +use crate::hir_def::types::{BinaryTypeOperator, Kind, Type}; use crate::node_interner::NodeInterner; #[derive(Error, Debug, Clone, PartialEq, Eq)] @@ -34,12 +34,22 @@ pub enum Source { Return(FunctionReturnType, Span), } -#[derive(Error, Debug, Clone)] +#[derive(Error, Debug, Clone, PartialEq, Eq)] pub enum TypeCheckError { #[error("Operator {op:?} cannot be used in a {place:?}")] OpCannotBeUsed { op: HirBinaryOp, place: &'static str, span: Span }, + #[error("Division by zero: {lhs} / {rhs}")] + DivisionByZero { lhs: FieldElement, rhs: FieldElement, span: Span }, + #[error("Modulo on Field elements: {lhs} % {rhs}")] + ModuloOnFields { lhs: FieldElement, rhs: FieldElement, span: Span }, #[error("The value `{expr:?}` cannot fit into `{ty}` which has range `{range}`")] OverflowingAssignment { expr: FieldElement, ty: Type, range: String, span: Span }, + #[error( + "The value `{value}` cannot fit into `{kind}` which has a maximum size of `{maximum_size}`" + )] + OverflowingConstant { value: FieldElement, kind: Kind, maximum_size: FieldElement, span: Span }, + #[error("Evaluating `{op}` on `{lhs}`, `{rhs}` failed")] + FailingBinaryOp { op: BinaryTypeOperator, lhs: i128, rhs: i128, span: Span }, #[error("Type {typ:?} cannot be used in a {place:?}")] TypeCannotBeUsed { typ: Type, place: &'static str, span: Span }, #[error("Expected type {expected_typ:?} is not the same as {expr_typ:?}")] @@ -48,6 +58,14 @@ pub enum TypeCheckError { TypeMismatchWithSource { expected: Type, actual: Type, span: Span, source: Source }, #[error("Expected type {expected_kind:?} is not the same as {expr_kind:?}")] TypeKindMismatch { expected_kind: String, expr_kind: String, expr_span: Span }, + #[error("Evaluating {to} resulted in {to_value}, but {from_value} was expected")] + TypeCanonicalizationMismatch { + to: Type, + from: Type, + to_value: FieldElement, + from_value: FieldElement, + span: Span, + }, // TODO(https://github.com/noir-lang/noir/issues/6238): implement handling for larger types #[error("Expected type {expected_kind} when evaluating globals, but found {expr_kind} (this warning may become an error in the future)")] EvaluatedGlobalIsntU32 { expected_kind: String, expr_kind: String, expr_span: Span }, @@ -158,6 +176,8 @@ pub enum TypeCheckError { Unsafe { span: Span }, #[error("Converting an unconstrained fn to a non-unconstrained fn is unsafe")] UnsafeFn { span: Span }, + #[error("Expected a constant, but found `{typ}`")] + NonConstantEvaluated { typ: Type, span: Span }, #[error("Slices must have constant length")] NonConstantSliceLength { span: Span }, #[error("Only sized types may be used in the entry point to a program")] @@ -196,6 +216,10 @@ impl TypeCheckError { pub fn add_context(self, ctx: &'static str) -> Self { TypeCheckError::Context { err: Box::new(self), ctx } } + + pub(crate) fn is_non_constant_evaluated(&self) -> bool { + matches!(self, TypeCheckError::NonConstantEvaluated { .. }) + } } impl<'a> From<&'a TypeCheckError> for Diagnostic { @@ -216,6 +240,16 @@ impl<'a> From<&'a TypeCheckError> for Diagnostic { String::new(), *span, ), + TypeCheckError::DivisionByZero { lhs, rhs, span } => Diagnostic::simple_error( + format!("Division by zero: {lhs} / {rhs}"), + String::new(), + *span, + ), + TypeCheckError::ModuloOnFields { lhs, rhs, span } => Diagnostic::simple_error( + format!("Modulo on Field elements: {lhs} % {rhs}"), + String::new(), + *span, + ), TypeCheckError::TypeMismatch { expected_typ, expr_typ, expr_span } => { Diagnostic::simple_error( format!("Expected type {expected_typ}, found type {expr_typ}"), @@ -230,6 +264,13 @@ impl<'a> From<&'a TypeCheckError> for Diagnostic { *expr_span, ) } + TypeCheckError::TypeCanonicalizationMismatch { to, from, to_value, from_value, span } => { + Diagnostic::simple_error( + format!("Evaluating {to} resulted in {to_value}, but {from_value} was expected"), + format!("from evaluating {from} without simplifications"), + *span, + ) + } // TODO(https://github.com/noir-lang/noir/issues/6238): implement // handling for larger types TypeCheckError::EvaluatedGlobalIsntU32 { expected_kind, expr_kind, expr_span } => { @@ -320,11 +361,14 @@ impl<'a> From<&'a TypeCheckError> for Diagnostic { | TypeCheckError::AmbiguousBitWidth { span, .. } | TypeCheckError::IntegerAndFieldBinaryOperation { span } | TypeCheckError::OverflowingAssignment { span, .. } + | TypeCheckError::OverflowingConstant { span, .. } + | TypeCheckError::FailingBinaryOp { span, .. } | TypeCheckError::FieldModulo { span } | TypeCheckError::FieldNot { span } | TypeCheckError::ConstrainedReferenceToUnconstrained { span } | TypeCheckError::UnconstrainedReferenceToConstrained { span } | TypeCheckError::UnconstrainedSliceReturnToConstrained { span } + | TypeCheckError::NonConstantEvaluated { span, .. } | TypeCheckError::NonConstantSliceLength { span } | TypeCheckError::StringIndexAssign { span } | TypeCheckError::InvalidShiftSize { span } => { diff --git a/compiler/noirc_frontend/src/hir_def/stmt.rs b/compiler/noirc_frontend/src/hir_def/stmt.rs index b97e99583bb..0258cfd8ddb 100644 --- a/compiler/noirc_frontend/src/hir_def/stmt.rs +++ b/compiler/noirc_frontend/src/hir_def/stmt.rs @@ -105,7 +105,7 @@ impl HirPattern { } } - pub(crate) fn location(&self) -> Location { + pub fn location(&self) -> Location { match self { HirPattern::Identifier(ident) => ident.location, HirPattern::Mutable(_, location) diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 0eed79348e2..97a39297ab1 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -91,6 +91,14 @@ pub enum Type { /// like `fn foo(...) {}`. Unlike TypeVariables, they cannot be bound over. NamedGeneric(TypeVariable, Rc), + /// A cast (to, from) that's checked at monomorphization. + /// + /// Simplifications on arithmetic generics are only allowed on the LHS. + CheckedCast { + from: Box, + to: Box, + }, + /// A functions with arguments, a return type and environment. /// the environment should be `Unit` by default, /// for closures it should contain a `Tuple` type with the captured @@ -255,10 +263,21 @@ impl Kind { } /// Ensure the given value fits in self.integral_maximum_size() - fn ensure_value_fits(&self, value: FieldElement) -> Option { + fn ensure_value_fits( + &self, + value: FieldElement, + span: Span, + ) -> Result { match self.integral_maximum_size() { - None => Some(value), - Some(maximum_size) => (value <= maximum_size).then_some(value), + None => Ok(value), + Some(maximum_size) => (value <= maximum_size).then_some(value).ok_or_else(|| { + TypeCheckError::OverflowingConstant { + value, + kind: self.clone(), + maximum_size, + span, + } + }), } } } @@ -863,6 +882,7 @@ impl std::fmt::Display for Type { TypeBinding::Unbound(_, _) if name.is_empty() => write!(f, "_"), TypeBinding::Unbound(_, _) => write!(f, "{name}"), }, + Type::CheckedCast { to, .. } => write!(f, "{to}"), Type::Constant(x, _kind) => write!(f, "{x}"), Type::Forall(typevars, typ) => { let typevars = vecmap(typevars, |var| var.id().to_string()); @@ -887,7 +907,7 @@ impl std::fmt::Display for Type { } Type::Quoted(quoted) => write!(f, "{}", quoted), Type::InfixExpr(lhs, op, rhs) => { - let this = self.canonicalize(); + let this = self.canonicalize_checked(); // Prevent infinite recursion if this != *self { @@ -1064,6 +1084,7 @@ impl Type { | Type::TypeVariable(..) | Type::TraitAsType(..) | Type::NamedGeneric(..) + | Type::CheckedCast { .. } | Type::Forall(..) | Type::Constant(..) | Type::Quoted(..) @@ -1106,6 +1127,10 @@ impl Type { Type::NamedGeneric(_, _) => { named_generic_is_numeric(self, found_names); } + Type::CheckedCast { from, to } => { + to.find_numeric_type_vars(found_names); + from.find_numeric_type_vars(found_names); + } Type::TraitAsType(_, _, args) => { for arg in args.ordered.iter() { @@ -1190,6 +1215,8 @@ impl Type { | Type::InfixExpr(_, _, _) | Type::TraitAsType(..) => false, + Type::CheckedCast { to, .. } => to.is_valid_for_program_input(), + Type::Alias(alias, generics) => { let alias = alias.borrow(); alias.get_type(generics).is_valid_for_program_input() @@ -1239,6 +1266,8 @@ impl Type { | Type::Quoted(_) | Type::TraitAsType(..) => false, + Type::CheckedCast { to, .. } => to.is_valid_non_inlined_function_input(), + Type::Alias(alias, generics) => { let alias = alias.borrow(); alias.get_type(generics).is_valid_non_inlined_function_input() @@ -1280,6 +1309,8 @@ impl Type { } } + Type::CheckedCast { to, .. } => to.is_valid_for_unconstrained_boundary(), + // Quoted objects only exist at compile-time where the only execution // environment is the interpreter. In this environment, they are valid. Type::Quoted(_) => true, @@ -1312,6 +1343,7 @@ impl Type { pub fn generic_count(&self) -> usize { match self { Type::Forall(generics, _) => generics.len(), + Type::CheckedCast { to, .. } => to.generic_count(), Type::TypeVariable(type_variable) | Type::NamedGeneric(type_variable, _) => { match &*type_variable.borrow() { TypeBinding::Bound(binding) => binding.generic_count(), @@ -1351,6 +1383,7 @@ impl Type { pub(crate) fn kind(&self) -> Kind { match self { + Type::CheckedCast { to, .. } => to.kind(), Type::NamedGeneric(var, _) => var.kind(), Type::Constant(_, kind) => kind.clone(), Type::TypeVariable(var) => match &*var.borrow() { @@ -1391,27 +1424,28 @@ impl Type { } /// Returns the number of field elements required to represent the type once encoded. - pub fn field_count(&self) -> u32 { + pub fn field_count(&self, location: &Location) -> u32 { match self { Type::FieldElement | Type::Integer { .. } | Type::Bool => 1, Type::Array(size, typ) => { let length = size - .evaluate_to_u32() + .evaluate_to_u32(location.span) .expect("Cannot have variable sized arrays as a parameter to main"); let typ = typ.as_ref(); - length * typ.field_count() + length * typ.field_count(location) } Type::Struct(def, args) => { let struct_type = def.borrow(); let fields = struct_type.get_fields(args); - fields.iter().fold(0, |acc, (_, field_type)| acc + field_type.field_count()) + fields.iter().fold(0, |acc, (_, field_type)| acc + field_type.field_count(location)) } - Type::Alias(def, generics) => def.borrow().get_type(generics).field_count(), + Type::CheckedCast { to, .. } => to.field_count(location), + Type::Alias(def, generics) => def.borrow().get_type(generics).field_count(location), Type::Tuple(fields) => { - fields.iter().fold(0, |acc, field_typ| acc + field_typ.field_count()) + fields.iter().fold(0, |acc, field_typ| acc + field_typ.field_count(location)) } Type::String(size) => size - .evaluate_to_u32() + .evaluate_to_u32(location.span) .expect("Cannot have variable sized strings as a parameter to main"), Type::FmtString(_, _) | Type::Unit @@ -1582,6 +1616,7 @@ impl Type { match self { Type::TypeVariable(var) => Some((var.1.clone(), var.kind())), Type::NamedGeneric(var, _) => Some((var.1.clone(), var.kind())), + Type::CheckedCast { to, .. } => to.get_inner_type_variable(), _ => None, } } @@ -1696,6 +1731,10 @@ impl Type { } } + (CheckedCast { to, .. }, other) | (other, CheckedCast { to, .. }) => { + to.try_unify(other, bindings) + } + (NamedGeneric(binding, _), other) | (other, NamedGeneric(binding, _)) if !binding.borrow().is_unbound() => { @@ -1758,7 +1797,8 @@ impl Type { } (Constant(value, kind), other) | (other, Constant(value, kind)) => { - if let Some(other_value) = other.evaluate_to_field_element(kind) { + let dummy_span = Span::default(); + if let Ok(other_value) = other.evaluate_to_field_element(kind, dummy_span) { if *value == other_value && kind.unifies(&other.kind()) { Ok(()) } else { @@ -1926,41 +1966,106 @@ impl Type { /// If this type is a Type::Constant (used in array lengths), or is bound /// to a Type::Constant, return the constant as a u32. - pub fn evaluate_to_u32(&self) -> Option { - self.evaluate_to_field_element(&Kind::u32()) - .and_then(|field_element| field_element.try_to_u32()) + pub fn evaluate_to_u32(&self, span: Span) -> Result { + self.evaluate_to_field_element(&Kind::u32(), span).map(|field_element| { + field_element + .try_to_u32() + .expect("ICE: size should have already been checked by evaluate_to_field_element") + }) } // TODO(https://github.com/noir-lang/noir/issues/6260): remove // the unifies checks once all kinds checks are implemented? - pub(crate) fn evaluate_to_field_element(&self, kind: &Kind) -> Option { + pub(crate) fn evaluate_to_field_element( + &self, + kind: &Kind, + span: Span, + ) -> Result { + let run_simplifications = true; + self.evaluate_to_field_element_helper(kind, span, run_simplifications) + } + + /// evaluate_to_field_element with optional generic arithmetic simplifications + pub(crate) fn evaluate_to_field_element_helper( + &self, + kind: &Kind, + span: Span, + run_simplifications: bool, + ) -> Result { if let Some((binding, binding_kind)) = self.get_inner_type_variable() { if let TypeBinding::Bound(binding) = &*binding.borrow() { if kind.unifies(&binding_kind) { - return binding.evaluate_to_field_element(&binding_kind); + return binding.evaluate_to_field_element_helper( + &binding_kind, + span, + run_simplifications, + ); } } } - match self.canonicalize() { + let could_be_checked_cast = false; + match self.canonicalize_helper(could_be_checked_cast, run_simplifications) { Type::Constant(x, constant_kind) => { if kind.unifies(&constant_kind) { - kind.ensure_value_fits(x) + kind.ensure_value_fits(x, span) } else { - None + Err(TypeCheckError::TypeKindMismatch { + expected_kind: format!("{}", constant_kind), + expr_kind: format!("{}", kind), + expr_span: span, + }) } } Type::InfixExpr(lhs, op, rhs) => { let infix_kind = lhs.infix_kind(&rhs); if kind.unifies(&infix_kind) { - let lhs_value = lhs.evaluate_to_field_element(&infix_kind)?; - let rhs_value = rhs.evaluate_to_field_element(&infix_kind)?; - op.function(lhs_value, rhs_value, &infix_kind) + let lhs_value = lhs.evaluate_to_field_element_helper( + &infix_kind, + span, + run_simplifications, + )?; + let rhs_value = rhs.evaluate_to_field_element_helper( + &infix_kind, + span, + run_simplifications, + )?; + op.function(lhs_value, rhs_value, &infix_kind, span) } else { - None + Err(TypeCheckError::TypeKindMismatch { + expected_kind: format!("{}", kind), + expr_kind: format!("{}", infix_kind), + expr_span: span, + }) } } - _ => None, + Type::CheckedCast { from, to } => { + let to_value = to.evaluate_to_field_element(kind, span)?; + + // if both 'to' and 'from' evaluate to a constant, + // return None unless they match + let skip_simplifications = false; + if let Ok(from_value) = + from.evaluate_to_field_element_helper(kind, span, skip_simplifications) + { + if to_value == from_value { + Ok(to_value) + } else { + let to = *to.clone(); + let from = *from.clone(); + Err(TypeCheckError::TypeCanonicalizationMismatch { + to, + from, + to_value, + from_value, + span, + }) + } + } else { + Ok(to_value) + } + } + other => Err(TypeCheckError::NonConstantEvaluated { typ: other, span }), } } @@ -2182,6 +2287,11 @@ impl Type { let fields = fields.substitute_helper(type_bindings, substitute_bound_typevars); Type::FmtString(Box::new(size), Box::new(fields)) } + Type::CheckedCast { from, to } => { + let from = from.substitute_helper(type_bindings, substitute_bound_typevars); + let to = to.substitute_helper(type_bindings, substitute_bound_typevars); + Type::CheckedCast { from: Box::new(from), to: Box::new(to) } + } Type::NamedGeneric(binding, _) | Type::TypeVariable(binding) => { substitute_binding(binding) } @@ -2271,6 +2381,7 @@ impl Type { || args.named.iter().any(|arg| arg.typ.occurs(target_id)) } Type::Tuple(fields) => fields.iter().any(|field| field.occurs(target_id)), + Type::CheckedCast { from, to } => from.occurs(target_id) || to.occurs(target_id), Type::NamedGeneric(type_var, _) | Type::TypeVariable(type_var) => { match &*type_var.borrow() { TypeBinding::Bound(binding) => { @@ -2329,6 +2440,11 @@ impl Type { def.borrow().get_type(args).follow_bindings() } Tuple(args) => Tuple(vecmap(args, |arg| arg.follow_bindings())), + CheckedCast { from, to } => { + let from = Box::new(from.follow_bindings()); + let to = Box::new(to.follow_bindings()); + CheckedCast { from, to } + } TypeVariable(var) | NamedGeneric(var, _) => { if let TypeBinding::Bound(typ) = &*var.borrow() { return typ.follow_bindings(); @@ -2425,6 +2541,10 @@ impl Type { generic.typ.replace_named_generics_with_type_variables(); } } + Type::CheckedCast { from, to } => { + from.replace_named_generics_with_type_variables(); + to.replace_named_generics_with_type_variables(); + } Type::NamedGeneric(var, _) => { let type_binding = var.borrow(); if let TypeBinding::Bound(binding) = &*type_binding { @@ -2482,6 +2602,7 @@ impl Type { } } Type::Alias(alias, args) => alias.borrow().get_type(args).integral_maximum_size(), + Type::CheckedCast { to, .. } => to.integral_maximum_size(), Type::NamedGeneric(binding, _name) => match &*binding.borrow() { TypeBinding::Bound(typ) => typ.integral_maximum_size(), TypeBinding::Unbound(_, kind) => kind.integral_maximum_size(), @@ -2541,28 +2662,39 @@ fn convert_array_expression_to_slice( impl BinaryTypeOperator { /// Perform the actual rust numeric operation associated with this operator - pub fn function(self, a: FieldElement, b: FieldElement, kind: &Kind) -> Option { + pub fn function( + self, + a: FieldElement, + b: FieldElement, + kind: &Kind, + span: Span, + ) -> Result { match kind.follow_bindings().integral_maximum_size() { None => match self { - BinaryTypeOperator::Addition => Some(a + b), - BinaryTypeOperator::Subtraction => Some(a - b), - BinaryTypeOperator::Multiplication => Some(a * b), - BinaryTypeOperator::Division => (b != FieldElement::zero()).then(|| a / b), - BinaryTypeOperator::Modulo => None, + BinaryTypeOperator::Addition => Ok(a + b), + BinaryTypeOperator::Subtraction => Ok(a - b), + BinaryTypeOperator::Multiplication => Ok(a * b), + BinaryTypeOperator::Division => (b != FieldElement::zero()) + .then(|| a / b) + .ok_or(TypeCheckError::DivisionByZero { lhs: a, rhs: b, span }), + BinaryTypeOperator::Modulo => { + Err(TypeCheckError::ModuloOnFields { lhs: a, rhs: b, span }) + } }, Some(_maximum_size) => { let a = a.to_i128(); let b = b.to_i128(); + let err = TypeCheckError::FailingBinaryOp { op: self, lhs: a, rhs: b, span }; let result = match self { - BinaryTypeOperator::Addition => a.checked_add(b)?, - BinaryTypeOperator::Subtraction => a.checked_sub(b)?, - BinaryTypeOperator::Multiplication => a.checked_mul(b)?, - BinaryTypeOperator::Division => a.checked_div(b)?, - BinaryTypeOperator::Modulo => a.checked_rem(b)?, + BinaryTypeOperator::Addition => a.checked_add(b).ok_or(err)?, + BinaryTypeOperator::Subtraction => a.checked_sub(b).ok_or(err)?, + BinaryTypeOperator::Multiplication => a.checked_mul(b).ok_or(err)?, + BinaryTypeOperator::Division => a.checked_div(b).ok_or(err)?, + BinaryTypeOperator::Modulo => a.checked_rem(b).ok_or(err)?, }; - Some(result.into()) + Ok(result.into()) } } } @@ -2607,7 +2739,9 @@ impl From<&Type> for PrintableType { match value { Type::FieldElement => PrintableType::Field, Type::Array(size, typ) => { - let length = size.evaluate_to_u32().expect("Cannot print variable sized arrays"); + let dummy_span = Span::default(); + let length = + size.evaluate_to_u32(dummy_span).expect("Cannot print variable sized arrays"); let typ = typ.as_ref(); PrintableType::Array { length, typ: Box::new(typ.into()) } } @@ -2632,7 +2766,9 @@ impl From<&Type> for PrintableType { }, Type::Bool => PrintableType::Boolean, Type::String(size) => { - let size = size.evaluate_to_u32().expect("Cannot print variable sized strings"); + let dummy_span = Span::default(); + let size = + size.evaluate_to_u32(dummy_span).expect("Cannot print variable sized strings"); PrintableType::String { length: size } } Type::FmtString(_, _) => unreachable!("format strings cannot be printed"), @@ -2648,6 +2784,7 @@ impl From<&Type> for PrintableType { Type::Alias(alias, args) => alias.borrow().get_type(args).into(), Type::TraitAsType(..) => unreachable!(), Type::Tuple(types) => PrintableType::Tuple { types: vecmap(types, |typ| typ.into()) }, + Type::CheckedCast { to, .. } => to.as_ref().into(), Type::NamedGeneric(..) => unreachable!(), Type::Forall(..) => unreachable!(), Type::Function(arguments, return_type, env, unconstrained) => PrintableType::Function { @@ -2722,6 +2859,7 @@ impl std::fmt::Debug for Type { } Type::Unit => write!(f, "()"), Type::Error => write!(f, "error"), + Type::CheckedCast { to, .. } => write!(f, "{:?}", to), Type::NamedGeneric(binding, name) => match binding.kind() { Kind::Any | Kind::Normal | Kind::Integer | Kind::IntegerOrField => { write!(f, "{}{:?}", name, binding) @@ -2836,6 +2974,7 @@ impl std::hash::Hash for Type { vars.hash(state); typ.hash(state); } + Type::CheckedCast { to, .. } => to.hash(state), Type::Constant(value, _) => value.hash(state), Type::Quoted(typ) => typ.hash(state), Type::InfixExpr(lhs, op, rhs) => { @@ -2902,6 +3041,7 @@ impl PartialEq for Type { (Forall(lhs_vars, lhs_type), Forall(rhs_vars, rhs_type)) => { lhs_vars == rhs_vars && lhs_type == rhs_type } + (CheckedCast { to, .. }, other) | (other, CheckedCast { to, .. }) => **to == *other, (Constant(lhs, lhs_kind), Constant(rhs, rhs_kind)) => { lhs == rhs && lhs_kind == rhs_kind } diff --git a/compiler/noirc_frontend/src/hir_def/types/arithmetic.rs b/compiler/noirc_frontend/src/hir_def/types/arithmetic.rs index 81f638ebca4..263a102bee1 100644 --- a/compiler/noirc_frontend/src/hir_def/types/arithmetic.rs +++ b/compiler/noirc_frontend/src/hir_def/types/arithmetic.rs @@ -1,6 +1,7 @@ use std::collections::BTreeMap; use acvm::{AcirField, FieldElement}; +use noirc_errors::Span; use crate::{BinaryTypeOperator, Type, TypeBindings, UnificationError}; @@ -15,34 +16,81 @@ impl Type { /// - `canonicalize[((1 + N) + M) + 2] = (M + N) + 3` /// - `canonicalize[A + 2 * B + 3 - 2] = A + (B * 2) + 3 - 2` pub fn canonicalize(&self) -> Type { + match self.follow_bindings() { + Type::CheckedCast { from, to } => Type::CheckedCast { + from: Box::new(from.canonicalize_checked()), + to: Box::new(to.canonicalize_unchecked()), + }, + + other => { + let non_checked_cast = false; + let run_simplifications = true; + other.canonicalize_helper(non_checked_cast, run_simplifications) + } + } + } + + /// Only simplify constants and drop/skip any CheckedCast's + pub(crate) fn canonicalize_checked(&self) -> Type { + let found_checked_cast = true; + let skip_simplifications = false; + self.canonicalize_helper(found_checked_cast, skip_simplifications) + } + + /// Run all simplifications and drop/skip any CheckedCast's + fn canonicalize_unchecked(&self) -> Type { + let found_checked_cast = true; + let run_simplifications = true; + self.canonicalize_helper(found_checked_cast, run_simplifications) + } + + /// If found_checked_cast, then drop additional CheckedCast's + /// + /// If run_simplifications is false, then only: + /// - Attempt to evaluate each sub-expression to a constant + /// - Drop nested CheckedCast's + /// + /// Otherwise also attempt try_simplify_partial_constants, sort_commutative, + /// and other simplifications + pub(crate) fn canonicalize_helper( + &self, + found_checked_cast: bool, + run_simplifications: bool, + ) -> Type { match self.follow_bindings() { Type::InfixExpr(lhs, op, rhs) => { let kind = lhs.infix_kind(&rhs); + let dummy_span = Span::default(); // evaluate_to_field_element also calls canonicalize so if we just called // `self.evaluate_to_field_element(..)` we'd get infinite recursion. - if let (Some(lhs_u32), Some(rhs_u32)) = - (lhs.evaluate_to_field_element(&kind), rhs.evaluate_to_field_element(&kind)) - { - let kind = lhs.infix_kind(&rhs); - if let Some(result) = op.function(lhs_u32, rhs_u32, &kind) { + if let (Ok(lhs_value), Ok(rhs_value)) = ( + lhs.evaluate_to_field_element_helper(&kind, dummy_span, run_simplifications), + rhs.evaluate_to_field_element_helper(&kind, dummy_span, run_simplifications), + ) { + if let Ok(result) = op.function(lhs_value, rhs_value, &kind, dummy_span) { return Type::Constant(result, kind); } } - let lhs = lhs.canonicalize(); - let rhs = rhs.canonicalize(); + let lhs = lhs.canonicalize_helper(found_checked_cast, run_simplifications); + let rhs = rhs.canonicalize_helper(found_checked_cast, run_simplifications); + + if !run_simplifications { + return Type::InfixExpr(Box::new(lhs), op, Box::new(rhs)); + } + if let Some(result) = Self::try_simplify_non_constants_in_lhs(&lhs, op, &rhs) { - return result.canonicalize(); + return result.canonicalize_unchecked(); } if let Some(result) = Self::try_simplify_non_constants_in_rhs(&lhs, op, &rhs) { - return result.canonicalize(); + return result.canonicalize_unchecked(); } // Try to simplify partially constant expressions in the form `(N op1 C1) op2 C2` // where C1 and C2 are constants that can be combined (e.g. N + 5 - 3 = N + 2) if let Some(result) = Self::try_simplify_partial_constants(&lhs, op, &rhs) { - return result.canonicalize(); + return result.canonicalize_unchecked(); } if op.is_commutative() { @@ -51,6 +99,18 @@ impl Type { Type::InfixExpr(Box::new(lhs), op, Box::new(rhs)) } + Type::CheckedCast { from, to } => { + let inner_found_checked_cast = true; + let to = to.canonicalize_helper(inner_found_checked_cast, run_simplifications); + + if found_checked_cast { + return to; + } + + let from = from.canonicalize_checked(); + + Type::CheckedCast { from: Box::new(from), to: Box::new(to) } + } other => other, } } @@ -70,13 +130,16 @@ impl Type { // Push each non-constant term to `sorted` to sort them. Recur on InfixExprs with the same operator. while let Some(item) = queue.pop() { - match item.canonicalize() { - Type::InfixExpr(lhs, new_op, rhs) if new_op == op => { - queue.push(*lhs); - queue.push(*rhs); + match item.canonicalize_unchecked() { + Type::InfixExpr(lhs_inner, new_op, rhs_inner) if new_op == op => { + queue.push(*lhs_inner); + queue.push(*rhs_inner); } Type::Constant(new_constant, new_constant_kind) => { - if let Some(result) = op.function(constant, new_constant, &new_constant_kind) { + let dummy_span = Span::default(); + if let Ok(result) = + op.function(constant, new_constant, &new_constant_kind, dummy_span) + { constant = result; } else { let constant = Type::Constant(new_constant, new_constant_kind); @@ -134,7 +197,7 @@ impl Type { // `rhs` is expected to already be in canonical form. if l_op.approx_inverse() != Some(op) || l_op == BinaryTypeOperator::Division - || l_rhs.canonicalize() != *rhs + || l_rhs.canonicalize_unchecked() != *rhs { return None; } @@ -168,7 +231,7 @@ impl Type { // Note that this is exact, syntactic equality, not unification. // `lhs` is expected to already be in canonical form. - if r_op.inverse() != Some(op) || *lhs != r_rhs.canonicalize() { + if r_op.inverse() != Some(op) || *lhs != r_rhs.canonicalize_unchecked() { return None; } @@ -187,13 +250,15 @@ impl Type { rhs: &Type, ) -> Option<(Box, BinaryTypeOperator, FieldElement, FieldElement)> { let kind = lhs.infix_kind(rhs); - let rhs = rhs.evaluate_to_field_element(&kind)?; + let dummy_span = Span::default(); + let rhs = rhs.evaluate_to_field_element(&kind, dummy_span).ok()?; let Type::InfixExpr(l_type, l_op, l_rhs) = lhs.follow_bindings() else { return None; }; - let l_rhs = l_rhs.evaluate_to_field_element(&kind)?; + let dummy_span = Span::default(); + let l_rhs = l_rhs.evaluate_to_field_element(&kind, dummy_span).ok()?; Some((l_type, l_op, l_rhs, rhs)) } @@ -218,7 +283,9 @@ impl Type { if l_op == Subtraction { op = op.inverse()?; } - let result = op.function(l_const, r_const, &lhs.infix_kind(rhs))?; + let dummy_span = Span::default(); + let result = + op.function(l_const, r_const, &lhs.infix_kind(rhs), dummy_span).ok()?; let constant = Type::Constant(result, lhs.infix_kind(rhs)); Some(Type::InfixExpr(l_type, l_op, Box::new(constant))) } @@ -231,7 +298,9 @@ impl Type { if op == Division && (r_const == FieldElement::zero() || !divides_evenly) { None } else { - let result = op.function(l_const, r_const, &lhs.infix_kind(rhs))?; + let dummy_span = Span::default(); + let result = + op.function(l_const, r_const, &lhs.infix_kind(rhs), dummy_span).ok()?; let constant = Box::new(Type::Constant(result, lhs.infix_kind(rhs))); Some(Type::InfixExpr(l_type, l_op, constant)) } @@ -250,7 +319,8 @@ impl Type { if let Type::InfixExpr(lhs_a, op_a, rhs_a) = self { if let Some(inverse) = op_a.approx_inverse() { let kind = lhs_a.infix_kind(rhs_a); - if let Some(rhs_a_value) = rhs_a.evaluate_to_field_element(&kind) { + let dummy_span = Span::default(); + if let Ok(rhs_a_value) = rhs_a.evaluate_to_field_element(&kind, dummy_span) { let rhs_a = Box::new(Type::Constant(rhs_a_value, kind)); let new_other = Type::InfixExpr(Box::new(other.clone()), inverse, rhs_a); @@ -266,7 +336,8 @@ impl Type { if let Type::InfixExpr(lhs_b, op_b, rhs_b) = other { if let Some(inverse) = op_b.approx_inverse() { let kind = lhs_b.infix_kind(rhs_b); - if let Some(rhs_b_value) = rhs_b.evaluate_to_field_element(&kind) { + let dummy_span = Span::default(); + if let Ok(rhs_b_value) = rhs_b.evaluate_to_field_element(&kind, dummy_span) { let rhs_b = Box::new(Type::Constant(rhs_b_value, kind)); let new_self = Type::InfixExpr(Box::new(self.clone()), inverse, rhs_b); diff --git a/compiler/noirc_frontend/src/monomorphization/errors.rs b/compiler/noirc_frontend/src/monomorphization/errors.rs index e2ff94f521b..c137a6fc90a 100644 --- a/compiler/noirc_frontend/src/monomorphization/errors.rs +++ b/compiler/noirc_frontend/src/monomorphization/errors.rs @@ -1,10 +1,13 @@ use noirc_errors::{CustomDiagnostic, FileDiagnostic, Location}; -use crate::{hir::comptime::InterpreterError, Type}; +use crate::{ + hir::{comptime::InterpreterError, type_check::TypeCheckError}, + Type, +}; #[derive(Debug)] pub enum MonomorphizationError { - UnknownArrayLength { length: Type, location: Location }, + UnknownArrayLength { length: Type, err: TypeCheckError, location: Location }, UnknownConstant { location: Location }, NoDefaultType { location: Location }, InternalError { message: &'static str, location: Location }, @@ -12,6 +15,7 @@ pub enum MonomorphizationError { ComptimeFnInRuntimeCode { name: String, location: Location }, ComptimeTypeInRuntimeCode { typ: String, location: Location }, CheckedTransmuteFailed { actual: Type, expected: Type, location: Location }, + CheckedCastFailed { actual: Type, expected: Type, location: Location }, } impl MonomorphizationError { @@ -23,6 +27,7 @@ impl MonomorphizationError { | MonomorphizationError::ComptimeFnInRuntimeCode { location, .. } | MonomorphizationError::ComptimeTypeInRuntimeCode { location, .. } | MonomorphizationError::CheckedTransmuteFailed { location, .. } + | MonomorphizationError::CheckedCastFailed { location, .. } | MonomorphizationError::NoDefaultType { location, .. } => *location, MonomorphizationError::InterpreterError(error) => error.get_location(), } @@ -41,14 +46,17 @@ impl From for FileDiagnostic { impl MonomorphizationError { fn into_diagnostic(self) -> CustomDiagnostic { let message = match &self { - MonomorphizationError::UnknownArrayLength { length, .. } => { - format!("Could not determine array length `{length}`") + MonomorphizationError::UnknownArrayLength { length, err, .. } => { + format!("Could not determine array length `{length}`, encountered error: `{err}`") } MonomorphizationError::UnknownConstant { .. } => { "Could not resolve constant".to_string() } MonomorphizationError::CheckedTransmuteFailed { actual, expected, .. } => { - format!("checked_transmute failed: `{actual}` != `{expected}`") + format!("checked_transmute failed: `{actual:?}` != `{expected:?}`") + } + MonomorphizationError::CheckedCastFailed { actual, expected, .. } => { + format!("Arithmetic generics simplification failed: `{actual:?}` != `{expected:?}`") } MonomorphizationError::NoDefaultType { location } => { let message = "Type annotation needed".into(); diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 3ca4c5651ec..7e1dfda9ffb 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -10,7 +10,7 @@ //! function, will monomorphize the entire reachable program. use crate::ast::{FunctionKind, IntegerBitSize, Signedness, UnaryOp, Visibility}; use crate::hir::comptime::InterpreterError; -use crate::hir::type_check::NoMatchingImplFoundError; +use crate::hir::type_check::{NoMatchingImplFoundError, TypeCheckError}; use crate::node_interner::{ExprId, ImplSearchErrorKind}; use crate::{ debug::DebugInstrumenter, @@ -576,9 +576,9 @@ impl<'interner> Monomorphizer<'interner> { let location = self.interner.expr_location(&array); let typ = Self::convert_type(&self.interner.id_type(array), location)?; - let length = length.evaluate_to_u32().ok_or_else(|| { + let length = length.evaluate_to_u32(location.span).map_err(|err| { let location = self.interner.expr_location(&array); - MonomorphizationError::UnknownArrayLength { location, length } + MonomorphizationError::UnknownArrayLength { location, err, length } })?; let contents = try_vecmap(0..length, |_| self.expr(repeated_element))?; @@ -928,11 +928,19 @@ impl<'interner> Monomorphizer<'interner> { TypeBinding::Unbound(_, _) => { unreachable!("Unbound type variable used in expression") } - TypeBinding::Bound(binding) => binding - .evaluate_to_field_element(&Kind::Numeric(numeric_typ.clone())) - .unwrap_or_else(|| { - panic!("Non-numeric type variable used in expression expecting a value") - }), + TypeBinding::Bound(binding) => { + let location = self.interner.id_location(expr_id); + binding + .evaluate_to_field_element( + &Kind::Numeric(numeric_typ.clone()), + location.span, + ) + .map_err(|err| MonomorphizationError::UnknownArrayLength { + length: binding.clone(), + err, + location, + })? + } }; let location = self.interner.id_location(expr_id); @@ -957,20 +965,51 @@ impl<'interner> Monomorphizer<'interner> { HirType::FieldElement => ast::Type::Field, HirType::Integer(sign, bits) => ast::Type::Integer(*sign, *bits), HirType::Bool => ast::Type::Bool, - HirType::String(size) => ast::Type::String(size.evaluate_to_u32().unwrap_or(0)), + HirType::String(size) => { + let size = match size.evaluate_to_u32(location.span) { + Ok(size) => size, + // only default variable sizes to size 0 + Err(TypeCheckError::NonConstantEvaluated { .. }) => 0, + Err(err) => { + let length = size.as_ref().clone(); + return Err(MonomorphizationError::UnknownArrayLength { + location, + err, + length, + }); + } + }; + ast::Type::String(size) + } HirType::FmtString(size, fields) => { - let size = size.evaluate_to_u32().unwrap_or(0); + let size = match size.evaluate_to_u32(location.span) { + Ok(size) => size, + // only default variable sizes to size 0 + Err(TypeCheckError::NonConstantEvaluated { .. }) => 0, + Err(err) => { + let length = size.as_ref().clone(); + return Err(MonomorphizationError::UnknownArrayLength { + location, + err, + length, + }); + } + }; let fields = Box::new(Self::convert_type(fields.as_ref(), location)?); ast::Type::FmtString(size, fields) } HirType::Unit => ast::Type::Unit, HirType::Array(length, element) => { let element = Box::new(Self::convert_type(element.as_ref(), location)?); - let length = match length.evaluate_to_u32() { - Some(length) => length, - None => { + let length = match length.evaluate_to_u32(location.span) { + Ok(length) => length, + Err(err) => { let length = length.as_ref().clone(); - return Err(MonomorphizationError::UnknownArrayLength { location, length }); + return Err(MonomorphizationError::UnknownArrayLength { + location, + err, + length, + }); } }; ast::Type::Array(length, element) @@ -994,6 +1033,11 @@ impl<'interner> Monomorphizer<'interner> { ast::Type::Field } + HirType::CheckedCast { from, to } => { + Self::check_checked_cast(from, to, location)?; + Self::convert_type(to, location)? + } + HirType::TypeVariable(ref binding) => { let type_var_kind = match &*binding.borrow() { TypeBinding::Bound(ref binding) => { @@ -1100,6 +1144,11 @@ impl<'interner> Monomorphizer<'interner> { Ok(()) } } + HirType::CheckedCast { from, to } => { + Self::check_checked_cast(from, to, location)?; + Self::check_type(to, location) + } + HirType::FmtString(_size, fields) => Self::check_type(fields.as_ref(), location), HirType::Array(_length, element) => Self::check_type(element.as_ref(), location), HirType::Slice(element) => Self::check_type(element.as_ref(), location), @@ -1171,6 +1220,40 @@ impl<'interner> Monomorphizer<'interner> { } } + /// Check that the 'from' and to' sides of a CheckedCast unify and + /// that if the 'to' side evaluates to a field element, that the 'from' side + /// evaluates to the same field element + fn check_checked_cast( + from: &Type, + to: &Type, + location: Location, + ) -> Result<(), MonomorphizationError> { + if from.unify(to).is_err() { + return Err(MonomorphizationError::CheckedCastFailed { + actual: to.clone(), + expected: from.clone(), + location, + }); + } + let to_value = to.evaluate_to_field_element(&to.kind(), location.span); + if to_value.is_ok() { + let skip_simplifications = false; + let from_value = from.evaluate_to_field_element_helper( + &to.kind(), + location.span, + skip_simplifications, + ); + if from_value.is_err() || from_value.unwrap() != to_value.clone().unwrap() { + return Err(MonomorphizationError::CheckedCastFailed { + actual: HirType::Constant(to_value.unwrap(), to.kind()), + expected: from.clone(), + location, + }); + } + } + Ok(()) + } + fn is_function_closure(&self, t: ast::Type) -> bool { if self.is_function_closure_type(&t) { true diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index c743d564339..736d37fe83f 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -2439,6 +2439,7 @@ fn get_type_method_key(typ: &Type) -> Option { | Type::Error | Type::Struct(_, _) | Type::InfixExpr(..) + | Type::CheckedCast { .. } | Type::TraitAsType(..) => None, } } diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 17accbd8366..056113a6e89 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -16,6 +16,7 @@ mod visibility; // A test harness will allow for more expressive and readable tests use std::collections::BTreeMap; +use acvm::{AcirField, FieldElement}; use fm::FileId; use iter_extended::vecmap; @@ -36,6 +37,9 @@ use crate::hir::def_collector::dc_crate::DefCollector; use crate::hir::def_map::{CrateDefMap, LocalModuleId}; use crate::hir_def::expr::HirExpression; use crate::hir_def::stmt::HirStatement; +use crate::hir_def::types::{BinaryTypeOperator, Type}; +use crate::monomorphization::ast::Program; +use crate::monomorphization::errors::MonomorphizationError; use crate::monomorphization::monomorphize; use crate::parser::{ItemKind, ParserErrorReason}; use crate::token::SecondaryAttribute; @@ -1239,10 +1243,18 @@ fn resolve_fmt_strings() { } } -fn check_rewrite(src: &str, expected: &str) { +fn monomorphize_program(src: &str) -> Result { let (_program, mut context, _errors) = get_program(src); let main_func_id = context.def_interner.find_function("main").unwrap(); - let program = monomorphize(main_func_id, &mut context.def_interner).unwrap(); + monomorphize(main_func_id, &mut context.def_interner) +} + +fn get_monomorphization_error(src: &str) -> Option { + monomorphize_program(src).err() +} + +fn check_rewrite(src: &str, expected: &str) { + let program = monomorphize_program(src).unwrap(); assert!(format!("{}", program) == expected); } @@ -3208,6 +3220,103 @@ fn arithmetic_generics_canonicalization_deduplication_regression() { assert_eq!(errors.len(), 0); } +#[test] +fn arithmetic_generics_checked_cast_zeros() { + let source = r#" + struct W {} + + fn foo(_x: W) -> W<(0 * N) / (N % N)> { + W {} + } + + fn bar(_x: W) -> u1 { + N + } + + fn main() -> pub u1 { + let w_0: W<0> = W {}; + let w: W<_> = foo(w_0); + bar(w) + } + "#; + + let errors = get_program_errors(source); + assert_eq!(errors.len(), 0); + + let monomorphization_error = get_monomorphization_error(source); + assert!(monomorphization_error.is_some()); + + // Expect a CheckedCast (0 % 0) failure + let monomorphization_error = monomorphization_error.unwrap(); + if let MonomorphizationError::UnknownArrayLength { ref length, ref err, location: _ } = + monomorphization_error + { + match length { + Type::CheckedCast { from, to } => { + assert!(matches!(*from.clone(), Type::InfixExpr { .. })); + assert!(matches!(*to.clone(), Type::InfixExpr { .. })); + } + _ => panic!("unexpected length: {:?}", length), + } + assert!(matches!( + err, + TypeCheckError::FailingBinaryOp { op: BinaryTypeOperator::Modulo, lhs: 0, rhs: 0, .. } + )); + } else { + panic!("unexpected error: {:?}", monomorphization_error); + } +} + +#[test] +fn arithmetic_generics_checked_cast_indirect_zeros() { + let source = r#" + struct W {} + + fn foo(_x: W) -> W<(N - N) % (N - N)> { + W {} + } + + fn bar(_x: W) -> Field { + N + } + + fn main() { + let w_0: W<0> = W {}; + let w = foo(w_0); + let _ = bar(w); + } + "#; + + let errors = get_program_errors(source); + assert_eq!(errors.len(), 0); + + let monomorphization_error = get_monomorphization_error(source); + assert!(monomorphization_error.is_some()); + + // Expect a CheckedCast (0 % 0) failure + let monomorphization_error = monomorphization_error.unwrap(); + if let MonomorphizationError::UnknownArrayLength { ref length, ref err, location: _ } = + monomorphization_error + { + match length { + Type::CheckedCast { from, to } => { + assert!(matches!(*from.clone(), Type::InfixExpr { .. })); + assert!(matches!(*to.clone(), Type::InfixExpr { .. })); + } + _ => panic!("unexpected length: {:?}", length), + } + match err { + TypeCheckError::ModuloOnFields { lhs, rhs, .. } => { + assert_eq!(lhs.clone(), FieldElement::zero()); + assert_eq!(rhs.clone(), FieldElement::zero()); + } + _ => panic!("expected ModuloOnFields, but found: {:?}", err), + } + } else { + panic!("unexpected error: {:?}", monomorphization_error); + } +} + #[test] fn infer_globals_to_u32_from_type_use() { let src = r#" @@ -3393,6 +3502,36 @@ fn arithmetic_generics_rounding_fail() { let errors = get_program_errors(src); assert_eq!(errors.len(), 1); + assert!(matches!( + errors[0].0, + CompilationError::TypeError(TypeCheckError::TypeMismatch { .. }) + )); +} + +#[test] +fn arithmetic_generics_rounding_fail_on_struct() { + let src = r#" + struct W {} + + fn foo(_x: W, _y: W) -> W { + W {} + } + + fn main() { + let w_2: W<2> = W {}; + let w_3: W<3> = W {}; + // Do not simplify N/M*M to just N + // This should be 3/2*2 = 2, not 3 + let _: W<3> = foo(w_3, w_2); + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + assert!(matches!( + errors[0].0, + CompilationError::TypeError(TypeCheckError::TypeMismatch { .. }) + )); } #[test] diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index 3bfb411ea4d..3f329ddb979 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -584,6 +584,14 @@ impl<'a> NodeFinder<'a> { self_prefix, ); } + Type::CheckedCast { to, .. } => { + return self.complete_type_fields_and_methods( + to, + prefix, + function_completion_kind, + self_prefix, + ); + } Type::Tuple(types) => { self.complete_tuple_fields(types, self_prefix); } diff --git a/tooling/lsp/src/requests/hover.rs b/tooling/lsp/src/requests/hover.rs index 64974a42133..bb1ea661719 100644 --- a/tooling/lsp/src/requests/hover.rs +++ b/tooling/lsp/src/requests/hover.rs @@ -679,6 +679,7 @@ impl<'a> TypeLinksGatherer<'a> { self.gather_type_links(lhs); self.gather_type_links(rhs); } + Type::CheckedCast { to, .. } => self.gather_type_links(to), Type::FieldElement | Type::Integer(..) | Type::Bool diff --git a/tooling/lsp/src/requests/inlay_hint.rs b/tooling/lsp/src/requests/inlay_hint.rs index f7b3e6a748d..c6415acb545 100644 --- a/tooling/lsp/src/requests/inlay_hint.rs +++ b/tooling/lsp/src/requests/inlay_hint.rs @@ -471,6 +471,7 @@ fn push_type_parts(typ: &Type, parts: &mut Vec, files: &File push_type_variable_parts(binding, parts, files); } } + Type::CheckedCast { to, .. } => push_type_parts(to, parts, files), Type::FieldElement | Type::Integer(..) diff --git a/tooling/lsp/src/trait_impl_method_stub_generator.rs b/tooling/lsp/src/trait_impl_method_stub_generator.rs index b433ee2ec88..2ae0d526f3e 100644 --- a/tooling/lsp/src/trait_impl_method_stub_generator.rs +++ b/tooling/lsp/src/trait_impl_method_stub_generator.rs @@ -368,6 +368,7 @@ impl<'a> TraitImplMethodStubGenerator<'a> { self.string.push(' '); self.append_type(right); } + Type::CheckedCast { to, .. } => self.append_type(to), Type::Constant(..) | Type::Integer(_, _) | Type::Bool