ConsenSysMesh / solidity-parser

Solidity Parser in Javascript
138 stars 54 forks source link

Allow numeric and string literals for AssemblyLocalBinding rule #66

Closed cgewecke closed 7 years ago

cgewecke commented 7 years ago

It looks like the published Solidity grammar might have an error in its AssemblyLocalDefinition rule (AssemblyLocalBinding in solidity-parser). Solc compiles these assembly statements without complaint:

let v := 1
let x := 0x00
let z := "hello"

but the official grammar doesn't specify that numeric and string literals are allowed.

Solc rejects this hex string literal:

let y := hex "aaaaa"

Is there anything else allowed on the right hand side here?

Have modified the AssemblyLocalBinding rule to parse numbers and strings and included examples for the doc_examples test. Have not included a build.

federicobond commented 7 years ago

I think this is a good improvement, but can we make it more through? Please check the call graph from the official parser, and use those names for the rules (prefixed with Assembly where appropiate), starting from here: https://github.com/ethereum/solidity/blob/develop/libsolidity/inlineasm/AsmParser.cpp#L218-L222

So this AssemblyLocalBindingRightHandExpression should be AssemblyExpression, consisting of AssemblyElementalOperation and AssemblyFunctionalInstruction and so on.

federicobond commented 7 years ago

No need to do it for the complete ASM implementation if you don't have time. Just let's make sure we are moving in the right direction.

cgewecke commented 7 years ago

@federicobond I'm having a difficult time inferring what the rule that AssemblyElementalOperation implements would look like, esp. with regard to the section that handles the return, identifier, byte, and address tokens. What's happening there? Have you seen this solasm peg?

If solasm is accurate, SP might look something like this:

AssemblyLiteral
  = NumericLiteral
  / StringLiteral
  / HexStringLiteral

AssemblyExpression 
  = AssemblyLiteral
  / FunctionalAssemblyExpression (or AssemblyFunctionalInstruction)
  / Identifier 

AssemblyLocalBinding (or AssemblyVariableDeclaration)
  = 'let' __ name:Identifier __ ':=' __ expression:AssemblyExpression
  //  --> etc 
cgewecke commented 7 years ago

Have opened an issue in Solidity asking for some guidance about what the grammar is for this case.

cgewecke commented 7 years ago

@federicobond PR amended per your suggestions. FunctionalAssemblyExpression has been renamed FunctionalAssemblyInstruction and have written two sub rules named AssemblyExpression and ElementaryAssemblyOperation to handle the assignment cases. New code also adds Identifier to the latter. Examples test is updated accordingly.

A caveat - this PR may be a superficial gloss of the grammar. When I asked for clarification at Solidity about what parseElementaryOperation does I received this response:

That piece of code translates three parser tokens (return, byte and address) into strings literals. Then all string literals are compared against the EVM instructions list and those on the list become Instruction AST items, while those which aren't become Literals.

federicobond commented 7 years ago

Sorry man! I had some crazy days at a conference in Frankfurt and couldn't follow up. Your solution looks good.

cgewecke commented 7 years ago

No worries! Thanks so much Federico!

On Apr 30, 2017 4:17 PM, "Federico Bond" notifications@github.com wrote:

Merged #66 https://github.com/ConsenSys/solidity-parser/pull/66.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ConsenSys/solidity-parser/pull/66#event-1063578449, or mute the thread https://github.com/notifications/unsubscribe-auth/AG_guj53906yLkegecYqN4LbU3hSSQ8kks5r1RaLgaJpZM4NIOhF .