Handle commas in assembly assignment

[fvictorio]

This is valid code but it generates a parsing error in the let i, j line:

contract Test {
    function foo() public pure {
        assembly {
            function bar() -> a, b {
                a := 1
                b := 2
            }

            let i, j := bar()
        }
    }
}

[potomak]

Would this issue be a good candidate for a new code contributor?

[fvictorio]

I'm not sure how hard it is, but feel free to give it a try and let me know if you need help!

The grammar seems to already support this kind of assignment (although it's not included in the test file), so my guess is that the problem is in the ASTBuilder. The relevant section in that module is this one. That also looks correct, so I'm not sure why it's not working tbh.

[fvictorio]

Oh, I see the problem now. It seems that multiple assignments are only supported when you use parentheses. Maybe this is how solidity used to work but it changed at some point?

So, the things to do are:

1. Update the grammar to allow multiple assignments without parentheses
2. Update the test.sol file
3. Update the AST tests in the parser
4. I think the ASTBuilder should just work, so these tests might pass after that. But if not, we should check what's going on.

Updating the grammar without causing breaking changes is always tricky. My guess is that just changing this:

assemblyIdentifierOrList
  : identifier | assemblyMember | '(' assemblyIdentifierList ')' ;

to this:

assemblyIdentifierOrList
  : identifier | assemblyMember | assemblyIdentifierList | '(' assemblyIdentifierList ')' ;

might work.

[potomak]

Thanks for the feedback!

I think that would fix the issue, but it would be a bit too permissive.

I found in SolidityParser.g4 that multi-assignments require a function call:

Any expression can be assigned to a single Yul variable, whereas
multi-assignments require a function call on the right-hand side.

Because of that I don't think the current implementation is correct. I mean can assemblyIdentifierOrList be used generically in assemblyLocalDefinition where assemblyExpression can be something different from assemblyCall?

This is a more generic question, maybe a bit out of scope of this issue: is there any plan to swap the dependency from solidity-parser/antlr with the official parser defined in ethereum/solidity?

[fvictorio]

Because of that I don't think the current implementation is correct. I mean can assemblyIdentifierOrList be used generically in assemblyLocalDefinition where assemblyExpression can be something different from assemblyCall?

This parser is very permissive and processes a lot of invalid (that is: non-compilable) solidity code. The main reason is that it has to work with all versions of solidity, and the easiest way to do that is to accept a lot of things that are not semantically valid.

This is a more generic question, maybe a bit out of scope of this issue: is there any plan to swap the dependency from solidity-parser/antlr with the official parser defined in ethereum/solidity?

I don't think so, for the same reason: the parser in ethereum/solidity is per-version, while this one works with any version of solidity. This parser is mainly used by tools (like Hardhat, prettier solidity, solidity-coverage, etc.) and therefore it's very important that it can parse pretty much any file.