enable CPython's _ast module

[aisk]

Just copied & modified CPython's asdl stuff to generate _ast module.

issule: #902

[kmod]

Hey @aisk are you still interested in working on this? With the CPython parser in-tree now, I think it should be much easier to do this now.

[aisk]

@kmod Hi I will continue trying this in this weekends 😄

[Daetalus]

@aisk Glad to hear, I need this module too.

[aisk]

Update:
Now just enabled the CPython's _ast module, and fixed the builtin function compile to use CPython's functions to compile source to AST and AST to runable object, and removed the runtime/builtin_modules/ast.cpp file.

[kmod]

Nice! Looks like the issue is that it's having trouble with some of the static objects defined in the Python-ast.c file -- I think the fix should be a matter of adding PyGC_AddRoot calls to where it assigns to the static variables (in init_types()).

If you could, I think it would be nice to take a look at enabling test_ast.py and test_compile.py. I'm not sure how close those are to passing, but I think otherwise we won't have great coverage of the ast/compile() functionality.

[aisk]

Oh there were other issues in cpythonToPystonAST, I saw some convert function is not handled in some situation?

[kmod]

Yeah looks like the handling of "module"-type objects isn't great and needs to be fleshed out a little bit. (I didn't/don't know how it works so I just had it abort.) It looks like we don't have any test coverage of the compile(AST) path that hits this, so I'd recommend 1) getting some tests in (either test_ast.py, or ideally some custom tests as well), and 2) changing the compile(str) path to be more similar to the compile(AST) path. Specifically, one path uses the CPython logic, and the other uses the Pyston logic, so they behave pretty differently, and all the testing we have of the compile(str) path doesn't help us with making sure that compile(AST) does the right thing. I would try changing the calls to parseExec/parseEval to calls to PyParser_ASTFromString.

As for the specific problem, what I think is happening here is different handling of different module types. This is metadata about the string that changes the way the string is interpreted, and is what the "eval"/"exec"/"single" strings mean. Pyston code handles this completely at parse time by post-processing the raw AST node to reflect the metadata (this happens in parseEval/parseExec). CPython, on the other hand, just stores this metadata as part of the returned AST object, and then does the actual transformation when compiling to bytecode. So when we go through a code path that uses CPython's parser but our compiler, neither part will do the transformation (see below for the kind of thing that ends up getting missed). One way forward could be to use CPython's parser, and then in cpythonToPystonAST, do the transformation (ie call makeModuleInteractive() or whatever else is necessary).

The transformation is some simple things like "in 'single' mode, print out all bare expressions". Here's an example of something that isn't working in this PR:

a = compile("'hello world'", "test.py", "single", 0x400)
exec compile(a, "test.py", "single")

This should print 'hello world' but currently in this PR doesn't print anything; we need to figure out a place to call makeModuleInteractive.

Sorry this is messy! As we switch closer to the way CPython handles it, things will become more straightforward.

[aisk]

Hi @kmod , sorry since the Pyston AST, CPython native AST and Python AST are complicated to me, I think I'm understood the exactly problem was:

In the old parse process in builtin compile function, we trust all modes as exec (compile as module), and the parsed AST is marked as Module_Kind, then we do the AST transform. This works good without using _ast.PyCF_ONLY_AST param.

But with it, we must compile the AST which follows the mode param. If we use the eval mode, which compiles the Expression_Kind, then when we compile(AST), the cpythonToPystonAST function can't handle the Expression_Kind.

So the next thing to do is:

• complete the cpythonToPystonAST function to let it handle Expression_Kind
• refactor the code, remove the parseXXX function call in compile, just parse the code to AST with the mode in param, then do the AST transform in cpythonToPystonAST

Is this right, or some misunderstanding I am making?

[kmod]

Sounds good! I think we're coming at this from slightly different perspectives but I think we're getting to the same place. Concretely, I would recommend handling Expression_Kind by calling makeModuleInteractive().

[undingen]

PyGC_AddRoot(PyGC_AddRoot( -> PyGC_AddRoot(

[undingen]

I'm not really sure what to think about this added PyGC_AddRoot because if they are required I think that make_type should also call PyGC_AddRoot on the return value.

Looks good on first sight! I will test it out later but I noticed completely lack of testing. Could you please add some tests. Thanks!

[aisk]

@undingen Thanks for review, I'll add more testing now!

[aisk]

Oh I'm not familiar with the GC, but before PyGC_AddRoot there were some exception happened, so I think it's needed. And I'll have a try to see the GC details. 😀

[undingen]

I think we should call PyGC_AddRoot inside make_type.

The 'continue' not properly in loop etc error checking is as you properly noticed quite a mess currently :-(. How do you feel about not fixing this in the PR but doing it in another PR later on (you don't have to do it - I can take a look at it) and instead just disabling the failing test cases in this PR?
Because we not only have to move the error checking form the pypa parsing step and cpython AST to our AST converting step into the computeCFG we also have to make sure that computeCFG get's called at the correct time and not lazily like we do now and I think this is easier to keep track of if it's done in a separate PR.

[aisk]

@undingen sorry, I'll update this PR as soon as I can. And should I keep the syntax check in cpythonToPystonAST as this PR now, or just reset it back?

[undingen]

I think it's best to keep the syntax checks otherwise this will break existing tests. Disabling the failing parts of the new cpython AST test is fine (in the test/tests/ copy). I had to do something similar in #1045

[undingen]

I think you meant to do a if (loop_depth == 0) error check not two in_finally checks

[undingen]

LGTM,
I noticed that this will now always use cpythons parser for parsing strings inside compile(). Because we plan to switch away from pypa to the cpython parser this should be fine and doing additional work to support something we will remove soon is probably not worth it.

Thanks for the patch!

[aisk]

😀