diff --git a/sway-core/src/semantic_analysis/namespace/items.rs b/sway-core/src/semantic_analysis/namespace/items.rs index fca38496363..8e96e04dbd7 100644 --- a/sway-core/src/semantic_analysis/namespace/items.rs +++ b/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(), + }); } _ => {} } diff --git a/sway-core/src/semantic_analysis/namespace/module.rs b/sway-core/src/semantic_analysis/namespace/module.rs index 555c49b2c6b..abf2f70e7e4 100644 --- a/sway-core/src/semantic_analysis/namespace/module.rs +++ b/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( diff --git a/sway-error/src/error.rs b/sway-error/src/error.rs index 747025f52bb..67c1df6f96d 100644 --- a/sway-error/src/error.rs +++ b/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, diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/Forc.lock new file mode 100644 index 00000000000..c6addf56fad --- /dev/null +++ b/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"] diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/Forc.toml new file mode 100644 index 00000000000..845110d2d42 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/Forc.toml @@ -0,0 +1,8 @@ +[project] +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" +name = "deduplication_of_shadowing_errors" + +[dependencies] +std = { path = "../../../../../../sway-lib-std" } diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/json_abi_oracle.json b/test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/json_abi_oracle.json new file mode 100644 index 00000000000..905c3bd1034 --- /dev/null +++ b/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" + } +] \ No newline at end of file diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/src/lib.sw b/test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/src/lib.sw new file mode 100644 index 00000000000..fd02869807c --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/src/lib.sw @@ -0,0 +1,3 @@ +library; + +pub struct Struct { } \ No newline at end of file diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/src/main.sw new file mode 100644 index 00000000000..b1e8af99707 --- /dev/null +++ b/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(_x: T) { } \ No newline at end of file diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/test.toml b/test/src/e2e_vm_tests/test_programs/should_fail/deduplication_of_shadowing_errors/test.toml new file mode 100644 index 00000000000..895e150b749 --- /dev/null +++ b/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(_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(_x: T) { } +#check: $()The name "T" is already used for a generic parameter in this scope.