Emit compile error if a match arm variable is declared more than once

forc-util/src/lib.rs

@@ -640,6 +640,7 @@ fn construct_slice(labels: Vec<&Label>) -> Slice {
 fn label_type_to_annotation_type(label_type: LabelType) -> AnnotationType {
     match label_type {
         LabelType::Info => AnnotationType::Info,
+        LabelType::Help => AnnotationType::Help,
         LabelType::Warning => AnnotationType::Warning,
         LabelType::Error => AnnotationType::Error,
     }

sway-core/Cargo.toml

@@ -12,7 +12,7 @@ repository.workspace = true
 clap = { version = "3.1", features = ["derive"] }
 derivative = "2.2.0"
 dirs = "3.0"
-either = "1.6"
+either = "1.9.0"
 ethabi = { package = "fuel-ethabi", version = "18.0.0" }
 etk-asm = { package = "fuel-etk-asm", version = "0.3.1-dev", features = [
     "backtraces",

sway-core/src/semantic_analysis/ast_node/expression/match_expression/analysis/duplicates.rs

@@ -0,0 +1,249 @@
+use std::collections::HashMap;
+
+use sway_types::{Ident, Span, Spanned};
+
+use crate::language::ty::{self, TyScrutinee};
+
+/// First tuple filed is `true` if the variable represented with [Span] is a struct field, otherwise `false`.
+pub(crate) type MatchVariable = (bool, Span);
+
+pub(crate) struct MatchVariableDuplicate {
+    pub duplicate: MatchVariable,
+    pub first_definition: MatchVariable,
+}
+
+/// Returns [MatchVariableDuplicate]s for all the duplicates found in the `scrutinee`,
+/// or empty [Vec] if there are no duplicate variables in the `scrutinee`.
+///
+/// Alternatives are what make the algorithm more complex then just straightforward
+/// scan of the `scrutinee` for variables of the same name.
+/// In case of alternatives, we must have the same variables in all of the alternatives,
+/// and these are, of course, not duplicates.
+/// But we can still have duplicates within the alternatives, and between the alternatives
+/// and the other parts of the match arm.
+///
+/// Consider the following examples:
+///
+/// ```ignore
+/// Struct { x, y: x, z: x } => x.0,
+/// ```
+/// The second and the third `x` are the duplicates of the first `x`.
+///
+/// ```ignore
+/// Struct { x, .. } | Struct { x, .. } => x,
+/// (Struct { x, .. } | Struct { x, .. }, Struct { y, .. } | Struct { y, .. }) => if x { 0 } else { 1 } + y,
+/// ```
+/// Here there are no duplicates.
+///
+/// ```ignore
+/// (Struct { x, .. } | Struct { x, .. }, Struct { x, .. } | Struct { x, .. }) => if x { 0 } else { 1 } + y,
+/// ```
+/// The second `x` is not a duplicate, but the third and fourth are duplicates of the first one.
+///
+/// ```ignore
+/// Struct { x, y: x, z: x } | Struct { x, y: x, z: x } => x,
+/// ```
+/// The second and the third `x` are duplicates of the first one, and the last two of the fourth one.
+///
+/// ```ignore
+/// (x, Struct { x, .. } | Struct { x, .. }, y) => y,
+/// ```
+/// The second and the third `x` are duplicates of the first one.
+///
+/// ```ignore
+/// (0, Struct { x, y: x, .. } | Struct { x, .. }, x) => x,
+/// ```
+/// The second and the last `x` are duplicates of the first one. The third one is not a duplicate.
+///
+/// ```ignore
+/// (x, Struct { x, y: x, .. } | Struct { x, .. }, x) => x,
+/// ```
+/// All `x`s are duplicates of the first one.
+///
+/// Why not extend `matcher` to do this analysis?
+/// It would be a mixing of concerns and would complicate a clean implementation of the `matcher`.
+/// The `matcher`'s purpose is to detect variables and their types and properly bind them.
+/// Duplicates are seen as shadowing of variables which is perfectly fine from the `matcher`
+/// perspective.
+///
+/// How the algorithm works?
+///
+/// For a match arm represented by the `scrutinee` it creates a tree whose nodes are variable names.
+/// Variables are added by moving through the match arm left to right.
+/// Branching in the tree occurs in the case of alternatives.
+/// The algorithm traverses the branches dept-first and collects all the unique duplicates for every branch.
+/// Unique means that a duplicate can occur only in one branch.
+/// At the end it merges the result of all the branches in a single result.
+///
+/// The algorithm assumes that the `matcher` already checked the match arm.
+/// This gives us the guarantee that every alternative contains the same variables and that for
+/// the parts of the match arm that follows alternatives, we need to consider only the left-most
+/// alternative as a potential holder of the already defined variables.
+///
+/// For the examples given above, the corresponding trees look like this:
+///
+/// ```ignore
+/// Struct { x, y: x, z: x } => x.0,
+/// - x - x - x
+///
+/// Struct { x, .. } | Struct { x, .. } => x,
+///  / x
+/// -
+///  \ x <= this is the first, left-most x
+///
+/// (Struct { x, .. } | Struct { x, .. }, Struct { y, .. } | Struct { y, .. }) => if x { 0 } else { 1 } + y,
+///  / x
+/// -     / y
+///  \ x -
+///       \ y <= this is the left-most y
+///
+/// (Struct { x, .. } | Struct { x, .. }, Struct { x, .. } | Struct { x, .. }) => if x { 0 } else { 1 } + y,
+///  / x
+/// -     / x
+///  \ x -
+///       \ x
+///
+/// Struct { x, y: x, z: x } | Struct { x, y: x, z: x } => x,
+///  / x - x - x
+/// -
+///  \ x - x - x
+///
+/// (x, Struct { x, .. } | Struct { x, .. }, y) => y,
+///    / x
+/// -x-
+///    \ x - y
+///
+/// (0, Struct { x, y: x, .. } | Struct { x, .. }, x) => x,
+///  / x
+/// -
+///  \ x - x - x
+///
+/// (x, Struct { x, y: x, .. } | Struct { x, .. }, x) => x,
+///    / x
+/// -x-
+///    \ x - x - x
+///
+/// ```
+///
+/// And here is a some general example with nested alternatives, several variables etc.
+///
+/// ```ignore
+/// (x, y, x | x | x, Struct { x, y, z } | Struct { x: y | y | y, x, z }, z, x | x, z | z | z)
+///
+///         / x
+///        /         / y
+/// - x - y - x   / -  y
+///        \     /   \ y - x - z
+///         \ x -
+///              \               / x
+///               \ x - y - z - z     / z
+///                              \ x -  z
+///                                   \ z
+/// ```
+pub(crate) fn collect_duplicate_match_pattern_variables(
+    scrutinee: &TyScrutinee,
+) -> Vec<MatchVariableDuplicate> {
+    let mut left_most_branch = HashMap::new();
+    let mut branches = vec![];
+
+    recursively_collect_duplicate_variables(&mut branches, &mut left_most_branch, scrutinee);
+
+    branches.push(left_most_branch);
+
+    let mut result = vec![];
+    for mut branch in branches {
+        for (ident, (is_struct_field, duplicates)) in branch.iter_mut() {
+            for duplicate in duplicates {
+                result.push(MatchVariableDuplicate {
+                    duplicate: (duplicate.0, duplicate.1.clone()),
+                    first_definition: (*is_struct_field, ident.span()),
+                });
+            }
+        }
+    }
+
+    result.sort_by(|a, b| a.duplicate.1.cmp(&b.duplicate.1));
+
+    return result;
+
+    fn recursively_collect_duplicate_variables(
+        branches: &mut Vec<HashMap<Ident, (bool, Vec<MatchVariable>)>>,
+        left_most_branch: &mut HashMap<Ident, (bool, Vec<MatchVariable>)>,
+        scrutinee: &TyScrutinee,
+    ) {
+        match &scrutinee.variant {
+            ty::TyScrutineeVariant::CatchAll => (),
+            ty::TyScrutineeVariant::Variable(ident) => add_variable(left_most_branch, ident, false),
+            ty::TyScrutineeVariant::Literal(_) => (),
+            ty::TyScrutineeVariant::Constant { .. } => (),
+            ty::TyScrutineeVariant::StructScrutinee { fields, .. } => {
+                // If a filed does not have a scrutinee, the field itself is a variable.
+                for field in fields {
+                    match &field.scrutinee {
+                        Some(scrutinee) => recursively_collect_duplicate_variables(
+                            branches,
+                            left_most_branch,
+                            scrutinee,
+                        ),
+                        None => add_variable(left_most_branch, &field.field, true),
+                    }
+                }
+            }
+            ty::TyScrutineeVariant::Or(scrutinees) => {
+                let (first, others) = scrutinees
+                    .split_first()
+                    .expect("There must be at least two alternatives in TyScrutineeVariant::Or.");
+
+                // For all other alternatives then the first (left-most) one, span a new branch and pass it as a left-most.
+                // The new branch contains the identifiers collected so far in the left-most branch,
+                // but without duplicates collected so far. We want to have only unique duplicates in each branch.
+                for scrutinee in others {
+                    let mut branch: HashMap<Ident, (bool, Vec<(bool, Span)>)> = left_most_branch
+                        .iter()
+                        .map(|(ident, (is_struct_field, _))| {
+                            (
+                                ident.clone(),
+                                (*is_struct_field, Vec::<(bool, Span)>::new()),
+                            )
+                        })
+                        .collect();
+
+                    recursively_collect_duplicate_variables(branches, &mut branch, scrutinee);
+
+                    branches.push(branch);
+                }
+
+                // The variables in the left-most alternative go to the original left-most branch.
+                recursively_collect_duplicate_variables(branches, left_most_branch, first);
+            }
+            ty::TyScrutineeVariant::Tuple(scrutinees) => {
+                for scrutinee in scrutinees {
+                    match &scrutinee.variant {
+                        ty::TyScrutineeVariant::Variable(ident) => {
+                            add_variable(left_most_branch, ident, false)
+                        }
+                        _ => recursively_collect_duplicate_variables(
+                            branches,
+                            left_most_branch,
+                            scrutinee,
+                        ),
+                    };
+                }
+            }
+            ty::TyScrutineeVariant::EnumScrutinee { value, .. } => {
+                recursively_collect_duplicate_variables(branches, left_most_branch, value)
+            }
+        }
+
+        fn add_variable(
+            duplicate_variables: &mut HashMap<Ident, (bool, Vec<MatchVariable>)>,
+            ident: &Ident,
+            is_struct_field: bool,
+        ) {
+            duplicate_variables
+                .entry(ident.clone())
+                .and_modify(|(_, vec)| vec.push((is_struct_field, ident.span())))
+                .or_insert((is_struct_field, vec![]));
+        }
+    }
+}

sway-core/src/semantic_analysis/ast_node/expression/match_expression/analysis/mod.rs

@@ -1,4 +1,5 @@
 mod constructor_factory;
+mod duplicates;
 mod matrix;
 mod patstack;
 mod pattern;
@@ -7,5 +8,6 @@ mod reachable_report;
 mod usefulness;
 mod witness_report;
 
+pub(crate) use duplicates::collect_duplicate_match_pattern_variables;
 pub(in crate::semantic_analysis::ast_node::expression) use reachable_report::ReachableReport;
 pub(crate) use usefulness::check_match_expression_usefulness;

sway-core/src/semantic_analysis/ast_node/expression/typed_expression.rs

@@ -43,6 +43,8 @@ use sway_types::{integer_bits::IntegerBits, u256::U256, Ident, Named, Span, Span
 
 use rustc_hash::FxHashSet;
 
+use either::Either;
+
 use std::collections::{HashMap, VecDeque};
 
 #[allow(clippy::too_many_arguments)]
@@ -657,11 +659,12 @@ impl ty::TyExpression {
                     span: reachable_report.scrutinee.span.clone(),
                     warning_content: Warning::MatchExpressionUnreachableArm {
                         match_value: value.span(),
-                        preceding_arms: arms_reachability[catch_all_arm_position]
-                            .scrutinee
-                            .span
-                            .clone(),
-                        preceding_arm_is_catch_all: true,
+                        preceding_arms: Either::Right(
+                            arms_reachability[catch_all_arm_position]
+                                .scrutinee
+                                .span
+                                .clone(),
+                        ),
                         unreachable_arm: reachable_report.scrutinee.span.clone(),
                         // In this case id doesn't matter if the concrete unreachable arm is
                         // the last arm or a catch-all arm itself.
@@ -697,12 +700,12 @@ impl ty::TyExpression {
                     span: last_arm_report.scrutinee.span.clone(),
                     warning_content: Warning::MatchExpressionUnreachableArm {
                         match_value: value.span(),
-                        preceding_arms: Span::join_all(
+                        preceding_arms: Either::Left(
                             other_arms_reachability
                                 .iter()
-                                .map(|report| report.scrutinee.span.clone()),
+                                .map(|report| report.scrutinee.span.clone())
+                                .collect(),
                         ),
-                        preceding_arm_is_catch_all: false,
                         unreachable_arm: last_arm_report.scrutinee.span.clone(),
                         is_last_arm: true,
                         is_catch_all_arm: last_arm_report.scrutinee.is_catch_all(),
@@ -711,6 +714,21 @@ impl ty::TyExpression {
             }
         }
 
+        // Emit errors for eventual multiple definitions of variables.
+        // These errors can be carried on. The desugared version will treat
+        // the duplicates as shadowing, which is fine for the rest of compilation.
+        for scrutinee in typed_scrutinees.iter() {
+            for duplicate in collect_duplicate_match_pattern_variables(scrutinee) {
+                handler.emit_err(CompileError::MultipleDefinitionsOfMatchArmVariable {
+                    match_value: value.span(),
+                    first_definition: duplicate.first_definition.1,
+                    first_definition_is_struct_field: duplicate.first_definition.0,
+                    duplicate: duplicate.duplicate.1,
+                    duplicate_is_struct_field: duplicate.duplicate.0,
+                });
+            }
+        }
+
         if witness_report.has_witnesses() {
             return Err(
                 handler.emit_err(CompileError::MatchExpressionNonExhaustive {
@@ -758,12 +776,12 @@ impl ty::TyExpression {
                         span: reachable_report.scrutinee.span.clone(),
                         warning_content: Warning::MatchExpressionUnreachableArm {
                             match_value: match_value.clone(),
-                            preceding_arms: Span::join_all(
+                            preceding_arms: Either::Left(
                                 arms_reachability[..index]
                                     .iter()
-                                    .map(|report| report.scrutinee.span.clone()),
+                                    .map(|report| report.scrutinee.span.clone())
+                                    .collect(),
                             ),
-                            preceding_arm_is_catch_all: false,
                             unreachable_arm: reachable_report.scrutinee.span.clone(),
                             is_last_arm: false,
                             is_catch_all_arm: false,

sway-error/Cargo.toml

@@ -9,6 +9,7 @@ license.workspace = true
 repository.workspace = true
 
 [dependencies]
+either = "1.9.0"
 num-traits = "0.2.14"
 smallvec = "1.7"
 sway-types = { version = "0.45.0", path = "../sway-types" }