Skip to content

Object comprehensions (lightscript/lightscript#6) #7

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 26, 2017
Merged

Object comprehensions (lightscript/lightscript#6) #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/parser/expression.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,14 @@ pp.parseObj = function (isPattern, refShorthandDefaultPos) {
node.properties = [];
this.next();

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

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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...

Copy link
Author

@wcjohnson wcjohnson Mar 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

if (this.lookahead().type !== tt.colon) {
return this.parseObjectComprehension(node);
}
}

let firstRestLocation = null;

while (!this.eat(tt.braceR)) {
Expand Down
7 changes: 7 additions & 0 deletions src/plugins/lightscript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,13 @@ pp.parseArrayComprehension = function (node) {
return this.finishNode(node, "ArrayComprehension");
};

pp.parseObjectComprehension = function(node) {
const loop = this.startNode();
node.loop = this.parseForStatement(loop);
this.expect(tt.braceR);
return this.finishNode(node, "ObjectComprehension");
};

pp.isNumberStartingWithDot = function () {
return (
this.match(tt.num) &&
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/lightscript/comprehension/for-as-obj-key/actual.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{for: 4}
121 changes: 121 additions & 0 deletions test/fixtures/lightscript/comprehension/for-as-obj-key/expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
{
"type": "File",
"start": 0,
"end": 8,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 8
}
},
"program": {
"type": "Program",
"start": 0,
"end": 8,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 8
}
},
"sourceType": "script",
"body": [
{
"type": "ExpressionStatement",
"start": 0,
"end": 8,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 8
}
},
"expression": {
"type": "ObjectExpression",
"start": 0,
"end": 8,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 8
}
},
"properties": [
{
"type": "ObjectProperty",
"start": 1,
"end": 7,
"loc": {
"start": {
"line": 1,
"column": 1
},
"end": {
"line": 1,
"column": 7
}
},
"method": false,
"shorthand": false,
"computed": false,
"key": {
"type": "Identifier",
"start": 1,
"end": 4,
"loc": {
"start": {
"line": 1,
"column": 1
},
"end": {
"line": 1,
"column": 4
},
"identifierName": "for"
},
"name": "for"
},
"value": {
"type": "NumericLiteral",
"start": 6,
"end": 7,
"loc": {
"start": {
"line": 1,
"column": 6
},
"end": {
"line": 1,
"column": 7
}
},
"extra": {
"rawValue": 4,
"raw": "4"
},
"value": 4
}
}
]
}
}
],
"directives": []
}
}
1 change: 1 addition & 0 deletions test/fixtures/lightscript/comprehension/malformed-as-pattern/actual.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
const { for elem x in arr: x, x } = {}
3 changes: 3 additions & 0 deletions test/fixtures/lightscript/comprehension/malformed-as-pattern/options.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"throws": "Binding invalid left-hand side in variable declaration (1:6)"
}
1 change: 1 addition & 0 deletions test/fixtures/lightscript/comprehension/object-basic/actual.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{for idx i in Array(10): (i,i)}
221 changes: 221 additions & 0 deletions test/fixtures/lightscript/comprehension/object-basic/expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,221 @@
{
"type": "File",
"start": 0,
"end": 31,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 31
}
},
"program": {
"type": "Program",
"start": 0,
"end": 31,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 31
}
},
"sourceType": "script",
"body": [
{
"type": "ExpressionStatement",
"start": 0,
"end": 31,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 31
}
},
"expression": {
"type": "ObjectComprehension",
"start": 0,
"end": 31,
"loc": {
"start": {
"line": 1,
"column": 0
},
"end": {
"line": 1,
"column": 31
}
},
"properties": [],
"loop": {
"type": "ForInArrayStatement",
"start": 1,
"end": 30,
"loc": {
"start": {
"line": 1,
"column": 1
},
"end": {
"line": 1,
"column": 30
}
},
"idx": {
"type": "Identifier",
"start": 9,
"end": 10,
"loc": {
"start": {
"line": 1,
"column": 9
},
"end": {
"line": 1,
"column": 10
},
"identifierName": "i"
},
"name": "i"
},
"body": {
"type": "ExpressionStatement",
"start": 25,
"end": 30,
"loc": {
"start": {
"line": 1,
"column": 25
},
"end": {
"line": 1,
"column": 30
}
},
"expression": {
"type": "SequenceExpression",
"start": 26,
"end": 29,
"loc": {
"start": {
"line": 1,
"column": 26
},
"end": {
"line": 1,
"column": 29
}
},
"expressions": [
{
"type": "Identifier",
"start": 26,
"end": 27,
"loc": {
"start": {
"line": 1,
"column": 26
},
"end": {
"line": 1,
"column": 27
},
"identifierName": "i"
},
"name": "i"
},
{
"type": "Identifier",
"start": 28,
"end": 29,
"loc": {
"start": {
"line": 1,
"column": 28
},
"end": {
"line": 1,
"column": 29
},
"identifierName": "i"
},
"name": "i"
}
],
"extra": {
"parenthesized": true,
"parenStart": 25
}
}
},
"array": {
"type": "CallExpression",
"start": 14,
"end": 23,
"loc": {
"start": {
"line": 1,
"column": 14
},
"end": {
"line": 1,
"column": 23
}
},
"callee": {
"type": "Identifier",
"start": 14,
"end": 19,
"loc": {
"start": {
"line": 1,
"column": 14
},
"end": {
"line": 1,
"column": 19
},
"identifierName": "Array"
},
"name": "Array"
},
"arguments": [
{
"type": "NumericLiteral",
"start": 20,
"end": 22,
"loc": {
"start": {
"line": 1,
"column": 20
},
"end": {
"line": 1,
"column": 22
}
},
"extra": {
"rawValue": 10,
"raw": "10"
},
"value": 10
}
]
}
}
}
}
],
"directives": []
}
}
1 change: 1 addition & 0 deletions test/fixtures/lightscript/comprehension/object-curly/actual.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{for idx i in Array(10) { (i, i) }}
Loading