Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move solidity-parser to the trufflesuite organization? #89

Open
tcoulter opened this issue Jul 17, 2017 · 14 comments
Open

Move solidity-parser to the trufflesuite organization? #89

tcoulter opened this issue Jul 17, 2017 · 14 comments

Comments

@tcoulter
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

tcoulter commented Jul 17, 2017

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

  • The lexer/grammar is language-agnostic. Parsers for different languages can just implement the action code for each node of the parse tree and benefit from a well-tested grammar specification.

  • In terms of performance, it's around 10-15% faster than solidity-parser.

  • Error reporting is way better, and even supports a tolerant mode that can recover from small errors (like a missing semicolon).

@tcoulter
Copy link
Contributor Author

tcoulter commented Jul 18, 2017

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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:

  • Create truffle-extract-imports or similar with simplified parser and API (up for grabs)
  • Transfer solidity-parser npm package to federicobond (@tcoulter)
  • Publish a 1.x version of solidity-parser to npm with new codebase (@federicobond)

@tcoulter
Copy link
Contributor Author

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

@tcoulter
Copy link
Contributor Author

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
Copy link
Contributor

@federicobond Please do consider the following issues & their fixes:
duaraghav8/Ethlint#95
duaraghav8/Ethlint#107
duaraghav8/Ethlint#111
duaraghav8/Ethlint#112

@federicobond
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants