Bugfix: Do not mangle overridden non-overloaded virtual function names (

hyperledger#1624)

Closes hyperledger#1623 

The basic idea of the algorithm checking whether a function name is
eligible for mangling or not is to mangle the name if that function is
publicly callable but the functions name appears multiple time in the
contract. But this doesn't account for virtual functions also appearing
more than one time in the same contract if they are overridden. With
this PR, we bail early if the function we are checking overrides,
marking only the single one non-overriding implementation as eligible
for mangling. Consequently, functions which override but do not overload
are no longer unnecessarily mangled.

Co-authored-by: Lucas Steuernagel <38472950+LucasSte@users.noreply.github.com>

src/sema/contracts.rs

@@ -1258,21 +1258,22 @@ fn compatible_visibility(left: &pt::Visibility, right: &pt::Visibility) -> bool
 }
 
 /// This function checks which function names must be mangled given a contract.
-/// Mangling happens when there is more than one function with the same name in a give contract.
+/// Mangling happens when there is more than one function with the same name in the given `contract_no`.
 fn mangle_function_names(contract_no: usize, ns: &mut Namespace) {
     let mut repeated_names: HashMap<String, usize> = HashMap::new();
 
     for func_no in ns.contracts[contract_no].all_functions.keys() {
-        if !ns.functions[*func_no].is_public()
-            && (ns.functions[*func_no].ty != pt::FunctionTy::Function
-                || ns.functions[*func_no].ty != pt::FunctionTy::Constructor)
-        {
+        let function = &ns.functions[*func_no];
+
+        let not_callable = !function.is_public()
+            && (function.ty != pt::FunctionTy::Function
+                || function.ty != pt::FunctionTy::Constructor);
+
+        if function.is_override.is_some() || not_callable {
             continue;
         }
 
-        if let Some(old_no) =
-            repeated_names.insert(ns.functions[*func_no].id.name.clone(), *func_no)
-        {
+        if let Some(old_no) = repeated_names.insert(function.id.name.clone(), *func_no) {
             ns.functions[old_no]
                 .mangled_name_contracts
                 .insert(contract_no);

tests/polkadot_tests/abi.rs

@@ -6,7 +6,7 @@ use once_cell::sync::Lazy;
 use scale_info::{
     form::PortableForm, Path, TypeDef, TypeDefComposite, TypeDefPrimitive, TypeDefVariant,
 };
-use std::sync::Mutex;
+use std::{collections::HashSet, sync::Mutex};
 
 macro_rules! path {
     ($( $segments:expr ),*) => {
@@ -284,3 +284,57 @@ fn custom_errors_in_metadata() {
         _ => panic!("expected uint256 type"),
     }
 }
+
+#[test]
+fn overriden_but_not_overloaded_function_not_mangled() {
+    let src = r#"abstract contract Base {
+        function test() public pure virtual returns (uint8) {
+            return 42;
+        }
+    }
+    
+    contract TestA is Base {}
+    
+    contract TestB is Base {
+        function test() public pure override returns (uint8) {
+            return 42;
+        }
+    }"#;
+
+    for abi in build_wasm(src, false).iter().map(|(_, abi)| load_abi(abi)) {
+        assert_eq!(abi.spec().messages().len(), 1);
+        assert_eq!(abi.spec().messages().first().unwrap().label(), "test");
+    }
+}
+
+#[test]
+fn overloaded_but_not_overridden_function_is_mangled() {
+    let src = r#"abstract contract Base {
+        function test() public pure virtual returns (uint8) {
+            return 42;
+        }
+    
+    }
+    
+    contract TestA is Base {
+        function test(uint8 foo) public pure returns (uint8) {
+            return foo;
+        }
+    }"#;
+
+    let mut expected_function_names = HashSet::from(["test_", "test_uint8"]);
+
+    for message_spec in build_wasm(src, false)
+        .first()
+        .map(|(_, abi)| load_abi(abi))
+        .expect("there should be a contract")
+        .spec()
+        .messages()
+    {
+        expected_function_names
+            .take(message_spec.label().as_str())
+            .unwrap_or_else(|| panic!("{} should be present", message_spec.label()));
+    }
+
+    assert!(expected_function_names.is_empty());
+}