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.

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

Jump to bottom

[CS2] Destructuring #4478

Merged
merged 21 commits into from
Apr 6, 2017
Merged

[CS2] Destructuring #4478

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
59959a6
Output simple array destructuring assignments to ES2015
Mar 30, 2017
b9f8f5d
Output simple object destructured assignments to ES2015
Mar 30, 2017
066071f
Compile shorthand object properties to ES2015 shorthand properties
Mar 30, 2017
2e0dab3
Compile default values in destructured assignment to ES2015
Apr 1, 2017
a196614
Rename `wrapInBraces` to `wrapInParentheses`, and `compilePatternMatc…
GeoffreyBooth Apr 2, 2017
b11ba7f
Don’t confuse the syntax highlighter
GeoffreyBooth Feb 5, 2017
c5f7279
Comment Assign::compilePatternMatch a bit
lydell Feb 5, 2017
93c8f8e
Assignment expressions in conditionals are a bad practice
GeoffreyBooth Feb 9, 2017
8840aa9
Optional check for existence that only checks `!== undefined`, not `!…
GeoffreyBooth Mar 29, 2017
7786899
The fallback destructuring code should apply default values only if `…
GeoffreyBooth Apr 2, 2017
1e6d910
Add comments; remove unnecessary array splats in function tests
GeoffreyBooth Apr 2, 2017
6c814a0
Support destructuring in function parameters (first pass); catch dest…
GeoffreyBooth Apr 2, 2017
5fc4e19
Destructured variables in function parameter lists shouldn’t be added…
GeoffreyBooth Apr 2, 2017
770ddcd
Remove redundancy in undefined-only check for existence; fix passing …
GeoffreyBooth Apr 2, 2017
d110b9a
Fix undefined redundancy
GeoffreyBooth Apr 2, 2017
678e931
Simplify getting the variable name
GeoffreyBooth Apr 2, 2017
60f046e
Reimplement “check for existence if not undefined” without creating a…
GeoffreyBooth Apr 2, 2017
05c1f46
`Obj::isAssignable` should not mutate; pass `lhs` property in from `A…
GeoffreyBooth Apr 2, 2017
923b931
Merge branch '2' into destructuring
GeoffreyBooth Apr 3, 2017
bd8cf29
Revert changes to tests
GeoffreyBooth Apr 5, 2017
7e81fa0
Restore revised test for empty destructuring assignment
GeoffreyBooth Apr 5, 2017
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
2 changes: 2 additions & 0 deletions src/nodes.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1765,6 +1765,8 @@ exports.Assign = class Assign extends Base
return compiledName.concat @makeCode(": "), val

answer = compiledName.concat @makeCode(" #{ @context or '=' } "), val
# Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Assignment_without_declaration,
# if we’re destructuring without declaring, the destructuring assignment must be wrapped in parentheses.
if o.level > LEVEL_LIST or (isValue and @variable.base instanceof Obj) then @wrapInParentheses answer else answer

# Brief implementation of recursive pattern matching, when assigning array or
Expand Down
6 changes: 3 additions & 3 deletions test/assignment.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ test "simple array destructuring defaults", ->
[a = 2] = [undefined]
eq 2, a
[a = 3] = [null]
eq null, a
eq null, a # Breaking change in CS2: per ES2015, default values are applied for `undefined` but not for `null`.
[a = 4] = [0]
eq 0, a
arr = [a = 5]
Expand All @@ -315,7 +315,7 @@ test "simple object destructuring defaults", ->
{b = 2} = {b: undefined}
eq b, 2
{b = 3} = {b: null}
eq b, null
eq b, null # Breaking change in CS2: per ES2015, default values are applied for `undefined` but not for `null`.
{b = 4} = {b: 0}
eq b, 0

Expand All @@ -324,7 +324,7 @@ test "simple object destructuring defaults", ->
{b: c = 2} = {b: undefined}
eq c, 2
{b: c = 3} = {b: null}
eq c, null
eq c, null # Breaking change in CS2: per ES2015, default values are applied for `undefined` but not for `null`.
{b: c = 4} = {b: 0}
eq c, 0

Expand Down
4 changes: 2 additions & 2 deletions test/functions.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,13 +150,13 @@ test "@-parameters and splats with constructors", ->
eq b, obj.last

test "destructuring in function definition", ->
(([{a: [b], c}]...) ->
(({a: [b], c}) ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did we need to remove these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So there was a period when I was struggling to get splatted arrays to work as function parameters, and so I modified these tests. Later on I added an exception for this case that just dumped such cases back into the legacy compilation, so now we could revert these test changes. But I think we shouldn’t, because the splat-array wrapper feels like a workaround. The version without the wrapper looks to me much more like something someone would intentionally write.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is acceptable. There are lots of things in programming languages that people never write, but still need to be supported for consistency.

eq 1, b
eq 2, c
) {a: [1], c: 2}

context = {}
(([{a: [b, c = 2], @d, e = 4}]...) ->
(({a: [b, c = 2], @d, e = 4}) ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

also here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These tests pass with or without this change. I just thought the revised version was a more realistic test. We could test both versions if you want, but the []... syntax is still tested in the #4005: `([a = {}]..., b) ->` weirdness test, so I’m not sure we need to test it here too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So @lydell, do you want me to revert these tests? They pass either way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

They've been like this since 2010. Let's keep them that way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay.

eq 1, b
eq 2, c
eq @d, 3
Expand Down