ConsenSysMesh / solidity-parser

Solidity Parser in Javascript
138 stars 54 forks source link

Move solidity-parser to the trufflesuite organization? #89

Open tcoulter opened 7 years ago

tcoulter commented 7 years ago

Hi all, @federicobond,

The Truffle team has grown. In order to help us maintain this repository, we'd love to move it to the trufflesuite organization, where issues can be more easily managed by the Truffle team and we can incorporate them into our development process. The name and the npm module name will not change.

Adding this ticket to solicit feedback. Would love your thoughts.

Thanks! Tim

federicobond commented 7 years ago

Hi Tim, I think it's a good decision. I am slowly disengaging from the project actually. I think it works fine for simple imports parsing but for full Solidity parsing its structure is just too different from the official Solidity specification, which almost guarantees that it will keep breaking for all sorts of edge cases.

I have been working on an alternative parser which should be production ready by now: https://github.com/federicobond/solidity-parser-antlr

It should avoid most of the problems people are encountering due to differences between solc and solidity-parser.

tcoulter commented 7 years ago

Would you be interested in merging solidty-parser and solidity-parsaer-antlr? We could make it the next major version. (You'd be given admin access to the repo and npm module, of course)

tcoulter commented 7 years ago

Hmm, actually, now that I think about it, it might be best to go the other way around (move this repo to your org and let you control it)

federicobond commented 7 years ago

That seems reasonable. A good Solidity parser is critical for several components in Truffle and the current one is not being actively maintained.

I would be happy to take control of solidity-parser in npm and publish a new version 1.0.x with my new code base, while we deprecate and eventually delete this repo. Would that minimize the chance of accidental breakage?

Also, let me know if you need any features not implemented in the new code base.

tcoulter commented 7 years ago

Thanks for the response @federicobond. Can you give me quick rundown of what makes ANTLR4 better? Are there any non-functional differences (speed, etc.)?

federicobond commented 7 years ago
tcoulter commented 7 years ago

Sounds awesome. Biggest need for Truffle, beyond a consistent AST, is the ability to parse for imports without parsing the rest of the code. This is mainly for performance, as when building a dependency tree of solidity files parsing the rest of the code is unnecessary (for solidity-parser, there's a significant speedup with the imports parser vs. the normal parser). Is it easy for you to add that functionality into solidity-parser-antlr?

federicobond commented 7 years ago

I think so, but it's not a use case I am actively interested in maintaining. Why not create a truffle-extract-imports package with your simplified PEG.js grammar which already covers that use case pretty well?

tcoulter commented 7 years ago

Probably worth it. solc-js is, or will be, exposing their parser (that doesn't fail on unresolved imports) as well. Will have to do some testing to figure out the right path forward.

federicobond commented 7 years ago

I did some tests with solc-js to have a baseline before starting to work on solidity-parser-antlr and I did not find its performance acceptable. It can improve a little if they explicitly commit to supporting that use case, but I don't think by much.

So...

Next steps:

tcoulter commented 7 years ago

Thanks @federicobond. Give me some time to explore options on my own and discuss with my team before we proceed.

tcoulter commented 7 years ago

Hey @federicobond. I've talked to my team, as well as Consensys. We'd like to transfer this repository to a place that you control. Additionally, I'd like to transfer ownership of the solidity-parser node module over to you. If you can provide me with an organization to transfer this over to (or just you), as well as your npm username, I'll get started.

duaraghav8 commented 7 years ago

@federicobond Please do consider the following issues & their fixes: https://github.com/duaraghav8/Solium/issues/95 https://github.com/duaraghav8/Solium/issues/107 https://github.com/duaraghav8/Solium/issues/111 https://github.com/duaraghav8/Solium/issues/112

federicobond commented 7 years ago

Hi, sorry for the delay. You can transfer it to federicobond for now.

@duaraghav8 please know that the PEG.js grammar will be deprecated in a 1.0 release, so I advice you to start migrating your code to the solidity-parser-antlr version.