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

Update try-expression. #197

Closed
wants to merge 1 commit into from

Conversation

marchaefner
Copy link
Contributor

To conclude jashkenas/coffeescript#2900, this PR brings the try-expression into line with CS1:

  • Allow parameterless catch.
  • Imply catch only for a lone try clause (without finally).
  • Allow empty body in catch and finally clauses.
  • Don't allow then after try or finally.

Synchronize syntax and compilation with the original compiler:

  * Allow empty body in catch and finally clauses.
  * Allow parameter-less catch.
  * Don't allow `then` after `try` or `finally`.
  * Imply `catch` only for a lone try clause (without `finally`).
michaelficarra added a commit that referenced this pull request May 7, 2013
@michaelficarra
Copy link
Owner

Merged as 37a7cef. Thanks.

@marchaefner
Copy link
Contributor Author

Regarding the clean-up:

Something like @hasCatch/@hasFinally is necessary, since the body of catch and finally clauses may be empty (i.e. non-existent). But the absence of a body does not always coincide with the absence of the keyword.

Example (with the clean-up):

try
    do stuff
finally
    # XXX commented out by an irresponsible person
    # handle.release()

compiles to:

try {
  stuff();
} catch (e$) {
}

The last test does not work as i (naively) assumed and fails silently. Compare:

$ bin/coffee -j --cli 'try throw {} catch finally ok yes'
// Generated by CoffeeScript 2.0.0-beta4
try {
  throw {};
} finally {
  ok(true);
}

While the @has... flags are indeed quite ugly, the compiler needs those 2 bits. (And i just can't come up with a prettier solution.)

@michaelficarra
Copy link
Owner

@marchaefner: I can't seem to create a failing test case. My tests along with http://es5.github.io/#x12.14 indicate that the existence of a finally pretty much implies an empty catch.

Never mind, I was wrong.

@michaelficarra
Copy link
Owner

@marchaefner: Thanks for the persistence. I think I've fixed it with 8ec74fd. Can you try that out for me?

@marchaefner
Copy link
Contributor Author

Great. A much better solution.

There only remains the issue with a dangling finally as in the first example. (Which is easily fixed by doing the same thing in finallyClause.)

@michaelficarra
Copy link
Owner

@marchaefner: A dangling finally doesn't have any observable effects. It's dead code and should be eliminated.

@marchaefner
Copy link
Contributor Author

A dangling finally doesn't have any observable effects.

Yes, but:

try x() finally

currently compiles to

try {
  x();
} catch (e$) {
}

It should either:

  1. compile, as in CS1, to:

    try {
        x();
    } finally {
    }
    
  2. or even better to:

    x();
    
  3. or not compile at all (i.e. don't allow an empty body in finallyClause).

It's dead code and should be eliminated.

Aye (see 2nd compilation suggestion). But i guess this should be done by the optimiser.

@michaelficarra
Copy link
Owner

@marchaefner: try does imply an empty catch unless it is explicitly specified (and in CS 1.x, unless a finally is given, which is an inconsistency that must be a bug). So only the empty finally body needed to be eliminated, which it was.

@marchaefner
Copy link
Contributor Author

try does imply an empty catch

I would rather say that only a lone try (which would otherwise be an illegal construct) implies an empty catch. And from this point of view CS1s behavior is consistent, while the above compilations are quite unexpected.

@michaelficarra
Copy link
Owner

Alright, that's plausible, though I would say unlikely. I'd like to hear the opinion/wisdom of @jashkenas, @Nami-Doc, or @satyr on this.

@michaelficarra michaelficarra reopened this Jun 9, 2013
@vendethiel
Copy link
Contributor

Not sure I have much wisdom to bring, but I think that a try implies an empty catch in every case. I admit I never saw finally actually in JS. Or maybe in a keyword list ...

@vendethiel
Copy link
Contributor

Even github searching only brings keyword lists and a lot of tests.

@michaelficarra
Copy link
Owner

@Nami-Doc: We all agree on that. The question is whether a try-finally implies an empty catch (as it does without a finally) or whether it specifies that the catch is omitted. I personally think it's a bug that adding a finally would cause the catch in the output to be removed.

@vendethiel
Copy link
Contributor

I think you missed my edit (since github doesn't "refresh" posts)

but I think that a try implies an empty catch in every case.

@michaelficarra
Copy link
Owner

Okay, re-closing until someone else makes noise about this. I think we've fixed what was a bug in jashkenas/coffee-script. I'll document it along with the other intentional deviations from jashkenas/coffee-script on the wiki.

@jashkenas
Copy link

I think we've fixed what was a bug in jashkenas/coffee-script.

I'd agree. We should fix it there as well.

@jphaas
Copy link

jphaas commented Dec 1, 2013

This change will break any code that's using a try { use a resource } finally { clean up the resource } pattern by silently absorbing any errors instead of throwing them. Given that in javascript and in historical coffeescript, try...finally without a catch does not absorb errors, my guess is that this would be very surprising behavior. (I'd have to revisit all my code that uses finally statements since about half the time I deliberately omit the catch statement).

Implicitly creating catch statements seems like a dangerous default, especially since it's rarely the correct behavior to discard thrown errors without any handling at all. This seems like something javascript got right... Rather than implicitly creating catch, I'd rather a try without a catch or finally be a syntax error.

@skozin
Copy link

skozin commented Jan 9, 2014

I totally agree with @jphaas. Try..finally without a catch block is a perfectly valid construction. It is not so frequently used, but it can be very convenient when there is a need to cleanup some internal state while handling exception up the scope. That's why catch block is optional in almost any language that supports exceptions: JS, Java, C#, Ruby, Python, to name a few.

Slightly contrived example:

initializeAudioPlayback: ->
  try
    # any of these functions can throw an exception
    @checkBrowserCapabilities()
    @decodeAudioData()
    @schedulePlayback()
  catch e
    console.log('cannot initialize playback: ' + e);
    trigger 'error', e.message

checkBrowserCapabilities: ->
  try
    # try creating and manipulating DOM objects
  finally
    # remove any created objects from the DOM

# etc.

Generating implicit catch block will break any code that uses similar technique.

@michaelficarra
Copy link
Owner

@jashkenas: Were we wrong here to change this behaviour? I try to never write code like the above (mutation and complex control flow, blech), but it seems some people would find use in catch-less try-finally constructs. I think that we might want to err on the side of caution and revert the behaviour back to how it was.

@jashkenas
Copy link

some people would find use in catch-less try-finally constructs. I think that we might want to err on the side of caution and revert the behaviour back to how it was

Agreed.

@skozin
Copy link

skozin commented Jan 11, 2014

@michaelficarra, I don't think that the above example is a good code, but it is a valid use case, however. Thank you for reverting this behavior!

@michaelficarra
Copy link
Owner

Okay, after looking into this deeper, I see where the mistake was made. When @jphaas resurrected this thread, he had missed the fact that a catch is only implied for empty finally blocks. And since it was 6 months since we had last discussed this issue, I had missed it, too. I've changed the wording of the wiki page to make it clear that this is only a very specific, rare case. Closing #274, as this is definitely a bug fix.

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

Successfully merging this pull request may close these issues.

None yet

6 participants