Fix abi.encodeCall() argument parsing (#1612)

The arguments to the function should be passed as a tuple, if there are
more than one. A single argument does not require a tuple.

Signed-off-by: Sean Young <sean@mess.org>

.github/workflows/test.yml

@@ -356,9 +356,6 @@ jobs:
     - name: Start substrate contracts node
       run: echo id=$(docker run -d -p 9944:9944 ghcr.io/hyperledger/solang-substrate-ci:054bef6 substrate-contracts-node --dev --rpc-external) >> $GITHUB_OUTPUT
       id: substrate
-    - uses: actions/setup-node@v3
-      with:
-        node-version: '16'
     - uses: actions/download-artifact@v3
       with:
         name: solang-linux-x86-64
@@ -366,6 +363,9 @@ jobs:
     - run: |
         chmod 755 ./bin/solang
         echo "$(pwd)/bin" >> $GITHUB_PATH
+    - uses: actions/setup-node@v3
+      with:
+        node-version: '16'
     - run: npm install
       working-directory: ./integration/polkadot
     - name: Build ink! contracts
@@ -398,7 +398,7 @@ jobs:
     - name: Start substrate
       run: echo id=$(docker run -d -p 9944:9944 ghcr.io/hyperledger/solang-substrate-ci:054bef6 substrate-contracts-node --dev --rpc-external) >> $GITHUB_OUTPUT
       id: substrate
-    - uses: actions/download-artifact@master
+    - uses: actions/download-artifact@v3
       with:
         name: solang-linux-x86-64
         path: bin
@@ -497,32 +497,32 @@ jobs:
       - name: Install cargo-llvm-cov
         uses: taiki-e/install-action@cargo-llvm-cov
       - name: Download Rust coverage files
-        uses: actions/download-artifact@master
+        uses: actions/download-artifact@v3
         with:
          name: rust-tests.tar.gz
          path: ./target
       - name: Download Solana coverage files
-        uses: actions/download-artifact@master
+        uses: actions/download-artifact@v3
         with:
           name: solana-tests
           path: ./target
       - name: Download Polkadot coverage files
-        uses: actions/download-artifact@master
+        uses: actions/download-artifact@v3
         with:
           name: polkadot-tests
           path: ./target
       - name: Download Polkadot subxt coverage files
-        uses: actions/download-artifact@master
+        uses: actions/download-artifact@v3
         with:
           name: polkadot-subxt-tests
           path: ./target
       - name: Download Anchor coverage files
-        uses: actions/download-artifact@master
+        uses: actions/download-artifact@v3
         with:
           name: anchor-tests
           path: ./target
       - name: Download VSCode coverage files
-        uses: actions/download-artifact@master
+        uses: actions/download-artifact@v3
         with:
           name: vscode-tests
           path: ./target

docs/examples/abi_encode_call.sol

@@ -1,6 +1,6 @@
 contract c {
     function f1() public {
-        bytes foo = abi.encodeCall(c.bar, 102, true);
+        bytes foo = abi.encodeCall(c.bar, (102, true));
     }
 
     function bar(int256 a, bool b) public {}

docs/language/builtins.rst

@@ -293,7 +293,8 @@ abi.encodeCall(function, ...)
 +++++++++++++++++++++++++++++
 
 ABI encodes the function call to the function which should be specified as ``ContractName.FunctionName``. The arguments
-are cast and checked against the function specified as the first argument.
+are cast and checked against the function specified as the first argument. The arguments must be in a tuple, e.g.
+``(a, b, c)``. If there is a single argument no tuple is required.
 
 .. include:: ../examples/abi_encode_call.sol
   :code: solidity
@@ -340,7 +341,7 @@ looks like in a solidity contract:
 is_contract(address AccountId) returns (bool)
 +++++++++++++++++++++++++++++++++++++++++++++
 
-Only available on Polkadot. Checks whether the given address is a contract address. 
+Only available on Polkadot. Checks whether the given address is a contract address.
 
 set_code_hash(uint8[32] hash) returns (uint32)
 ++++++++++++++++++++++++++++++++++++++++++++++
@@ -351,17 +352,17 @@ A return value of 0 indicates success; a return value of 7 indicates that there
 
 .. note::
 
-    This is a low level function. We strongly advise consulting the underlying 
-    `API documentation <https://docs.rs/pallet-contracts/latest/pallet_contracts/api_doc/trait.Version0.html#tymethod.set_code_hash>`_ 
+    This is a low level function. We strongly advise consulting the underlying
+    `API documentation <https://docs.rs/pallet-contracts/latest/pallet_contracts/api_doc/trait.Version0.html#tymethod.set_code_hash>`_
     to obtain a full understanding of its implications.
 
-This functionality is intended to be used for implementing upgradeable contracts. 
+This functionality is intended to be used for implementing upgradeable contracts.
 Pitfalls generally applying to writing
-`upgradeable contracts <https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable>`_ 
+`upgradeable contracts <https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable>`_
 must be considered whenever using this builtin function, most notably:
 
 * The contract must safeguard access to this functionality, so that it is only callable by priviledged users.
-* The code you are upgrading to must be 
+* The code you are upgrading to must be
   `storage compatible <https://docs.openzeppelin.com/upgrades-plugins/1.x/proxies#storage-collisions-between-implementation-versions>`_
   with the existing code.
 * Constructors and any other initializers, including initial storage value definitions, won't be executed.

src/codegen/mod.rs

@@ -1671,7 +1671,7 @@ impl Expression {
 
     fn external_function_selector(&self) -> Expression {
         debug_assert!(
-            matches!(self.ty(), Type::ExternalFunction { .. }),
+            matches!(self.ty().deref_any(), Type::ExternalFunction { .. }),
             "This is not an external function"
         );
         let loc = self.loc();

src/sema/builtin.rs

@@ -8,9 +8,12 @@ use super::diagnostics::Diagnostics;
 use super::eval::eval_const_number;
 use super::expression::{ExprContext, ResolveTo};
 use super::symtable::Symtable;
-use crate::sema::ast::{RetrieveType, Tag, UserTypeDecl};
-use crate::sema::expression::{function_call::evaluate_argument, resolve_expression::expression};
-use crate::sema::namespace::ResolveTypeContext;
+use crate::sema::{
+    ast::{RetrieveType, Tag, UserTypeDecl},
+    expression::{function_call::evaluate_argument, resolve_expression::expression},
+    namespace::ResolveTypeContext,
+    statements::parameter_list_to_expr_list,
+};
 use crate::Target;
 use num_bigint::BigInt;
 use num_traits::One;
@@ -1132,69 +1135,24 @@ pub(super) fn resolve_namespace_call(
         let mut tys = Vec::new();
         let mut broken = false;
 
-        match &args[1] {
-            pt::Expression::List(_, list) => {
-                for (loc, param) in list {
-                    if let Some(param) = param {
-                        let ty = ns.resolve_type(
-                            context.file_no,
-                            context.contract_no,
-                            ResolveTypeContext::None,
-                            &param.ty,
-                            diagnostics,
-                        )?;
-
-                        if let Some(storage) = &param.storage {
-                            diagnostics.push(Diagnostic::error(
-                                storage.loc(),
-                                format!("storage modifier '{storage}' not allowed"),
-                            ));
-                            broken = true;
-                        }
-
-                        if let Some(name) = &param.name {
-                            diagnostics.push(Diagnostic::error(
-                                name.loc,
-                                format!("unexpected identifier '{}' in type", name.name),
-                            ));
-                            broken = true;
-                        }
-
-                        if ty.is_mapping() || ty.is_recursive(ns) {
-                            diagnostics.push(Diagnostic::error(
-                                *loc,
-                        format!("Invalid type '{}': mappings and recursive types cannot be abi decoded or encoded", ty.to_string(ns)))
-                            );
-                            broken = true;
-                        }
-
-                        tys.push(ty);
-                    } else {
-                        diagnostics.push(Diagnostic::error(*loc, "missing type".to_string()));
+        for arg in parameter_list_to_expr_list(&args[1], diagnostics)? {
+            let ty = ns.resolve_type(
+                context.file_no,
+                context.contract_no,
+                ResolveTypeContext::None,
+                arg.strip_parentheses(),
+                diagnostics,
+            )?;
 
-                        broken = true;
-                    }
-                }
+            if ty.is_mapping() || ty.is_recursive(ns) {
+                diagnostics.push(Diagnostic::error(
+                    *loc,
+                    format!("Invalid type '{}': mappings and recursive types cannot be abi decoded or encoded", ty.to_string(ns))
+                ));
+                broken = true;
             }
-            _ => {
-                let ty = ns.resolve_type(
-                    context.file_no,
-                    context.contract_no,
-                    ResolveTypeContext::None,
-                    args[1].remove_parenthesis(),
-                    diagnostics,
-                )?;
 
-                if ty.is_mapping() || ty.is_recursive(ns) {
-                    diagnostics.push(Diagnostic::error(
-                        *loc,
-                        format!("Invalid type '{}': mappings and recursive types cannot be abi decoded or encoded", ty.to_string(ns))
-                    ));
-                    broken = true;
-                }
-
-                tys.push(ty);
-            }
+            tys.push(ty);
         }
 
         return if broken {
@@ -1248,82 +1206,102 @@ pub(super) fn resolve_namespace_call(
             }
         }
         Builtin::AbiEncodeCall => {
+            if args.len() != 2 {
+                diagnostics.push(Diagnostic::error(
+                    *loc,
+                    format!("function expects {} arguments, {} provided", 2, args.len()),
+                ));
+
+                return Err(());
+            }
+
             // first argument is function
-            if let Some(function) = args_iter.next() {
-                let function = expression(
-                    function,
-                    context,
-                    ns,
-                    symtable,
-                    diagnostics,
-                    ResolveTo::Unknown,
-                )?;
+            let function = expression(
+                &args[0],
+                context,
+                ns,
+                symtable,
+                diagnostics,
+                ResolveTo::Unknown,
+            )?;
 
-                match function.ty() {
-                    Type::ExternalFunction { params, .. }
-                    | Type::InternalFunction { params, .. } => {
-                        resolved_args.push(function);
-
-                        if args.len() - 1 != params.len() {
-                            diagnostics.push(Diagnostic::error(
-                                *loc,
-                                format!(
-                                    "function takes {} arguments, {} provided",
-                                    params.len(),
-                                    args.len() - 1
-                                ),
-                            ));
-
-                            return Err(());
-                        }
+            let ty = function.ty();
 
-                        for (arg_no, arg) in args_iter.enumerate() {
-                            let mut expr = expression(
-                                arg,
-                                context,
-                                ns,
-                                symtable,
-                                diagnostics,
-                                ResolveTo::Type(&params[arg_no]),
-                            )?;
-
-                            expr = expr.cast(&arg.loc(), &params[arg_no], true, ns, diagnostics)?;
-
-                            // A string or hex literal should be encoded as a string
-                            if let Expression::BytesLiteral { .. } = &expr {
-                                expr =
-                                    expr.cast(&arg.loc(), &Type::String, true, ns, diagnostics)?;
-                            }
-
-                            resolved_args.push(expr);
-                        }
+            match function.cast(&function.loc(), ty.deref_any(), true, ns, diagnostics)? {
+                Expression::ExternalFunction { function_no, .. }
+                | Expression::InternalFunction { function_no, .. } => {
+                    let func = &ns.functions[function_no];
 
-                        return Ok(Expression::Builtin {
-                            loc: *loc,
-                            tys: vec![Type::DynamicBytes],
-                            kind: builtin,
-                            args: resolved_args,
-                        });
+                    if !func.is_public() {
+                        diagnostics.push(Diagnostic::error_with_note(
+                            function.loc(),
+                            "function is not public or external".into(),
+                            func.loc,
+                            format!("definition of {}", func.id.name),
+                        ));
                     }
-                    ty => {
-                        diagnostics.push(Diagnostic::error(
+
+                    let params = &func.params;
+
+                    resolved_args.push(function);
+
+                    let args = parameter_list_to_expr_list(&args[1], diagnostics)?;
+
+                    if args.len() != params.len() {
+                        diagnostics.push(Diagnostic::error_with_note(
                             *loc,
                             format!(
-                                "first argument should be function, got '{}'",
-                                ty.to_string(ns)
+                                "function takes {} arguments, {} provided",
+                                params.len(),
+                                args.len()
                             ),
+                            func.loc,
+                            format!("definition of {}", func.id.name),
                         ));
 
                         return Err(());
                     }
+
+                    for (arg_no, arg) in args.iter().enumerate() {
+                        let ty = ns.functions[function_no].params[arg_no].ty.clone();
+
+                        let mut expr = expression(
+                            arg,
+                            context,
+                            ns,
+                            symtable,
+                            diagnostics,
+                            ResolveTo::Type(&ty),
+                        )?;
+
+                        expr = expr.cast(&arg.loc(), &ty, true, ns, diagnostics)?;
+
+                        // A string or hex literal should be encoded as a string
+                        if let Expression::BytesLiteral { .. } = &expr {
+                            expr = expr.cast(&arg.loc(), &Type::String, true, ns, diagnostics)?;
+                        }
+
+                        resolved_args.push(expr);
+                    }
+
+                    return Ok(Expression::Builtin {
+                        loc: *loc,
+                        tys: vec![Type::DynamicBytes],
+                        kind: builtin,
+                        args: resolved_args,
+                    });
                 }
-            } else {
-                diagnostics.push(Diagnostic::error(
-                    *loc,
-                    "least one function argument required".to_string(),
-                ));
+                expr => {
+                    diagnostics.push(Diagnostic::error(
+                        *loc,
+                        format!(
+                            "first argument should be function, got '{}'",
+                            expr.ty().to_string(ns)
+                        ),
+                    ));
 
-                return Err(());
+                    return Err(());
+                }
             }
         }
         Builtin::AbiEncodeWithSignature => {

src/sema/expression/mod.rs

@@ -843,17 +843,7 @@ impl Expression {
             }
             // Lengthing or shorting a fixed bytes array
             (Type::Bytes(from_len), Type::Bytes(to_len)) => {
-                if implicit {
-                    diagnostics.push(Diagnostic::cast_error(
-                        *loc,
-                        format!(
-                            "implicit conversion would truncate from {} to {}",
-                            from.to_string(ns),
-                            to.to_string(ns)
-                        ),
-                    ));
-                    Err(())
-                } else if to_len > from_len {
+                if to_len > from_len {
                     let shift = (to_len - from_len) * 8;
 
                     Ok(Expression::ShiftLeft {
@@ -870,6 +860,16 @@ impl Expression {
                             value: BigInt::from_u8(shift).unwrap(),
                         }),
                     })
+                } else if implicit {
+                    diagnostics.push(Diagnostic::cast_error(
+                        *loc,
+                        format!(
+                            "implicit conversion would truncate from {} to {}",
+                            from.to_string(ns),
+                            to.to_string(ns)
+                        ),
+                    ));
+                    Err(())
                 } else {
                     let shift = (from_len - to_len) * 8;
 

src/sema/namespace.rs

@@ -1443,6 +1443,8 @@ impl Namespace {
         let mut dimensions = vec![];
 
         loop {
+            expr = expr.strip_parentheses();
+
             expr = match expr {
                 pt::Expression::ArraySubscript(_, r, None) => {
                     dimensions.push(None);

src/sema/statements.rs

@@ -2208,7 +2208,7 @@ pub fn parameter_list_to_expr_list<'a>(
                         assert!(annotation.is_none());
                         diagnostics.push(Diagnostic::error(
                             name.loc,
-                            "single value expected".to_string(),
+                            format!("unexpected identifier '{}'", name.name),
                         ));
                         broken = true;
                     }

tests/contract_testcases/evm/bytes_cast.sol

@@ -0,0 +1,16 @@
+contract C {
+	function test1(bytes10 x) public returns (bytes8) {
+		return x;
+	}
+
+	function test2(bytes10 x) public returns (bytes10) {
+		return x;
+	}
+
+	function test3(bytes10 x) public returns (bytes12) {
+		return x;
+	}
+}
+
+// ---- Expect: diagnostics ----
+// error: 3:10-11: implicit conversion would truncate from bytes10 to bytes8

tests/contract_testcases/polkadot/builtins/abi_decode_02.sol

@@ -5,4 +5,4 @@
             }
         }
 // ---- Expect: diagnostics ----
-// error: 4:52-55: unexpected identifier 'feh' in type
+// error: 4:52-55: unexpected identifier 'feh'

tests/contract_testcases/polkadot/builtins/abi_decode_03.sol

@@ -5,4 +5,4 @@
             }
         }
 // ---- Expect: diagnostics ----
-// error: 4:52: missing type
+// error: 4:52: stray comma

tests/contract_testcases/solana/call/abi_encode_call.sol

@@ -1,18 +1,37 @@
 contract abi_encode_call {
+    int[2] ints;
+
     function test1() public {
-        bytes bs = abi.encodeCall(other.foo, 1);
+        bytes memory bs = abi.encodeCall(other.foo, 1);
     }
 
     function test2() public {
-        bytes bs = abi.encodeCall(other.foo, 1, true);
+        bytes memory bs = abi.encodeCall(other.foo, (1, true));
+    }
+
+    function test3() public {
+        bytes memory bs = abi.encodeCall(other.baz, (1, 2, ints));
+    }
+
+    function test4() public {
+        bytes memory bs = abi.encodeCall(other.foo, (1, 2), 3);
     }
 }
 
 contract other {
     function foo(int foo, int bar) public {}
+
+    function baz(int foo, int bar, int[2] storage) internal {}
 }
+
 // ---- Expect: diagnostics ----
-// error: 3:20-48: function takes 2 arguments, 1 provided
-// error: 7:49-53: conversion from bool to int256 not possible
-// warning: 12:22-25: declaration of 'foo' shadows function
-// 	note 12:14-17: previous declaration of function
+// error: 5:27-55: function takes 2 arguments, 1 provided
+// 	note 22:5-45: definition of foo
+// error: 9:57-61: conversion from bool to int256 not possible
+// error: 13:42-47: function is not public or external
+// 	note 24:5-63: definition of baz
+// error: 17:27-63: function expects 2 arguments, 3 provided
+// warning: 22:22-25: declaration of 'foo' shadows function
+// 	note 22:14-17: previous declaration of function
+// warning: 24:22-25: declaration of 'foo' shadows function
+// 	note 22:14-17: previous declaration of function

tests/evm.rs

@@ -255,7 +255,7 @@ fn ethereum_solidity_tests() {
         })
         .sum();
 
-    assert_eq!(errors, 909);
+    assert_eq!(errors, 897);
 }
 
 fn set_file_contents(source: &str, path: &Path) -> (FileResolver, Vec<String>) {

tests/solana_tests/call.rs

@@ -322,6 +322,7 @@ fn encode_call() {
         r#"
         contract bar0 {
             bytes8 private constant SELECTOR = bytes8(sha256(bytes('global:test_bar')));
+            bytes8 private constant SELECTOR2 = bytes8(sha256(bytes('global:test_baz')));
 
             @account(bar1_pid)
             function test_other() external returns (int64) {
@@ -332,12 +333,25 @@ fn encode_call() {
                 (int64 v) = abi.decode(raw, (int64));
                 return v + 5;
             }
+
+            @account(bar1_pid)
+            function test_other2() external returns (int64) {
+                bytes select = abi.encodeWithSelector(SELECTOR2, int64(7), int64(5));
+                bytes signature = abi.encodeCall(bar1.test_baz, (7, 5));
+                require(select == signature, "must be the same");
+                (, bytes raw) = tx.accounts.bar1_pid.key.call{accounts: []}(signature);
+                (int64 v) = abi.decode(raw, (int64));
+                return v + 5;
+            }
         }
 
         contract bar1 {
             function test_bar(int64 y) public pure returns (int64) {
                 return 3 + y;
             }
+            function test_baz(int64 y, int64 x) public pure returns (int64) {
+                return 3 + y + x;
+            }
         }"#,
     );
 
@@ -387,6 +401,23 @@ fn encode_call() {
             value: BigInt::from(15u8)
         }
     );
+
+    let res = vm
+        .function("test_other2")
+        .accounts(vec![
+            ("bar1_pid", bar1_program_id),
+            ("systemProgram", [0; 32]),
+        ])
+        .call()
+        .unwrap();
+
+    assert_eq!(
+        res,
+        BorshToken::Int {
+            width: 64,
+            value: BigInt::from(20u8)
+        }
+    );
 }
 
 #[test]