Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incompatible code generated with 1.12.6 using function call followed by if statement. #4568

Closed
jo-soft opened this issue Jun 12, 2017 · 9 comments · Fixed by #4598
Closed
Labels

Comments

@jo-soft
Copy link

jo-soft commented Jun 12, 2017

Until version 1.12.5 the code foo bar if baz transpiled to if(baz){foo(bar)}
since 1.12.6 this code transpiles to foo(baz ? bar: void 0) which breaks old code.

But when i use mood = greatlyImproved if singing (from docs) it compiles to if (singing) {mood = greatlyImproved;}
Is this indented? Maybe by 'Bugfixes for incorrect code generated by the ? operator within a termary if statement`? I cant find any Bug report for that.

bg,
Johannes

@GeoffreyBooth
Copy link
Collaborator

Are you sure?

@connec
Copy link
Collaborator

connec commented Jun 12, 2017

Perhaps you somehow triggered it being parsed as foo(bar if baz)? It would help if you could paste a full example that's reproducible in Try CoffeeScript (or a link).

@jo-soft
Copy link
Author

jo-soft commented Jun 13, 2017

bigger example of code failing in 1.12.6 bug working in 1.12.5 and lower:

test:
  bam: () ->
    fn bar: {
      foo: 123
    } if not condition

results in 1.12.6 (example generated by: http://coffeescript.org/#try)

({
  test: {
    bam: function() {
      return fn({
        bar: !condition ? {
          foo: 123
        } : void 0
      });
    }
  }
});

results in 1.12.5. (example generated by js2.coffee:

({
  test: {
    bam: function() {
      if (!condition) {
        return fn({
          bar: {
            foo: 123
          }
        });
      }
    }
  }
});

both examples can also locally be reproduced by transpiling locally

@vendethiel vendethiel reopened this Jun 13, 2017
@vendethiel
Copy link
Collaborator

Paging @helixbass since it might be from #4554 (?).

@helixbass
Copy link
Collaborator

@vendethiel not a bad guess but #4554 was only merged into 2

@GeoffreyBooth
Copy link
Collaborator

It’s from #4532, allowing implicit braces after a return or export default. Basically, in this code:

test:
  bam: () ->
    fn bar: {
      foo: 123
    } if not condition

it’s ambiguous what the if block should be applied to. “Trailing if“ is only meant for a one-line expression; if anything, we should be throwing an error here. How far back is this if supposed to look to determine whether something should be output? Just { foo: 123 }? Or { bar: { foo: 123 } }? Or fn { bar: { foo: 123 } } or return fn { bar: { foo: 123 } }?

If you insist on a trailing if, make it explicit:

test:
  bam: () ->
    (fn bar: {
      foo: 123
    }) if not condition

I don’t think there’s a bug here. Yes the output changed, but it’s as a result of fixing another bug regarding implicit braces and return. Basically the output here was relying on that bug to create the desired effect.

@GeoffreyBooth
Copy link
Collaborator

Actually it's probably due to #4534. But my point still stands.

@vendethiel
Copy link
Collaborator

I don't think we should be changing such semantics on 1.x.

@GeoffreyBooth
Copy link
Collaborator

Yeah we shouldn’t have merged #4534 into master (assuming that is, in fact, the cause of this). My argument is less valid against that PR than it is against fixing braces after return, because braces after return is more clearly a bug whereas #4534 adjusts behavior to the way it “should” be.

So I guess the question is what to do about it. We’re keeping #4534 on 2, so it seems odd to revert it on master just to keep it in 2. I was hoping to avoid any more 1.x releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants