AST module #991

acbart · 2019-12-18T02:41:07Z

With my semester wrapping up, I wanted to make some pull requests for features I added during the semester. This one is probably not the most important, but its a big part of BlockPy's toolchain, so I wanted to get a sense of your interest. Others include exec, more info in exceptions, and a fix to special method lookup in classes.

This PR adds in an importable ast module that has the most of the core functionality you'd expect from CPython's ast library, including AST node classes, a NodeVisitor class, and a parse function. It also expands nodes to include ending lines and columns in addition to the starting attributes.

If you're interested, I'm willing to spend some time cleaning up some of the code further - I am passing all the test cases I wrote, at least, and none of the existing tests seem to be broken. Not sure how compatible this is with the other pull requests open right now. Probably should merge them in first and then make further modifications as required.

acbart · 2019-12-18T02:43:18Z

The parser also collects comments now too. That's a necessary bit for BlockMirror's block/text conversion algorithm, though it might be esoteric for others.

Oh, and I think some code for modules ended up in here, which I can move to the exec PR. That's related to the crazy global mangling that has to go on to make things exec keep namespaces separate.

rixner · 2020-02-04T16:17:30Z

I am in favor of this pull request. I looked over the code and it seems like this doesn't disrupt anything else in the core and it adds new functionality we don't currently have.

However, I can't run it right now, perhaps because I resolved the conflicts incorrectly (I didn't try very hard, admittedly)? I'm also not sure why Travis is saying it passes the tests when there is a conflict?

Also, you changed Sk.builtin.module to take a module name as an argument, but you didn't change existing callers (I think there's only one in import.js) to properly pass the name.

Anyway, @acbart if you can resolve the conflicts and get things in a condition you are happy with, I will be happy to approve this!

acbart · 2020-02-24T09:27:45Z

Sorry I've been utterly unable to work on this lately! Our semester started a few weeks ago, which means my long winter break ended without the chance for me to contribute anything more while I was waiting for this to be approved. It seems like you've been able to incorporate some of my changes on your own, anyway. When the summer rolls around, hopefully I can get the rest of my changes integrated into wherever you end up.

rixner · 2020-07-08T13:38:05Z

I would still very much like to see this merged. @acbart do you have time to bring it up to date?

acbart · 2020-07-08T13:45:22Z

I plumb forgot about this, but I do have time now. When integrating @s-cork 's #1092 pull request I did have to make some changes to this module. Is it worth waiting till that PR is merged in before trying to get this merged in? Or vice versa?

rixner · 2020-07-08T13:56:20Z

I'd vote to clear the backlog of pull requests before we even consider #1092. So, my vote is this should go first.

albertjan · 2020-07-11T13:17:02Z

@acbart can you do this module without adding end_line_no and end_col_offset change?

rixner · 2020-07-11T13:37:14Z

@albertjan is there a reason you don't want those changes? I think we want those. It's one of the things I needed to add for #1044.

albertjan · 2020-07-11T15:28:41Z

Sure but I would rather see them as a separate PR. As they aren’t strictly necessary for the ast module. And it makes this PR harder to merge. Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Scott Rixner <[email protected]> Sent: Saturday, July 11, 2020 2:37:25 PM To: skulpt/skulpt <[email protected]> Cc: Albert-Jan Nijburg <[email protected]>; Mention <[email protected]> Subject: Re: [skulpt/skulpt] AST module (#991) @albertjan<https://github.com/albertjan> is there a reason you don't want those changes? I think we want those. It's one of the things I needed to add for #1044<#1044>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#991 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AACJVXZX3FR6PUJOYAH24HTR3BTJLANCNFSM4J4EN7VA>.

acbart · 2020-07-11T18:11:08Z

I appreciate wanting to have them be separate, but that would be an awful lot of work on changing something I've already done. Plus, I'd really like to have the line endings in upstream Skulpt, it's currently making it much harder for me to merge in upstream changes. I have a little time today to make the already requested changes and resolve the simple merge conflict that was there, but if you want that change, it'll be a lot longer before I'd have the time needed.

acbart · 2020-07-11T19:07:10Z

Okay, want to take another look? I'm passing all test cases and I don't think there are any conflicts to resolve.

albertjan · 2020-07-11T19:49:04Z

src/ast.js

@@ -3363,5 +3378,115 @@ Sk.astDump = function (node) {
    return _format(node, "");
 };

+
+Sk.INHERITANCE_MAP = {


what does this do is this automatically generated?

Not automatically generated. I believe this data ended up here largely out of convenience that came when authoring it, and I'm thinking now it needs to be moved into the src/lib/ast.js file instead. The idea is that this encodes the inheritance relationships that exist between nodes. Want it moved to the library AST file?

albertjan · 2020-07-11T19:50:46Z

src/parser.js

@@ -68,6 +69,11 @@ function findInDfa (a, obj) {
    return false;
 }

+// Add a comment


our parser throws out comments? Does the cpython parser leave them in?

If I recall, both Skulpt and cpython toss the comments out.

But you keep them in here? But I don't see where you use them.

I don't use them anywhere in here, but they're essential for BlockMirror's ability to take Skulpt parse trees and convert them into Blockly code. I know this may not be useful to anyone else, so I suppose it's worth talking about removing. Personally, I think it's nice to have them collected, especially if you ever want to add special behavior like #: type comments.

Is there a better solution to parse comment ? I think this is not the optimal way

albertjan · 2020-07-11T19:55:07Z

Can you see how many of these tests pass? https://github.com/python/cpython/blob/3.8/Lib/test/test_ast.py feel free to comment out anything that doesn't apply or that skulpt can't handle.

…s to ast.parse.

… there.

…brary

…ray per new style, add in missing new keywords.

acbart · 2020-07-14T18:48:07Z

To answer the question of how many tests pass: apparently almost none of them (I think 3 passed).

I haven't implemented any of the functionality for things like fix_missing_locations, get_source_segment, and get_docstring.
I added in a little functionality to create nodes manually (limited utility for my purposes), but that doesn't prepopulate their fields.
There's some wacky inheritance things that happen with the new Constant nodes that I don't think we should bother with until we have proper metaclassing. It's weird.
I believe I messed up the keyword arguments for parse and dump, but I don't know what the exact style is supposed to be. Is that documented somewhere?

Only 4 seems worth fixing to me, right now. I think someone else will have to feel strongly about 1-3, since I have no particular reason to make them happen.

albertjan · 2020-07-14T19:37:07Z

Thanks @acbart. I'll try to have look tomorrow.

s-cork · 2020-07-15T16:17:22Z

src/module.js

+    }
+    this["$d"] = {__name__: name};
+    this["$d"]["__dict__"] = this["$d"];
+    return this;
 };


Should we have the __dict__ here? this would return a javascript object to the user no?

I totally defer to you on this one, I figured whatever I had in here would have to be heavily changed for your new PRs anyway. This is just what I had at this point in time. I note that my own local Skulpt (which integrates most of your changes, but not the latest ones) actually looks like this:

Sk.builtin.module = Sk.abstr.buildNativeClass("module", { // ... tp$init: function(args, kwargs) { Sk.abstr.checkArgsLen(this.tp$name, args, 1, 3); this["$d"] = { "__name__": args[0], "__package__": Sk.builtin.none.none$, }; return Sk.builtin.none.none$; },

Nice. Yeah, For this pr I'd remove the __dict__ since the user won't get a python object if they try to access it. I guess this really should be a python dictionary in the future...

Does b92c363 handle this? I feel silly for letting my module stuff slip into this PR, it's not really all that relevant, ultimately.

…brary

… new work anyway.

s-cork · 2020-08-06T01:30:56Z

test/unit3/test_ast.py

+('Expression', ('Tuple', (1, 0), [], ('Load',))),
+('Expression', ('Call', (1, 0), ('Attribute', (1, 0), ('Attribute', (1, 0), ('Attribute', (1, 0), ('Name', (1, 0), 'a', ('Load',)), 'b', ('Load',)), 'c', ('Load',)), 'd', ('Load',)), [('Subscript', (1, 8), ('Attribute', (1, 8), ('Name', (1, 8), 'a', ('Load',)), 'b', ('Load',)), ('Slice', ('Constant', (1, 12), 1, None), ('Constant', (1, 14), 2, None), None), ('Load',))], [])),
+]
+main()


for this pr we'd need this unit test module to pass. I'd suggest commenting out all the tests that don't pass here...

s-cork · 2020-08-06T01:31:48Z

test/test_ast.py

@@ -0,0 +1,94 @@
+import ast


these tests don't fit with the style of skulpt.
Can we turn these into unit tests?

Ported over modfications from BlockPy fork

df3bc7f

rixner mentioned this pull request Feb 11, 2020

Update node packages and engines. #1017

Merged

Remove unnecessary commented out code

4a7f577

acbart added 3 commits July 11, 2020 14:29

Added in missing module name parameter.

c328c0e

Integrate new changes, add in missing line/col endings

13784f1

Use most recent version of package.json for simplicity

ef148ae

albertjan reviewed Jul 11, 2020

View reviewed changes

acbart added 6 commits July 14, 2020 12:49

Put in heavily commented-out test_ast.py file.

777efbf

Some typos with the latest of src/ast.js, fix to be n instead of c.

7f5ee0a

Support creating nodes on their own, add in missing keyword parameter…

79e5319

…s to ast.parse.

Move INHERITANCE_MAP into the library, also fix the linting while I'm…

d99bf57

… there.

Merge branch 'master' of https://github.com/skulpt/skulpt into ast_li…

d613d18

…brary

Fix regression caused by misnaming the arguments_ node, use callsimAr…

1830da3

…ray per new style, add in missing new keywords.

s-cork reviewed Jul 15, 2020

View reviewed changes

Merge branch 'master' of https://github.com/skulpt/skulpt into ast_li…

2d51e38

…brary

acbart and others added 2 commits July 17, 2020 10:51

Remove __dict__ for module; all this will be superceded by S-corker's…

b92c363

… new work anyway.

Merge branch 'master' into ast_library

f4d1863

rixner mentioned this pull request Aug 5, 2020

Bytes Class #989

Merged

s-cork reviewed Aug 6, 2020

View reviewed changes

test/test_ast.py

@@ -0,0 +1,94 @@

import ast

Copy link

Contributor

s-cork Aug 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these tests don't fit with the style of skulpt.
Can we turn these into unit tests?

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AST module #991

AST module #991

acbart commented Dec 18, 2019

acbart commented Dec 18, 2019

rixner commented Feb 4, 2020

acbart commented Feb 24, 2020

rixner commented Jul 8, 2020

acbart commented Jul 8, 2020

rixner commented Jul 8, 2020

albertjan commented Jul 11, 2020

rixner commented Jul 11, 2020

albertjan commented Jul 11, 2020 via email

acbart commented Jul 11, 2020

acbart commented Jul 11, 2020

albertjan Jul 11, 2020

acbart Jul 12, 2020

albertjan Jul 11, 2020

acbart Jul 12, 2020

albertjan Jul 15, 2020

acbart Jul 15, 2020

kolowy Jul 3, 2023

albertjan commented Jul 11, 2020 •

edited

Loading

acbart commented Jul 14, 2020 •

edited

Loading

albertjan commented Jul 14, 2020

s-cork Jul 15, 2020

acbart Jul 15, 2020

s-cork Jul 15, 2020

acbart Jul 17, 2020

s-cork Aug 6, 2020

s-cork Aug 6, 2020

AST module #991

Are you sure you want to change the base?

AST module #991

Conversation

acbart commented Dec 18, 2019

acbart commented Dec 18, 2019

rixner commented Feb 4, 2020

acbart commented Feb 24, 2020

rixner commented Jul 8, 2020

acbart commented Jul 8, 2020

rixner commented Jul 8, 2020

albertjan commented Jul 11, 2020

rixner commented Jul 11, 2020

albertjan commented Jul 11, 2020 via email

acbart commented Jul 11, 2020

acbart commented Jul 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertjan commented Jul 11, 2020 • edited Loading

acbart commented Jul 14, 2020 • edited Loading

albertjan commented Jul 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertjan commented Jul 11, 2020 •

edited

Loading

acbart commented Jul 14, 2020 •

edited

Loading