1363 vyper ast

[jacqueswww]

What I did

Step 1 of #1363 .

How I did it

How to verify it

Description for the changelog

Cute Animal Picture

[davesque]

I’m a bit concerned about the way this is happening.

When I set out doing the stand-alone parser project, I was thinking that it would be good to keep things separate until I had a decent chunk of work completed. My initial plan was to create a drop-in replacement for the native python parser that we could then customize or simplify to our needs. I'm still in the middle of doing this or even figuring out if it's possible or realistic.

I feel like incorporating this PR into the main vyper code base is going to make it difficult for me to continue working on this functionality as it will make it harder to merge future work from the stand-alone parser project. If and when I finish the separate parser work, and if this PR gets merged, I'll have the extra task of figuring out what parts of things got added and whether or not they were added in a way that faithfully represented what I was trying to do.

I'd like to request that we hold off on trying to do this until I've had time to complete what I was trying to do and that I eventually be allowed to merge the work in myself. Is it possible that other core devs on this project can focus on other areas of work for the time being?

[fubuloubu]

It sounds like there's a big misalignment on what the expectations were, and how to proceed. Let's discuss this on the call Monday (#1368) to make sure we are aligned on the path forward and ensure progress is not wasted on anyone's side.

[jacqueswww]

@fubuloubu @davesque

The goal of step 1 was clearly highlighted in #1363 as well as my comment that I would start work on step 1. We also have discussed doing this exact at length on many calls before, without any disputes - as well as the release call before #1363. Which if you remember correctly is why I created #1363 in the first place.

I don't think aligning to these classes is a problem for an independent/alternative parser at all. These classes are basically just a one to one translation to the python ones, but we get the added benefit of:

a.) Visiblity of the classes
b.) Not having fields that are mysteriously populated, ie. #1359.

For the above reasons alone, it's more than reasonable to expect to merge this. As this will provide much improved stability to the codebase, which is what the meeting of before #1363, was about after all.

As I have stated many times before, creating your own parser is a very slippery slope. Just have a look at serpent for an example. But it was again and again brought up as necessary. So I did the first step that would require the integration into the existing code base, whichever parser we end up using (as these are still super experimental) - will have to adhere with the vyper codebase not the other way around.

As stated on call rc1, will not use a custom parser, but will use the python ast module. (Also: there is plenty of time to merge this for RC1).

We can voice call on Monday or sooner if you guys prefer.

[jacqueswww]

@pipermerriam thanks for the review, will make the necessary changes 👍

[jacqueswww]

Added above mentioned changes.

[jacqueswww]

@fubuloubu @davesque I believe this is ready to be merged now. Let me know if I have to add anything.

[fubuloubu]

One unresolved review I see.

Also, if @charles-cooper could give a brief look about the upgraded objects, that would help.

[charles-cooper]

Also, if @charles-cooper could give a brief look about the upgraded objects, that would help.

Seemed pretty decent, although I was wondering why the inheritance hierarchy is 'flat'. E.g., I would think Add and co inherit from BinOp, which in turn inherits from Op, which inherits from Expr.

[fubuloubu]

@charles-cooper I think that was done to mimic the Python inheritance structure, but I could be wrong. We should mimic the Python structure if that is not true.

[jacqueswww]

@charles-cooper @fubuloubu I didn't extend the classes further because we want to replace them with David's ones once this is merged. https://github.com/davesque/vyper-parser/blob/master/vyper_parser/ast.py

[fubuloubu]

@jacqueswww gotcha, let's do that!

[davesque]

@jacqueswww @fubuloubu Yeah, what's here is fine. I think we can just make modifications with further PRs as necessary. Sorry I was absent in this discussion.