Object comprehensions (lightscript/lightscript#6) #7

wcjohnson · 2017-03-25T18:36:38Z

Added object comprehension parsing
Parse tree node is now Comprehension with a qualifier comprehends=object or comprehends=array

rattrayalex

nice, looking good!

rattrayalex · 2017-03-25T19:40:10Z

src/parser/expression.js


+  // `for` keyword begins an object comprehension.
+  if (this.hasPlugin("lightscript") && this.match(tt._for)) {
+    // ...however, `{ for: x }` is a legal JS object.


ah, that does complicate things...

At least we dont have to deal with myAnnoyingDestructuredObj = { for }.

I'd almost rather just add a restriction to the language saying that if you want an object where the first key is for, you need to wrap it in quotes: { "for": "reasons" }.

It'd be less confusing as a reader in that case anyway. Thoughts?

Generally speaking I am really against weird edge cases like this that programmers have to remember. I think we can fairly frictionlessly integrate with previously-legal JS at the cost of a lookahead in the parser, which is not that big of a deal. My opinion is we should let people write { for: 4} if they really want to.

I don't have hard numbers on lookahead perf, but I'm pretty sure it's just about the slowest thing that we can do in the parser 😉 compiler perf can really make a difference to UX.

hmm...

I've been looking at the lookahead code and I think we can actually do this in a performant way. Since our lookahead here isn't actually relying on the whole parser state, nor do we call any functions during the lookahead, we may be able to do a "cheap" lookahead where we ignore the parser state altogether.

That should eliminate any object copying and shouldn't be much more expensive than calling substr or whatever the tokenizer does.

This is the sort of thing I am less likely to worry about than getting the semantics right, but hey, everyone wants their code to compile fast.

I'm pretty nervous about doing something like that... My impression of the way the parser is set up makes me think that could really open a can of worms... would much rather just take the perf hit

rattrayalex · 2017-03-25T19:45:48Z

src/plugins/lightscript.js

  this.expect(tt.bracketR);
-  return this.finishNode(node, "ArrayComprehension");
+  node.comprehends = "array";
+  return this.finishNode(node, "Comprehension");


I think I'd rather stick with two node types, ArrayComprehension and ObjectComprehension. Sound ok to you?

I'm OK either way (in fact, leaving it is less work for me on updating unit tests) but my thinking here is that in later ast transforms we may want a single node type. Especially since the code for both types of comprehensions is going to be essentially the same.

On second thoughts I think I agree with you, worst case we can just do a type union in the ast visitor. I will use two different node types.

Yeah, see comment in lightscript/lightscript#6, would rather do code-sharing with helper-functions. Node types should be based around what the thing is, not what makes for easier internals

rattrayalex · 2017-03-25T19:47:01Z

test/fixtures/lightscript/comprehension/object-basic/actual.js

@@ -0,0 +1 @@
+{for i=0; i<10; i++: (i,i)}


yeah, let's avoid for-;; in tests if possible...

It's your show, but I don't fully agree with this. I think a key philosophical point in eliminating the "big table of for" is that good old fashioned for loops are perfectly acceptable for just counting numbers.

sorry, should have clarified that one of the reasons is that they just generate much larger AST's than simpler for-loops, making them harder to review

but point taken

oh, I see what you mean. OK, will change

wcjohnson · 2017-03-25T23:21:24Z

@rattrayalex Comments addressed. Working on the AST transform next.

rattrayalex · 2017-03-25T23:36:57Z

Cool, I haven't combed through the tests a second time but I assume that's good; want to squash and I'll merge?

Oh, we may want to add a test that it errors acceptably if an object comprehension is put somewhere that an object pattern is expected. Eg;

const { for elem x in arr: x, x } = {}

wcjohnson · 2017-03-25T23:42:18Z

Good point, will add that test

wcjohnson · 2017-03-25T23:45:55Z

All tests passing, everything looks good over here, if there's nothing else I will squash the branch.

wcjohnson · 2017-03-25T23:53:43Z

@rattrayalex Squashed anyway, can still make further changes if needed.

rattrayalex

A bunch of these tests are probably unnecessary/redundant, but not worth taking the time to decide which ones.

One thing I don't see are any tests checking for the SequenceExpression without parens. Parens may be a good style for that (not sure what I think personally yet) but both need to be parseable.

A single test without the parens would probably be fine; I can't think of a scenario here where I would expect it to cause an issue. But one can imagine a future change that might break.

wcjohnson · 2017-03-26T00:16:43Z

Will add additional test for that. I'm sure some of the fixtures are redundant, but I'd rather err on the side of having too many!

rattrayalex · 2017-03-26T00:20:19Z

Cool, should be good to merge after that!

(+7 squashed commits) Squashed commits: [ef65cfe] Update new fixtures to simplify parse tree output [850bd16] Return to using two different parse tree nodes for `ObjectComprehension` vs `ArrayComprehension` [9d9b90d] Revert "Updated existing test fixtures for node type change" This reverts commit d00dbd1. [47b5181] Added test case for `for` as object key [2f28e50] New test fixtures for object comprehensions [d00dbd1] Updated existing test fixtures for node type change [31fe13b] - Added parsing for Object comprehensions - Changed generated parse tree node to `Comprehension` type.

wcjohnson · 2017-03-26T00:26:06Z

@rattrayalex

Added the test case; the output looks correct. I just amended and force pushed to avoid having to do another squash.

rattrayalex · 2017-03-26T00:33:06Z

Yeah that's what I normally do. Cool will merge!

…

On Sat, Mar 25, 2017, 17:26 William C. Johnson ***@***.***> wrote: @rattrayalex <https://github.com/rattrayalex> Added the test case; the output looks correct. I just amended and force pushed to avoid having to do another squash. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAq_LrI5ZRaZhBFMtD_PuWEn8IPZFAPuks5rpbCegaJpZM4MpK0u> .

rattrayalex reviewed Mar 25, 2017

View reviewed changes

wcjohnson force-pushed the feature/comprehensions branch from 1d9e7e0 to 5a0125f Compare March 25, 2017 23:53

wcjohnson changed the title ~~[WIP] Comprehensions (lightscript/lightscript#6)~~ Object comprehensions (lightscript/lightscript#6) Mar 25, 2017

rattrayalex approved these changes Mar 26, 2017

View reviewed changes

wcjohnson force-pushed the feature/comprehensions branch from 5a0125f to 88785c3 Compare March 26, 2017 00:25

rattrayalex merged commit fec945b into lightscript:lightscript Mar 26, 2017

wcjohnson deleted the feature/comprehensions branch March 26, 2017 02:46

Object comprehensions (lightscript/lightscript#6) #7

Object comprehensions (lightscript/lightscript#6) #7

Uh oh!

Conversation

wcjohnson commented Mar 25, 2017

Uh oh!

rattrayalex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wcjohnson Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wcjohnson commented Mar 25, 2017

Uh oh!

rattrayalex commented Mar 25, 2017

Uh oh!

wcjohnson commented Mar 25, 2017

Uh oh!

wcjohnson commented Mar 25, 2017

Uh oh!

wcjohnson commented Mar 25, 2017

Uh oh!

rattrayalex left a comment

Choose a reason for hiding this comment

Uh oh!

wcjohnson commented Mar 26, 2017

Uh oh!

rattrayalex commented Mar 26, 2017

Uh oh!

wcjohnson commented Mar 26, 2017

Uh oh!

rattrayalex commented Mar 26, 2017 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wcjohnson Mar 25, 2017 •

edited

Loading