ConsenSysMesh / solidity-parser

Solidity Parser in Javascript
137 stars 53 forks source link

Parse function calls that use a name-value list #72

Closed cgewecke closed 7 years ago

cgewecke commented 7 years ago

This PR addresses #71, where SP throws an error when instantiating a struct as below:

Voter me = Voter({
        weight: 2, 
        voted: false
    });

The relevant part of the Solidity grammar looks like this:

ExpressionList = Expression ( ',' Expression )*
NameValueList = Identifier ':' Expression ( ',' Identifier ':' Expression )*
FunctionCall =  [ . . .etc*. . .]  '(' FunctionCallArguments ')'
FunctionCallArguments = '{' NameValueList? '}' | ExpressionList?
federicobond commented 7 years ago

This looks good, but I wonder wether it's better to make the resulting AST compatible with positional arguments, so that tools don't have to implement named arguments support separately.

What do you think?

cgewecke commented 7 years ago

Oh yes, that's a good suggestion. Do you think it's worth trying to preserve the name?

For example NameValueAssignment could be rewritten:

NameValueAssignment
  = id:Identifier __ ":" __ value:AssignmentExpression __ {
      value.argName = id.name;
      return value;
    }
federicobond commented 7 years ago

Sorry, I have been thinking more about this and now I am not so sure. If we just discarded the start and end positions of the identifiers from the resulting AST we would be losing too much information.

A fairly obvious use case for them is a linter highlighting the argument name because there are no fields named that way, or suggesting the possibility of a typo because a similar name is present in the original struct definition.

Now I am more inclined to keep this as is and just rename id to name so that the resulting NameValueAssignment actually has both name and value properties.

federicobond commented 7 years ago

Of course, we could embed the whole Identifier node inside the Expression node under the argName key as you suggested, but it would look quite weird.

cgewecke commented 7 years ago

Ok cool that makes sense. I'll push the id/name change in a sec. Thanks Federico.

federicobond commented 7 years ago

When I rewrote my final comment I forgot to mention also using the whole Identifier node as the value for the name key. I am going to amend it and merge it manually.

federicobond commented 7 years ago

Done. Commit is still attributed to you. Thanks @cgewecke!

cgewecke commented 7 years ago

👍