SyntaxError when parsing empty array expression

[duaraghav8]

Originally reported here: https://github.com/duaraghav8/Solium/issues/85

expression address[] is supported by solc but produces syntax error when used like:

contract Test {
    function crash() returns (address[]) {
    }
}

Output:

SyntaxError: Expected "[", "memory", "storage", comment, end of line, identifier, or whitespace but ")" found. Line: 15, Column: 40
<rest is all stack trace>

The code compiles on solc (although with formal errors), so whether this syntax is actually correct needs to be discussed. But since solc compiles this contract, IMO it should get parsed by SP.

[cgewecke]

@duaraghav8 That seems right to me.

It looks like the line they're trying to parse is in the uport-identity/contracts here and has running tests. Are you opening a PR for this? 😄

[duaraghav8]

@cgewecke yep working on it!

[duaraghav8]

Ok so this one is giving me a hard time -.-" (probably because I haven't been in touch with developments in SP recently). The trail is: FunctionDeclaration accepts ModifierArgumentList, which takes ModifierArgument which accepts an Identifier followed by ArgumentList.

See https://github.com/ConsenSys/solidity-parser/blob/master/solidity.pegjs#L1367

My doubts are:

1. What is an Identifier used for? Reserved Words (like returns in case of ModifierArgument) or a variable name (like myVar123)?

2. Why is ArgumentList accepting AssignmentExpression? AE would be something like x = y whereas AL is generally something like (x, y, z) (function arguments) - there is no need for assignment.

@cgewecke @federicobond Would be great if anyone could clarify these for me

[cgewecke]

Hi @duaraghav8. Thanks for working on this. My two cents are:

1. Becausereturns is plural, it's just getting read as an Identifier like myVar123 and is mapped to the name field of the ModifierArgument. But it's hacky right? It's not a modifier. Not sure what's best here.

2. AssignmentExpression: madness . . .

Looking at / translating from solidity-antlr4 - it might be possible to replace AssignmentExpression in this cluster of rules with something like:

ArgumentExpression
  = PrimaryExpression
  / Type
  / LogicalORExpression

Not sure that's adequate. I think Type will cover this syntax error case anyway.

[duaraghav8]

MAJOR BREAKTHROUGH!!! Ok not a breakthrough but I finally understood what needs to be done.

https://github.com/ethereum/solidity/blob/develop/docs/grammar.txt#L29

In SP, currently, The ModifierArgumentList also consumes the returns (..) we specify. returns is NOT a part of modifiers. Modifiers are either like payable (do not take any arguments so no brackets required either) or owned (myPrice, owner) - so a modifier is never supposed to receive a type like uint (even currently, if you provide returns (uint), uint ends up getting parsed as Identifier).

Correct way:

function foobar (uint a) payable owned (price, owner) returns (uint, address[]) {
    //blah blah
}

The ModifierArgument list should only contain payable & owned related information. The FunctionDeclaration Node should have another attribute returns whose value describes the returned types (Type).

I'm working on implementing this, please let me know in case I was unclear or you disagree in some way.

[cgewecke]

@duaraghav8 Yes yes that looks right to me. You're dramatically improving the AST coherence there.