feat: Order attribute execution by their source ordering (#6326)

Co-authored-by: Tom French <[email protected]>

compiler/noirc_frontend/src/elaborator/comptime.rs

@@ -39,6 +39,8 @@ struct AttributeContext {
     attribute_module: LocalModuleId,
 }
 
+type CollectedAttributes = Vec<(FuncId, Value, Vec<Expression>, AttributeContext, Span)>;
+
 impl AttributeContext {
     fn new(file: FileId, module: LocalModuleId) -> Self {
         Self { file, module, attribute_file: file, attribute_module: module }
@@ -131,64 +133,57 @@ impl<'context> Elaborator<'context> {
         }
     }
 
-    fn run_comptime_attributes_on_item(
+    fn collect_comptime_attributes_on_item(
         &mut self,
         attributes: &[SecondaryAttribute],
         item: Value,
-        span: Span,
         attribute_context: AttributeContext,
-        generated_items: &mut CollectedItems,
+        attributes_to_run: &mut CollectedAttributes,
     ) {
         for attribute in attributes {
-            self.run_comptime_attribute_on_item(
+            self.collect_comptime_attribute_on_item(
                 attribute,
                 &item,
-                span,
                 attribute_context,
-                generated_items,
+                attributes_to_run,
             );
         }
     }
 
-    fn run_comptime_attribute_on_item(
+    fn collect_comptime_attribute_on_item(
         &mut self,
         attribute: &SecondaryAttribute,
         item: &Value,
-        span: Span,
         attribute_context: AttributeContext,
-        generated_items: &mut CollectedItems,
+        attributes_to_run: &mut CollectedAttributes,
     ) {
         if let SecondaryAttribute::Meta(attribute) = attribute {
             self.elaborate_in_comptime_context(|this| {
-                if let Err(error) = this.run_comptime_attribute_name_on_item(
+                if let Err(error) = this.collect_comptime_attribute_name_on_item(
                     attribute,
                     item.clone(),
-                    span,
                     attribute_context,
-                    generated_items,
+                    attributes_to_run,
                 ) {
                     this.errors.push(error);
                 }
             });
         }
     }
 
-    fn run_comptime_attribute_name_on_item(
+    /// Resolve an attribute to the function it refers to and add it to `attributes_to_run`
+    fn collect_comptime_attribute_name_on_item(
         &mut self,
         attribute: &MetaAttribute,
         item: Value,
-        span: Span,
         attribute_context: AttributeContext,
-        generated_items: &mut CollectedItems,
+        attributes_to_run: &mut CollectedAttributes,
     ) -> Result<(), (CompilationError, FileId)> {
         self.file = attribute_context.attribute_file;
         self.local_module = attribute_context.attribute_module;
+        let span = attribute.span;
 
-        let location = Location::new(attribute.span, self.file);
-        let function = Expression {
-            kind: ExpressionKind::Variable(attribute.name.clone()),
-            span: attribute.span,
-        };
+        let function = Expression { kind: ExpressionKind::Variable(attribute.name.clone()), span };
         let arguments = attribute.arguments.clone();
 
         // Elaborate the function, rolling back any errors generated in case it is unknown
@@ -200,32 +195,34 @@ impl<'context> Elaborator<'context> {
         let definition_id = match self.interner.expression(&function) {
             HirExpression::Ident(ident, _) => ident.id,
             _ => {
-                return Err((
-                    ResolverError::AttributeFunctionIsNotAPath {
-                        function: function_string,
-                        span: attribute.span,
-                    }
-                    .into(),
-                    self.file,
-                ))
+                let error =
+                    ResolverError::AttributeFunctionIsNotAPath { function: function_string, span };
+                return Err((error.into(), self.file));
             }
         };
 
         let Some(definition) = self.interner.try_definition(definition_id) else {
-            return Err((
-                ResolverError::AttributeFunctionNotInScope {
-                    name: function_string,
-                    span: attribute.span,
-                }
-                .into(),
-                self.file,
-            ));
+            let error = ResolverError::AttributeFunctionNotInScope { name: function_string, span };
+            return Err((error.into(), self.file));
         };
 
         let DefinitionKind::Function(function) = definition.kind else {
             return Err((ResolverError::NonFunctionInAnnotation { span }.into(), self.file));
         };
 
+        attributes_to_run.push((function, item, arguments, attribute_context, span));
+        Ok(())
+    }
+
+    fn run_attribute(
+        &mut self,
+        attribute_context: AttributeContext,
+        function: FuncId,
+        arguments: Vec<Expression>,
+        item: Value,
+        location: Location,
+        generated_items: &mut CollectedItems,
+    ) -> Result<(), (CompilationError, FileId)> {
         self.file = attribute_context.file;
         self.local_module = attribute_context.module;
 
@@ -237,10 +234,7 @@ impl<'context> Elaborator<'context> {
             arguments,
             location,
         )
-        .map_err(|error| {
-            let file = error.get_location().file;
-            (error.into(), file)
-        })?;
+        .map_err(|error| error.into_compilation_error_pair())?;
 
         arguments.insert(0, (item, location));
 
@@ -496,65 +490,91 @@ impl<'context> Elaborator<'context> {
         }
     }
 
-    /// Run all the attributes on each item. The ordering is unspecified to users but currently
-    /// we run trait attributes first to (e.g.) register derive handlers before derive is
-    /// called on structs.
-    /// Returns any new items generated by attributes.
+    /// Run all the attributes on each item in the crate in source-order.
+    /// Source-order is defined as running all child modules before their parent modules are run.
+    /// Child modules of a parent are run in order of their `mod foo;` declarations in the parent.
     pub(super) fn run_attributes(
         &mut self,
         traits: &BTreeMap<TraitId, UnresolvedTrait>,
         types: &BTreeMap<StructId, UnresolvedStruct>,
         functions: &[UnresolvedFunctions],
         module_attributes: &[ModuleAttribute],
-    ) -> CollectedItems {
-        let mut generated_items = CollectedItems::default();
+    ) {
+        let mut attributes_to_run = Vec::new();
 
         for (trait_id, trait_) in traits {
             let attributes = &trait_.trait_def.attributes;
             let item = Value::TraitDefinition(*trait_id);
-            let span = trait_.trait_def.span;
             let context = AttributeContext::new(trait_.file_id, trait_.module_id);
-            self.run_comptime_attributes_on_item(
+            self.collect_comptime_attributes_on_item(
                 attributes,
                 item,
-                span,
                 context,
-                &mut generated_items,
+                &mut attributes_to_run,
             );
         }
 
         for (struct_id, struct_def) in types {
             let attributes = &struct_def.struct_def.attributes;
             let item = Value::StructDefinition(*struct_id);
-            let span = struct_def.struct_def.span;
             let context = AttributeContext::new(struct_def.file_id, struct_def.module_id);
-            self.run_comptime_attributes_on_item(
+            self.collect_comptime_attributes_on_item(
                 attributes,
                 item,
-                span,
                 context,
-                &mut generated_items,
+                &mut attributes_to_run,
             );
         }
 
-        self.run_attributes_on_functions(functions, &mut generated_items);
+        self.collect_attributes_on_functions(functions, &mut attributes_to_run);
+        self.collect_attributes_on_modules(module_attributes, &mut attributes_to_run);
+
+        self.sort_attributes_by_run_order(&mut attributes_to_run);
 
-        self.run_attributes_on_modules(module_attributes, &mut generated_items);
+        // run
+        for (attribute, item, args, context, span) in attributes_to_run {
+            let location = Location::new(span, context.attribute_file);
 
-        generated_items
+            let mut generated_items = CollectedItems::default();
+            self.elaborate_in_comptime_context(|this| {
+                if let Err(error) = this.run_attribute(
+                    context,
+                    attribute,
+                    args,
+                    item,
+                    location,
+                    &mut generated_items,
+                ) {
+                    this.errors.push(error);
+                }
+            });
+
+            if !generated_items.is_empty() {
+                self.elaborate_items(generated_items);
+            }
+        }
     }
 
-    fn run_attributes_on_modules(
+    fn sort_attributes_by_run_order(&self, attributes: &mut CollectedAttributes) {
+        let module_order = self.def_maps[&self.crate_id].get_module_topological_order();
+
+        // Sort each attribute by (module, location in file) so that we can execute in
+        // the order they were defined in, running attributes in child modules first.
+        attributes.sort_by_key(|(_, _, _, ctx, span)| {
+            (module_order[&ctx.attribute_module], span.start())
+        });
+    }
+
+    fn collect_attributes_on_modules(
         &mut self,
         module_attributes: &[ModuleAttribute],
-        generated_items: &mut CollectedItems,
+        attributes_to_run: &mut CollectedAttributes,
     ) {
         for module_attribute in module_attributes {
             let local_id = module_attribute.module_id;
             let module_id = ModuleId { krate: self.crate_id, local_id };
             let item = Value::ModuleDefinition(module_id);
             let attribute = &module_attribute.attribute;
-            let span = Span::default();
 
             let context = AttributeContext {
                 file: module_attribute.file_id,
@@ -563,14 +583,14 @@ impl<'context> Elaborator<'context> {
                 attribute_module: module_attribute.attribute_module_id,
             };
 
-            self.run_comptime_attribute_on_item(attribute, &item, span, context, generated_items);
+            self.collect_comptime_attribute_on_item(attribute, &item, context, attributes_to_run);
         }
     }
 
-    fn run_attributes_on_functions(
+    fn collect_attributes_on_functions(
         &mut self,
         function_sets: &[UnresolvedFunctions],
-        generated_items: &mut CollectedItems,
+        attributes_to_run: &mut CollectedAttributes,
     ) {
         for function_set in function_sets {
             self.self_type = function_set.self_type.clone();
@@ -579,13 +599,11 @@ impl<'context> Elaborator<'context> {
                 let context = AttributeContext::new(function_set.file_id, *local_module);
                 let attributes = function.secondary_attributes();
                 let item = Value::FunctionDefinition(*function_id);
-                let span = function.span();
-                self.run_comptime_attributes_on_item(
+                self.collect_comptime_attributes_on_item(
                     attributes,
                     item,
-                    span,
                     context,
-                    generated_items,
+                    attributes_to_run,
                 );
             }
         }

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -307,23 +307,13 @@ impl<'context> Elaborator<'context> {
 
         // We have to run any comptime attributes on functions before the function is elaborated
         // since the generated items are checked beforehand as well.
-        let generated_items = self.run_attributes(
+        self.run_attributes(
             &items.traits,
             &items.types,
             &items.functions,
             &items.module_attributes,
         );
 
-        // After everything is collected, we can elaborate our generated items.
-        // It may be better to inline these within `items` entirely since elaborating them
-        // all here means any globals will not see these. Inlining them completely within `items`
-        // means we must be more careful about missing any additional items that need to be already
-        // elaborated. E.g. if a new struct is created, we've already passed the code path to
-        // elaborate them.
-        if !generated_items.is_empty() {
-            self.elaborate_items(generated_items);
-        }
-
         for functions in items.functions {
             self.elaborate_functions(functions);
         }

compiler/noirc_frontend/src/elaborator/types.rs

@@ -576,7 +576,7 @@ impl<'context> Elaborator<'context> {
     fn resolve_trait_static_method(&mut self, path: &Path) -> Option<TraitPathResolution> {
         let path_resolution = self.resolve_path(path.clone()).ok()?;
         let func_id = path_resolution.item.function_id()?;
-        let meta = self.interner.function_meta(&func_id);
+        let meta = self.interner.try_function_meta(&func_id)?;
         let the_trait = self.interner.get_trait(meta.trait_id?);
         let method = the_trait.find_method(path.last_name())?;
         let constraint = the_trait.as_constraint(path.span);

compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs

@@ -119,9 +119,11 @@ pub struct ModuleAttribute {
     pub file_id: FileId,
     // The module this attribute is attached to
     pub module_id: LocalModuleId,
+
     // The file where the attribute exists (it could be the same as `file_id`
-    // or a different one if it's an inner attribute in a different file)
+    // or a different one if it is an outer attribute in the parent of the module it applies to)
     pub attribute_file_id: FileId,
+
     // The module where the attribute is defined (similar to `attribute_file_id`,
     // it could be different than `module_id` for inner attributes)
     pub attribute_module_id: LocalModuleId,

compiler/noirc_frontend/src/hir/def_map/mod.rs

@@ -289,6 +289,29 @@ impl CrateDefMap {
             String::new()
         }
     }
+
+    /// Return a topological ordering of each module such that any child modules
+    /// are before their parent modules. Sibling modules will respect the ordering
+    /// declared from their parent module (the `mod foo; mod bar;` declarations).
+    pub fn get_module_topological_order(&self) -> HashMap<LocalModuleId, usize> {
+        let mut ordering = HashMap::default();
+        self.topologically_sort_modules(self.root, &mut 0, &mut ordering);
+        ordering
+    }
+
+    fn topologically_sort_modules(
+        &self,
+        current: LocalModuleId,
+        index: &mut usize,
+        ordering: &mut HashMap<LocalModuleId, usize>,
+    ) {
+        for child in &self.modules[current.0].child_declaration_order {
+            self.topologically_sort_modules(*child, index, ordering);
+        }
+
+        ordering.insert(current, *index);
+        *index += 1;
+    }
 }
 
 /// Specifies a contract function and extra metadata that