michaelficarra / CoffeeScriptRedux

:sweat: rewrite of the CoffeeScript compiler with proper compiler design principles and a focus on robustness and extensibility
https://michaelficarra.github.com/CoffeeScriptRedux/
BSD 3-Clause "New" or "Revised" License
1.84k stars 110 forks source link

Is this a reportable issue? #45

Open ghost opened 12 years ago

ghost commented 12 years ago

There's a lot of probably bad conventions of gotten used to being able to do in coffee that I'm not even sure are supposed to be ok in coffeescript, but since it was the first syntax errors I encountered in my own code with this project I thought I'd ask:

something = () -> 1
something
  a: 2
.something 3

error

Is this something worth reporting at this point? Is this project's goal to handle all issues like this, or was that bad coffeescript?

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

mark-hahn commented 12 years ago

I think you need to show a better example. Not only would that example give a run-time error but I can't think of why anyone would code something so wrong.

vendethiel commented 12 years ago

jQuery code, for example ? ;)

connec commented 12 years ago

That kind of syntax was fantastic with jQuery:

$('<a/>').css
  'color':           'blue'
  'text-decoration': 'underline'
.appendTo 'body'

==

$('<a/>').css({
  'color': 'blue',
  'text-decoration': 'underline'
}).appendTo('body');
ghost commented 12 years ago

Thanks connec - that's more accurate to the way I'm actually doing it, I was just trying to provide a really simple example :)

There's a few modules/libraries (jQuery, fs.createWriteStream pipe, underscore) that use "chaining" (is that the right word?) like that, and the habit of being able to put a period at the start of the line for those chains, separating them onto separate lines makes them easy for me personally to read/undertsand. I'm sure it's wrong as mark-hahn said, so I was really hoping to ask more generally if coffeescriptredux intends to support those wrong things that folks like me are doing :)

michaelficarra commented 12 years ago

After looking into this syntax, I've come to the conclusion that this was not an intentional feature. At the end of a function call, an implicit object may be given on the next line, specifying that it is to be used as the last argument. You shouldn't then be able to chain to that result without using parentheses to explicitly specify the target.

Instead, use

(($ '<a/>').css
  color: 'blue'
  'text-decoration': 'underline'
).appendTo 'body'

or

$('<a/>').css({
  color: 'blue'
  'text-decoration': 'underline'
}).appendTo 'body'

or

(($ '<a/>').css color: 'blue', 'text-decoration': 'underline').appendTo 'body'
epidemian commented 12 years ago

Hmm... i feel this is yet another case for a nice chaining syntax. For example, this is valid in LiveScript:

$ '<a/>'
  .css color: 'blue', 'text-decoration': 'underline'
  .appendTo 'body'

However, it seems this is not supported:

$ '<a/>'
  .css 
    color: 'blue'
    'text-decoration': 'underline'
  .appendTo 'body'

A proper chaining syntax does not belong to this issue, and it has been discussed many times before, but i just wanted to comment that this might be another case for it :)

vendethiel commented 12 years ago

satyr/coco#64

connec commented 12 years ago

I'd be very disappointed if this didn't make it into Redux, clean chaining syntax like this is a pretty big deal for myself, and I expect others, who regularly use jQuery and other similar APIs and wish to fully utilise CoffeeScript's clean, low-parenthesis syntax.

Also, from what you've said ("specifying that it is to be used as the last argument"), would the following also not be intentional:

some_func
  object: 'argument'
, another_param
michaelficarra commented 12 years ago

@connec: You've got it. I would run screaming from code written like that.

mark-hahn commented 12 years ago

Putting a comma in the front of a line is the standard way to ensure it is treated as a new argument. You can't pull that.

FWIW, I'm a bit concerned that redux is pulling a lot of useful features when it was supposed to be compatible.

On Tue, Sep 18, 2012 at 3:50 PM, Michael Ficarra notifications@github.comwrote:

@connec https://github.com/connec: You've got it. I would run screaming from code written like that.

— Reply to this email directly or view it on GitHubhttps://github.com/michaelficarra/CoffeeScriptRedux/issues/45#issuecomment-8673599.

connec commented 12 years ago

@michaelficarra: OK how about (I can't run the Redux compiler on this system, sorry):

setTimeout ->
  console.log 'Look ma! No brackets!'
, 10

Not supporting these syntaxes could break a significant amount of production code...

michaelficarra commented 12 years ago

@mark-hahn: That is also a concern of mine. I really do not want this compiler to change the CoffeeScript language or break our users' production CoffeeScript code. But there has to be a line we draw between intentional features and quirks. I am not duplicating quirks. The original compiler was very, very permissive, allowing unintentional and inconsistent syntax.

mark-hahn commented 12 years ago

I am not duplicating quirks.

One man's meat is another man's poison. And it isn't just quirks. Requiring parens where they weren't needed before is taking the language backwards, not matter how quirky it is. If you do that you are not going to be very popular. Sorry if this comes across as harsh.

BTW, some of these issues fall under the category of line continuations. Line continuations are and always have been really screwed up in CS. I can find a blog for you that lists many of the problems. I have had to resort to just using \ at the end of the line for almost every continuation.

I'm not suggesting a continuation fix, just stating the problem.

On Tue, Sep 18, 2012 at 4:11 PM, Michael Ficarra notifications@github.comwrote:

@mark-hahn https://github.com/mark-hahn: That is also a concern of mine. I really do not want this compiler to change the CoffeeScript language or break our users' production CoffeeScript code. But there has to be a line we draw between intentional features and quirks. I am not duplicating quirks. The original compiler was very, very permissive, allowing unintentional and inconsistent syntax.

— Reply to this email directly or view it on GitHubhttps://github.com/michaelficarra/CoffeeScriptRedux/issues/45#issuecomment-8674046.

ScatteredRay commented 11 years ago

So, I'm not sure what the decided action is on this sort of thing, however it seems to me that:

$('<a/>').css
  'color':           'blue'
  'text-decoration': 'underline'
.appendTo 'body'

is entirely non-ambiguous and likely used all over production code. IMO, I think the initial release of CoffeeScriptRedux is probably not the place to be making choices that significantly change the semantics of the languages for items that seem to be already well specified, even if a bit clunky, or cumbersome.

Now, I only stand behind the argument so long as this sort of thing is both unambiguous and well specified, but it does seem so to me.

mark-hahn commented 11 years ago

+1 It would be a step backwards to lose this construct.

On Sun, Dec 16, 2012 at 2:46 AM, Arelius notifications@github.com wrote:

$('').css 'color': 'blue' 'text-decoration': 'underline' .appendTo 'body'

delaaxe commented 11 years ago

-1 I find those constructs completely unreadable for anyone not expert in CoffeeScript (like other employees, which I'm trying to convince to use CoffeeScript). Less parens are counter productive IMO

ScatteredRay commented 11 years ago

@delaaxe I'd tend to agree, in fact I feel any instance of implicit parens are error-prone and counter-productive.

But this is about the fact that this is a compiler for an existing, and reasonably well used existing language. With what's quite possibly hundreds of thousands of existing lines of code, many of which use these more poorly specified quirks of the language. And it's awfully naive to assume that the community is going to manually rewrite thousands of lines of code for dubious gains.

Frankly disallowing this sort of code is the place of a strict-mode, or perhaps even something much more like coffeelint. Or perhaps this could be a valid approach, if someone wrote a source rewriting tool to perform these modifications automatically, however the problem with that, is first a parser needs to exist that can fully understand legacy CoffeeScript, and also do source location tracking. Yet since one of the major goals behind the Redux rewrite was to implement that behavior, that legacy parser is clearly not to come from the mainline codebase.

epidemian commented 11 years ago

While i agree with the general sentiment in that it would be cool to have a more clear way to express chaining (i.e. chaining special syntax), i also agree with @mark-hahn in this one, forbidding these constructs altogether will break lots of today-legal code.

This a sample snippet from a Stack Overflow answer i saw today:

items
  .map (item) ->
    item.price * item.quantity
  .reduce (x, y) ->
    x + y

These constructs are valid in today's CS and lots of people are very used to using them. It's hard to imagine someone fixing their codebase all over the place just to support a new compiler. That's why i think it would be cool if Redux supported this arguably "bad" syntax constructs.

mark-hahn commented 11 years ago

this arguably "bad" syntax constructs.

It may be arguable but I'd take the side of the argument saying it is a very "good" construct. I find it very readable and noise-free.

On Wed, Dec 19, 2012 at 8:23 AM, Demian notifications@github.com wrote:

items .map (item) -> item.price * item.quantity .reduce (x, y) -> x + y

asalant commented 11 years ago

+1 We use this style of chaining extensively. Chaining is encouraged by libraries like jQuery and underscorejs so I expect many coffeescript users are using chaining sugar like this.

neersighted commented 11 years ago

:+1:

hden commented 11 years ago

The current Redux allows multi-lined chaining

chart = d3.select('#graph')
.append('svg')
.attr('width', width)
.attr('height', height)

There are lots of codes such as this one below, and I would say that there is no confusion what the programmer's intensions are. Like @epidemian said, lots of people have to adjust their styles if Redux drop this.

chart = d3.select('#graph')
          .append('svg')
          .attr('width', width)
          .attr('height', height)

Sometimes unintended creations works as well, no?

devongovett commented 11 years ago

Yep, just tried to compile a pretty big CoffeeScript app with Redux and ran into this. CoffeeScript mainline has always been a bit fiddly with chaining, but this always worked:

a.b()
 .c()

That now throws an error in Redux but not the original compiler. Redux does allow you to chain without the spaces in front of the c() call, but that doesn't look as nice as when the dots all line up nicely. Chaining is very common in JavaScript and CoffeeScript code so I think it's important that Redux keeps production code working properly.

michaelficarra commented 11 years ago

I'll be fixing this issue as soon as #112 is finished.

Message to self: #79 is outdated, but it has some nice tests for this.

kisPocok commented 11 years ago

Vote up for task :+1: thanks your work michaelficarra!

dbrans commented 11 years ago

Any update on this?

michaelficarra commented 11 years ago

@dbrans: see the roadmap, my comment above, and the indented-member-access branch.

dbrans commented 11 years ago

Awesome, thanks.

mzedeler commented 11 years ago

+1 to this issue. Very happy that it has been left open and keep up the good work.

myrne commented 11 years ago

I stumbled upon this compatibility issue a few days ago, while exploring good ways to deal with promises in CoffeeScript.

Currently, CoffeeScript allows me to do the following:

getSomePromise(arg)
.then (result) ->
  console.log result
.then null, (error) ->
  console.error error

or

getSomePromise(arg)
  .then (result) ->
    console.log result
  .then null, (error) ->
    console.error error

Both compile to the same JavaScript. Indentation doesn't matter, as long as the "dot-calls" are indented the same. If one would indent the second .then to the right, then the .then would be applied to the result expression.

I think syntax in both cases is clear, and the results not surprising or quirky. I can't judge other implications of the parsing logic currently enabling this. If this an accident, it's a good accident, and must be kept. I could live with rules becoming stricter, in the sense that indentation used would matter. Either first or second example could be a parse error for all I care, potentially reserved for a different meaning later, or just for enforcing consistent indentation in source code.

Note that I use a second .then as a trick to prevent need for parentheses. Extended exposure to CoffeeScript has made me develop an allergy to parentheses. (I do realize that using a second .then is not equivalent to just a single then, but because how errors propagate forward, it's equivalent in practice if code in first .then callback does not throw an error.)

Also, note that if one would do getSomePromise arg, then CoffeeScript would apply the first .then to the arg expression, (regardless of indentation of .then). For my purposes, I'd prefer if the first line would be taken as a "finished" expression, with .then applied to it afterwards. But I can certainly live with current state. Others might not like the implications of my preference.

michaelficarra commented 11 years ago

@meryn: This is already pretty much fixed. I just need to finish up the WIP commit on the indented-member-access branch. I've just been working on more fulfilling OSS projects recently.

myrne commented 11 years ago

Great to hear!

I've just been working on more fulfilling OSS projects recently.

What's fulfilling for you? Do you think this (CSR dev) is an "unthankful" job, or does it not excite you technically anymore?

Commonjs-everywhere is certainly cool. Do you think you could explain somewhere how it compares to browserify? I have relied on simple concatenation of plain compiled scripts up to now, and certainly would appreciate the low-down on these matters. Also because I'm looking to open source more of my work, and therefore need to decouple stuff more. CommonJS modules are a very alluring target. Although I may in the end prefer something with asynchronous requires (e.g. AMD/RequireJS) so not all code has to be loaded at once.

michaelficarra commented 11 years ago

Do you think this (CSR dev) is an "unthankful" job, or does it not excite you technically anymore?

No, not thankless. It's just that the remaining things to do (super, splicing, this issue, a few obscure bugs) just don't ever affect me. Also, they're not particularly challenging. I'm using the compiler without any problems, so it's hard to get motivated.

With commonjs-everywhere, I truly feel I've created the best bundler available for modules that conform to the CommonJS spec. Thanks for the suggestion, I will write up a comparison between commonjs-everywhere and similar bundlers. I'm extremely proud of it :smile:.

myrne commented 11 years ago

It's just that the remaining things to do (super, splicing, this issue, a few obscure bugs) just don't ever affect me. Also, they're not particularly challenging.

I'd contribute if I'd understand. I really don't grok the whole thing. I wish I would. I don't know where to start really. What I'm thinking of now, is that perhaps it would be educative for me to see a full blown example different components used in a very minimal example.

Could you for example, explain in words to me what you have done, and what still needs to be done for this particular feature? Perhaps I could help then. For me it's a learning opportunity, for you it's having something a whole lot smarter than a computer at your disposal, although I'm not as good in crunching numbers.

If you do, please keep in mind that I know next to nothing about compilers.

myrne commented 11 years ago

So if stack trace line numbers work correctly, the failing test is

    eq nonce, o
      .a()
        .b
          .c

I approach this quite naively. Is it even asked for that it passes? Who asks for that this indentation style is supported?

myrne commented 11 years ago
    eq nonce, o
    .a()
    .b
    .c

apparently passes, as does

    eq nonce, o
      .a()
      .b
      .c

Well if I'm not mistaken, that were the two things I asked for. If I see these other types of indentation, I wouldn't even want that to be valid coffeescript. I even have doubts on supporting these two styles above.

myrne commented 11 years ago

In particular this:

    eq nonce, o
      .a()
        .b
      .c

As I see coffeescript, indentation has meaning. Here, because it's supposed to be equivalent to a consistently indented piece of code, it seems as though the indentation has become meaningless. Is that desirable?

To me, it seems as though this member access thing would be one of the few cases (the only case?) where I could arbitrarily indent my code. And yes, if CoffeeScript 1.6 currently supports this, I'd consider this an undesirable thing. It does enforce consistent indentation in other places.

myrne commented 11 years ago

All examples I see in this thread use consistent indentation. Some indeed want to indent the lines, to match up with an earlier dot for example.

vendethiel commented 11 years ago

@meryn Yes, they work, because OUTDENT DOT is a specialcase.

myrne commented 11 years ago

Thanks for chiming in Nami Doc. How do you feel about that being the case?

vendethiel commented 11 years ago

I agree this should work :). Actually, we have a PR for that : #79.(not sure what "chiming in" means)

myrne commented 11 years ago

My way to make a consistent rule about this would be this: Consider every "expression" on a line an "invitation" to start a new "block" (just like if expression is such an invitation). Any block that follows an invitation for a block must be indented relative to the previous block. Perhaps some (many) people out there currently don't indent their "member accesses" blocks. Or let's call them "dot-blocks". Complain loudly in the next release. Deprecated. Next release after that: kill it.

myrne commented 11 years ago

Nami: " to break into a conversation or discussion especially to express an opinion" (never thought I'd be teaching anyone English, lol).

myrne commented 11 years ago

Nami: You think this inconsistent indenting should be allowed?

myrne commented 11 years ago

I'm talking about this specific syntax:

    eq nonce, o
      .a()
        .b
      .c

not this

eq nonce, o
      .a()
      .b
      .c

the last example is a consistently indented "dot-block".

myrne commented 11 years ago

Examples here https://github.com/michaelficarra/CoffeeScriptRedux/issues/121 also have consistently indented dot-blocks.

vendethiel commented 11 years ago

Yes, I think it should. I think jashkenas said it was "taking indentation to the next level" - can't find the issue right now, I must be tired.

    eq nonce, o
      .a()
        .b
      .c

This is the only discutable case, imho.

(Sorry 'bout the english bit, I'm french kiddie, so my english is rather poor :p.)

michaelficarra commented 11 years ago

And yes, if CoffeeScript 1.6 currently supports this, I'd consider this an undesirable thing. It does enforce consistent indentation in other places.

@meryn: I agree, but it seems a lot of others don't. The CoffeeScript language isn't exactly consistent. It is not designed to be a good language or to be liked by PLT enthusiasts, but instead to be accepted by the masses, who often don't know what's best for them. That's why I started coffee-of-my-dreams.

vendethiel commented 11 years ago

Ah, I found it back. I couldn't find it because, well, it's from HN, not coffee issues. (as Coffee's Thunder-rolling-janitor, I still claim I can find any issue :p).

myrne commented 11 years ago

Nami: Wait, what exactly are you arguing for? You don't like the back-and-forth indenting, it seems. Well me neither, but I don't like the "run-away" indenting either. Normally, indenting means something. It means a change in something. Here, it changes nothing. Or at least, that's what I'm discussing now. :) Maybe

    eq nonce, o
      .a()
        .b
          .c

should mean something, but I don't want it to mean the same as

eq nonce, o
      .a()
      .b
      .c

Do you get what I mean by "dot-blocks". Blocks are things that have the same indenting, and therefore belong to each other. You do "if something", change indenting, do a few things, go back (outdent), and then you're in the original indenting. Back in original "context". dot-blocks are special blocks that all start with a dot, and are intended for multi-line member-access. The point is that you can't arbitrarily change indenting: You'd leave the block. That's a nice model I think.