Merge branch 'master' into ab/trait-functions-in-scope-for-primitives

compiler/noirc_evaluator/src/ssa.rs

@@ -170,10 +170,11 @@ fn optimize_all(builder: SsaBuilder, options: &SsaEvaluatorOptions) -> Result<Ss
             "Unrolling",
         )?
         .run_pass(Ssa::simplify_cfg, "Simplifying (2nd)")
+        .run_pass(Ssa::mem2reg, "Mem2Reg (2nd)")
         .run_pass(Ssa::flatten_cfg, "Flattening")
-        .run_pass(Ssa::remove_bit_shifts, "After Removing Bit Shifts")
+        .run_pass(Ssa::remove_bit_shifts, "Removing Bit Shifts")
         // Run mem2reg once more with the flattened CFG to catch any remaining loads/stores
-        .run_pass(Ssa::mem2reg, "Mem2Reg (2nd)")
+        .run_pass(Ssa::mem2reg, "Mem2Reg (3rd)")
         // Run the inlining pass again to handle functions with `InlineType::NoPredicates`.
         // Before flattening is run, we treat functions marked with the `InlineType::NoPredicates` as an entry point.
         // This pass must come immediately following `mem2reg` as the succeeding passes

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

@@ -416,10 +416,15 @@ impl DataFlowGraph {
     pub(crate) fn get_value_max_num_bits(&self, value: ValueId) -> u32 {
         match self[value] {
             Value::Instruction { instruction, .. } => {
+                let value_bit_size = self.type_of_value(value).bit_size();
                 if let Instruction::Cast(original_value, _) = self[instruction] {
-                    self.type_of_value(original_value).bit_size()
+                    let original_bit_size = self.type_of_value(original_value).bit_size();
+                    // We might have cast e.g. `u1` to `u8` to be able to do arithmetic,
+                    // in which case we want to recover the original smaller bit size;
+                    // OTOH if we cast down, then we don't need the higher original size.
+                    value_bit_size.min(original_bit_size)
                 } else {
-                    self.type_of_value(value).bit_size()
+                    value_bit_size
                 }
             }
 

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

@@ -473,6 +473,9 @@ impl Instruction {
             // removed entirely.
             Noop => true,
 
+            // Cast instructions can always be deduplicated
+            Cast(_, _) => true,
+
             // Arrays can be mutated in unconstrained code so code that handles this case must
             // take care to track whether the array was possibly mutated or not before
             // deduplicating. Since we don't know if the containing pass checks for this, we
@@ -484,7 +487,6 @@ impl Instruction {
             // with one that was disabled. See
             // https://github.com/noir-lang/noir/pull/4716#issuecomment-2047846328.
             Binary(_)
-            | Cast(_, _)
             | Not(_)
             | Truncate { .. }
             | IfElse { .. }

compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs

@@ -7,7 +7,7 @@ use crate::ssa::{
         basic_block::BasicBlockId,
         call_stack::CallStackId,
         dfg::InsertInstructionResult,
-        function::{Function, RuntimeType},
+        function::Function,
         instruction::{Binary, BinaryOp, Endian, Instruction, InstructionId, Intrinsic},
         types::{NumericType, Type},
         value::ValueId,
@@ -32,7 +32,7 @@ impl Function {
     /// The structure of this pass is simple:
     /// Go through each block and re-insert all instructions.
     pub(crate) fn remove_bit_shifts(&mut self) {
-        if matches!(self.runtime(), RuntimeType::Brillig(_)) {
+        if self.runtime().is_brillig() {
             return;
         }
 
@@ -120,8 +120,11 @@ impl Context<'_> {
             let pow = self.numeric_constant(FieldElement::from(rhs_bit_size_pow_2), typ);
 
             let max_lhs_bits = self.function.dfg.get_value_max_num_bits(lhs);
-
-            (max_lhs_bits + bit_shift_size, pow)
+            let max_bit_size = max_lhs_bits + bit_shift_size;
+            // There is no point trying to truncate to more than the Field size.
+            // A higher `max_lhs_bits` input can come from trying to left-shift a Field.
+            let max_bit_size = max_bit_size.min(NumericType::NativeField.bit_size());
+            (max_bit_size, pow)
         } else {
             // we use a predicate to nullify the result in case of overflow
             let u8_type = NumericType::unsigned(8);

compiler/noirc_frontend/src/debug/mod.rs

@@ -523,8 +523,8 @@ pub fn build_debug_crate_file() -> String {
                 __debug_var_assign_oracle(var_id, value);
             }
             pub fn __debug_var_assign<T>(var_id: u32, value: T) {
+                /// Safety: debug context
                 unsafe {
-                //@safety: debug context
                 {
                     __debug_var_assign_inner(var_id, value);
                 }}
@@ -536,8 +536,8 @@ pub fn build_debug_crate_file() -> String {
                 __debug_var_drop_oracle(var_id);
             }
             pub fn __debug_var_drop(var_id: u32) {
+                /// Safety: debug context
                 unsafe {
-                //@safety: debug context
                 {
                     __debug_var_drop_inner(var_id);
                 }}
@@ -549,8 +549,8 @@ pub fn build_debug_crate_file() -> String {
                 __debug_fn_enter_oracle(fn_id);
             }
             pub fn __debug_fn_enter(fn_id: u32) {
+                /// Safety: debug context
                 unsafe {
-                //@safety: debug context
                 {
                     __debug_fn_enter_inner(fn_id);
                 }}
@@ -562,8 +562,8 @@ pub fn build_debug_crate_file() -> String {
                 __debug_fn_exit_oracle(fn_id);
             }
             pub fn __debug_fn_exit(fn_id: u32) {
+                /// Safety: debug context
                 unsafe {
-                //@safety: debug context
                 {
                     __debug_fn_exit_inner(fn_id);
                 }}
@@ -575,8 +575,8 @@ pub fn build_debug_crate_file() -> String {
                 __debug_dereference_assign_oracle(var_id, value);
             }
             pub fn __debug_dereference_assign<T>(var_id: u32, value: T) {
+                /// Safety: debug context
                 unsafe {
-                //@safety: debug context
                 {
                     __debug_dereference_assign_inner(var_id, value);
                 }}
@@ -603,8 +603,8 @@ pub fn build_debug_crate_file() -> String {
                     __debug_oracle_member_assign_{n}(var_id, value, {vars});
                 }}
                 pub fn __debug_member_assign_{n}<T, Index>(var_id: u32, value: T, {var_sig}) {{
+                    /// Safety: debug context
                     unsafe {{
-                        //@safety: debug context
                         __debug_inner_member_assign_{n}(var_id, value, {vars});
                     }}
                 }}

compiler/noirc_frontend/src/elaborator/expressions.rs

@@ -58,8 +58,8 @@ impl<'context> Elaborator<'context> {
             ExpressionKind::Comptime(comptime, _) => {
                 return self.elaborate_comptime_block(comptime, expr.span)
             }
-            ExpressionKind::Unsafe(block_expression, _) => {
-                self.elaborate_unsafe_block(block_expression, expr.span)
+            ExpressionKind::Unsafe(block_expression, span) => {
+                self.elaborate_unsafe_block(block_expression, span)
             }
             ExpressionKind::Resolved(id) => return (id, self.interner.id_type(id)),
             ExpressionKind::Interned(id) => {

compiler/noirc_frontend/src/elaborator/types.rs

@@ -30,7 +30,7 @@ use crate::{
         traits::{NamedType, ResolvedTraitBound, Trait, TraitConstraint},
     },
     node_interner::{
-        DependencyId, ExprId, GlobalValue, ImplSearchErrorKind, NodeInterner, TraitId,
+        DependencyId, ExprId, FuncId, GlobalValue, ImplSearchErrorKind, NodeInterner, TraitId,
         TraitImplKind, TraitMethodId,
     },
     token::SecondaryAttribute,
@@ -1415,7 +1415,7 @@ impl<'context> Elaborator<'context> {
         // Only keep unique trait IDs: multiple trait methods might come from the same trait
         // but implemented with different generics (like `Convert<Field>` and `Convert<i32>`).
         let traits: HashSet<TraitId> =
-            trait_methods.into_iter().map(|(_, trait_id)| trait_id).collect();
+            trait_methods.iter().map(|(_, trait_id)| *trait_id).collect();
 
         let traits_in_scope: Vec<_> = traits
             .iter()
@@ -1446,15 +1446,18 @@ impl<'context> Elaborator<'context> {
                 let trait_id = *traits.iter().next().unwrap();
                 let trait_ = self.interner.get_trait(trait_id);
                 let trait_name = self.fully_qualified_trait_path(trait_);
-                let generics = trait_.as_constraint(span).trait_bound.trait_generics;
-                let trait_method_id = trait_.find_method(method_name).unwrap();
 
                 self.push_err(PathResolutionError::TraitMethodNotInScope {
                     ident: Ident::new(method_name.into(), span),
                     trait_name,
                 });
 
-                return Some(HirMethodReference::TraitMethodId(trait_method_id, generics, false));
+                return Some(self.trait_hir_method_reference(
+                    trait_id,
+                    trait_methods,
+                    method_name,
+                    span,
+                ));
             } else {
                 let traits = vecmap(traits, |trait_id| {
                     let trait_ = self.interner.get_trait(trait_id);
@@ -1480,12 +1483,28 @@ impl<'context> Elaborator<'context> {
             return None;
         }
 
-        // Return a TraitMethodId with unbound generics. These will later be bound by the type-checker.
         let trait_id = traits_in_scope[0].0;
+        Some(self.trait_hir_method_reference(trait_id, trait_methods, method_name, span))
+    }
+
+    fn trait_hir_method_reference(
+        &self,
+        trait_id: TraitId,
+        trait_methods: Vec<(FuncId, TraitId)>,
+        method_name: &str,
+        span: Span,
+    ) -> HirMethodReference {
+        // If we find a single trait impl method, return it so we don't have to later determine the impl
+        if trait_methods.len() == 1 {
+            let (func_id, _) = trait_methods[0];
+            return HirMethodReference::FuncId(func_id);
+        }
+
+        // Return a TraitMethodId with unbound generics. These will later be bound by the type-checker.
         let trait_ = self.interner.get_trait(trait_id);
         let generics = trait_.as_constraint(span).trait_bound.trait_generics;
         let trait_method_id = trait_.find_method(method_name).unwrap();
-        Some(HirMethodReference::TraitMethodId(trait_method_id, generics, false))
+        HirMethodReference::TraitMethodId(trait_method_id, generics, false)
     }
 
     fn lookup_method_in_trait_constraints(

compiler/noirc_frontend/src/lexer/lexer.rs

@@ -726,7 +726,7 @@ impl<'a> Lexer<'a> {
     }
 
     fn parse_comment(&mut self, start: u32) -> SpannedTokenResult {
-        let mut doc_style = match self.peek_char() {
+        let doc_style = match self.peek_char() {
             Some('!') => {
                 self.next_char();
                 Some(DocStyle::Inner)
@@ -735,17 +735,9 @@ impl<'a> Lexer<'a> {
                 self.next_char();
                 Some(DocStyle::Outer)
             }
-            Some('@') => Some(DocStyle::Safety),
             _ => None,
         };
-        let mut comment = self.eat_while(None, |ch| ch != '\n');
-        if doc_style == Some(DocStyle::Safety) {
-            if comment.starts_with("@safety") {
-                comment = comment["@safety".len()..].to_string();
-            } else {
-                doc_style = None;
-            }
-        }
+        let comment = self.eat_while(None, |ch| ch != '\n');
 
         if !comment.is_ascii() {
             let span = Span::from(start..self.position);
@@ -760,7 +752,7 @@ impl<'a> Lexer<'a> {
     }
 
     fn parse_block_comment(&mut self, start: u32) -> SpannedTokenResult {
-        let mut doc_style = match self.peek_char() {
+        let doc_style = match self.peek_char() {
             Some('!') => {
                 self.next_char();
                 Some(DocStyle::Inner)
@@ -769,7 +761,6 @@ impl<'a> Lexer<'a> {
                 self.next_char();
                 Some(DocStyle::Outer)
             }
-            Some('@') => Some(DocStyle::Safety),
             _ => None,
         };
 
@@ -796,13 +787,6 @@ impl<'a> Lexer<'a> {
                 ch => content.push(ch),
             }
         }
-        if doc_style == Some(DocStyle::Safety) {
-            if content.starts_with("@safety") {
-                content = content["@safety".len()..].to_string();
-            } else {
-                doc_style = None;
-            }
-        }
         if depth == 0 {
             if !content.is_ascii() {
                 let span = Span::from(start..self.position);

compiler/noirc_frontend/src/lexer/token.rs

@@ -345,7 +345,6 @@ impl Display for FmtStrFragment {
 pub enum DocStyle {
     Outer,
     Inner,
-    Safety,
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
@@ -425,13 +424,11 @@ impl fmt::Display for Token {
             Token::LineComment(ref s, style) => match style {
                 Some(DocStyle::Inner) => write!(f, "//!{s}"),
                 Some(DocStyle::Outer) => write!(f, "///{s}"),
-                Some(DocStyle::Safety) => write!(f, "//@safety{s}"),
                 None => write!(f, "//{s}"),
             },
             Token::BlockComment(ref s, style) => match style {
                 Some(DocStyle::Inner) => write!(f, "/*!{s}*/"),
                 Some(DocStyle::Outer) => write!(f, "/**{s}*/"),
-                Some(DocStyle::Safety) => write!(f, "/*@safety{s}*/"),
                 None => write!(f, "/*{s}*/"),
             },
             Token::Quote(ref stream) => {

compiler/noirc_frontend/src/parser/errors.rs

@@ -110,8 +110,10 @@ pub enum ParserErrorReason {
     WrongNumberOfAttributeArguments { name: String, min: usize, max: usize, found: usize },
     #[error("The `deprecated` attribute expects a string argument")]
     DeprecatedAttributeExpectsAStringArgument,
-    #[error("Unsafe block must start with a safety comment")]
+    #[error("Unsafe block must have a safety doc comment above it")]
     MissingSafetyComment,
+    #[error("Unsafe block must start with a safety comment")]
+    UnsafeDocCommentDoesNotStartWithSafety,
     #[error("Missing parameters for function definition")]
     MissingParametersForFunctionDefinition,
 }
@@ -283,10 +285,18 @@ impl<'a> From<&'a ParserError> for Diagnostic {
                     error.span,
                 ),
                 ParserErrorReason::MissingSafetyComment => Diagnostic::simple_warning(
-                    "Missing Safety Comment".into(),
-                    "Unsafe block must start with a safety comment: //@safety".into(),
+                    "Unsafe block must have a safety doc comment above it".into(),
+                    "The doc comment must start with the \"Safety: \" word".into(),
                     error.span,
                 ),
+                ParserErrorReason::UnsafeDocCommentDoesNotStartWithSafety => {
+                    Diagnostic::simple_warning(
+                        "Unsafe block must start with a safety comment".into(),
+                        "The doc comment above this unsafe block must start with the \"Safety: \" word"
+                            .into(),
+                        error.span,
+                    )
+                }
                 ParserErrorReason::MissingParametersForFunctionDefinition => {
                     Diagnostic::simple_error(
                         "Missing parameters for function definition".into(),

compiler/noirc_frontend/src/parser/parser.rs

@@ -83,6 +83,27 @@ pub struct Parser<'a> {
     next_token: SpannedToken,
     current_token_span: Span,
     previous_token_span: Span,
+
+    /// The current statement's doc comments.
+    /// This is used to eventually know if an `unsafe { ... }` expression is documented
+    /// in its containing statement. For example:
+    ///
+    /// ```noir
+    /// /// Safety: test
+    /// let x = unsafe { call() };
+    /// ```
+    statement_doc_comments: Option<StatementDocComments>,
+}
+
+#[derive(Debug)]
+pub(crate) struct StatementDocComments {
+    pub(crate) doc_comments: Vec<String>,
+    pub(crate) start_span: Span,
+    pub(crate) end_span: Span,
+
+    /// Were these doc comments "read" by an unsafe statement?
+    /// If not, these doc comments aren't documenting anything and they produce an error.
+    pub(crate) read: bool,
 }
 
 impl<'a> Parser<'a> {
@@ -107,6 +128,7 @@ impl<'a> Parser<'a> {
             next_token: eof_spanned_token(),
             current_token_span: Default::default(),
             previous_token_span: Default::default(),
+            statement_doc_comments: None,
         };
         parser.read_two_first_tokens();
         parser