From 5841122627ed7f786d24ea2fef3008126569fd55 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 31 Jan 2025 11:02:30 -0500 Subject: [PATCH 1/2] fix(unrolling): Fetch original bytecode size from the original function (#7253) --- compiler/noirc_evaluator/src/ssa/opt/unrolling.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 52e3b63322d..efdb5f05d32 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -74,7 +74,7 @@ impl Ssa { if let Some(max_incr_pct) = max_bytecode_increase_percent { let orig_function = orig_function.expect("took snapshot to compare"); let new_size = function.num_instructions(); - let orig_size = function.num_instructions(); + let orig_size = orig_function.num_instructions(); if !is_new_size_ok(orig_size, new_size, max_incr_pct) { *function = orig_function; } From 0c6c6371c6d0a19be3f4347678fab52f6c91f79a Mon Sep 17 00:00:00 2001 From: jfecher Date: Fri, 31 Jan 2025 10:07:47 -0600 Subject: [PATCH 2/2] feat(experimental): Implement enum tag constants (#7183) Co-authored-by: Ary Borenszweig --- .../noirc_frontend/src/ast/enumeration.rs | 14 ++- compiler/noirc_frontend/src/ast/visitor.rs | 6 +- .../noirc_frontend/src/elaborator/enums.rs | 110 ++++++++++++++++-- compiler/noirc_frontend/src/elaborator/mod.rs | 14 +-- .../src/hir/comptime/interpreter.rs | 2 +- .../noirc_frontend/src/hir/comptime/value.rs | 3 +- .../src/monomorphization/mod.rs | 2 +- .../noirc_frontend/src/parser/parser/enums.rs | 17 ++- .../comptime_enums/src/main.nr | 2 +- .../compile_success_empty/enums/src/main.nr | 6 +- tooling/nargo_fmt/src/formatter/enums.rs | 8 +- 11 files changed, 144 insertions(+), 40 deletions(-) diff --git a/compiler/noirc_frontend/src/ast/enumeration.rs b/compiler/noirc_frontend/src/ast/enumeration.rs index eeeb823b9fc..6789a200e6a 100644 --- a/compiler/noirc_frontend/src/ast/enumeration.rs +++ b/compiler/noirc_frontend/src/ast/enumeration.rs @@ -30,7 +30,11 @@ impl NoirEnumeration { #[derive(Clone, Debug, PartialEq, Eq)] pub struct EnumVariant { pub name: Ident, - pub parameters: Vec, + + /// This is None for tag variants without parameters. + /// A value of `Some(vec![])` corresponds to a variant defined as `Foo()` + /// with parenthesis but no parameters. + pub parameters: Option>, } impl Display for NoirEnumeration { @@ -41,8 +45,12 @@ impl Display for NoirEnumeration { writeln!(f, "enum {}{} {{", self.name, generics)?; for variant in self.variants.iter() { - let parameters = vecmap(&variant.item.parameters, ToString::to_string).join(", "); - writeln!(f, " {}({}),", variant.item.name, parameters)?; + if let Some(parameters) = &variant.item.parameters { + let parameters = vecmap(parameters, ToString::to_string).join(", "); + writeln!(f, " {}({}),", variant.item.name, parameters)?; + } else { + writeln!(f, " {},", variant.item.name)?; + } } write!(f, "}}") diff --git a/compiler/noirc_frontend/src/ast/visitor.rs b/compiler/noirc_frontend/src/ast/visitor.rs index d7fe63a6a45..30b8deb4925 100644 --- a/compiler/noirc_frontend/src/ast/visitor.rs +++ b/compiler/noirc_frontend/src/ast/visitor.rs @@ -795,8 +795,10 @@ impl NoirEnumeration { } for variant in &self.variants { - for parameter in &variant.item.parameters { - parameter.accept(visitor); + if let Some(parameters) = &variant.item.parameters { + for parameter in parameters { + parameter.accept(visitor); + } } } } diff --git a/compiler/noirc_frontend/src/elaborator/enums.rs b/compiler/noirc_frontend/src/elaborator/enums.rs index 76c5ade9421..5153845a57c 100644 --- a/compiler/noirc_frontend/src/elaborator/enums.rs +++ b/compiler/noirc_frontend/src/elaborator/enums.rs @@ -8,7 +8,7 @@ use crate::{ function::{FuncMeta, FunctionBody, HirFunction, Parameters}, stmt::HirPattern, }, - node_interner::{DefinitionKind, FuncId, FunctionModifiers, TypeId}, + node_interner::{DefinitionKind, ExprId, FunctionModifiers, GlobalValue, TypeId}, token::Attributes, DataType, Shared, Type, }; @@ -16,8 +16,96 @@ use crate::{ use super::Elaborator; impl Elaborator<'_> { + /// Defines the value of an enum variant that we resolve an enum + /// variant expression to. E.g. `Foo::Bar` in `Foo::Bar(baz)`. + /// + /// If the variant requires arguments we should define a function, + /// otherwise we define a polymorphic global containing the tag value. #[allow(clippy::too_many_arguments)] - pub(super) fn define_enum_variant_function( + pub(super) fn define_enum_variant_constructor( + &mut self, + enum_: &NoirEnumeration, + type_id: TypeId, + variant: &EnumVariant, + variant_arg_types: Option>, + variant_index: usize, + datatype: &Shared, + self_type: &Type, + self_type_unresolved: UnresolvedType, + ) { + match variant_arg_types { + Some(args) => self.define_enum_variant_function( + enum_, + type_id, + variant, + args, + variant_index, + datatype, + self_type, + self_type_unresolved, + ), + None => self.define_enum_variant_global( + enum_, + type_id, + variant, + variant_index, + datatype, + self_type, + ), + } + } + + #[allow(clippy::too_many_arguments)] + fn define_enum_variant_global( + &mut self, + enum_: &NoirEnumeration, + type_id: TypeId, + variant: &EnumVariant, + variant_index: usize, + datatype: &Shared, + self_type: &Type, + ) { + let name = &variant.name; + let location = Location::new(variant.name.span(), self.file); + + let global_id = self.interner.push_empty_global( + name.clone(), + type_id.local_module_id(), + type_id.krate(), + self.file, + Vec::new(), + false, + false, + ); + + let mut typ = self_type.clone(); + if !datatype.borrow().generics.is_empty() { + let typevars = vecmap(&datatype.borrow().generics, |generic| generic.type_var.clone()); + typ = Type::Forall(typevars, Box::new(typ)); + } + + let definition_id = self.interner.get_global(global_id).definition_id; + self.interner.push_definition_type(definition_id, typ.clone()); + + let no_parameters = Parameters(Vec::new()); + let global_body = + self.make_enum_variant_constructor(datatype, variant_index, &no_parameters, location); + let let_statement = crate::hir_def::stmt::HirStatement::Expression(global_body); + + let statement_id = self.interner.get_global(global_id).let_statement; + self.interner.replace_statement(statement_id, let_statement); + + self.interner.get_global_mut(global_id).value = GlobalValue::Resolved( + crate::hir::comptime::Value::Enum(variant_index, Vec::new(), typ), + ); + + Self::get_module_mut(self.def_maps, type_id.module_id()) + .declare_global(name.clone(), enum_.visibility, global_id) + .ok(); + } + + #[allow(clippy::too_many_arguments)] + fn define_enum_variant_function( &mut self, enum_: &NoirEnumeration, type_id: TypeId, @@ -48,7 +136,10 @@ impl Elaborator<'_> { let hir_name = HirIdent::non_trait_method(definition_id, location); let parameters = self.make_enum_variant_parameters(variant_arg_types, location); - self.push_enum_variant_function_body(id, datatype, variant_index, ¶meters, location); + + let body = + self.make_enum_variant_constructor(datatype, variant_index, ¶meters, location); + self.interner.update_fn(id, HirFunction::unchecked_from_expr(body)); let function_type = datatype_ref.variant_function_type_with_forall(variant_index, datatype.clone()); @@ -106,14 +197,13 @@ impl Elaborator<'_> { // } // } // ``` - fn push_enum_variant_function_body( + fn make_enum_variant_constructor( &mut self, - id: FuncId, self_type: &Shared, variant_index: usize, parameters: &Parameters, location: Location, - ) { + ) -> ExprId { // Each parameter of the enum variant function is used as a parameter of the enum // constructor expression let arguments = vecmap(¶meters.0, |(pattern, typ, _)| match pattern { @@ -126,18 +216,18 @@ impl Elaborator<'_> { _ => unreachable!(), }); - let enum_generics = self_type.borrow().generic_types(); - let construct_variant = HirExpression::EnumConstructor(HirEnumConstructorExpression { + let constructor = HirExpression::EnumConstructor(HirEnumConstructorExpression { r#type: self_type.clone(), arguments, variant_index, }); - let body = self.interner.push_expr(construct_variant); - self.interner.update_fn(id, HirFunction::unchecked_from_expr(body)); + let body = self.interner.push_expr(constructor); + let enum_generics = self_type.borrow().generic_types(); let typ = Type::DataType(self_type.clone(), enum_generics); self.interner.push_expr_type(body, typ); self.interner.push_expr_location(body, location.span, location.file); + body } fn make_enum_variant_parameters( diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 981c69df82a..c895f87ef88 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1841,16 +1841,16 @@ impl<'context> Elaborator<'context> { let module_id = ModuleId { krate: self.crate_id, local_id: typ.module_id }; for (i, variant) in typ.enum_def.variants.iter().enumerate() { - let types = vecmap(&variant.item.parameters, |typ| self.resolve_type(typ.clone())); + let parameters = variant.item.parameters.as_ref(); + let types = + parameters.map(|params| vecmap(params, |typ| self.resolve_type(typ.clone()))); let name = variant.item.name.clone(); - // false here is for the eventual change to allow enum "constants" rather than - // always having them be called as functions. This can be replaced with an actual - // check once #7172 is implemented. - datatype.borrow_mut().push_variant(EnumVariant::new(name, types.clone(), false)); + let is_function = types.is_some(); + let params = types.clone().unwrap_or_default(); + datatype.borrow_mut().push_variant(EnumVariant::new(name, params, is_function)); - // Define a function for each variant to construct it - self.define_enum_variant_function( + self.define_enum_variant_constructor( &typ.enum_def, *type_id, &variant.item, diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index 6f0997d19d3..33f8e43863e 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -1294,7 +1294,7 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { id: ExprId, ) -> IResult { let fields = try_vecmap(constructor.arguments, |arg| self.evaluate(arg))?; - let typ = self.elaborator.interner.id_type(id).follow_bindings(); + let typ = self.elaborator.interner.id_type(id).unwrap_forall().1.follow_bindings(); Ok(Value::Enum(constructor.variant_index, fields, typ)) } diff --git a/compiler/noirc_frontend/src/hir/comptime/value.rs b/compiler/noirc_frontend/src/hir/comptime/value.rs index 93590096b79..c1a831c70a8 100644 --- a/compiler/noirc_frontend/src/hir/comptime/value.rs +++ b/compiler/noirc_frontend/src/hir/comptime/value.rs @@ -405,7 +405,8 @@ impl Value { }) } Value::Enum(variant_index, args, typ) => { - let r#type = match typ.follow_bindings() { + // Enum constants can have generic types but aren't functions + let r#type = match typ.unwrap_forall().1.follow_bindings() { Type::DataType(def, _) => def, _ => return Err(InterpreterError::NonEnumInConstructor { typ, location }), }; diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 8788f7284cb..7ad703523d4 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -2209,7 +2209,7 @@ fn unwrap_enum_type( typ: &HirType, location: Location, ) -> Result)>, MonomorphizationError> { - match typ.follow_bindings() { + match typ.unwrap_forall().1.follow_bindings() { HirType::DataType(def, args) => { // Some of args might not be mentioned in fields, so we need to check that they aren't unbound. for arg in &args { diff --git a/compiler/noirc_frontend/src/parser/parser/enums.rs b/compiler/noirc_frontend/src/parser/parser/enums.rs index f95c0f8f72b..3b496a438cf 100644 --- a/compiler/noirc_frontend/src/parser/parser/enums.rs +++ b/compiler/noirc_frontend/src/parser/parser/enums.rs @@ -92,12 +92,10 @@ impl<'a> Parser<'a> { self.bump(); } - let mut parameters = Vec::new(); - - if self.eat_left_paren() { + let parameters = self.eat_left_paren().then(|| { let comma_separated = separated_by_comma_until_right_paren(); - parameters = self.parse_many("variant parameters", comma_separated, Self::parse_type); - } + self.parse_many("variant parameters", comma_separated, Self::parse_type) + }); Some(Documented::new(EnumVariant { name, parameters }, doc_comments)) } @@ -189,18 +187,19 @@ mod tests { let variant = noir_enum.variants.remove(0).item; assert_eq!("X", variant.name.to_string()); assert!(matches!( - variant.parameters[0].typ, + variant.parameters.as_ref().unwrap()[0].typ, UnresolvedTypeData::Integer(Signedness::Signed, IntegerBitSize::ThirtyTwo) )); let variant = noir_enum.variants.remove(0).item; assert_eq!("y", variant.name.to_string()); - assert!(matches!(variant.parameters[0].typ, UnresolvedTypeData::FieldElement)); - assert!(matches!(variant.parameters[1].typ, UnresolvedTypeData::Integer(..))); + let parameters = variant.parameters.as_ref().unwrap(); + assert!(matches!(parameters[0].typ, UnresolvedTypeData::FieldElement)); + assert!(matches!(parameters[1].typ, UnresolvedTypeData::Integer(..))); let variant = noir_enum.variants.remove(0).item; assert_eq!("Z", variant.name.to_string()); - assert_eq!(variant.parameters.len(), 0); + assert!(variant.parameters.is_none()); } #[test] diff --git a/test_programs/compile_success_empty/comptime_enums/src/main.nr b/test_programs/compile_success_empty/comptime_enums/src/main.nr index 78835a9bd5a..a16a4cf4da4 100644 --- a/test_programs/compile_success_empty/comptime_enums/src/main.nr +++ b/test_programs/compile_success_empty/comptime_enums/src/main.nr @@ -2,7 +2,7 @@ fn main() { comptime { let _two = Foo::Couple(1, 2); let _one = Foo::One(3); - let _none = Foo::None(); + let _none = Foo::None; } } diff --git a/test_programs/compile_success_empty/enums/src/main.nr b/test_programs/compile_success_empty/enums/src/main.nr index 31619bca596..03a64d57dcf 100644 --- a/test_programs/compile_success_empty/enums/src/main.nr +++ b/test_programs/compile_success_empty/enums/src/main.nr @@ -3,9 +3,10 @@ fn main() { let _b: Foo = Foo::B(3); let _c = Foo::C(4); - // (#7172): Single variant enums must be called as functions currently let _d: fn() -> Foo<(i32, i32)> = Foo::D; let _d: Foo<(i32, i32)> = Foo::D(); + let _e: Foo = Foo::E; + let _e: Foo = Foo::E; // Ensure we can still use Foo::E polymorphically // Enum variants are functions and can be passed around as such let _many_cs = [1, 2, 3].map(Foo::C); @@ -15,5 +16,6 @@ enum Foo { A(Field, Field), B(u32), C(T), - D, + D(), + E, } diff --git a/tooling/nargo_fmt/src/formatter/enums.rs b/tooling/nargo_fmt/src/formatter/enums.rs index b596ec95c94..2d1182a941c 100644 --- a/tooling/nargo_fmt/src/formatter/enums.rs +++ b/tooling/nargo_fmt/src/formatter/enums.rs @@ -48,9 +48,9 @@ impl<'a> Formatter<'a> { self.write_indentation(); self.write_identifier(variant.name); - if !variant.parameters.is_empty() { + if let Some(parameters) = variant.parameters { self.write_token(Token::LeftParen); - for (i, parameter) in variant.parameters.into_iter().enumerate() { + for (i, parameter) in parameters.into_iter().enumerate() { if i != 0 { self.write_comma(); self.write_space(); @@ -118,6 +118,7 @@ mod tests { Variant ( Field , i32 ) , // comment Another ( ), + Constant , } }"; let expected = "mod moo { enum Foo { @@ -125,7 +126,8 @@ mod tests { /// comment Variant(Field, i32), // comment - Another, + Another(), + Constant, } } ";