`throw` as expression

[satyr]

$ coffee -bpe 'ng and throw e'
Error: Parse error on line 1: Unexpected 'TERMINATOR'
...

$ coffee -bpe 'ng and (throw e)'
ng && ((function() {
  throw e;
})());

Former should be valid as well.

[TrevorBurnham]

Definitely something that should be fixed, but I'm also curious about the function wrapper in the latter. I see that ng && throw e; doesn't parse as valid JS (at least not under Node.js), but it's unfortunate to have an extra anonymous function in the output (it's a possible "huh?" in the stack trace, not to mention the extra bytes). I think this would more efficient and yield equivalent behavior:

For any expression expr,

expr and throw e

or expr and (throw e) should compile to

if (expr) { throw e; }

Analogously,

expr or throw e

or expr or (throw e) should compile to

if (!(expr)) { throw e; }

Also, if someone tries something like a and (throw e) and b, the compiler should produce an error, since anything after throw e is unreachable code.

[odf]

@TrevorBurnham: I think your suggestions only work if the values are not used.

[TrevorBurnham]

@odf If which values are not used? Could you show me an example where it wouldn't work?

[odf]

I mean if you wrote a = expr or throw e, you still wanted a to be assigned the value of expr if it is truthy, right? But you can't translate that into a = if (!expr) { throw e; }.

Or did you simply mean to say that Coffeescript should optimize the case in which the value is not used?

[TrevorBurnham]

True; when there's an assignment, and expr is a non-trivial expression (i.e. not just an identifier), one option would be to convert

a = expr() or (throw e)

to

if (__ref = expr()) { a = __ref; } else { throw e; }

This strikes me as a slight improvement in efficiency and readability over the anonymous function.

[odf]

Hmm, but then you're optimising for the exceptional case, which strikes me as counter-intuitive. The current version, a = expr() || (function() { throw e; }()); is both more readable and more efficient for the case where expr() has a non-null value.

[TrevorBurnham]

I don't find it more readable, because it makes me think "what the heck is that function doing there? I didn't define any function!" And it negatively impacts functionality, by adding an additional line to the stack trace. Observe:

obj =
  errorProneFunc: -> false or (throw new Error)
obj.errorProneFunc()

gives you

Error
    at /:6:15
    at Object.errorProneFunc (/:7:8)

I think a lot of people get turned off by CoffeeScript when they see things like that.

Also, the performance impact of an extra assignment is pretty negligible, and the if code actually minifies slightly better. And if we make the optimization for cases where the value isn't used (where it's clearly an improvement), then it seems to me that it's better to use that approach consistently. Don't you think?

[odf]

The stack trace argument convinced me. I hadn't thought of that.

[satyr]

Consider:

r = if a
  b
else
  throw e if c
  d

http://satyr.github.com/cup/#c:r%20=%20if%20a%0A%20%20b%0Aelse%0A%20%20throw%20e%20if%20c%0A%20%20d

r = if a
  b
else
  c and throw e
  d

http://satyr.github.com/cup/#c:r%20=%20if%20a%0A%20%20b%0Aelse%0A%20%20c%20and%20throw%20e%0A%20%20d

The latter is more efficient and readable (or at least concise), dontya think?

[TrevorBurnham]

Yes, in that case the latter is certainly better, since there's an anonymous function in either case.

I have another thought. Consider this example:

obj =
  foo: ->
    r = if false
      b
    else
      true and (throw new Error('message'))
      d

obj.foo()

Because the stack trace is created in the Error constructor, not by the throw, it's possible to get the desired behavior (at a small penalty in efficiency) by compiling to the equivalent of

obj =
  foo: ->
    r = if false
      b
    else
      __err = new Error('message')
      true and (throw __err)
      d

obj.foo()

Then the stack trace will show the error as occurring in obj.foo().

Unfortunately, this solution isn't always plausible; suppose, for instance, that the line were

setErrorMessage() and (throw new Error(errorMessage))

So I'm at a loss for coming up with a more general solution, but I would like to see the anonymous wrapper killed in the case where the value of the expression isn't used, e.g. foo() or throw e.

[jashkenas]

I'm afraid that "throw" is inherently a statement. There's no way that it can be used as part of a larger expression in a meaningful way. Having the syntax error is the right thing to do here.

[satyr]

Having the syntax error is the right thing to do here

Then the latter example in OP should be illegal. Reopening as bug.

[TrevorBurnham]

Yes, I'm with satyr. throw e should have the same behavior that (throw e) has now.

[zmthy]

Closed by 96b22a16eb583af6df02385a9f1b44f235511b7e. Throw cannot be directly used as an expression with parens, but can appear as a statement in an expression.

[al6x]

+1 I believe throw as an expression may have sense in some cases, like this:

success = options.success || throw('no callback!')

[marijnh]

I'd be in favour of reverting 96b22a16eb583af6df02385a9f1b44f235511b7e . Using throw as an expression is useful, as alexeypetrushin pointed out. There are other situations in which CoffeeScript adds wrapper function, and thus adds noise to the stack traces. This is really not an issue.

[jashkenas]

@marijnh: You're quite right. Reverted (by a different means) in the above commit.

[marijnh]

Thanks! The check added by the old commit is now useless, though, and should probably be removed.

[jashkenas]

Yep -- if you look at the source, you'll notice that that bit had been removed in the meantime, and Git managed to figure out that the revert was effectively a no-op.

[marijnh]

Err I found this issue by following the breadcrumbs from that very line. Seems you did revert it in 2fb6d0beb96c4ac8892fafbef4012b1f74d9eff0 , which you also just pushed. Anyway, problem solved!