ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.12k stars 5.73k forks source link

Parser fails when decoding with abi.encode, thus it doesn't allow unpacking to variables that are declared differently (e.g. statement-declared and the ones declared in advance) #15167

Closed milaabl closed 3 months ago

milaabl commented 4 months ago

Description

The Solidity parser fails when decoding with abi.encode, thus it doesn't allow unpacking to variables that are declared differently (e.g. statement-declared and the ones declared in advance).

The code statement on the line 13 doesn't pass the parser's checks, however the syntax should be supported (shouldn't it?):

1: // SPDX-License-Identifier: UNLICENSED
2: pragma solidity ^0.8.13;
3:
4.
5. contract Test {
6.     function test_Bug(bytes memory _input, uint256 _nonce) external {
7.         bytes memory data;
8.
9.         bytes memory encodedThisCall = abi.encodeWithSelector(this.test_Bug.selector, _input, _nonce);
10.        
11.        uint256 decodedNonceLocal;
12. 
13.        /* this doesn't compile: ===>*/ // (data, uint256 decodedNonce) = abi.decode(encodedThisCall, (bytes, uint256));
14:        /* but this does:        ===>*/ (data, decodedNonceLocal) = abi.decode(encodedThisCall, (bytes, uint256));
15:     }
16:  }

↑ The parsing fails with the error message: ParserError: Expected ',' but got identifier.

Environment

Steps to Reproduce

↑ Check the above attached code.

I'm still doubtful whether this issue is a feature request or a bug, but logically it feels like unpacking should be possible to different variables, both the expression-declared variables and the ones that were declared in advance. Please let me know your thoughts!!

EduardoMelo00 commented 3 months ago

hey @milaabl

this way it compiled normally, the difference is that you need to create the variables (bytes memory data , uint256 decodedNonce) or

use the ones created before

pragma solidity ^0.8.13;
 contract Test {
     function test_Bug(bytes memory _input, uint256 _nonce) external {

         bytes memory encodedThisCall = abi.encodeWithSelector(this.test_Bug.selector, _input, _nonce);

        uint256 decodedNonceLocal;

       /* compiled:        ===>*/ (bytes memory data , uint256 decodedNonce) = abi.decode(encodedThisCall, (bytes, uint256));
     }
  }

I hope this help.

milaabl commented 3 months ago

Hi @EduardoMelo00,

Thank you! :)

However, that (↓) is exactly the point of my concerns:

the difference is that you need to create the variables (bytes memory data , uint256 decodedNonce)

I'm wondering whether I can abi.decode if the variables on the left-hand side were declared differently, (for example if one of the variables in the (...) = abi.decode expression's (...) brackets is an inline-declared variable [like uint256 decodedNonce], and the other one was declared in advance [for instance, it's a function's argument, like if there was a function function test_Bug(bytes memory data) external {}]), that kind of syntax is supposed to be supported in Solidity (and if it is supposed to be supported, then the parser warning me about it is a bug), or only same-type declarations (like the example you provided) are supported.

nikola-matic commented 3 months ago

Hi @EduardoMelo00,

Thank you! :)

However, that (↓) is exactly the point of my concerns:

the difference is that you need to create the variables (bytes memory data , uint256 decodedNonce)

_I'm wondering whether I can abi.decode if the variables on the left-hand side were declared differently, (for example if one of the variables in the (...) = abi.decode expression's (...) brackets is an inline-declared variable [like uint256 decodedNonce], and the other one was declared in advance [for instance, it's a function's argument, like if there was a function function test_Bug(bytes memory data) external {}]), that kind of syntax is supposed to be supported in Solidity (and if it is supposed to be supported, then the parser warning me about it is a bug), or only same-type declarations (like the example you provided) are supported._

Hey, so the short story is that this is not a bug - Solidity doesn't allow destructuring via tuples unless either all variables on the left hand side have been declared previously (outside of the tuple), or unless all of them have been declared inside the tuple, i.e. mixing and matching is not allowed. It's technically feasible to support this, however, we have other ideas. This same question was asked on the forum some time ago, and if you're interested, you can read through the discussion there to get a deeper understanding on our thought process regarding this.

milaabl commented 3 months ago

Hi @EduardoMelo00, Thank you! :) However, that (↓) is exactly the point of my concerns:

the difference is that you need to create the variables (bytes memory data , uint256 decodedNonce)

_I'm wondering whether I can abi.decode if the variables on the left-hand side were declared differently, (for example if one of the variables in the (...) = abi.decode expression's (...) brackets is an inline-declared variable [like uint256 decodedNonce], and the other one was declared in advance [for instance, it's a function's argument, like if there was a function function test_Bug(bytes memory data) external {}]), that kind of syntax is supposed to be supported in Solidity (and if it is supposed to be supported, then the parser warning me about it is a bug), or only same-type declarations (like the example you provided) are supported._

Hey, so the short story is that this is not a bug - Solidity doesn't allow destructuring via tuples unless either all variables on the left hand side have been declared previously (outside of the tuple), or unless all of them have been declared inside the tuple, i.e. mixing and matching is not allowed. It's technically feasible to support this, however, we have other ideas. This same question was asked on the forum some time ago, and if you're interested, you can read through the discussion there to get a deeper understanding on our thought process regarding this.

Thank you so much for addressing my concerns perfectly!! :) I see the discussion now.

EduardoMelo00 commented 3 months ago

Great !! See you !! :)