Skip to content
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.

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

object destructuring #4

Closed
wants to merge 22 commits into from
Closed

object destructuring #4

wants to merge 22 commits into from

Conversation

zdenko
Copy link
Collaborator

@zdenko zdenko commented Mar 26, 2017

@GeoffreyBooth I've made some improvements regarding the object destructuring assignment and rest elements.
Basically, implement proposal for rest properties for ECMAScript in CS: {a, b, c...} = x

Since this proposal isn't yet at stage-4, CS doesn't compile to ES.
In my PR I catch rest element and assign remaining values to it:
{a, b, c...} = x compiles to:
a = x.a, b = x.b, c = extractObjectWithoutKeys(x, ['a', 'b'])

Multiple rest elements are disallowed. ES also requires the rest element to be the last, so compiler currently throws an error.
IMHO, CS should allow rest element anywhere, just like for arrays.
Later, when proposal reaches stage-4 we could implement similar conversion as you did for the function parameters, and ensure rest element is the last in the compiled JS.

@zdenko
Copy link
Collaborator Author

zdenko commented Mar 27, 2017

I removed the requirement for the position of the rest element.
CS will always compile it as the last element.

Currently, {a, b, c...} = x and {a, c..., b} = x are compiled to
a = x.a, b = x.b, c = extractObjectWithoutKeys(x, ['a', 'b']).

When ES proposal hits the stage-4 it will compile to {a, b, ...c} = x

Copy link
Owner

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

I haven’t reviewed the guts of what you’ve done, just some superficial comments on the stuff around it. Bravo though for implementing this feature without needing to output the stage-3 syntax. Perhaps you should add some comments like “When XXX is finalized, remove these lines” or somesuch so that we know what to revise once the syntax gets final approval?

@@ -8,3 +8,4 @@ test/*.js
parser.output
/node_modules
npm-debug.log*
yarn.lock
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be in here?

src/nodes.coffee Outdated
@@ -3028,6 +3063,20 @@ UTILITIES =
return -1;
}
"

# copy object properties excluding the list of keys
extractObjectWithoutKeys: (o) -> "
Copy link
Owner

Choose a reason for hiding this comment

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

CS2 assumes ES2015+. Do we need this function? Is there no ES function like Object.keys that can do some or all of this work for us?

I’m not opposed to the function if we need it; just trying to avoid helpers unless we need them.

test "destructuring assignment with objects and splats: ES2015", ->
obj = {a:1, b:2, c:3, d:4, e:5}

# throws (-> CoffeeScript.compile "{a, r..., b} = x"), null, "rest element must be the last"
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove commented-out code.

eq w, a
eq r['b'], obj.b

# throws (-> CoffeeScript.compile "{a, b: {r..., c}} = x"), null, "rest element must be the last"
Copy link
Owner

Choose a reason for hiding this comment

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

Ditto. Please also follow the style of the file, two empty lines between tests.

@@ -189,6 +189,9 @@ grammar =
# the ordinary **Assign** is that these allow numbers and strings as keys.
AssignObj: [
o 'ObjAssignable', -> new Value $1

Copy link
Owner

Choose a reason for hiding this comment

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

Please follow the style of the file, without empty lines here.

@zdenko
Copy link
Collaborator Author

zdenko commented Mar 30, 2017

I've made some progress yesterday and currently, I'm able to compile:

{a, b:z, c} = {a:1, b:2, c:3, d:4, e:5}

obj1 = {f:1, g:2, h:3}
{f, g} = obj1

into

var obj1;

({a, b:z, c} = {a:1, b:2, c:3, d:4, e:5});

obj1 = {f:1, g:2, h:3};
({f, g} = obj1);

and with rest element

{a, b:z, c, r...} = {a:1, b:2, c:3, d:4, e:5}

obj1 = {f:1, g:2, h:3}
{f, q...} = obj1

into

var obj1, q, r, ref;

ref = {a: 1, b: 2, c: 3, d: 4, e: 5}, ({a, b: z, c} = ref), 
r = Object.keys(ref).filter((k) => ['a','b','c'].indexOf(k) == -1).reduce((a, c) => (a[c] = ref[c], a), {});

obj1 = {f: 1, g: 2, h: 3};
({f} = obj1), 
q = Object.keys(obj1).filter((k) => ['f'].indexOf(k) == -1).reduce((a, c) => (a[c] = obj1[c], a), {});

I'm stuck with declarition of the variables from the object at the top of the scope.
Any hints or suggestions how to approach this?

So this:

obj1 = {f:1, g:2, h:3}
{f, g} = obj1

should compile to

var obj1, f, g;

obj1 = {f:1, g:2, h:3};
({f, g} = obj1);

@GeoffreyBooth
Copy link
Owner

Take a look at jashkenas#4478. I was working last night too and I got that part working. Merge my branch into yours.

The declaration happens automatically when you do new Assign.

@zdenko
Copy link
Collaborator Author

zdenko commented Apr 3, 2017

So, with the latest commit, I covered simple and nested destructuring assignment:

obj = {a:1, b:2, c:3, d:4, e:5}
{a, b:z, d, r...} = obj

a1={}; b1={}; c1={}; d1={}
obj = {
    a: a1
    b: {
      'c': {
        d: {
          b1
          e: c1
          f: d1
        }
      }
    }
    b2: {b1, c1}
}
{a:w, 'b':{c:{d:{b1:bb, r1...}}}, r2...} = obj

I added the compileObjectDestruct method to the Assign class, in which I basically traverse thru the object, remove splats and create a list of assignments which are returned back to the compileNode.
For that reason, I also added answers variable in the compileNode where possible splat assignments are stored.
When ES proposal hits stage-4, it should be pretty simple to remove compileObjectDestruct method and answers variable.

I don't check for the position of the splat. Currently, it is removed and the compiled code is correct, and afterward, it can be always compiled as the last element: {a, r..., b} => {a, b, ...r}

I guess the next step could be splat in function parameters, which could compile similar as array splats:

fn = ({a, b, r...}) -> {}
fn = function(arg) {
   var a, b, r;
   ({a, b} = arg), r = Object.keys(arg)......
}

@GeoffreyBooth
Copy link
Owner

Well . . . what about function parameters, though? As we've been discussing in jashkenas#4478, parameters don't get passed through Assign.

@zdenko
Copy link
Collaborator Author

zdenko commented Apr 4, 2017

I've made attempt to enable splats in function parameters destructuring.

fn = ({a, b, r...}) -> {}
fn = function(arg) {
   var arg, a, b, r;
   ({a, b} = arg), r = Object.keys(arg)......
}

When parsing parameters I'm checking for possible splats. If they exists, I pass the object through Assign, push results to the function body and create required variables in function scope.

@zdenko
Copy link
Collaborator Author

zdenko commented Apr 6, 2017

I saw you closed this PR. Any feedback on last commits?
Should I continue with the work?
I'm almost done with the spread properties
https://github.com/tc39/proposal-object-rest-spread/blob/master/Spread.md

@GeoffreyBooth
Copy link
Owner

Sorry I didn’t intentionally close it, it got closed because my destructuring branch got merged (and then deleted). We need to change the target of this PR to the jashkenas:coffeescript 2 branch.

@GeoffreyBooth
Copy link
Owner

Looks like I can’t modify the merge target. Can you edit the PR to point to the 2 branch?

And yes please continue working, I’m eager to get this merged in! Thanks!

@zdenko
Copy link
Collaborator Author

zdenko commented Apr 6, 2017

Did you perhaps check the function parameters destructuring (the last commit)?
Is the approach acceptable?
And sorry, I wasn't paying full attention to the discussion in jashkenas#4478
Bussy days. Plus, I just dived into this without a deeper overview of CS internals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants