From 8e92fc78670510b6749cba10f075993c5483fc5e Mon Sep 17 00:00:00 2001 From: Marcos Henrich Date: Mon, 2 Jan 2023 17:11:23 +0000 Subject: [PATCH 1/5] Fixes issue with generic trait impls using the same method names. When two generic trait implementation use the same method implementation an error such as `Duplicate definition for method "convert" for type "A"` would occur. This fix does remove the error when a method with diferent type parameters is added to another trait implementation with different type parameters. The fix also ensures that find_method_for_type returns a method whose parameters types matches the given arguments expressions return types. Closes #3633. --- .../semantic_analysis/namespace/namespace.rs | 51 +++++++++++++++++-- .../semantic_analysis/namespace/trait_map.rs | 33 +++++++++++- .../src/main.sw | 27 ++++++++++ .../test.toml | 3 ++ .../language/generic_traits/src/main.sw | 39 +++++++++++++- 5 files changed, 147 insertions(+), 6 deletions(-) diff --git a/sway-core/src/semantic_analysis/namespace/namespace.rs b/sway-core/src/semantic_analysis/namespace/namespace.rs index c6d18bab074..217d3ce66ec 100644 --- a/sway-core/src/semantic_analysis/namespace/namespace.rs +++ b/sway-core/src/semantic_analysis/namespace/namespace.rs @@ -7,12 +7,15 @@ use crate::{ CompileResult, Ident, }; -use super::{module::Module, root::Root, submodule_namespace::SubmoduleNamespace, Path, PathBuf}; +use super::{ + module::Module, root::Root, submodule_namespace::SubmoduleNamespace, + trait_map::are_equal_minus_dynamic_types, Path, PathBuf, +}; use sway_error::error::CompileError; use sway_types::{span::Span, Spanned}; -use std::collections::VecDeque; +use std::{cmp::Ordering, collections::VecDeque}; /// The set of items that represent the namespace context passed throughout type checking. #[derive(Clone, Debug)] @@ -217,6 +220,8 @@ impl Namespace { let mut methods = local_methods; methods.append(&mut type_methods); + let mut matching_method_decl_ids: Vec = vec![]; + for decl_id in methods.into_iter() { let method = check!( CompileResult::from(decl_engine.get_function(decl_id.clone(), &decl_id.span())), @@ -225,8 +230,48 @@ impl Namespace { errors ); if &method.name == method_name { - return ok(decl_id, warnings, errors); + matching_method_decl_ids.push(decl_id); + } + } + + let matching_method_decl_id = match matching_method_decl_ids.len().cmp(&1) { + Ordering::Equal => Some(matching_method_decl_ids[0].clone()), + Ordering::Greater => { + // Case where multiple methods exist with the same name + // This is the case of https://github.com/FuelLabs/sway/issues/3633 + // where multiple generic trait impls use the same method name but with different parameter types + let mut matching_method_decl_ids2: Vec = vec![]; + for decl_id in matching_method_decl_ids.clone().into_iter() { + let method = check!( + CompileResult::from( + decl_engine.get_function(decl_id.clone(), &decl_id.span()) + ), + return err(warnings, errors), + warnings, + errors + ); + if method.parameters.len() == args_buf.len() + && !method.parameters.iter().zip(args_buf.iter()).any(|(p, a)| { + !are_equal_minus_dynamic_types(engines, p.type_id, a.return_type) + }) + { + matching_method_decl_ids2.push(decl_id); + } + } + if matching_method_decl_ids2.is_empty() { + // When we can't match any method with parameter types we still return the first method found + // This was the behavior before introducing the parameter type matching + Some(matching_method_decl_ids[0].clone()) + } else { + // In case one or more methods match the parameter types we return the first match. + Some(matching_method_decl_ids2[0].clone()) + } } + Ordering::Less => None, + }; + + if let Some(method_decl_id) = matching_method_decl_id { + return ok(method_decl_id, warnings, errors); } if !args_buf diff --git a/sway-core/src/semantic_analysis/namespace/trait_map.rs b/sway-core/src/semantic_analysis/namespace/trait_map.rs index 4f9edd2260e..8d7eb29e955 100644 --- a/sway-core/src/semantic_analysis/namespace/trait_map.rs +++ b/sway-core/src/semantic_analysis/namespace/trait_map.rs @@ -191,8 +191,14 @@ impl TraitMap { second_impl_span: impl_span.clone(), }); } else if types_are_subset { + let type_args_are_equal = trait_type_args.len() == map_trait_type_args.len() + && !trait_type_args + .iter() + .zip(map_trait_type_args.iter()) + .any(|(a1, a2)| !a1.eq(a2, engines)); + for (name, decl_id) in trait_methods.iter() { - if map_trait_methods.get(name).is_some() { + if let Some(map_trait_method_decl_id) = map_trait_methods.get(name) { let method = check!( CompileResult::from( decl_engine.get_function(decl_id.clone(), impl_span) @@ -201,6 +207,29 @@ impl TraitMap { warnings, errors ); + let map_trait_method = check!( + CompileResult::from( + decl_engine + .get_function(map_trait_method_decl_id.clone(), impl_span) + ), + return err(warnings, errors), + warnings, + errors + ); + if !type_args_are_equal + && (method.parameters.len() != map_trait_method.parameters.len() + || method + .parameters + .iter() + .zip(map_trait_method.parameters.iter()) + .any(|(p1, p2)| { + !are_equal_minus_dynamic_types( + engines, p1.type_id, p2.type_id, + ) + })) + { + continue; + } errors.push(CompileError::DuplicateMethodsDefinedForType { func_name: method.name.to_string(), type_implementing_for: engines.help_out(type_id).to_string(), @@ -772,7 +801,7 @@ impl TraitMap { } } -fn are_equal_minus_dynamic_types(engines: Engines<'_>, left: TypeId, right: TypeId) -> bool { +pub fn are_equal_minus_dynamic_types(engines: Engines<'_>, left: TypeId, right: TypeId) -> bool { if left.index() == right.index() { return true; } diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/src/main.sw index cbc6e92de02..fd53372f191 100644 --- a/test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/src/main.sw +++ b/test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/src/main.sw @@ -62,6 +62,33 @@ impl Data { } } +pub struct A { + a: u64, +} + +pub struct B { + b: u64, +} + +pub struct C { + c: u64, +} + +pub trait Convert { + fn convert(t: u64); +} + +impl Convert for A { + fn convert(t: u64) { + } +} + +impl Convert for A { + // duplicate definition + fn convert(t: u64) { + } +} + fn main() { } diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/test.toml b/test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/test.toml index 7c383bbf1db..86608eebd5a 100644 --- a/test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/test.toml +++ b/test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/test.toml @@ -11,3 +11,6 @@ category = "fail" # check: $()fn my_add(self, other: Self) -> Self { # check: $()Duplicate definitions for the method "my_add" for type "Data". + +# check: $()fn convert(t: u64) { +# check: $()Duplicate definitions for the method "convert" for type "A". diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/language/generic_traits/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/language/generic_traits/src/main.sw index 02c0c3f764d..5bf6a5e3c1d 100644 --- a/test/src/e2e_vm_tests/test_programs/should_pass/language/generic_traits/src/main.sw +++ b/test/src/e2e_vm_tests/test_programs/should_pass/language/generic_traits/src/main.sw @@ -130,6 +130,38 @@ impl MyTriple for MyU64 { } } +pub struct A { + a: u64, +} + +pub struct B { + b: u64, +} + +pub struct C { + c: u64, +} + +pub trait Convert { + fn convert(t: T) -> Self; +} + +impl Convert for A { + fn convert(t: B) -> Self { + A { + a: t.b + } + } +} + +impl Convert for A { + fn convert(t: C) -> Self { + A { + a: t.c + } + } +} + fn main() -> u64 { let a = FooBarData { value: 1u8 @@ -170,6 +202,9 @@ fn main() -> u64 { }; let q = p.my_triple(1); + let r_b = B { b: 42 }; + let r_c = C { c: 42 }; + if c == 42u8 && d && e == 9u64 @@ -179,7 +214,9 @@ fn main() -> u64 { && l == 50 && n == 240 && o == 360 - && q == 93 { + && q == 93 + && A::convert(r_b).a == 42 + && A::convert(r_c).a == 42 { 42 } else { 7 From 160ff6e9c032affd2b4ef9f08503169ad4527249 Mon Sep 17 00:00:00 2001 From: Marcos Henrich Date: Mon, 16 Jan 2023 17:37:55 +0000 Subject: [PATCH 2/5] Applies review changes. --- .../src/semantic_analysis/namespace/namespace.rs | 13 +++++++------ .../src/semantic_analysis/namespace/trait_map.rs | 6 +++++- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/sway-core/src/semantic_analysis/namespace/namespace.rs b/sway-core/src/semantic_analysis/namespace/namespace.rs index 217d3ce66ec..0181b9db01e 100644 --- a/sway-core/src/semantic_analysis/namespace/namespace.rs +++ b/sway-core/src/semantic_analysis/namespace/namespace.rs @@ -240,7 +240,7 @@ impl Namespace { // Case where multiple methods exist with the same name // This is the case of https://github.com/FuelLabs/sway/issues/3633 // where multiple generic trait impls use the same method name but with different parameter types - let mut matching_method_decl_ids2: Vec = vec![]; + let mut maybe_method_decl_id: Option = Option::None; for decl_id in matching_method_decl_ids.clone().into_iter() { let method = check!( CompileResult::from( @@ -255,16 +255,17 @@ impl Namespace { !are_equal_minus_dynamic_types(engines, p.type_id, a.return_type) }) { - matching_method_decl_ids2.push(decl_id); + maybe_method_decl_id = Some(decl_id); + break; } } - if matching_method_decl_ids2.is_empty() { + if let Some(matching_method_decl_id) = maybe_method_decl_id { + // In case one or more methods match the parameter types we return the first match. + Some(matching_method_decl_id) + } else { // When we can't match any method with parameter types we still return the first method found // This was the behavior before introducing the parameter type matching Some(matching_method_decl_ids[0].clone()) - } else { - // In case one or more methods match the parameter types we return the first match. - Some(matching_method_decl_ids2[0].clone()) } } Ordering::Less => None, diff --git a/sway-core/src/semantic_analysis/namespace/trait_map.rs b/sway-core/src/semantic_analysis/namespace/trait_map.rs index 8d7eb29e955..9943da888ad 100644 --- a/sway-core/src/semantic_analysis/namespace/trait_map.rs +++ b/sway-core/src/semantic_analysis/namespace/trait_map.rs @@ -801,7 +801,11 @@ impl TraitMap { } } -pub fn are_equal_minus_dynamic_types(engines: Engines<'_>, left: TypeId, right: TypeId) -> bool { +pub(crate) fn are_equal_minus_dynamic_types( + engines: Engines<'_>, + left: TypeId, + right: TypeId, +) -> bool { if left.index() == right.index() { return true; } From 355d1280105cf0c9864b7c651f8695b59375dc30 Mon Sep 17 00:00:00 2001 From: Marcos Henrich Date: Thu, 19 Jan 2023 13:09:42 +0000 Subject: [PATCH 3/5] Applies review changes. --- sway-core/src/semantic_analysis/namespace/namespace.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sway-core/src/semantic_analysis/namespace/namespace.rs b/sway-core/src/semantic_analysis/namespace/namespace.rs index 0181b9db01e..fc9a22475d3 100644 --- a/sway-core/src/semantic_analysis/namespace/namespace.rs +++ b/sway-core/src/semantic_analysis/namespace/namespace.rs @@ -235,7 +235,7 @@ impl Namespace { } let matching_method_decl_id = match matching_method_decl_ids.len().cmp(&1) { - Ordering::Equal => Some(matching_method_decl_ids[0].clone()), + Ordering::Equal => matching_method_decl_ids.get(0).cloned(), Ordering::Greater => { // Case where multiple methods exist with the same name // This is the case of https://github.com/FuelLabs/sway/issues/3633 @@ -265,7 +265,7 @@ impl Namespace { } else { // When we can't match any method with parameter types we still return the first method found // This was the behavior before introducing the parameter type matching - Some(matching_method_decl_ids[0].clone()) + matching_method_decl_ids.get(0).cloned() } } Ordering::Less => None, From 4154540d343a5c0a572ea8384bbdd29b2417492e Mon Sep 17 00:00:00 2001 From: Marcos Henrich Date: Thu, 19 Jan 2023 13:30:58 +0000 Subject: [PATCH 4/5] Eliminates type_args_are_equal usage. --- sway-core/src/semantic_analysis/namespace/trait_map.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/sway-core/src/semantic_analysis/namespace/trait_map.rs b/sway-core/src/semantic_analysis/namespace/trait_map.rs index 9943da888ad..200440fcbf2 100644 --- a/sway-core/src/semantic_analysis/namespace/trait_map.rs +++ b/sway-core/src/semantic_analysis/namespace/trait_map.rs @@ -191,12 +191,6 @@ impl TraitMap { second_impl_span: impl_span.clone(), }); } else if types_are_subset { - let type_args_are_equal = trait_type_args.len() == map_trait_type_args.len() - && !trait_type_args - .iter() - .zip(map_trait_type_args.iter()) - .any(|(a1, a2)| !a1.eq(a2, engines)); - for (name, decl_id) in trait_methods.iter() { if let Some(map_trait_method_decl_id) = map_trait_methods.get(name) { let method = check!( @@ -216,7 +210,8 @@ impl TraitMap { warnings, errors ); - if !type_args_are_equal + if !traits_are_subset + && !is_impl_self && (method.parameters.len() != map_trait_method.parameters.len() || method .parameters From 5bcdcad1144e91dcac707cf166f08743860c5f03 Mon Sep 17 00:00:00 2001 From: Marcos Henrich Date: Mon, 30 Jan 2023 19:09:11 +0000 Subject: [PATCH 5/5] Removes wrong test case, and applies review changes. --- .../semantic_analysis/namespace/trait_map.rs | 28 ++----------------- .../src/main.sw | 27 ------------------ .../test.toml | 3 -- 3 files changed, 2 insertions(+), 56 deletions(-) diff --git a/sway-core/src/semantic_analysis/namespace/trait_map.rs b/sway-core/src/semantic_analysis/namespace/trait_map.rs index 200440fcbf2..87bdccc8cf7 100644 --- a/sway-core/src/semantic_analysis/namespace/trait_map.rs +++ b/sway-core/src/semantic_analysis/namespace/trait_map.rs @@ -190,9 +190,9 @@ impl TraitMap { type_implementing_for: engines.help_out(type_id).to_string(), second_impl_span: impl_span.clone(), }); - } else if types_are_subset { + } else if types_are_subset && (traits_are_subset || is_impl_self) { for (name, decl_id) in trait_methods.iter() { - if let Some(map_trait_method_decl_id) = map_trait_methods.get(name) { + if map_trait_methods.get(name).is_some() { let method = check!( CompileResult::from( decl_engine.get_function(decl_id.clone(), impl_span) @@ -201,30 +201,6 @@ impl TraitMap { warnings, errors ); - let map_trait_method = check!( - CompileResult::from( - decl_engine - .get_function(map_trait_method_decl_id.clone(), impl_span) - ), - return err(warnings, errors), - warnings, - errors - ); - if !traits_are_subset - && !is_impl_self - && (method.parameters.len() != map_trait_method.parameters.len() - || method - .parameters - .iter() - .zip(map_trait_method.parameters.iter()) - .any(|(p1, p2)| { - !are_equal_minus_dynamic_types( - engines, p1.type_id, p2.type_id, - ) - })) - { - continue; - } errors.push(CompileError::DuplicateMethodsDefinedForType { func_name: method.name.to_string(), type_implementing_for: engines.help_out(type_id).to_string(), diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/src/main.sw index fd53372f191..cbc6e92de02 100644 --- a/test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/src/main.sw +++ b/test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/src/main.sw @@ -62,33 +62,6 @@ impl Data { } } -pub struct A { - a: u64, -} - -pub struct B { - b: u64, -} - -pub struct C { - c: u64, -} - -pub trait Convert { - fn convert(t: u64); -} - -impl Convert for A { - fn convert(t: u64) { - } -} - -impl Convert for A { - // duplicate definition - fn convert(t: u64) { - } -} - fn main() { } diff --git a/test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/test.toml b/test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/test.toml index 86608eebd5a..7c383bbf1db 100644 --- a/test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/test.toml +++ b/test/src/e2e_vm_tests/test_programs/should_fail/multiple_methods_with_the_same_name/test.toml @@ -11,6 +11,3 @@ category = "fail" # check: $()fn my_add(self, other: Self) -> Self { # check: $()Duplicate definitions for the method "my_add" for type "Data". - -# check: $()fn convert(t: u64) { -# check: $()Duplicate definitions for the method "convert" for type "A".