Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Implement #156, Added expansion to destructuring #3268

Merged
merged 1 commit into from Jan 24, 2014

Conversation

Projects
None yet
9 participants
Contributor

xixixao commented Dec 1, 2013

Implements #156 (oldest open issue currently). Namely @epidemian's proposal (same as #870).

[first, middle..., last] = array # works today
[first, ..., last] = array # works with this PR

[..., last] = ratherLongVariableName
last = ratherLongVariableName[ratherLongVariableName.length - 1] # rather long
[..., last] = a
last = a[a.length - 1] # still longer, repetitive 

This matches nicely the ability of taking first n elements out of an array.

[first, ..., last] = tuple # first = tuple[0], last = tuple[tuple.length - 1];

edited to match final PR

Contributor

davidchambers commented Dec 1, 2013

[first, middle..., last] = array # works today
[first, .., last] = array # works with this PR

It seems to me the elision should be represented by ... rather than .., to match middle....

Collaborator

michaelficarra commented Dec 1, 2013

I'm pretty sure we decided against this in #870, opting to recommend [first, [], last] = array. Also, for pulling off the last element without mentioning the list more than once, I usually use something like this: generateList()[-1..][0]. It's not pretty, but it works.

Collaborator

lydell commented Dec 1, 2013

@michaelficarra you mean [first, []..., last] = array, right? Your code means [first, [], third] = array.

Also note that {} can be used in place of []: [first, {}..., last] = array, [first, {}, third] = array.

Contributor

xixixao commented Dec 1, 2013

@davidchambers I used .. because ... is already used for splats, and this is something different. Plus it's shorter.

@michaelficarra generateList()[-1..][0] is awefully inefficient (I have seen the code around as well, such strong is the urge of not repeating the array). This PR can compile straight to array[array.length - 1]. I think #870 got side tracked by talking about empty assignments.

@lydell You are correct, but the compiled code is still inefficient:

[first, []..., last] = a # first = a[0], 3 <= a.length ? __slice.call(a, 1, _i = a.length - 1) : (_i = 1, []), last = a[_i++];
#vs
[first, .., last] = a # first = a[0], last = [a.length - 1]

We could open up an issue for the compilation and fix it, but imho this syntax is utterly ugly and non-intuitive. Mind that #870 did not offer the change as a solution to #156. @jashkenas ?

Collaborator

vendethiel commented Dec 1, 2013

Seems to me that splat should work here, without the need for [] before (coco allows that). Can you explain why you think it's different ?

Contributor

xixixao commented Dec 1, 2013

@nami-doc Because splat never stands on its own, it always serves as either assignment target or application.

(a, splat...) -> # splat is assigned
fn splat... # splat is applied
[.., last] = a # do not assign the values until the last one
               # "stretch the empty space" or "shift the assigned index towards end" 

I think this should be completely disallowed, see the nonsensical output:

[[]...] = a # var __slice = [].slice; 1 <= a.length ? __slice.call(a, 0) : [];

I see how splats make sense coming from that syntax, but I think given no one actually knows it, it doesn't make the syntax more intuitive. But if that's the only problem stopping us, happy to go for one more dot.

erisdev commented Dec 1, 2013

@xixixao there's actually a problem with your proposed "efficient" compilation here. What happens when a isn't at least two elements?

a = ['first']
[first, .., last] = a # first = a[0], last = [a.length - 1]

first # => 'first'
last # => 'first'

Even crazier hypothetical case:

a = []
[first, ..., last] = a

first # => undefined
last # => ???

OK, in this case, last would be undefined, but if for some ungodly reason there were a -1 property assigned elsewhere… This is more of an obscure corner case that should never happen, but it makes a good demonstration I think.

Contributor

xixixao commented Dec 1, 2013

@erisdiscord
The first is a good catch. I think both compilations make sense. We could check for the length in such case. Or we could leave it as it is, after all, what is the last element in the array a?

I don't think the second is a problem:

a = []
a[-1] = 'I am a devil'
last = a[a.length - 1] # well, that really isn't our fault
Contributor

xixixao commented Dec 15, 2013

I have more time now again, so I could finish this up. Do we want this? @nami-doc ?

@vendethiel vendethiel and 1 other commented on an outdated diff Dec 15, 2013

src/nodes.coffee
@@ -1236,12 +1237,21 @@ exports.Assign = class Assign extends Base
val += ") : []"
val = new Literal val
splat = "#{ivar}++"
+ else if not expansion and obj instanceof Expansion
@vendethiel

vendethiel Dec 15, 2013

Collaborator

maybe we should check here for multiple expansions. Minor nitpick though ...

@xixixao

xixixao Dec 15, 2013

Contributor

I just copied the way Splats are handled. Would you change both? (I don't remember whether there's a reason for the deferral of the error to the last block).

Collaborator

vendethiel commented Dec 15, 2013

I still believe ... makes more sense than ..; but I'm not one to decide

Collaborator

lydell commented Dec 17, 2013

I like this, because the current way is just a hack. Compare the following two:

[first, []..., last] = array
# Destructure `array`. Save its first value into `first`. Soak up the second til the next-to-last
# value (if any) into an array, but don't save it to a variable. Instead, immediately
# destructure that array. However, don't save any of its values to any variable. Which is like
# skipping to the last value of the initial array. Save that value to `last`. Phew!
[first, .., last] = array
# Desctructure `array`. Save its first value in `first`. Skip to the last value. Save that value to
# `last`. Done.

Moreover, it provides a solution to that old "I wish array[-1] returned the last element of the array" problem. Instead of using first = array[0] and last = array[-1], we can use the much nicer [first] = array and [.., last] = array, or together: [first, .., last] = array.

As for .. vs ...: Doesn't matter to me.

My vote would be for three dots. In my head, three dots is "ellipsis/splat/soak", and two dots is "go back a level".

Contributor

xixixao commented Dec 19, 2013

@bjmiller Why "go back a level"? @lydell That's exactly my thinking.

So I polished this up, fixed error handling and optimized the simple compilation:

[first, .., last] = tuple # first = tuple[0], last = tuple[tuple.length - 1];

@jashkenas ?

erisdev commented Dec 21, 2013

@xixixao because of POSIX paths I'd assume

@xixixao: @erisdiscord is correct. Ever since my very first Unix login long ago, ".." has been burned into my brain as the way to go up the directory tree.

Contributor

xixixao commented Dec 21, 2013

Ah, I even had alias ..="cd .." in bash (fish doesn't even need it). I don't think that's really relevant here though (like the meaning of square brackets say).

In CS, two dots currently have two meanings: [a..b] inclusive range and [a..] expands range to the end. The second one is where I get my reasoning for two dots, but it really doesn't matter much, I like it for being shorter.

@xixixao xixixao referenced this pull request Dec 24, 2013

Closed

release 1.6.4 #3141

Owner

jashkenas commented Jan 24, 2014

My thoughts: If y'all want this, it should definitely be three dots instead of two — an ellipsis is a real thing. And it should work as a standalone token in any reasonable circumstance. That means the here-suggested:

[a, ..., b] = list

... as well as things like this:

func = (first, ..., last) ->

func = (..., last) ->

... and so on.

Another thing to think about, syntactically, is the strangeness / ugliness of an ellipsis followed by a comma, or preceded by one: , ..., as part of your line of code. One possible alternative is allowing the commas to be dropped:

[a ... b] = list

func = (first ... last) ->

Just a notion.

Contributor

epidemian commented Jan 24, 2014

@jashkenas,

Another thing to think about, syntactically, is the strangeness / ugliness of an ellipsis followed by a comma, or preceded by one: , ..., as part of your line of code.

I think that notation is pretty natural, at least in a mathematical context. For example, take a look at some of the sequences mentioned in the Wikipedia article for sequence:

For instance, the sequence of the first 10 square numbers could be written as

😺

Contributor

xixixao commented Jan 24, 2014

[first, second...butOne, last] = a
[first, second, ..., butOne, last] = a

Latter reads better for me. Secondly, as I mentioned, it would be really hard to parse if it coincided with ranges.

Collaborator

vendethiel commented Jan 24, 2014

Agreed, I'd really prefer if we kept the comma mandatory

Owner

jashkenas commented Jan 24, 2014

Agreed, I'd really prefer if we kept the comma mandatory

Ok. Sounds good. Is it ready to merge, then?

@vendethiel vendethiel commented on an outdated diff Jan 24, 2014

src/nodes.coffee
@@ -1206,7 +1206,7 @@ exports.Assign = class Assign extends Base
vvar = value.compileToFragments o, LEVEL_LIST
vvarText = fragmentsToText vvar
assigns = []
- splat = false
+ expandedIdx = false
@vendethiel

vendethiel Jan 24, 2014

Collaborator

if we can't keep the align, we probably should keep only one space

Collaborator

vendethiel commented Jan 24, 2014

@jashkenas If it works with argument lists as you said, I'd say yes (once the sigil is fixed). Glad to see that in

Contributor

xixixao commented Jan 24, 2014

Ready to squash @nami-doc ?

@vendethiel vendethiel and 1 other commented on an outdated diff Jan 24, 2014

src/nodes.coffee
if p.this then p = p.properties[0].name
if p.value then o.scope.add p.value, 'var', yes
splats = new Assign new Value(new Arr(p.asReference o for p in @params)),
new Value new Literal 'arguments'
break
for param in @params
+ debugger
@vendethiel

vendethiel Jan 24, 2014

Collaborator

whoops ;)

@xixixao

xixixao Jan 24, 2014

Contributor

Ha :)

@vendethiel vendethiel and 1 other commented on an outdated diff Jan 24, 2014

test/functions.coffee
@@ -178,6 +178,16 @@ test "default values with splatted arguments", ->
eq 1, withSplats(1,1,1)
eq 2, withSplats(1,1,1,1)
+test "#156: parameter lists with expansion", ->
+ a = (first, ..., lastButOne, last) ->
+ eq 1, first
+ eq 4, lastButOne
+ last
+ eq 5, (a 1, 2, 3, 4, 5)
+
+ throws (-> CoffeeScript.compile "(..., a, b...) ->"), null, "prohibit expansion and a splat"
+ throws (-> CoffeeScript.compile "(...) ->"), null, "prohibit lone expansion"
@vendethiel

vendethiel Jan 24, 2014

Collaborator

I'd maybe add one other case containing both ((..., [...b]) ->. Not sure though ...

@xixixao

xixixao Jan 24, 2014

Contributor

Great catch.

Contributor

xixixao commented Jan 24, 2014

Now should be ready.

jashkenas added a commit that referenced this pull request Jan 24, 2014

Merge pull request #3268 from xixixao/issue156
Implement #156, Added expansion to destructuring

@jashkenas jashkenas merged commit daa6ad5 into jashkenas:master Jan 24, 2014

Collaborator

michaelficarra commented Jan 24, 2014

What are we going to do about the single-argument-skipping syntax? Just yesterday I had to use this ugly hack:

[[], commit, timestamp, author] = stdout.match /([A-F\d]{40}) (\d+) ([^@\s]+@[^@\s]+)/i
Owner

jashkenas commented Jan 24, 2014

What are we going to do about the single-argument-skipping syntax?

Just name it?

Collaborator

michaelficarra commented Jan 24, 2014

Gross. That makes it look like you mean to use it later, and will cause linting tools to (perfectly legitimately) complain about an unused variable.

Contributor

xixixao commented Jan 24, 2014

I agree that is ugly, and in future I'd like to make [] and {} and their splats not assignable. Also I missed out a case, so there is a sibling PR coming.

Btw I had a similar case as Michael's above when writing the new error logic in coffee-script.coffee.

Collaborator

michaelficarra commented Jan 24, 2014

@xixixao: Do you mind opening an issue for it and summarising the opinions presented within the now closed #870? I'd rather not lose track of it, especially before it ever had a dedicated issue.

Contributor

xixixao commented Jan 24, 2014

Feel free to go ahead, I might get to it later but 1.7.0 is a priority now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment