Possible bug with dot at beginning of line, ternary and object

[tremby]

1.7.0 added "Leading . now closes all open calls, allowing for simpler chaining syntax."

This is great, but I may have just stumbled on a bug with that when using the ternary operator and an object argument.

gulp.task 'stylus', ->
    gulp.src [
        './src/stylus/vars.styl'
        './src/stylus/**/*.styl'
    ]
        .pipe if argv.sourcemaps then require('gulp-sourcemaps').init() else gutil.noop()
        .pipe require('gulp-concat') 'styles.styl'
        .pipe require('gulp-stylus')
            use: require('nib')()
        .pipe if isLocal() then gutil.noop() else require('gulp-minify-css')
            compatibility: '' # Empty string is IE9+ (this is default)
        .pipe if argv.sourcemaps then require('gulp-sourcemaps').write() else gutil.noop()
        .pipe gulp.dest './dist/css'

This is coming out as

// Generated by CoffeeScript 1.9.2
gulp.task('stylus', function() {
  return gulp.src(['./src/stylus/vars.styl', './src/stylus/**/*.styl']).pipe(argv.sourcemaps ? require('gulp-sourcemaps').init() : gutil.noop()).pipe(require('gulp-concat')('styles.styl')).pipe(require('gulp-stylus')({
    use: require('nib')()
  })).pipe(isLocal() ? gutil.noop() : require('gulp-minify-css')({
    compatilibilty: ''
  }).pipe(argv.sourcemaps ? require('gulp-sourcemaps').write() : gutil.noop())).pipe(gulp.dest('./dist/css'));
});

I was expecting

• compatibility: '' object to be an argument to require('gulp-minify-css') -- in this case the result is as expected.
• the dot at the start of the next line to close both the ternary and the pipe() from the preceding line -- in this case the result is not as expected: the next .pipe() is chained on the end of the gulp-minify-css object.

Note that the gulp-stylus pipe above also receives an object as an argument but does not have a ternary. This one compiles as I expect.

Note also that the gulp-sourcemaps pipes have ternaries but no object argument, and these ones also compile as I expect.

So it seems it's a combination of the tenary and the object argument on a new line.

Is this a bug or am I understanding something incorrectly?

[GeoffreyBooth]

There are many ways to confuse the compiler with leading periods. Please use parentheses to make your intentions clear:

gulp.task 'stylus', ->
    gulp.src([
        './src/stylus/vars.styl'
        './src/stylus/**/*.styl'
    ])
    .pipe if argv.sourcemaps then require('gulp-sourcemaps').init() else gutil.noop()
    .pipe require('gulp-concat') 'styles.styl'
    .pipe(require('gulp-stylus')(
        use: require('nib')())
    ).pipe(if isLocal() then gutil.noop() else require('gulp-minify-css')
        compatibility: '' # Empty string is IE9+ (this is default)
    ).pipe if argv.sourcemaps then require('gulp-sourcemaps').write() else gutil.noop()
    .pipe gulp.dest './dist/css'

[tremby]

That's a shame. What's the point in the feature if you can't use it? Or what's the rule on when it can be used and when it can't?

[GeoffreyBooth]

It works well when there aren’t indented lines inside the chaining (use and compatibility in your case). Those seem to confuse the compiler. Perhaps there is a way to disambiguate those, but I get the sense that there isn’t, because this has come up many times before. We probably should’ve disallowed leading period chaining in such scenarios, but it’s been around for so long now that doing so would break lots of code. If I’m wrong and there is a way to make the compiler do what you want, please submit a PR and I’d be happy to accept it.

It’s similar to how sometimes parentheses are required for function calls, for example if you have other code afterward on the same line. When you chain like this, you’re really creating one long line of code; so sometimes parentheses are required to disambiguate.

So to answer your question: use chaining when each line starts with a period. If you need a multiline block in one of the parts of the chain, wrap that block in parentheses (like I did for gulp-stylus above).

[connec]

So it seems it's a combination of the tenary and the object argument on a new line.

I agree. Perhaps the logic for closing implicit calls for leading .s won't continue through ternaries?

[tremby]

Would it help (and make sense? it would to me) to interpret deeper indentation on a following line as a continuation, versus equal (or lesser) indentation as the end of the preceding function call?

Having said that, my own example would break that rule; the first .pipe would then be called on the preceding array, I think. But if this line and succeeding ones were unindented one step it would be clear and unambiguous with that rule, as far as I can see.

[GeoffreyBooth]

Chaining is supposed to be unindented:

$('#button')
.show()
.hide()
.css('color', 'red')
.show()

Which is why it was surprising that your example worked at all.

[tremby]

Does it actually make a difference to the parser at present? If the next line is starting with a ., and is at the same indentation level, or deeper, is there any difference in the interpretation?

I always presumed not, and liked to indent chaining lines as an extra visual clue that they are a continuation of something earlier. (And it always worked for me when I expected it to -- or almost always.) But I have no problem changing the way I write if it would disambiguate and help in keeping code and diffs terse (read: fewer parens).

So perhaps the code example in this thread should now be this:

gulp.task 'stylus', ->
    gulp.src [
        './src/stylus/vars.styl'
        './src/stylus/**/*.styl'
    ]
    .pipe if argv.sourcemaps then require('gulp-sourcemaps').init() else gutil.noop()
    .pipe require('gulp-concat') 'styles.styl'
    .pipe require('gulp-stylus')
        use: require('nib')()
    .pipe if isLocal() then gutil.noop() else require('gulp-minify-css')
        compatibility: '' # Empty string is IE9+ (this is default)
    .pipe if argv.sourcemaps then require('gulp-sourcemaps').write() else gutil.noop()
    .pipe gulp.dest './dist/css'

[lydell]

Chaining is supposed to be unindented

I've never heard about chaining being "supposed" to be in any particular way. All CoffeeScript I've ever written has used indented chaining (because that's what I'm used to from JavaScript). I don't think anyone knows the exact rules. What makes things harder is that the compiler has always been a little too permissive and since CoffeeScript has been around for so long now, it's hard to tell intended behavior from accidental. CoffeeScriptRedux tried to define sane rules for stuff like this, but people have come to depend on accidental quirks (which is one reason people sadly had problems adopting CSR).

[jashkenas]

Chaining like this was not supposed to be supported in the first place. Originally, a leading . on a subsequent line was supposed to to connect to the immediately preceding expression — no closing of implicit parens or braces, period. The point was to avoid the ambiguities.

But that got changed when the PR got merged years ago.

[GeoffreyBooth]

Well that’s what happens when we don’t document a feature 😄

Its only documentation is in the changelog for 1.7.0: http://coffeescript.org/v2/#1.7.0, and there it’s unindented. I had always assumed the unindentation was intentional, since indentation implies a new block, and chaining most emphatically is not a new block—it’s really a single line of code. But clearly the compiler allows either.

We could fix this as part of the breaking changes for 2, but I don’t think we really gain anything other than intellectual cohesiveness, at the expense of lots of people needing to revise their code for no real reason.

[tremby]

But would it help in the context of this thread? Could it make it clear to the compiler what my intended meaning was? See the "I was expecting" list from my original post.

I am increasingly getting the impression that this indentation conversation is actually not relevant to the bug(?) I was pointing out. I think the real issue is that there's something awry when a ternary and object argument are given as part of chains. The dot at the start of the line is supposed to close all preceding function calls, and in this case this is not happening.

[GeoffreyBooth]

Related: #3866.

[GeoffreyBooth]

Here’s a minimal example:

a
.b if yes then 1 else c
  d: 1
.e()

currently compiles to:

a.b(true ? 1 : c({
  d: 1
}).e());

should compile to:

a.b(true ? 1 : c({
  d: 1
})).e();

[xixixao]

This is a bug, and should work regardless of indentation (I would prefer to enforce that the chain call be indented, but I guess a lot of ppl don't indent, including @GeoffreyBooth ).

As the documentation says:

Leading . now closes all open calls, allowing for simpler chaining syntax.

And clearly .b above is an open call.

I would guess that #3866 is a separate issue.

[GeoffreyBooth]

@xixixao When the feature was introduced, its only documentation was in the changelog (for 1.7.0) and that example didn’t indent. I assumed therefore that that was the only possible syntax, and that indentation would be invalid (as indentation implies a new block, but chaining is really a continuation of the previous line, certainly not a new block).

At this point we’re stuck supporting both variants, since there’s a lot of code out there that uses one or the other style; but based on the documentation and the general meaning of indentation, the non-indented style should be the only accepted variant; or at least the default variant for purposes of documentation and testing.

[xixixao]

@GeoffreyBooth Yeah, I have no clue why I didn't indent that example (I have a hunch I took it over from somewhere, but I might be completely wrong). Maybe I'm skewed by over a year of writing in ES6, but I would definitely encourage everyone to indent the method calls (i.e. we should change that example).

[GeoffreyBooth]

Again, we’re stuck supporting both styles now. But I think the example in the docs should stay as it is (and move into the regular documentation, not just in the changelog). Indentation in CoffeeScript represents a new block; indenting something and not having a new block form generally should throw an unexpected indentation error from the compiler. It’s way too late to fix that now for this feature, but if we were being intellectually coherent that’s how it should’ve behaved back when it was introduced.

I understand that the indented style looks better, and that’s why it’s popular in JavaScript; but we have significant indentations here.