fix: allow multiple _ parameters, and disallow _ as an expression you can read from (#6657)

Author: asterite

compiler/noirc_frontend/src/debug/mod.rs

@@ -67,12 +67,16 @@ impl DebugInstrumenter {
         self.insert_state_set_oracle(module, 8);
     }
 
-    fn insert_var(&mut self, var_name: &str) -> SourceVarId {
+    fn insert_var(&mut self, var_name: &str) -> Option<SourceVarId> {
+        if var_name == "_" {
+            return None;
+        }
+
         let var_id = SourceVarId(self.next_var_id);
         self.next_var_id += 1;
         self.variables.insert(var_id, var_name.to_string());
         self.scope.last_mut().unwrap().insert(var_name.to_string(), var_id);
-        var_id
+        Some(var_id)
     }
 
     fn lookup_var(&self, var_name: &str) -> Option<SourceVarId> {
@@ -107,9 +111,9 @@ impl DebugInstrumenter {
             .flat_map(|param| {
                 pattern_vars(&param.pattern)
                     .iter()
-                    .map(|(id, _is_mut)| {
-                        let var_id = self.insert_var(&id.0.contents);
-                        build_assign_var_stmt(var_id, id_expr(id))
+                    .filter_map(|(id, _is_mut)| {
+                        let var_id = self.insert_var(&id.0.contents)?;
+                        Some(build_assign_var_stmt(var_id, id_expr(id)))
                     })
                     .collect::<Vec<_>>()
             })
@@ -225,13 +229,28 @@ impl DebugInstrumenter {
                 }
             })
             .collect();
-        let vars_exprs: Vec<ast::Expression> = vars.iter().map(|(id, _)| id_expr(id)).collect();
+        let vars_exprs: Vec<ast::Expression> = vars
+            .iter()
+            .map(|(id, _)| {
+                // We don't want to generate an expression to read from "_".
+                // And since this expression is going to be assigned to "_" so it doesn't matter
+                // what it is, we can use `()` for it.
+                if id.0.contents == "_" {
+                    ast::Expression {
+                        kind: ast::ExpressionKind::Literal(ast::Literal::Unit),
+                        span: id.span(),
+                    }
+                } else {
+                    id_expr(id)
+                }
+            })
+            .collect();
 
         let mut block_stmts =
             vec![ast::Statement { kind: ast::StatementKind::Let(let_stmt.clone()), span: *span }];
-        block_stmts.extend(vars.iter().map(|(id, _)| {
-            let var_id = self.insert_var(&id.0.contents);
-            build_assign_var_stmt(var_id, id_expr(id))
+        block_stmts.extend(vars.iter().filter_map(|(id, _)| {
+            let var_id = self.insert_var(&id.0.contents)?;
+            Some(build_assign_var_stmt(var_id, id_expr(id)))
         }));
         block_stmts.push(ast::Statement {
             kind: ast::StatementKind::Expression(ast::Expression {
@@ -422,21 +441,31 @@ impl DebugInstrumenter {
         let var_name = &for_stmt.identifier.0.contents;
         let var_id = self.insert_var(var_name);
 
-        let set_stmt = build_assign_var_stmt(var_id, id_expr(&for_stmt.identifier));
-        let drop_stmt = build_drop_var_stmt(var_id, Span::empty(for_stmt.span.end()));
+        let set_and_drop_stmt = var_id.map(|var_id| {
+            (
+                build_assign_var_stmt(var_id, id_expr(&for_stmt.identifier)),
+                build_drop_var_stmt(var_id, Span::empty(for_stmt.span.end())),
+            )
+        });
 
         self.walk_expr(&mut for_stmt.block);
+
+        let mut statements = Vec::new();
+        let block_statement = ast::Statement {
+            kind: ast::StatementKind::Semi(for_stmt.block.clone()),
+            span: for_stmt.block.span,
+        };
+
+        if let Some((set_stmt, drop_stmt)) = set_and_drop_stmt {
+            statements.push(set_stmt);
+            statements.push(block_statement);
+            statements.push(drop_stmt);
+        } else {
+            statements.push(block_statement);
+        }
+
         for_stmt.block = ast::Expression {
-            kind: ast::ExpressionKind::Block(ast::BlockExpression {
-                statements: vec![
-                    set_stmt,
-                    ast::Statement {
-                        kind: ast::StatementKind::Semi(for_stmt.block.clone()),
-                        span: for_stmt.block.span,
-                    },
-                    drop_stmt,
-                ],
-            }),
+            kind: ast::ExpressionKind::Block(ast::BlockExpression { statements }),
             span: for_stmt.span,
         };
     }

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -440,6 +440,9 @@ impl<'context> Elaborator<'context> {
         // so we need to reintroduce the same IDs into scope here.
         for parameter in &func_meta.parameter_idents {
             let name = self.interner.definition_name(parameter.id).to_owned();
+            if name == "_" {
+                continue;
+            }
             let warn_if_unused = !(func_meta.trait_impl.is_some() && name == "self");
             self.add_existing_variable_to_scope(name, parameter.clone(), warn_if_unused);
         }

compiler/noirc_frontend/src/elaborator/patterns.rs

@@ -331,16 +331,18 @@ impl<'context> Elaborator<'context> {
         let resolver_meta =
             ResolverMeta { num_times_used: 0, ident: ident.clone(), warn_if_unused };
 
-        let scope = self.scopes.get_mut_scope();
-        let old_value = scope.add_key_value(name.clone(), resolver_meta);
-
-        if !allow_shadowing {
-            if let Some(old_value) = old_value {
-                self.push_err(ResolverError::DuplicateDefinition {
-                    name,
-                    first_span: old_value.ident.location.span,
-                    second_span: location.span,
-                });
+        if name != "_" {
+            let scope = self.scopes.get_mut_scope();
+            let old_value = scope.add_key_value(name.clone(), resolver_meta);
+
+            if !allow_shadowing {
+                if let Some(old_value) = old_value {
+                    self.push_err(ResolverError::DuplicateDefinition {
+                        name,
+                        first_span: old_value.ident.location.span,
+                        second_span: location.span,
+                    });
+                }
             }
         }
 

compiler/noirc_frontend/src/hir/resolution/errors.rs

@@ -223,11 +223,21 @@ impl<'a> From<&'a ResolverError> for Diagnostic {
                     *span,
                 )
             }
-            ResolverError::VariableNotDeclared { name, span } => Diagnostic::simple_error(
-                format!("cannot find `{name}` in this scope "),
-                "not found in this scope".to_string(),
-                *span,
-            ),
+            ResolverError::VariableNotDeclared { name, span } =>  {
+                if name == "_" {
+                    Diagnostic::simple_error(
+                        "in expressions, `_` can only be used on the left-hand side of an assignment".to_string(),
+                        "`_` not allowed here".to_string(),
+                        *span,
+                    )
+                } else {
+                    Diagnostic::simple_error(
+                        format!("cannot find `{name}` in this scope"),
+                        "not found in this scope".to_string(),
+                        *span,
+                    )
+                }
+            },
             ResolverError::PathIsNotIdent { span } => Diagnostic::simple_error(
                 "cannot use path as an identifier".to_string(),
                 String::new(),

compiler/noirc_frontend/src/tests.rs

@@ -3845,3 +3845,33 @@ fn disallows_export_attribute_on_trait_impl_method() {
         ));
     });
 }
+
+#[test]
+fn allows_multiple_underscore_parameters() {
+    let src = r#"
+        pub fn foo(_: i32, _: i64) {}
+
+        fn main() {}
+    "#;
+    assert_no_errors(src);
+}
+
+#[test]
+fn disallows_underscore_on_right_hand_side() {
+    let src = r#"
+        fn main() {
+            let _ = 1;
+            let _x = _;
+        }
+    "#;
+    let errors = get_program_errors(src);
+    assert_eq!(errors.len(), 1);
+
+    let CompilationError::ResolverError(ResolverError::VariableNotDeclared { name, .. }) =
+        &errors[0].0
+    else {
+        panic!("Expected a VariableNotDeclared error, got {:?}", errors[0].0);
+    };
+
+    assert_eq!(name, "_");
+}