Incorrect comprehension compilation in 1.2 #2207

Closed
topfunky opened this Issue Mar 21, 2012 · 13 comments

Projects

None yet

5 participants

Previous to 1.2, this was possible:

toJSON: ->
  dishes: dish.toJSON() for dish in @dishes
  price: @totalPrice()

Now, one must use do for the comprehension as the value of dishes:

toJSON: ->
  dishes: do => dish.toJSON() for dish in @dishes
  price: @totalPrice()

The current compiler compiles the first version to return dishes: and the value of the comprehension. It also emits a separate return for price, which seems incorrect.

This seems like a bug to me. The first code snippet seems like valid CoffeeScript and should compile so the method returns an object with dishes and price as keys.

Is this intentional or is it in fact a bug?

That's interesting. It should also work with just braces, rather than do, as in:

toJSON: ->
  dishes: ( dish.toJSON() for dish in @dishes )
  price: @totalPrice()

It looks like the compiler is instead seeing:

toJSON: ->
  ( dishes: dish.toJSON() ) for dish in @dishes
  price: @totalPrice()

...which is producing the weirdest chunk of code.

Explicitly starting the object seems to work too:

toJSON: ->
  {
    dishes: ( dish.toJSON() for dish in @dishes )
    price: @totalPrice()
  }
showell commented Mar 21, 2012

I'm not sure if the change is intentional, but I can see why there could be many competing interpretations of this type of syntax:

    item: item.method() for item in items

A newbie could reasonably expect the compiler to create a hash comprehension there (aka "dictionary comprehension" in Python). Of course, that's not what the compiler is doing there either, so I'm not defending the compiler.

Regardless of the CS version, I think parens would make your code more readable here, especially to folks with a Python background, who are inclined to think that the colon binds tightly by default:

>>> print {i : chr(65+i) for i in range(4)}
    {0 : 'A', 1 : 'B', 2 : 'C', 3 : 'D'}

http://www.python.org/dev/peps/pep-0274/ (dictionary comprehensions)

I'm not sure dictionary comprehensions have ever been proposed for CoffeeScript.

Collaborator

Comprehensions have a really low precedence for a reason. Remember, it's even lower than assignment. Should it be higher than : in an implicit object? I'm not sure. Probably. I don't think it's very common to want to create a list of same-keyed objects, so there shouldn't be a need to wrap the comprehension in parens here. +1 for now.

@showell

I'm not sure dictionary comprehensions have ever been proposed for CoffeeScript.

Plenty of times, actually, but #77 is the open issue.

showell commented Mar 21, 2012

Another interesting quirk of the syntax is that the below code parses to reasonable JS code:

toJSON: ->
  dishes: (for dish in @dishes then dish.toJSON())
  price: @totalPrice()

You have to put the parens there, though, otherwise you get the cryptic UNEXPECTED '}' syntax error, and the below code won't compile at all:

toJSON: ->
  dishes: for dish in @dishes then dish.toJSON()
  price: @totalPrice()
Collaborator

@showell:

the below code won't compile at all:

toJSON: ->
  dishes: for dish in @dishes then dish.toJSON()
  price: @totalPrice()

That's definitely a bug.

showell commented Mar 21, 2012

@michaelficarra I agree. It's nice that the code fails explicitly, but the compiler should handle that.

showell commented Mar 21, 2012

Just for the record, I don't support dictionary comprehensions in CS. I'm not sure how I even feel about them in Python. Just pointing out that the CS interpretation is gonna be a gotcha for Python people.

Fascinating conversation! I'll use parens for now.

Owner

I'm afraid that the current compilation looks correct to me. For consistency, we made the change that:

key = value for value in list

means:

(key = value) for value in list.

So it follows that this works the same way:

key: value for value in list.

... the rest of this behavior follows naturally from there. Either parentheses around the comprehension or an explicit object are both fine ways of disambiguating what you meant.

@jashkenas jashkenas closed this Apr 25, 2012
Collaborator

@jashkenas: See @showell's comment above. There's certainly a bug here somewhere. This isn't ambiguous, but fails:

a: for b in c then d

With explicit braces, it works:

{a: for b in c then d}

Re-opening.

Owner

Ah, thank you -- looking.

@jashkenas jashkenas closed this in 6bcc798 Apr 25, 2012
Owner

Feel free to double check me. I added a rule that if an implicit end appears as the immediate next value following the : in an implicit object, it is ignored.

Collaborator

That sounds like the rule I would have added.

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