Skip to content

Commit 8a3d8df

Browse files
jjcnnIGI-111JoshuaBatty
authored
Check for type aliases when implementing traits (#6875)
## Description Fixes #6329 . During insertion of `impl`s into the trait map we used to forget to check for alias types, which meant that it was possible to have multiple implementations of a trait for the same type, one for the original type and one for the alias. #6626 fixed this bug, but the error message was unhelpful in that it reported the error on the original type without mentioning the alias. This PR improves the error message, and also improves the error message for aliased types when they contain multiple declarations of the same member. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [ ] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [x] I have added tests that prove my fix is effective or that my feature works. - [x] 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: IGI-111 <[email protected]> Co-authored-by: Joshua Batty <[email protected]>
1 parent 8c0cc20 commit 8a3d8df

File tree

9 files changed

+169
-28
lines changed

9 files changed

+169
-28
lines changed

‎sway-core/src/semantic_analysis/namespace/trait_map.rs

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,7 @@ impl TraitMap {
237237
is_extending_existing_impl: IsExtendingExistingImpl,
238238
engines: &Engines,
239239
) -> Result<(), ErrorEmitted> {
240-
let type_id = engines.te().get_unaliased_type_id(type_id);
240+
let unaliased_type_id = engines.te().get_unaliased_type_id(type_id);
241241

242242
handler.scope(|handler| {
243243
let mut trait_items: TraitItems = HashMap::new();
@@ -267,7 +267,7 @@ impl TraitMap {
267267
}
268268
}
269269

270-
let trait_impls = self.get_impls_mut(engines, type_id);
270+
let trait_impls = self.get_impls_mut(engines, unaliased_type_id);
271271

272272
// check to see if adding this trait will produce a conflicting definition
273273
for TraitEntry {
@@ -295,11 +295,11 @@ impl TraitMap {
295295

296296
let unify_checker = UnifyCheck::non_generic_constraint_subset(engines);
297297

298-
// Types are subset if the `type_id` that we want to insert can unify with the
298+
// Types are subset if the `unaliased_type_id` that we want to insert can unify with the
299299
// existing `map_type_id`. In addition we need to additionally check for the case of
300300
// `&mut <type>` and `&<type>`.
301-
let types_are_subset = unify_checker.check(type_id, *map_type_id)
302-
&& is_unified_type_subset(engines.te(), type_id, *map_type_id);
301+
let types_are_subset = unify_checker.check(unaliased_type_id, *map_type_id)
302+
&& is_unified_type_subset(engines.te(), unaliased_type_id, *map_type_id);
303303

304304
/// `left` can unify into `right`. Additionally we need to check subset condition in case of
305305
/// [TypeInfo::Ref] types. Although `&mut <type>` can unify with `&<type>`
@@ -368,6 +368,9 @@ impl TraitMap {
368368
handler.emit_err(CompileError::ConflictingImplsForTraitAndType {
369369
trait_name: trait_name.to_string_with_args(engines, &trait_type_args),
370370
type_implementing_for: engines.help_out(type_id).to_string(),
371+
type_implementing_for_unaliased: engines
372+
.help_out(unaliased_type_id)
373+
.to_string(),
371374
existing_impl_span: existing_impl_span.clone(),
372375
second_impl_span: impl_span.clone(),
373376
});
@@ -382,43 +385,61 @@ impl TraitMap {
382385
ResolvedTraitImplItem::Parsed(_item) => todo!(),
383386
ResolvedTraitImplItem::Typed(item) => match item {
384387
ty::TyTraitItem::Fn(decl_ref) => {
385-
if map_trait_items.get(name).is_some() {
388+
if let Some(existing_item) = map_trait_items.get(name) {
386389
handler.emit_err(
387390
CompileError::DuplicateDeclDefinedForType {
388391
decl_kind: "method".into(),
389392
decl_name: decl_ref.name().to_string(),
390393
type_implementing_for: engines
391394
.help_out(type_id)
392395
.to_string(),
393-
span: decl_ref.name().span(),
396+
type_implementing_for_unaliased: engines
397+
.help_out(unaliased_type_id)
398+
.to_string(),
399+
existing_impl_span: existing_item
400+
.span(engines)
401+
.clone(),
402+
second_impl_span: decl_ref.name().span(),
394403
},
395404
);
396405
}
397406
}
398407
ty::TyTraitItem::Constant(decl_ref) => {
399-
if map_trait_items.get(name).is_some() {
408+
if let Some(existing_item) = map_trait_items.get(name) {
400409
handler.emit_err(
401410
CompileError::DuplicateDeclDefinedForType {
402411
decl_kind: "constant".into(),
403412
decl_name: decl_ref.name().to_string(),
404413
type_implementing_for: engines
405414
.help_out(type_id)
406415
.to_string(),
407-
span: decl_ref.name().span(),
416+
type_implementing_for_unaliased: engines
417+
.help_out(unaliased_type_id)
418+
.to_string(),
419+
existing_impl_span: existing_item
420+
.span(engines)
421+
.clone(),
422+
second_impl_span: decl_ref.name().span(),
408423
},
409424
);
410425
}
411426
}
412427
ty::TyTraitItem::Type(decl_ref) => {
413-
if map_trait_items.get(name).is_some() {
428+
if let Some(existing_item) = map_trait_items.get(name) {
414429
handler.emit_err(
415430
CompileError::DuplicateDeclDefinedForType {
416431
decl_kind: "type".into(),
417432
decl_name: decl_ref.name().to_string(),
418433
type_implementing_for: engines
419434
.help_out(type_id)
420435
.to_string(),
421-
span: decl_ref.name().span(),
436+
type_implementing_for_unaliased: engines
437+
.help_out(unaliased_type_id)
438+
.to_string(),
439+
existing_impl_span: existing_item
440+
.span(engines)
441+
.clone(),
442+
second_impl_span: decl_ref.name().span(),
422443
},
423444
);
424445
}
@@ -442,7 +463,7 @@ impl TraitMap {
442463
trait_name,
443464
impl_span.clone(),
444465
trait_decl_span,
445-
type_id,
466+
unaliased_type_id,
446467
trait_items,
447468
engines,
448469
);

‎sway-error/src/error.rs

Lines changed: 47 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -625,6 +625,7 @@ pub enum CompileError {
625625
ConflictingImplsForTraitAndType {
626626
trait_name: String,
627627
type_implementing_for: String,
628+
type_implementing_for_unaliased: String,
628629
existing_impl_span: Span,
629630
second_impl_span: Span,
630631
},
@@ -640,7 +641,9 @@ pub enum CompileError {
640641
decl_kind: String,
641642
decl_name: String,
642643
type_implementing_for: String,
643-
span: Span,
644+
type_implementing_for_unaliased: String,
645+
existing_impl_span: Span,
646+
second_impl_span: Span,
644647
},
645648
#[error("The function \"{fn_name}\" in {interface_name} is defined with {num_parameters} parameters, but the provided implementation has {provided_parameters} parameters.")]
646649
IncorrectNumberOfInterfaceSurfaceFunctionParameters {
@@ -1180,7 +1183,9 @@ impl Spanned for CompileError {
11801183
second_impl_span, ..
11811184
} => second_impl_span.clone(),
11821185
MarkerTraitExplicitlyImplemented { span, .. } => span.clone(),
1183-
DuplicateDeclDefinedForType { span, .. } => span.clone(),
1186+
DuplicateDeclDefinedForType {
1187+
second_impl_span, ..
1188+
} => second_impl_span.clone(),
11841189
IncorrectNumberOfInterfaceSurfaceFunctionParameters { span, .. } => span.clone(),
11851190
ArgumentParameterTypeMismatch { span, .. } => span.clone(),
11861191
RecursiveCall { span, .. } => span.clone(),
@@ -2317,27 +2322,62 @@ impl ToDiagnostic for CompileError {
23172322
format!("{}- referencing a mutable copy of \"{decl_name}\", by returning it from a block: `&mut {{ {decl_name} }}`.", Indent::Single)
23182323
],
23192324
},
2320-
ConflictingImplsForTraitAndType { trait_name, type_implementing_for, existing_impl_span, second_impl_span } => Diagnostic {
2325+
ConflictingImplsForTraitAndType { trait_name, type_implementing_for, type_implementing_for_unaliased, existing_impl_span, second_impl_span } => Diagnostic {
23212326
reason: Some(Reason::new(code(1), "Trait is already implemented for type".to_string())),
23222327
issue: Issue::error(
23232328
source_engine,
23242329
second_impl_span.clone(),
2325-
format!("Trait \"{trait_name}\" is already implemented for type \"{type_implementing_for}\".")
2330+
if type_implementing_for == type_implementing_for_unaliased {
2331+
format!("Trait \"{trait_name}\" is already implemented for type \"{type_implementing_for}\".")
2332+
} else {
2333+
format!("Trait \"{trait_name}\" is already implemented for type \"{type_implementing_for}\" (which is an alias for \"{type_implementing_for_unaliased}\").")
2334+
}
23262335
),
23272336
hints: vec![
23282337
Hint::info(
23292338
source_engine,
23302339
existing_impl_span.clone(),
2331-
format!("This is the already existing implementation of \"{}\" for \"{type_implementing_for}\".",
2332-
call_path_suffix_with_args(trait_name)
2333-
)
2340+
if type_implementing_for == type_implementing_for_unaliased {
2341+
format!("This is the already existing implementation of \"{}\" for \"{type_implementing_for}\".",
2342+
call_path_suffix_with_args(trait_name)
2343+
)
2344+
} else {
2345+
format!("This is the already existing implementation of \"{}\" for \"{type_implementing_for}\" (which is an alias for \"{type_implementing_for_unaliased}\").",
2346+
call_path_suffix_with_args(trait_name)
2347+
)
2348+
}
23342349
),
23352350
],
23362351
help: vec![
23372352
"In Sway, there can be at most one implementation of a trait for any given type.".to_string(),
23382353
"This property is called \"trait coherence\".".to_string(),
23392354
],
23402355
},
2356+
DuplicateDeclDefinedForType { decl_kind, decl_name, type_implementing_for, type_implementing_for_unaliased, existing_impl_span, second_impl_span } => {
2357+
let decl_kind_snake_case = sway_types::style::to_upper_camel_case(decl_kind);
2358+
Diagnostic {
2359+
reason: Some(Reason::new(code(1), "Type contains duplicate declarations".to_string())),
2360+
issue: Issue::error(
2361+
source_engine,
2362+
second_impl_span.clone(),
2363+
if type_implementing_for == type_implementing_for_unaliased {
2364+
format!("{decl_kind_snake_case} \"{decl_name}\" already declared in type \"{type_implementing_for}\".")
2365+
} else {
2366+
format!("{decl_kind_snake_case} \"{decl_name}\" already declared in type \"{type_implementing_for}\" (which is an alias for \"{type_implementing_for_unaliased}\").")
2367+
}
2368+
),
2369+
hints: vec![
2370+
Hint::info(
2371+
source_engine,
2372+
existing_impl_span.clone(),
2373+
format!("\"{decl_name}\" previously defined here.")
2374+
)
2375+
],
2376+
help: vec![
2377+
"A type may not contain two or more declarations of the same name".to_string(),
2378+
],
2379+
}
2380+
},
23412381
MarkerTraitExplicitlyImplemented { marker_trait_full_name, span} => Diagnostic {
23422382
reason: Some(Reason::new(code(1), "Marker traits cannot be explicitly implemented".to_string())),
23432383
issue: Issue::error(

‎test/src/e2e_vm_tests/test_programs/should_fail/impl_self_duplicated/test.toml

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,5 +2,9 @@ category = "fail"
22
validate_abi = false
33
expected_warnings = 3
44

5-
# check: fn foo(self) -> u64 {
6-
# nextln: $()Duplicate definitions for the method "foo" for type "S<T>".
5+
# check: $()Type contains duplicate declarations
6+
# check: $()fn foo(self) -> u64 {
7+
# check: $()"foo" previously defined here.
8+
# check: $()Method "foo" already declared in type "S<T>".
9+
10+
# check: $()Aborting due to 1 error.

‎test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/test.toml

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,27 @@
11
category = "fail"
22

3+
# check: $()Type contains duplicate declarations
34
# check: $()fn my_add(self, other: Self) -> Self {
4-
# check: $()Duplicate definitions for the method "my_add" for type "Data<T>".
5+
# check: $()"my_add" previously defined here.
6+
# check: $()fn my_add(self, other: Self) -> Self {
7+
# check: $()Method "my_add" already declared in type "Data<T>".
58

9+
# check: $()Type contains duplicate declarations
10+
# check: $()fn get_value(self) -> u64 {
11+
# check: $()"get_value" previously defined here.
612
# check: $()fn get_value(self) -> u64 {
7-
# check: $()Duplicate definitions for the method "get_value" for type "Data<u64>".
13+
# check: $(Method "get_value" already declared in type "Data<u64>".
814

15+
# check: $()Type contains duplicate declarations
16+
# check: $()fn my_add(self, other: Self) -> Self {
17+
# check: $()"my_add" previously defined here.
918
# check: $()fn my_add(self, other: Self) -> Self {
10-
# check: $()Duplicate definitions for the method "my_add" for type "Data<u64>".
19+
# check: $()Method "my_add" already declared in type "Data<u64>".
1120

21+
# check: $()Type contains duplicate declarations
1222
# check: $()fn my_add(self, other: Self) -> Self {
13-
# check: $()Duplicate definitions for the method "my_add" for type "Data<u32>".
23+
# check: $()"my_add" previously defined here.
24+
# check: $()fn my_add(self, other: Self) -> Self {
25+
# check: $()Method "my_add" already declared in type "Data<u64>".
26+
27+
# check: $()Aborting due to 6 errors

‎test/src/e2e_vm_tests/test_programs/should_fail/type_alias_shadowing_methods/test.toml

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
category = "fail"
22

3-
#check: $()impl Alias2 {
4-
#nextln: $()fn foo() {}
5-
#nextln: $()Duplicate definitions for the method "foo" for type "MyStruct1".
3+
#check: $()Type contains duplicate declarations
4+
#check: $()fn foo() {}
5+
#nextln: $()"foo" previously defined here.
6+
#check: $()fn foo() {}
7+
#nextln: $()Method "foo" already declared in type "Alias2" (which is an alias for "MyStruct1").

‎test/src/e2e_vm_tests/test_programs/should_fail/type_alias_unification/Forc.lock

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
[[package]]
2+
name = "core"
3+
source = "path+from-root-6E306998EEBF0DBD"
4+
5+
[[package]]
6+
name = "std"
7+
source = "path+from-root-6E306998EEBF0DBD"
8+
dependencies = ["core"]
9+
10+
[[package]]
11+
name = "type_alias_unification"
12+
source = "member"
13+
dependencies = ["std"]

‎test/src/e2e_vm_tests/test_programs/should_fail/type_alias_unification/Forc.toml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
[project]
2+
authors = ["Fuel Labs <[email protected]>"]
3+
entry = "main.sw"
4+
license = "Apache-2.0"
5+
name = "type_alias_unification"
6+
7+
[dependencies]
8+
std = { path = "../../../reduced_std_libs/sway-lib-std-assert" }

‎test/src/e2e_vm_tests/test_programs/should_fail/type_alias_unification/src/main.sw

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
script;
2+
3+
trait MyTrait {
4+
fn extract_a(self) -> u64;
5+
}
6+
7+
struct A {
8+
a: u64,
9+
}
10+
11+
impl MyTrait for A {
12+
fn extract_a(self) -> u64 {
13+
self.a
14+
}
15+
}
16+
17+
type B = A;
18+
19+
// B is an alias for A, and A already has an implementation of MyTrait,
20+
// so this should cause a compilation error.
21+
impl MyTrait for B {
22+
fn extract_a(self) -> u64 {
23+
self.a + 1
24+
}
25+
}
26+
27+
fn main() {
28+
let struct_a = A { a: 1 };
29+
let struct_b = B { a: 42 };
30+
assert(struct_a.extract_a() == 1);
31+
assert(struct_b.extract_a() == 42);
32+
}

‎test/src/e2e_vm_tests/test_programs/should_fail/type_alias_unification/test.toml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
category = "fail"
2+
3+
#check: $()Trait is already implemented for type
4+
#check: $()This is the already existing implementation of "MyTrait" for "B" (which is an alias for "A").
5+
#check: $()Trait "type_alias_unification::MyTrait" is already implemented for type "B" (which is an alias for "A").
6+
7+
#check: $()Aborting due to 1 error.

0 commit comments

Comments
 (0)