Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

@func inside a class definition captures next line as argument #1903

Closed
ktusznio opened this Issue · 16 comments

7 participants

@ktusznio
class A extends B
  @methodOnB a: b
  myMethod: ->

Compiles to:

A = (function() {

  __extends(A, B);

  function A() {
    A.__super__.constructor.apply(this, arguments);
  }

  A.methodOnB({
    a: b,
    myMethod: function() {}
  });

  return A;

})();

where myMethod is passed as an argument to methodOnB rather than being declared as a function on A.

@TrevorBurnham
Collaborator

Well, it's not really because you're in a class body; it's because when you have a: b as an argument to a function on one line and c: d on the next line, the CoffeeScript compiler assumes that {a: b, c: d} is one object being passed to the function. So you get the same behavior from

@methodOnB a: b
myMethod: ->

Mind you, I agree that this is not ideal, and is more likely to happen in a class body than elsewhere. Even adding an extra line before myMethod: doesn't fix the problem; you have to use explicit curly braces around {a: b}, or explicit parentheses for the function call.

I think the best solution would be to more strict about whitespace, as is being discussed over at #1495, and require that continued object arguments be indented. So

@methodOnB a: b
myMethod: ->

would be two separate expressions, while

@methodOnB a: b
  myMethod: ->

would be equivalent to @methodOnB {a: b, myMethod: ->}. What does @michaelficarra think?

@erisdev

Ouch! This happens anywhere, not just class bodies, but only when you pass an implicit object literal.

@methodOnB a: b
myMethod: ->

becomes

this.methodOnB({
  a: b,
  myMethod: function() {}
});

but

@methodOnB a
myMethod: ->

becomes

this.methodOnB(a);

({
  myMethod: function() {}
});

I agree with @TrevorBurnham, who beat me to the punch while I was half-distractedly writing my comment, about the whitespace solution. :Ɔ

@michaelficarra
Collaborator

@TrevorBurnham: I'm not sure what to do about this. It's a grammar ambiguity, and neither option is significantly more useful or expected than the other, assuming the user is familiar with both.

@TrevorBurnham
Collaborator

@michaelficarra But do you think that requiring indentation for the continued objects would be a good idea? It's already common convention. The way I see it, when you have

*expression*
foo: bar

the newline should indicate the end of the expression, unless the last token of the expression is = or something else that expressly requires continuation.

@michaelficarra
Collaborator

@TrevorBurnham: Wouldn't that go against the expectations expressed by our users (including you!) in #1873 and #1838? It seems people expect to be able to continue lines without indentation. Would you want these kinds of argument lists disallowed as well?

$ coffee -bep 'fn a,
b, c'
fn(a, b, c);
@ktusznio

I think continuing lines without indentation is a mistake, because the usual expectation is a new expression.

@michaelficarra
Collaborator

@TrevorBurnham: It occurred to me that you may only favour allowing line continuations without indentation when the first line has a trailing operator, which would not be related to the example I gave and would be consistent with your preference here. I'd be okay with the change you suggested.

@TrevorBurnham
Collaborator

@michaelficarra Exactly.

fn a,
b: c

should be one thing;

fn a
b: c

should be another. When a line ends with , (or =, etc.), that should be the only circumstance under which the next line at the same level of indentation is considered to be part of the same expression.

@ktusznio

What about:

a = false or
true

I think that should throw a syntax error, but #1838 seems to have concluded otherwise.

@shesek

I agree with TrevorBurnham - require indentation to continue expression from the last line, unless the line ends with something that requires a continuation (which includes or IMO, @ktusznio)

edit TBH, in cases where a line requires continuation and the line following isn't indented, I think its better to just throw a parse error - imho, its always better to use indentation to indicate its a continuation of the previous line and I see no real benefit in allowing unindented lines to continue the previous line - its just confusing and harder to visually parse and understand.

However, I think TrevorBurnham suggestion is a good compromise as it only allows it in cases where an syntax error would be thrown otherwise, so it won't have any effect on developers who prefer to avoid it, while it'll still be allowed for some use-cases for those who do want to use it.

@ktusznio

I'm good with that.

@satyr
Collaborator
$ coco -bps
  @methodOnB a: b
  myMethod: ->

this.methodOnB({
  a: b
});
({
  myMethod: function(){}
});
@jashkenas
Owner

I tend to think that Coco's behavior here isn't desirable:

obj = a:b
c:d

call a:b
c:d

Into:

var obj;
obj = {
  a: b,
  c: d
};
call({
  a: b
});
({
  c: d
});
@jashkenas
Owner

I'm also afraid that @TrevorBurnham's suggestion here, which many are in agreement with, isn't consistent with the rest of the language. Objects have significant indentation, so:

a:
  b: c

Means:

{
  a: {
    b: c
  }
}

Which implies that:

call a: b
  c: d

Means:

call({
  a: b({
    c: d
  })
})

... as it does currently.

@jashkenas jashkenas closed this issue from a commit
@jashkenas Fixes #1903 af0ee70
@jashkenas jashkenas closed this in af0ee70
@jashkenas
Owner

Ok -- so I've played around with this a bit. It's part of our devil's bargain with allowing braces to be omitted from objects, and particularly allowing commas to be omitted between key:value assignments. The benefit we gain is pretty objects like:

numbers = 
  one: 1
  two: 2
  three: 3

... but the downside is that it feels unexpected that:

call one: 1, two: 2

three: 3

... was actually a single object. If you had a comma after the 2, you would expect it to be ... but without that comma, it appears is if it should be two distinct objects.

So I've changed the rewriter to make a special exception for this case. If you have an implicit object, that does not start at the beginning of the line (is being used as part of a larger expression) ... the implicit object will now more aggressively close at the end of that same line ... unless you use a comma to signal that you mean to continue the keys and values below.

There was only one test (for this very situation) that broke after making this change, and the benefit is that @ktusznio's original code now works properly.

I'm not terribly happy with this, because it's a new special case in the grammar for single-line-implicit-objects. But I think it will tend to produce the expected results. Let me know what you think.

So, now:

class Document

  @joins hasMany: Note

  find: ->
var Document;

Document = (function() {

  function Document() {}

  Document.joins({
    note: 'document_id'
  });

  Document.prototype.find = function() {};

  return Document;

})();
@satyr
Collaborator

Looks good. I think you fixed #1116 altogether.

@satyr satyr referenced this issue from a commit in satyr/coco
@satyr satyr incorporated the fix for jashkenas/coffeescript#1903 c26255b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.