Fix bug where trait/abi method argument type mismatch went uncaught (F…

…uelLabs#1581)

* Fix bug where trait/abi method argument type mismatch went uncaught

Closes FuelLabs#1327.

Turns out unification was already catching this, but the produced error
was never returned from the trait impl checking function :D

It looks like a new `Vec` was introduced at one point to collect errors
in order to work around ownership issues involved with some fancy uses
of `iter().find_map()`, however these errors were never collected from
the outer scope. I've refactored this section to use basic for loops and
control flow instead which makes it easier for Rust to reason about
ownership over the top-level `errors` Vec and reduces the need for the
extra error-collecting `Vec`s.

Also adds two new `should_fail` tests to verify that we produce errors
when argument types differ between implementation and trait/abi
declaration.

* Avoid searching for trait fn decl twice by changing checklist to a map

sway-core/src/semantic_analysis/ast_node/impl_trait.rs

@@ -191,12 +191,12 @@ fn type_check_trait_implementation(
     let mut errors = vec![];
     let mut warnings = vec![];
     let self_type_id = type_implementing_for;
-    // this list keeps track of the remaining functions in the
+    // this map keeps track of the remaining functions in the
     // interface surface that still need to be implemented for the
     // trait to be fully implemented
-    let mut function_checklist: Vec<&Ident> = interface_surface
+    let mut function_checklist: std::collections::BTreeMap<&Ident, _> = interface_surface
         .iter()
-        .map(|TypedTraitFn { name, .. }| name)
+        .map(|decl| (&decl.name, decl))
         .collect();
     for fn_decl in functions {
         // replace SelfType with type of implementor
@@ -221,11 +221,8 @@ fn type_check_trait_implementation(
         );
         let fn_decl = fn_decl.replace_self_types(self_type_id);
         // remove this function from the "checklist"
-        let ix_of_thing_to_remove = match function_checklist
-            .iter()
-            .position(|name| **name == fn_decl.name)
-        {
-            Some(ix) => ix,
+        let trait_fn = match function_checklist.remove(&fn_decl.name) {
+            Some(trait_fn) => trait_fn,
             None => {
                 errors.push(CompileError::FunctionNotAPartOfInterfaceSurface {
                     name: fn_decl.name.clone(),
@@ -235,87 +232,67 @@ fn type_check_trait_implementation(
                 return err(warnings, errors);
             }
         };
-        function_checklist.remove(ix_of_thing_to_remove);
 
         // ensure this fn decl's parameters and signature lines up with the one
         // in the trait
-        if let Some(mut l_e) = interface_surface.iter().find_map(
-            |TypedTraitFn {
-                 name,
-                 parameters,
-                 return_type,
-                 return_type_span: _,
-             }| {
-                if fn_decl.name == *name {
-                    if fn_decl.parameters.len() != parameters.len() {
-                        errors.push(
-                            CompileError::IncorrectNumberOfInterfaceSurfaceFunctionParameters {
-                                span: fn_decl.parameters_span(),
-                                fn_name: fn_decl.name.clone(),
-                                trait_name: trait_name.suffix.clone(),
-                                num_args: parameters.len(),
-                                provided_args: fn_decl.parameters.len(),
-                            },
-                        );
-                    }
-                    let mut errors = vec![];
-                    if let Some(mut maybe_err) = parameters
-                        .iter()
-                        .zip(fn_decl.parameters.iter())
-                        .find_map(|(fn_decl_param, trait_param)| {
-                            let mut errors = vec![];
-                            // TODO use trait constraints as part of the type here to
-                            // implement trait constraint solver */
-                            let fn_decl_param_type = fn_decl_param.r#type;
-                            let trait_param_type = trait_param.r#type;
+        let TypedTraitFn {
+            name: _,
+            parameters,
+            return_type,
+            return_type_span: _,
+        } = trait_fn;
 
-                            let (mut new_warnings, new_errors) = unify_with_self(
-                                fn_decl_param_type,
-                                trait_param_type,
-                                self_type_id,
-                                &trait_param.type_span,
-                                "",
-                            );
-                            warnings.append(&mut new_warnings);
-                            if new_errors.is_empty() {
-                                None
-                            } else {
-                                errors.push(CompileError::MismatchedTypeInTrait {
-                                    span: trait_param.type_span.clone(),
-                                    given: fn_decl_param_type.friendly_type_str(),
-                                    expected: trait_param_type.friendly_type_str(),
-                                });
-                                Some(errors)
-                            }
-                        })
-                    {
-                        errors.append(&mut maybe_err);
-                    }
+        if fn_decl.parameters.len() != parameters.len() {
+            errors.push(
+                CompileError::IncorrectNumberOfInterfaceSurfaceFunctionParameters {
+                    span: fn_decl.parameters_span(),
+                    fn_name: fn_decl.name.clone(),
+                    trait_name: trait_name.suffix.clone(),
+                    num_args: parameters.len(),
+                    provided_args: fn_decl.parameters.len(),
+                },
+            );
+        }
 
-                    let (mut new_warnings, new_errors) = unify_with_self(
-                        *return_type,
-                        fn_decl.return_type,
-                        self_type_id,
-                        &fn_decl.return_type_span,
-                        "",
-                    );
-                    warnings.append(&mut new_warnings);
-                    if new_errors.is_empty() {
-                        None
-                    } else {
-                        errors.push(CompileError::MismatchedTypeInTrait {
-                            span: fn_decl.return_type_span.clone(),
-                            expected: return_type.friendly_type_str(),
-                            given: fn_decl.return_type.friendly_type_str(),
-                        });
-                        Some(errors)
-                    }
-                } else {
-                    None
-                }
-            },
-        ) {
-            errors.append(&mut l_e);
+        for (trait_param, fn_decl_param) in parameters.iter().zip(&fn_decl.parameters) {
+            // TODO use trait constraints as part of the type here to
+            // implement trait constraint solver */
+            let fn_decl_param_type = fn_decl_param.r#type;
+            let trait_param_type = trait_param.r#type;
+
+            let (mut new_warnings, new_errors) = unify_with_self(
+                fn_decl_param_type,
+                trait_param_type,
+                self_type_id,
+                &trait_param.type_span,
+                "",
+            );
+
+            warnings.append(&mut new_warnings);
+            if !new_errors.is_empty() {
+                errors.push(CompileError::MismatchedTypeInTrait {
+                    span: fn_decl_param.type_span.clone(),
+                    given: fn_decl_param_type.friendly_type_str(),
+                    expected: trait_param_type.friendly_type_str(),
+                });
+                break;
+            }
+        }
+
+        let (mut new_warnings, new_errors) = unify_with_self(
+            *return_type,
+            fn_decl.return_type,
+            self_type_id,
+            &fn_decl.return_type_span,
+            "",
+        );
+        warnings.append(&mut new_warnings);
+        if !new_errors.is_empty() {
+            errors.push(CompileError::MismatchedTypeInTrait {
+                span: fn_decl.return_type_span.clone(),
+                expected: return_type.friendly_type_str(),
+                given: fn_decl.return_type.friendly_type_str(),
+            });
             continue;
         }
 
@@ -384,7 +361,7 @@ fn type_check_trait_implementation(
             span: block_span.clone(),
             missing_functions: function_checklist
                 .into_iter()
-                .map(|ident| ident.as_str().to_string())
+                .map(|(ident, _)| ident.as_str().to_string())
                 .collect::<Vec<_>>()
                 .join("\n"),
         });

sway-core/src/type_engine/engine.rs

@@ -330,7 +330,6 @@ impl Engine {
         } else {
             expected
         };
-
         self.unify(received, expected, span, help_text)
     }
 

test/src/e2e_vm_tests/mod.rs

@@ -455,6 +455,8 @@ pub fn run(filter_regex: Option<regex::Regex>) {
         "should_fail/double_underscore_var",
         "should_fail/double_underscore_struct",
         "should_fail/double_underscore_enum",
+        "should_fail/abi_method_signature_mismatch",
+        "should_fail/trait_method_signature_mismatch",
     ];
     number_of_tests_run += negative_project_names.iter().fold(0, |acc, name| {
         if filter(name) {

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

@@ -0,0 +1,3 @@
+[[package]]
+name = 'abi_method_signature_mismatch'
+dependencies = []

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

@@ -0,0 +1,6 @@
+[project]
+authors = ["Fuel Labs <[email protected]>"]
+license = "Apache-2.0"
+name = "abi_method_signature_mismatch"
+entry = "main.sw"
+implicit-std = false

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

@@ -0,0 +1,14 @@
+// This should result in an error saying that the method signature of the
+// implementation does not match the declaration.
+
+contract;
+
+abi MyContract {
+    fn foo(x: u64) -> str[7];
+}
+
+impl MyContract for Contract {
+    fn foo(s: str[7]) -> str[7] {
+        s
+    }
+}

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

@@ -0,0 +1,3 @@
+[[package]]
+name = 'trait_method_signature_mismatch'
+dependencies = []

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

@@ -0,0 +1,6 @@
+[project]
+authors = ["Fuel Labs <[email protected]>"]
+license = "Apache-2.0"
+name = "trait_method_signature_mismatch"
+entry = "main.sw"
+implicit-std = false

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

@@ -0,0 +1,18 @@
+// This should result in an error saying that the method signature of the
+// implementation does not match the declaration.
+
+library test;
+
+trait Foo {
+    fn foo(x: u64) -> str[7];
+}
+
+struct S {
+    x: u64,
+}
+
+impl Foo for S {
+    fn foo(s: str[7]) -> str[7] {
+        s
+    }
+}