Improve warnings in match expressions (#4965)

## Description

At the moment, we have just one warning message in `match` expressions:
"This match arm is unreachable."

This PR:
- enriches the existing warning with more contextual information.
- gives specific warning for redundant catch-all last arm as proposed in
#3949.
- gives specific warning for catch-all arm preceding other arms; this
can easily happen by chance in more complex patterns, e.g., when
confusing variable `A` for enum variant `Enum::A`.

What we still want to do is give a hint to user when `A` is (very
likely) used instead of `Enum::A`. This will be a part of a separate PR.

The structure of the messages still follow the Elm approach as explained
in the [Expressive Diagnostics
RFC](FuelLabs/sway-rfcs#30 (comment)).
Removing the _issue_ from the title and simplifying the wording will be
a part of a different PR.

Closes #3949.

## Demo

![Unreachable middle
arm](https://github.com/FuelLabs/sway/assets/4142833/80a5c471-0995-45ec-96c4-ec43b9c09a9c)

![Unreachable last
arm](https://github.com/FuelLabs/sway/assets/4142833/7fdd5e2e-f195-49b6-8042-f17b82164c96)

![Interior catch-all arm makes arms below it
unreachable](https://github.com/FuelLabs/sway/assets/4142833/59d33c95-2409-48e9-8f12-d4e446997fa9)

![Redundant last catch-all
arm](https://github.com/FuelLabs/sway/assets/4142833/e1bbea08-e343-4fb1-a357-56123e05046d)

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.

---------

Co-authored-by: Joshua Batty <joshpbatty@gmail.com>

Author: ironcev

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

@@ -7,4 +7,5 @@ mod reachable_report;
 mod usefulness;
 mod witness_report;
 
+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/match_expression/analysis/reachable_report.rs

@@ -1,17 +1,15 @@
-use sway_types::Span;
-
 use crate::language::ty;
 
 pub(crate) struct ReachableReport {
     pub(crate) reachable: bool,
-    pub(crate) span: Span,
+    pub(crate) scrutinee: ty::TyScrutinee,
 }
 
 impl ReachableReport {
     pub(super) fn new(reachable: bool, scrutinee: ty::TyScrutinee) -> ReachableReport {
         ReachableReport {
             reachable,
-            span: scrutinee.span,
+            scrutinee,
         }
     }
 }

sway-core/src/semantic_analysis/ast_node/expression/match_expression/typed/matcher.rs

@@ -28,7 +28,7 @@ pub(crate) type MatcherResult = (MatchReqMap, MatchDeclMap);
 /// This algorithm desugars pattern matching into a [MatcherResult], by creating two lists,
 /// the [MatchReqMap] which is a list of requirements that a desugared if expression
 /// must include in the conditional in conjunctive normal form.
-/// and the [MatchImplMap] which is a list of variable
+/// and the [MatchDeclMap] which is a list of variable
 /// declarations that must be placed inside the body of the if expression.
 ///
 /// Given the following example
@@ -62,7 +62,7 @@ pub(crate) type MatcherResult = (MatchReqMap, MatchDeclMap);
 /// ]
 /// ```
 ///
-/// The first match arm would create a [MatchImplMap] of roughly:
+/// The first match arm would create a [MatchDeclMap] of roughly:
 ///
 /// ```ignore
 /// [
@@ -81,7 +81,7 @@ pub(crate) type MatcherResult = (MatchReqMap, MatchDeclMap);
 /// ]
 /// ```
 ///
-/// The second match arm would create a [MatchImplMap] of roughly:
+/// The second match arm would create a [MatchDeclMap] of roughly:
 ///
 /// ```ignore
 /// [
@@ -103,7 +103,7 @@ pub(crate) type MatcherResult = (MatchReqMap, MatchDeclMap);
 /// ]
 /// ```
 ///
-/// The third match arm would create a [MatchImplMap] of roughly:
+/// The third match arm would create a [MatchDeclMap] of roughly:
 ///
 /// ```ignore
 /// []

sway-core/src/semantic_analysis/ast_node/expression/match_expression/typed/typed_scrutinee.rs

@@ -104,6 +104,26 @@ impl ty::TyScrutinee {
             Scrutinee::Error { err, .. } => Err(err),
         }
     }
+
+    /// Returns true if the `TyScrutinee` consists only of catch-all scrutinee variants, recursively.
+    /// Catch-all variants are .., _, and variables.
+    /// E.g.: (_, x, Point { .. })
+    /// A catch-all scrutinee matches all the values of the corresponding type.
+    pub(crate) fn is_catch_all(&self) -> bool {
+        match &self.variant {
+            ty::TyScrutineeVariant::CatchAll => true,
+            ty::TyScrutineeVariant::Variable(_) => true,
+            ty::TyScrutineeVariant::Literal(_) => false,
+            ty::TyScrutineeVariant::Constant { .. } => false,
+            ty::TyScrutineeVariant::StructScrutinee { fields, .. } => fields
+                .iter()
+                .filter_map(|x| x.scrutinee.as_ref())
+                .all(|x| x.is_catch_all()),
+            ty::TyScrutineeVariant::Or(elems) => elems.iter().all(|x| x.is_catch_all()),
+            ty::TyScrutineeVariant::Tuple(elems) => elems.iter().all(|x| x.is_catch_all()),
+            ty::TyScrutineeVariant::EnumScrutinee { .. } => false,
+        }
+    }
 }
 
 fn type_check_variable(

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

@@ -10,6 +10,7 @@ mod tuple_index_access;
 mod unsafe_downcast;
 
 use self::constant_expression::instantiate_constant_expression;
+
 pub(crate) use self::{
     enum_instantiation::*, function_application::*, if_expression::*, lazy_operator::*,
     method_application::*, struct_field_access::*, struct_instantiation::*, tuple_index_access::*,
@@ -24,7 +25,7 @@ use crate::{
         ty::{self, TyImplItem},
         *,
     },
-    semantic_analysis::*,
+    semantic_analysis::{expression::ReachableReport, *},
     transform::to_parsed_lang::type_name_to_type_info_opt,
     type_system::*,
     Engines,
@@ -646,14 +647,69 @@ impl ty::TyExpression {
             typed_scrutinees.clone(),
             span.clone(),
         )?;
-        for reachable_report in arms_reachability.into_iter() {
-            if !reachable_report.reachable {
+
+        // if there is an interior catch-all arm
+        if let Some(catch_all_arm_position) = interior_catch_all_arm_position(&arms_reachability) {
+            // show the warning on the arms below it that it makes them unreachable...
+            for reachable_report in arms_reachability[catch_all_arm_position + 1..].iter() {
                 handler.emit_warn(CompileWarning {
-                    span: reachable_report.span,
-                    warning_content: Warning::MatchExpressionUnreachableArm,
+                    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,
+                        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.
+                        // We want to point out the interior catch-all arm as problematic.
+                        // So we simply put these two values both to false.
+                        is_last_arm: false,
+                        is_catch_all_arm: false,
+                    },
                 });
             }
+
+            //...but still check the arms above it for reachability
+            check_interior_non_catch_all_arms_for_reachability(
+                handler,
+                &value.span(),
+                &arms_reachability[..catch_all_arm_position],
+            );
         }
+        // if there are no interior catch-all arms and there is more then one arm
+        else if let Some((last_arm_report, other_arms_reachability)) =
+            arms_reachability.split_last()
+        {
+            // check reachable report for all the arms except the last one
+            check_interior_non_catch_all_arms_for_reachability(
+                handler,
+                &value.span(),
+                other_arms_reachability,
+            );
+
+            // for the last one, give a different warning if it is an unreachable catch-all arm
+            if !last_arm_report.reachable {
+                handler.emit_warn(CompileWarning {
+                    span: last_arm_report.scrutinee.span.clone(),
+                    warning_content: Warning::MatchExpressionUnreachableArm {
+                        match_value: value.span(),
+                        preceding_arms: Span::join_all(
+                            other_arms_reachability
+                                .iter()
+                                .map(|report| report.scrutinee.span.clone()),
+                        ),
+                        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(),
+                    },
+                });
+            }
+        }
+
         if witness_report.has_witnesses() {
             return Err(
                 handler.emit_err(CompileError::MatchExpressionNonExhaustive {
@@ -675,7 +731,46 @@ impl ty::TyExpression {
             },
         };
 
-        Ok(match_exp)
+        return Ok(match_exp);
+
+        /// Returns the position of the first match arm that is an "interior" arm, meaning:
+        ///  - arm is a catch-all arm
+        ///  - arm is not the last match arm
+        /// or `None` if such arm does not exist.
+        /// Note that the arm can be the first arm.
+        fn interior_catch_all_arm_position(arms_reachability: &[ReachableReport]) -> Option<usize> {
+            arms_reachability
+                .split_last()?
+                .1
+                .iter()
+                .position(|report| report.scrutinee.is_catch_all())
+        }
+
+        fn check_interior_non_catch_all_arms_for_reachability(
+            handler: &Handler,
+            match_value: &Span,
+            arms_reachability: &[ReachableReport],
+        ) {
+            for (index, reachable_report) in arms_reachability.iter().enumerate() {
+                if !reachable_report.reachable {
+                    handler.emit_warn(CompileWarning {
+                        span: reachable_report.scrutinee.span.clone(),
+                        warning_content: Warning::MatchExpressionUnreachableArm {
+                            match_value: match_value.clone(),
+                            preceding_arms: Span::join_all(
+                                arms_reachability[..index]
+                                    .iter()
+                                    .map(|report| report.scrutinee.span.clone()),
+                            ),
+                            preceding_arm_is_catch_all: false,
+                            unreachable_arm: reachable_report.scrutinee.span.clone(),
+                            is_last_arm: false,
+                            is_catch_all_arm: false,
+                        },
+                    });
+                }
+            }
+        }
     }
 
     #[allow(clippy::too_many_arguments)]

sway-error/src/diagnostic.rs

@@ -82,16 +82,22 @@ impl Diagnostic {
     pub fn related_sources(&self, include_issue_source: bool) -> Vec<&SourcePath> {
         let mut source_files = vec![];
 
-        // All unwrappings are safe because we check the existence
-        // either in is_in_source() or in in_source_info().
-        if self.issue.is_in_source() && include_issue_source {
+        let issue_is_in_source = self.issue.is_in_source();
+
+        // All source_path() unwrappings are safe because we check the existence
+        // of source in case of issue, and self.labels() returns
+        // only labels that are in source.
+        if issue_is_in_source && include_issue_source {
             source_files.push(self.issue.source_path().unwrap());
         }
 
         for hint in self.labels() {
             let file = hint.source_path().unwrap();
 
-            if !include_issue_source && file == self.issue.source_path().unwrap() {
+            if !include_issue_source
+                && issue_is_in_source
+                && file == self.issue.source_path().unwrap()
+            {
                 continue;
             }
 

sway-error/src/error.rs

@@ -910,7 +910,7 @@ impl ToDiagnostic for CompileError {
                     ),
                 ],
                 help: vec![
-                    "Unlike variables, constants cannot be shadowed by other constants or variables.".to_string(),
+                    format!("Unlike variables, constants cannot be shadowed by other constants or variables."),
                     match (variable_or_constant.as_str(), constant_decl.clone() != Span::dummy()) {
                         ("Variable", false) => format!("Consider renaming either the variable \"{name}\" or the constant \"{name}\"."),
                         ("Constant", false) => "Consider renaming one of the constants.".to_string(),
@@ -924,7 +924,7 @@ impl ToDiagnostic for CompileError {
                 ],
             },
             ConstantShadowsVariable { name , variable_span } => Diagnostic {
-                reason: Some(Reason::new(code(2), "Constants cannot shadow variables".to_string())),
+                reason: Some(Reason::new(code(1), "Constants cannot shadow variables".to_string())),
                 issue: Issue::error(
                     source_engine,
                     name.span(),
@@ -943,8 +943,8 @@ impl ToDiagnostic for CompileError {
                     ),
                 ],
                 help: vec![
-                    "Variables can shadow other variables, but constants cannot.".to_string(),
-                    "Consider renaming either the variable or the constant.".to_string(),
+                    format!("Variables can shadow other variables, but constants cannot."),
+                    format!("Consider renaming either the variable or the constant."),
                 ],
             },
            _ => Diagnostic {

sway-error/src/warning.rs

@@ -84,7 +84,14 @@ pub enum Warning {
     DeadStorageDeclarationForFunction {
         unneeded_attrib: String,
     },
-    MatchExpressionUnreachableArm,
+    MatchExpressionUnreachableArm {
+        match_value: Span,
+        preceding_arms: Span,
+        preceding_arm_is_catch_all: bool,
+        unreachable_arm: Span,
+        is_last_arm: bool,
+        is_catch_all_arm: bool,
+    },
     UnrecognizedAttribute {
         attrib_name: Ident,
     },
@@ -217,7 +224,7 @@ impl fmt::Display for Warning {
                 "This function's storage attributes declaration does not match its \
                  actual storage access pattern: '{unneeded_attrib}' attribute(s) can be removed."
             ),
-            MatchExpressionUnreachableArm => write!(f, "This match arm is unreachable."),
+            MatchExpressionUnreachableArm { .. } => write!(f, "This match arm is unreachable."),
             UnrecognizedAttribute {attrib_name} => write!(f, "Unknown attribute: \"{attrib_name}\"."),
             AttributeExpectedNumberOfArguments {attrib_name, received_args, expected_min_len, expected_max_len } => write!(
                 f,
@@ -258,21 +265,83 @@ impl ToDiagnostic for CompileWarning {
                 issue: Issue::warning(
                     source_engine,
                     name.span(),
-                    format!("Constant \"{name}\" should be SCREAMING_SNAKE_CASE")
+                    format!("Constant \"{name}\" should be SCREAMING_SNAKE_CASE"),
                 ),
                 hints: vec![
                     Hint::warning(
                         source_engine,
                         name.span(),
-                        format!("\"{name}\" should be SCREAMING_SNAKE_CASE, like \"{}\".", to_screaming_snake_case(name.as_str()))
+                        format!("\"{name}\" should be SCREAMING_SNAKE_CASE, like \"{}\".", to_screaming_snake_case(name.as_str())),
                     ),
                 ],
                 help: vec![
-                    "In Sway, ABIs, structs, traits, and enums are CapitalCase.".to_string(),
-                    "Modules, variables, and functions are snake_case, while constants are SCREAMING_SNAKE_CASE.".to_string(),
+                    format!("In Sway, ABIs, structs, traits, and enums are CapitalCase."),
+                    format!("Modules, variables, and functions are snake_case, while constants are SCREAMING_SNAKE_CASE."),
                     format!("Consider renaming the constant to, e.g., \"{}\".", to_screaming_snake_case(name.as_str())),
                 ],
             },
+            MatchExpressionUnreachableArm { match_value, preceding_arms, preceding_arm_is_catch_all, unreachable_arm, is_last_arm, is_catch_all_arm } => Diagnostic {
+                reason: Some(Reason::new(code(1), "Match arm is unreachable".to_string())),
+                issue: Issue::warning(
+                    source_engine,
+                    unreachable_arm.clone(),
+                    match (*is_last_arm, *is_catch_all_arm) {
+                        (true, true) => format!("Catch-all pattern \"{}\" in the last match arm will never be matched", unreachable_arm.as_str()),
+                        _ => format!("Pattern \"{}\" will never be matched", unreachable_arm.as_str())
+                    }
+                ),
+                hints: vec![
+                    Hint::warning(
+                        source_engine,
+                        unreachable_arm.clone(),
+                        format!("{} arm \"{}\" is unreachable.", if *is_last_arm && *is_catch_all_arm { "Last catch-all match" } else { "Match" }, unreachable_arm.as_str())
+                    ),
+                    Hint::info(
+                        source_engine,
+                        match_value.clone(),
+                        "This is the value to match on.".to_string()
+                    ),
+                    if *preceding_arm_is_catch_all {
+                        Hint::warning(
+                            source_engine,
+                            preceding_arms.clone(),
+                            format!("Catch-all arm \"{}\" makes all the arms below it unreachable.", preceding_arms.as_str())
+                        )
+                    }
+                    else {
+                        Hint::info(
+                            source_engine,
+                            preceding_arms.clone(),
+                            if *is_last_arm {
+                                format!("Preceding match arms already match all possible values of \"{}\".", match_value.as_str())
+                            }
+                            else {
+                                format!("Preceding match arms already match all the values that \"{}\" can match.", unreachable_arm.as_str())
+                            }
+                        )
+                    }
+                ],
+                help: if *preceding_arm_is_catch_all {
+                    vec![
+                        format!("Catch-all patterns make sense only in last match arms."),
+                        format!("Carefully check matching logic in all the arms and consider:"),
+                        format!(" - removing the catch-all arm \"{}\" or making it the last arm.", preceding_arms.as_str()),
+                        format!(" - removing the unreachable arms below \"{}\".", preceding_arms.as_str()),
+                    ]
+                }
+                else if *is_last_arm && *is_catch_all_arm {
+                    vec![
+                        format!("Catch-all patterns are often used in last match arms."),
+                        format!("But in this case, the preceding arms already match all possible values of \"{}\".", match_value.as_str()),
+                        format!("Carefully check matching logic in all the arms and consider removing the unreachable last catch-all arm."),
+                    ]
+                }
+                else {
+                    vec![
+                        format!("Carefully check matching logic in all the arms and consider removing the unreachable arm."),
+                    ]
+                }
+            },
            _ => Diagnostic {
                     // TODO: Temporary we use self here to achieve backward compatibility.
                     //       In general, self must not be used and will not be used once we