Draft fix for #26

[cgewecke]

This PR rewrites the declarative expression rule to allow descriptors like constant and internal to appear in any order.

It

• Creates a new rule for the set of descriptors
• Replaces DeclarativeExpression's optional descriptor series with greedy collection
• Extracts the descriptors from collected array & sets the relevant expression values

Also adds example to doc_examples and has rebuild.

[federicobond]

This is a step in the good direction, but I would prefer to separate state variable specifiers from local ones, and also replace all those boolean keys in AST nodes with visibility and storage string enum keys.

Local variables, as far as I understand, only accept a StorageLocation (see grammar.txt) while state variables can be either constant or not and also have a visibility specifier.

[cgewecke]

@federicobond I'd like to move this PR forward but I could use a little design help if that's ok.

In your view

• is it adequate to retain the genericDeclarativeExpression rule and just make it more discerning with the changes you've suggested OR should DeclarativeExpression be scrapped and more extensive revisions made so that the declaration's context is actually taken into account? For example - by replacing DeclarativeExpression with StateVariableDeclarativeExpression at the SourceElement rule and making whatever cascade of changes need to be made from there? And figuring something out for locals?
• should visibility specifiers be mutually exclusive (e.g. uint public private not allowed)?
• should storage specifiers be mutually exclusive?
• should constant remain boolean?

[federicobond]

Thank you for your time @cgewecke, I am happy to help.

As a general rule, whenever in doubt, check the result of the official solidity compiler AST or the grammar.txt document I linked above. While we are not aiming for an exact match at the moment, sticking as close as possible to it should minimize errors.

Regarding your questions:

• Let's split state from local variable declarations. Keep the current DeclarativeExpression for local variables (removing visibility specifiers) and create a new rule as you suggest for state ones.
  • Visibility specifiers should be mutually exclusive. The corresponding AST nodes should have a visibility key with a string value indicating the declaration visibility.
  • Storage specifiers should be mutually exclusive.
  • constant should remain boolean.

For state variables, I believe you can do ( StorageLocation __ ConstantToken? ) / ( ConstantToken __ StorageLocation ) or something similar to guarantee exclusivity in any order.

[cgewecke]

Great, thanks so much @federicobond. Will revise as you suggest.

[cgewecke]

@federicobond Sorry for letting this languish for weeks. One reason has been that while I agree the parser should have the precision of the grammar, the proposed changes to DeclarativeExpression will likely affect downstream projects (specifically Solcover and Solium)

I'm wondering if you would consider the following:

1. Accept this PR as a temporary fix (it stops solidity-parser from crashing on legal code).
2. Get a PR from @duaraghav8 for issue #9 (tuples) (also a crasher).
3. Help @duaraghav8 implement Solium's issue #58 (SP transition)
4. Help Solium (and others) integrate breaking changes in SP. (They're not complicated - but if types are added Solium needs to handle them).

I realize this is some insane hopscotching around but it might help knit some of the core utilities to a common parser base and prevent further drift.

I'd be very happy to help with the above. I also understand if it's too complicated or should be done in a different order . . .

[federicobond]

@cgewecke that sounds great. I thought about asking you for an update earlier today, but it realized it was too early in the year to bother volunteers :)

Your plan sounds good, and you have my full support for that. As long as every change is documented in the changelog, the worst that can happen is that some downstream project must fix their declared dependency versions until they migrate.

cc/ @tcoulter

[federicobond]

For reference, my Solidity ANTLR4 grammar is much more conformant with the reference implementation at this point and uncovered several errors in the official grammar as published in the docs.

You can use it as a reference when making changes.

[cgewecke]

@federicobond Awesome. Thanks for the grammar link.

[federicobond]

@cgewecke can you rebase and remove the built parser so it merges cleanly?

[federicobond]

While you are there, please rewrite your commit as "Allow declarative..."

[cgewecke]

@federicobond Would it be ok if I close this, refork and resubmit. The odds of me rebasing correctly are 0%. Also should I include a build or not?

[cgewecke]

@federicobond On second thought I'm actually in favor of closing this period and just not merging. You've proposed a much better set of changes that should be their own thing. I apologize for this whole mess.

[federicobond]

I think it's best to not include a build. Otherwise, any two simultaneous pull requests will most likely generate a merge conflict.

[federicobond]

OK, let me know if you need any help. I am a bit overworked at the moment but I would love to move this forward by next week.

[cgewecke]

Great - I'll write up your idea over the weekend and I think there will be some small PRs coming out of an analysis of the difference between solparse and solidity parser early next week too.