Fix missing shadowing errors due to error deduplication

sway-core/src/semantic_analysis/namespace/items.rs

@@ -141,7 +141,7 @@ impl Items {
                     ) => {
                         handler.emit_err(CompileError::ConstantsCannotBeShadowed {
                             variable_or_constant: "Variable".to_string(),
-                            name: name.clone(),
+                            name: (&name).into(),
                             constant_span: constant_ident.span(),
                             constant_decl: if is_imported_constant {
                                 constant_decl.decl_span.clone()
@@ -163,7 +163,7 @@ impl Items {
                     ) => {
                         handler.emit_err(CompileError::ConstantsCannotBeShadowed {
                             variable_or_constant: "Constant".to_string(),
-                            name: name.clone(),
+                            name: (&name).into(),
                             constant_span: constant_ident.span(),
                             constant_decl: if is_imported_constant {
                                 constant_decl.decl_span.clone()
@@ -176,7 +176,7 @@ impl Items {
                     // constant shadowing a variable
                     (_, VariableDecl(variable_decl), _, _, ConstantDecl { .. }, _, _) => {
                         handler.emit_err(CompileError::ConstantShadowsVariable {
-                            name: name.clone(),
+                            name: (&name).into(),
                             variable_span: variable_decl.name.span(),
                         });
                     }
@@ -230,8 +230,9 @@ impl Items {
                         _,
                         GenericShadowingMode::Disallow,
                     ) => {
-                        handler
-                            .emit_err(CompileError::GenericShadowsGeneric { name: name.clone() });
+                        handler.emit_err(CompileError::GenericShadowsGeneric {
+                            name: (&name).into(),
+                        });
                     }
                     _ => {}
                 }

sway-core/src/semantic_analysis/namespace/module.rs

@@ -427,7 +427,7 @@ impl Module {
                 let dst_ns = &mut self[dst];
                 let add_synonym = |name| {
                     if let Some((_, GlobImport::No, _, _)) = dst_ns.use_synonyms.get(name) {
-                        handler.emit_err(CompileError::ShadowsOtherSymbol { name: name.clone() });
+                        handler.emit_err(CompileError::ShadowsOtherSymbol { name: name.into() });
                     }
                     dst_ns.use_synonyms.insert(
                         name.clone(),
@@ -508,7 +508,7 @@ impl Module {
                         let mut add_synonym = |name| {
                             if let Some((_, GlobImport::No, _, _)) = dst_ns.use_synonyms.get(name) {
                                 handler.emit_err(CompileError::ShadowsOtherSymbol {
-                                    name: name.clone(),
+                                    name: name.into(),
                                 });
                             }
                             dst_ns.use_synonyms.insert(

sway-error/src/error.rs

@@ -1,3 +1,4 @@
+// This test proves that https://github.com/FuelLabs/sway/issues/5502 is fixed.
 use crate::convert_parse_tree_error::ConvertParseTreeError;
 use crate::diagnostic::{Code, Diagnostic, Hint, Issue, Reason, ToDiagnostic};
 use crate::formatting::*;
@@ -29,15 +30,23 @@ impl fmt::Display for InterfaceName {
 }
 
 // TODO: Since moving to using Idents instead of strings, there are a lot of redundant spans in
-//       this type.
-//       Beware!!! If we remove those redundant spans (and we should!) we can have a situation that
-//       deduplication of error messages might remove errors that are actually not duplicates because
-//       although they point to the same Ident (in terms of name), the span can be different.
-//       Deduplication works on hashes and Ident's hash contains only the name and not the span.
-//       That's why we should consider always using IdentUnique whenever we extract the span from
-//       the provided Ident.
-//       Using IdentUnique will also clearly communicate that we are extracting the span from the
-//       provided identifier.
+//       this type. When replacing Strings + Spans with Idents, be aware of the rule explained below.
+
+// When defining error structures that display identifiers, we prefer passing Idents over Strings.
+// The error span can come from that same Ident or can be a different span.
+// We handle those two cases in the following way:
+//   - If the error span equals Ident's span, we use IdentUnique and never the plain Ident.
+//   - If the error span is different then Ident's span, we pass Ident and Span as two separate fields.
+//
+// The reason for this rule is clearly communicating the difference of the two cases in every error,
+// as well as avoiding issues with the error message deduplication explained below.
+//
+// Deduplication of error messages might remove errors that are actually not duplicates because
+// although they point to the same Ident (in terms of the identifier's name), the span can be different.
+// Deduplication works on hashes and Ident's hash contains only the name and not the span.
+// That's why we always use IdentUnique whenever we extract the span from the provided Ident.
+// Using IdentUnique also clearly communicates that we are extracting the span from the
+// provided identifier.
 #[derive(Error, Debug, Clone, PartialEq, Eq, Hash)]
 pub enum CompileError {
     #[error(
@@ -548,17 +557,20 @@ pub enum CompileError {
     #[error("Constants cannot be shadowed. {variable_or_constant} \"{name}\" shadows constant with the same name.")]
     ConstantsCannotBeShadowed {
         variable_or_constant: String,
-        name: Ident,
+        name: IdentUnique,
         constant_span: Span,
         constant_decl: Span,
         is_alias: bool,
     },
     #[error("Constants cannot shadow variables. The constant \"{name}\" shadows variable with the same name.")]
-    ConstantShadowsVariable { name: Ident, variable_span: Span },
+    ConstantShadowsVariable {
+        name: IdentUnique,
+        variable_span: Span,
+    },
     #[error("The imported symbol \"{name}\" shadows another symbol with the same name.")]
-    ShadowsOtherSymbol { name: Ident },
+    ShadowsOtherSymbol { name: IdentUnique },
     #[error("The name \"{name}\" is already used for a generic parameter in this scope.")]
-    GenericShadowsGeneric { name: Ident },
+    GenericShadowsGeneric { name: IdentUnique },
     #[error("Non-exhaustive match expression. Missing patterns {missing_patterns}")]
     MatchExpressionNonExhaustive {
         missing_patterns: String,

test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/Forc.lock

@@ -0,0 +1,13 @@
+[[package]]
+name = "core"
+source = "path+from-root-4CB50D8895EED260"
+
+[[package]]
+name = "deduplication_of_shadowing_errors"
+source = "member"
+dependencies = ["std"]
+
+[[package]]
+name = "std"
+source = "path+from-root-4CB50D8895EED260"
+dependencies = ["core"]

test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/Forc.toml

@@ -0,0 +1,8 @@
+[project]
+authors = ["Fuel Labs <[email protected]>"]
+entry = "main.sw"
+license = "Apache-2.0"
+name = "deduplication_of_shadowing_errors"
+
+[dependencies]
+std = { path = "../../../../../../sway-lib-std" }

test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/json_abi_oracle.json

@@ -0,0 +1,14 @@
+[
+  {
+    "inputs": [],
+    "name": "main",
+    "outputs": [
+      {
+        "components": null,
+        "name": "",
+        "type": "u64"
+      }
+    ],
+    "type": "function"
+  }
+]

test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/src/lib.sw

@@ -0,0 +1,3 @@
+library;
+
+pub struct Struct { }

test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/src/main.sw

@@ -0,0 +1,30 @@
+// This test proves that https://github.com/FuelLabs/sway/issues/5502 is fixed.
+script;
+
+mod lib;
+
+use lib::Struct;
+use lib::Struct;
+use lib::Struct;
+
+const X = 0;
+
+fn main() -> () {
+    let X = 1;
+
+    let y = 3;
+
+    {
+        const y = 4;
+    }
+
+    {
+        const y = 6;
+    }
+}
+
+fn var_shadows_const_x() {
+    let X = 3;
+}
+
+fn generic<T, T, T>(_x: T) { }

test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/test.toml

@@ -0,0 +1,41 @@
+category = "fail"
+
+#check: $()error
+#nextln: $()main.sw:7
+#check: $()use lib::Struct;
+#check: $()The imported symbol "Struct" shadows another symbol with the same name.
+
+#check: $()error
+#nextln: $()main.sw:8
+#check: $()use lib::Struct;
+#check: $()The imported symbol "Struct" shadows another symbol with the same name.
+
+#check: $()Constants cannot be shadowed
+#nextln: $()main.sw:13
+#check: $()let X = 1;
+#nextln: $()Variable "X" shadows constant of the same name.
+
+#check: $()Constants cannot shadow variables
+#nextln: $()main.sw:18
+#check: $()const y = 4;
+#nextln: $()Constant "y" shadows variable of the same name.
+
+#check: $()Constants cannot shadow variables
+#nextln: $()main.sw:22
+#check: $()const y = 6;
+#nextln: $()Constant "y" shadows variable of the same name.
+
+#check: $()Constants cannot be shadowed
+#nextln: $()main.sw:27
+#check: $()let X = 3;
+#nextln: $()Variable "X" shadows constant of the same name.
+
+#check: $()error
+#nextln: $()main.sw:30:15
+#check: $()fn generic<T, T, T>(_x: T) { }
+#check: $()The name "T" is already used for a generic parameter in this scope.
+
+#check: $()error
+#nextln: $()main.sw:30:18
+#check: $()fn generic<T, T, T>(_x: T) { }
+#check: $()The name "T" is already used for a generic parameter in this scope.