Change in argument handling with 1.5 breaks code that worked in 1.4 #2715

Closed
akloster opened this Issue Feb 25, 2013 · 39 comments

Comments

Projects
None yet

I have a coffeescript codebase with ExtJs of about 3500 LOC. Installing coffeescript 1.5 completely broke it, and I have no choice but to run 1.4 until further notice.

Anyway, here is the problem. I used to do this for creating instances:

Ext.create 'Ext.Window'
  title: 'title'

This is a clear, concise syntax that makes less of a mess than you usually get in more complicated ExtJs configurations. Notice the missing comma,

Output from Coffeescript 1.4:

  Ext.create('Window', {
    title: 'title'
  });

Output from Coffeescript 1.5:

Ext.create('Ext.Window')({
  title: 'title'
});

It's no big deal really, but I'm not sure if that breakage was intentional.

There is another problem that breaks my codebase even when I add commas, but I haven't figured out what is happening there yet.

Collaborator

michaelficarra commented Feb 25, 2013

@jashkenas: These recent issues are why we shouldn't make a bunch of commits and immediately release a new version. This happens with almost every release. 😔

edit: this issue isn't the place for this

Collaborator

vendethiel commented Feb 25, 2013

wasn't that discouraged?

Sorry, I messed up the examples, I corrected that now ...
I don't want to rush anyone, but I'd like to know if there has been a policy decision about this syntax? I liked the old variation...

vendethiel referenced this issue in MayhemYDG/4chan-x Feb 25, 2013

lluchs commented Feb 25, 2013

Same thing happens when calling multiple functions, ending with an object:

@$el.html @template
  default: @options.prefill

now compiles to

this.$el.html(this.template)({
  "default": this.options.prefill
});

instead of

this.$el.html(this.template({
  "default": this.options.prefill
}));

The real issue here is if a new line should be treated like a space or like
a comma (in this case).

I would prefer to treat it like a comma, because I feel these cases are
occurring more often. Also most legacy coffeescript code relies on this
syntax.

2013/2/25 lluchs notifications@github.com

Same thing happens when calling multiple functions, ending with an object:

@$el.html @template
default: @options.prefill

now compiles to

this.$el.html(this.template)({
"default": this.options.prefill});

instead of

this.$el.html(this.template({
"default": this.options.prefill}));


Reply to this email directly or view it on GitHubhttps://github.com/jashkenas/coffee-script/issues/2715#issuecomment-14049176.

Almad commented Feb 25, 2013

While I like the new syntax more anyways, it should at least be mentioned in changelog

Welcome

Collaborator

vendethiel commented Feb 25, 2013

I don't think OP's syntax should be allowed, for consistency,a 'b' c is a syntax error.

Owner

jashkenas commented Feb 25, 2013

I agree that the OP's syntax probably shouldn't work, because of what @nami-doc says above, @lluchs' example probably should be fixed to work again.

a b
  c: d

Should compile to:

a(b({
  c: d
});

...which it currently isn't. A patch would be great.

byroot commented Feb 25, 2013

Using git bisect with this test script:

template = (context) ->
  '<html>'

update = (html) ->
  null

update template
  variable: 42

the commit that introduced the issue seems to be 21d69e3

byroot added a commit to byroot/coffee-script that referenced this issue Feb 25, 2013

Add a failing test case for multiline nested method calls #2715
e.g

```coffeescript
@$el.html @template
  variable: 42
```

It really makes your day to realize you built thousands of lines of code relying on a compiler bug.

Collaborator

vendethiel commented Feb 25, 2013

Would help to have specs i suppose

osuushi commented Feb 26, 2013

The style lluch demonstrated is definitely important. I very often use it to pass a newly constructed object to a function, like this:

petView.append new Button
    label: "Feed"
    color: "blue"
    onClick: => @feed pet

Wow just did an npm update -g and now my entire app is broken!?

No mention of this in the changelog... 😒

For now I can downgrade to 1.4, but should I start worrying about refactoring our entire app?

jkarmel commented Feb 26, 2013

This change broke my project as well, I've have examples of what both @akloster @lluchs in my code base.

pke commented Feb 26, 2013

Whether or not that was a compiler bug in the first place, this change now breaks most of our code.
This should at least generate a compiler error now or reverted. I prefer the comma-less version.

punund commented Feb 26, 2013

+1. I too had to fix it at 1.4.0 due to this silent behavior change.

👍 Our code was also broken today. We use @lluchs' example in our code quite a bit.

kavu commented Feb 26, 2013

"Glad" to see I am not alone. I am fixing on 1.4.0 now.

This may also be pertinent to this case:
xkcd workflow

jzorn commented Feb 26, 2013

+1

Collaborator

michaelficarra commented Feb 26, 2013

Okay, enough with the +1s. We get it, it's broken and the OP isn't the only one experiencing it. Please, from here on, only constructive input. Stick on 1.4 for now or use CoffeeScriptRedux which works.

Collaborator

vendethiel commented Feb 26, 2013

Collaborator

michaelficarra commented Feb 26, 2013

@nami-doc: CoffeeScript is not Coco/LiveScript which distinguish between f 'a' b and a = 'a'; f a b. That is valid JavaScript being generated. My compiler can't generate invalid JavaScript.

Collaborator

vendethiel commented Feb 26, 2013

I know Redux has no ACI, but this is invalid javascript -- string is not a function. And I just wanted to point out that Redux does not parse OP's case like coffee 1.4 did

Collaborator

michaelficarra commented Feb 26, 2013

@nami-doc: It is valid javascript, review the spec. It obeys the grammar. It is parsed correctly by any JS parser.

I believe OP's case was agreed to be invalid because it's inconsistent with the more basic case:

a
  b: c

It was only working in 1.4.0 because this compiler refuses to parse strings in function call position, so it fell back to something that was actually undesirable and inconsistent. The real issue here is the one that was mentioned by @lluchs:

a b
  c: d
Collaborator

vendethiel commented Feb 26, 2013

Spec#9.11, Table 16, IsCallable String : false.
I'm talking about this piece generated by my link : a('b'({ c: d }));

Collaborator

michaelficarra commented Feb 26, 2013

@nami-doc: Let's take this off this thread. I'll meet you in #coffeescript.

edit: log from #coffeescipt: http://irclogger.com/.coffeescript/2013-02-26#1361890009

@neumino neumino referenced this issue in rethinkdb/rethinkdb Feb 26, 2013

Closed

Web ui breaks with CoffeeScript 1.5 #380

@peacemaker11 The example was intentionally short. My normal usecase has 10-100 lines in the config object below that, often nested multiple levels.

you means like that http://goo.gl/xPgf0

pulse00 commented Feb 28, 2013

It would be great if the changelog would reflect this issue, as this might save some people a lot of debugging time.

Owner

jashkenas commented Feb 28, 2013

This should be fixed as of #2712, correct? Not for the OP's specific use case, but for all the other valid ones. Anyone with a bit of trouble in this thread want to give master a try and confirm that it works for them?

I can confirm that master/#2712 fixes the issues for me, although there was once instance of the OP's problem that caused a syntax issue this time (Unexpected INDENT) instead of compiling.

@jashkenas jashkenas closed this Feb 28, 2013

pulse00 commented Mar 2, 2013

Not for the OP's specific use case, but for all the other valid ones

@jashkenas: 2 questions:

First Question: Was the OP`s specific use case

Ext.create 'Ext.Window'
  title: 'title'

and does your answer imply that this was not valid coffeescript in everything < 1.5, meaning applications which
relied on that syntax will now need to refactor to

Ext.create 'Ext.Window' {
  title: 'title'
}

Second Question: What are the other valid use cases?

byroot commented Mar 2, 2013

Why not refactor into:

Ext.create 'Ext.Window',
  title: 'title'

(note the comma)

Because the code you proposed is still invalid.

Collaborator

michaelficarra commented Mar 2, 2013

@pulse00: even your "refactor"ed example is not what you intended, you need the comma in both cases.

pulse00 commented Mar 3, 2013

@byroot @michaelficarra you're right, my example was wrong. I was having the use case in mind we've stumbled upon after upgrading to 1.5.0.

someThing = new SomeThing
  foo: bar
  bar: foo

Which worked before 1.5.0 and broke with 1.5.0. I've tested this with master and it now has the behaviour like the pre 1.5.0 version.

Anyway, after reading the whole thread multiple times, i'm still not sure if there was a bug in pre 1.5.0 compiler or not, like @akloster mentioned a couple of comments above.

Collaborator

troels commented Mar 3, 2013

@pulse00 That which you have there should compile to:

someThing = new SomeThing({foo: bar, bar: foo});

and if it wasn't (as it may have failed to in 1.5.0), then that was a bug. That bug will certainly be fixed in 1.5.1, which I think will arrive within a few days. OP, was for a slightly different case which is not as clear-cut, and can't really be supported.

I suppose you can call it a bug that OP's code didn't break pre-1.5.0, but it was never explicitly supported nor mentioned in any documentation.

Feel free to test current master on your codebase. It's very close to being 1.5.1. If you should find any regressions we will very likely have them fixed for 1.5.1 :)

Collaborator

vendethiel commented Mar 3, 2013

Allowing STRNUM INDENT implicit object was the bug in the first place -- it was said it shouldn't have compiled

The rest is just, well, that coffee does not have tests for as simple cases as that (as "#{1/2}")

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