fix(experimental): Allow shadowing in match patterns (#7484)

Author: jfecher

compiler/noirc_frontend/src/elaborator/enums.rs

@@ -5,7 +5,7 @@ use noirc_errors::Location;
 
 use crate::{
     ast::{
-        EnumVariant, Expression, ExpressionKind, FunctionKind, Literal, NoirEnumeration,
+        EnumVariant, Expression, ExpressionKind, FunctionKind, Ident, Literal, NoirEnumeration,
         StatementKind, UnresolvedType, Visibility,
     },
     elaborator::path_resolution::PathResolutionItem,
@@ -271,7 +271,8 @@ impl Elaborator<'_> {
 
         let rows = vecmap(rules, |(pattern, branch)| {
             self.push_scope();
-            let pattern = self.expression_to_pattern(pattern, &expected_pattern_type);
+            let pattern =
+                self.expression_to_pattern(pattern, &expected_pattern_type, &mut Vec::new());
             let columns = vec![Column::new(variable_to_match, pattern)];
 
             let guard = None;
@@ -293,7 +294,12 @@ impl Elaborator<'_> {
     }
 
     /// Convert an expression into a Pattern, defining any variables within.
-    fn expression_to_pattern(&mut self, expression: Expression, expected_type: &Type) -> Pattern {
+    fn expression_to_pattern(
+        &mut self,
+        expression: Expression,
+        expected_type: &Type,
+        variables_defined: &mut Vec<Ident>,
+    ) -> Pattern {
         let expr_location = expression.type_location();
         let unify_with_expected_type = |this: &mut Self, actual| {
             this.unify(actual, expected_type, expr_location.file, || {
@@ -333,13 +339,25 @@ impl Elaborator<'_> {
                         Vec::new(),
                         expected_type,
                         location,
+                        variables_defined,
                     ),
                     Err(_) if path_len == 1 => {
                         // Define the variable
                         let kind = DefinitionKind::Local(None);
-                        // TODO: `allow_shadowing` is false while I'm too lazy to add a check that we
-                        // don't define the same name multiple times in one pattern.
-                        let id = self.add_variable_decl(last_ident, false, false, true, kind).id;
+
+                        if let Some(existing) =
+                            variables_defined.iter().find(|elem| *elem == &last_ident)
+                        {
+                            let error = ResolverError::VariableAlreadyDefinedInPattern {
+                                existing: existing.clone(),
+                                new_span: last_ident.span(),
+                            };
+                            self.push_err(error, self.file);
+                        } else {
+                            variables_defined.push(last_ident.clone());
+                        }
+
+                        let id = self.add_variable_decl(last_ident, false, true, true, kind).id;
                         self.interner.push_definition_type(id, expected_type.clone());
                         Pattern::Binding(id)
                     }
@@ -352,9 +370,12 @@ impl Elaborator<'_> {
                     }
                 }
             }
-            ExpressionKind::Call(call) => {
-                self.expression_to_constructor(*call.func, call.arguments, expected_type)
-            }
+            ExpressionKind::Call(call) => self.expression_to_constructor(
+                *call.func,
+                call.arguments,
+                expected_type,
+                variables_defined,
+            ),
             ExpressionKind::Constructor(_) => todo!("handle constructors"),
             ExpressionKind::Tuple(fields) => {
                 let field_types = vecmap(0..fields.len(), |_| self.interner.next_type_variable());
@@ -363,21 +384,23 @@ impl Elaborator<'_> {
 
                 let fields = vecmap(fields.into_iter().enumerate(), |(i, field)| {
                     let expected = field_types.get(i).unwrap_or(&Type::Error);
-                    self.expression_to_pattern(field, expected)
+                    self.expression_to_pattern(field, expected, variables_defined)
                 });
 
                 Pattern::Constructor(Constructor::Tuple(field_types.clone()), fields)
             }
 
-            ExpressionKind::Parenthesized(expr) => self.expression_to_pattern(*expr, expected_type),
+            ExpressionKind::Parenthesized(expr) => {
+                self.expression_to_pattern(*expr, expected_type, variables_defined)
+            }
             ExpressionKind::Interned(id) => {
                 let kind = self.interner.get_expression_kind(id);
                 let expr = Expression::new(kind.clone(), expression.location);
-                self.expression_to_pattern(expr, expected_type)
+                self.expression_to_pattern(expr, expected_type, variables_defined)
             }
             ExpressionKind::InternedStatement(id) => {
                 if let StatementKind::Expression(expr) = self.interner.get_statement_kind(id) {
-                    self.expression_to_pattern(expr.clone(), expected_type)
+                    self.expression_to_pattern(expr.clone(), expected_type, variables_defined)
                 } else {
                     panic!("Invalid expr kind {expression}")
                 }
@@ -413,6 +436,7 @@ impl Elaborator<'_> {
         name: Expression,
         args: Vec<Expression>,
         expected_type: &Type,
+        variables_defined: &mut Vec<Ident>,
     ) -> Pattern {
         match name.kind {
             ExpressionKind::Variable(path) => {
@@ -424,6 +448,7 @@ impl Elaborator<'_> {
                         args,
                         expected_type,
                         location,
+                        variables_defined,
                     ),
                     Err(error) => {
                         self.push_err(error, location.file);
@@ -433,16 +458,21 @@ impl Elaborator<'_> {
                 }
             }
             ExpressionKind::Parenthesized(expr) => {
-                self.expression_to_constructor(*expr, args, expected_type)
+                self.expression_to_constructor(*expr, args, expected_type, variables_defined)
             }
             ExpressionKind::Interned(id) => {
                 let kind = self.interner.get_expression_kind(id);
                 let expr = Expression::new(kind.clone(), name.location);
-                self.expression_to_constructor(expr, args, expected_type)
+                self.expression_to_constructor(expr, args, expected_type, variables_defined)
             }
             ExpressionKind::InternedStatement(id) => {
                 if let StatementKind::Expression(expr) = self.interner.get_statement_kind(id) {
-                    self.expression_to_constructor(expr.clone(), args, expected_type)
+                    self.expression_to_constructor(
+                        expr.clone(),
+                        args,
+                        expected_type,
+                        variables_defined,
+                    )
                 } else {
                     panic!("Invalid expr kind {name}")
                 }
@@ -457,6 +487,7 @@ impl Elaborator<'_> {
         args: Vec<Expression>,
         expected_type: &Type,
         location: Location,
+        variables_defined: &mut Vec<Ident>,
     ) -> Pattern {
         let span = location.span;
 
@@ -515,7 +546,7 @@ impl Elaborator<'_> {
 
         let args = args.into_iter().zip(expected_arg_types);
         let args = vecmap(args, |(arg, expected_arg_type)| {
-            self.expression_to_pattern(arg, &expected_arg_type)
+            self.expression_to_pattern(arg, &expected_arg_type, variables_defined)
         });
         let constructor = Constructor::Variant(actual_type, variant_index);
         Pattern::Constructor(constructor, args)

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

@@ -182,6 +182,8 @@ pub enum ResolverError {
     LoopNotYetSupported { span: Span },
     #[error("Expected a trait but found {found}")]
     ExpectedTrait { found: String, span: Span },
+    #[error("Variable '{existing}' was already defined in the same match pattern")]
+    VariableAlreadyDefinedInPattern { existing: Ident, new_span: Span },
 }
 
 impl ResolverError {
@@ -691,8 +693,14 @@ impl<'a> From<&'a ResolverError> for Diagnostic {
                     format!("Expected a trait, found {found}"), 
                     String::new(),
                     *span)
-
             }
+            ResolverError::VariableAlreadyDefinedInPattern { existing, new_span } => {
+                let message = format!("Variable `{existing}` was already defined in the same match pattern");
+                let secondary = format!("`{existing}` redefined here");
+                let mut error = Diagnostic::simple_error(message, secondary, *new_span);
+                error.add_secondary(format!("`{existing}` was previously defined here"), existing.span());
+                error
+            },
         }
     }
 }

compiler/noirc_frontend/src/tests.rs

@@ -4475,3 +4475,22 @@ fn errors_on_unspecified_unstable_match() {
 
     assert!(matches!(error.reason(), Some(ParserErrorReason::ExperimentalFeature(_))));
 }
+
+#[test]
+fn errors_on_repeated_match_variables_in_pattern() {
+    let src = r#"
+    fn main() {
+        match (1, 2) {
+            (_x, _x) => (),
+        }
+    }
+    "#;
+
+    let errors = get_program_errors(src);
+    assert_eq!(errors.len(), 1);
+
+    assert!(matches!(
+        &errors[0].0,
+        CompilationError::ResolverError(ResolverError::VariableAlreadyDefinedInPattern { .. })
+    ));
+}