Remove problematic Error.prepareStackTrace implementation

[murrayju]

to make sure that it never throws. Resolves #4391.

[GeoffreyBooth]

I’m not sure how to test your changes. Do you have some code that shows the previous version working as intended, that we could see still working in your revised version per this PR?

[jashkenas]

The way to fix this is not to try/catch. The way to fix it is to make coffee-script.js aware of the environment it's operating in.

[murrayju]

@GeoffreyBooth there are two existing tests in error_messages.coffee that are testing this functionality

• "patchStackTrace line patching"
• "patchStackTrace stack prelude consistent with V8"

I confirmed that modifying Error.prepareStackTrace to return garbage fails both of these tests.

@jashkenas I agree that what you suggested would be an improvement, but I think that this pull request would still be valid, since it is imperative that Error.prepareStackTrace should not throw.

[jashkenas]

It might be "imperative" — but we should never fix a thing the wrong way when it's easy to fix it the right way.

[murrayju]

@jashkenas I'm not ever opposed to doing it the "right way", but not being intimate with this project means I don't necessarily know what that would be. You say it is easy, so is this something that you plan to implement?

I could take a stab at it, but you'd have to point me in the right direction to make sure you get what you want. How would you do the environment detection (like this?)? At what level do you care about the environment (Error.prepareStackTrace vs _compileFile)? Etc...

[jashkenas]

Sure, happy to point.

From a quick glance, it looks like a lot of the source-mapping / error stuff was added pretty sloppily.

They're reusing exports._compileFile, which was only supposed to be used by register.coffee as a way to run file within Node "natively". The getSourceMap function assumes that we're running on the server.

In addition getSourceMap is recompiling files — that presumably must have already been compiled, because they're running in the process. It's sloppy in the first place, and broken in the browser.

Instead — if CoffeeScript is operating in "run" mode, we should be storing sourcemaps in memory for every script we compile, and referencing them if we need them. Not compiling a file twice, to get a sourcemap as a side effect.

[GeoffreyBooth]

@jashkenas did you intend to close this?

I agree source maps in general could be handled better, but perhaps there’s a more targeted solution that can address @murrayju’s issue without needing to refactor source maps more generally.

The way to fix this is not to try/catch. The way to fix it is to make coffee-script.js aware of the environment it’s operating in.

I gather you’re saying this because we should only be patching where we need to. Is there a way to determine if Error.prepareStackTrace requires patching other than inspecting the environment? Perhaps more to the point, should our revised Error.prepareStackTrace ever throw exceptions? Why or why not?

[jashkenas]

@jashkenas did you intend to close this?

I did — because adding a try/catch here just masks the real problem ... making it worse, not better.

Is there a way to determine if Error.prepareStackTrace requires patching other than inspecting the environment?

I don't know.

Perhaps more to the point, should our revised Error.prepareStackTrace ever throw exceptions? Why or why not?

It should definitely never throw an exception. It should also not be compiling code.

[murrayju]

adding a try/catch here just masks the real problem ... making it worse, not better

@jashkenas That depends on your definition of the problem. The problem for me is that the application is totally broken at load time. This is a major problem, not just some obscure stack trace debugging thing. The fact that Error.prepareStackTrace is throwing an exception is far worse than if the implementation were just incorrect. I am very much concerned with the unhandled exception that brings down my entire app, and far less concerned with the output of the stack trace being correct or incorrect.

Although this is due to some obscure interaction between requirejs, q, and coffee, and people are less likely to encounter it, it is still a critical issue. I think that this pull request (adding a try/catch) fixes the critical issue quickly and effectively without any negative side effects.

There is a separate, but less critical issue with this entire stack trace implementation. Unfortunately, I don't think I have the time to address this properly right now, because frankly this is not a priority for my project. I think this concern should be handled in a separate ticket.

To echo what @GeoffreyBooth said, if you are unwilling to accept the try/catch approach, what else do you propose that we can implement quickly and easily (targeted solution) to address this critical problem asap?

I also want to mention that this problem did not exist in v1.10, and was somehow introduced in v1.11.

[GeoffreyBooth]

It should definitely never throw an exception. It should also not be compiling code.

If it should never throw an exception, then what’s the problem with wrapping a try/catch block around it? Even after we refactor it so that it’s not compiling code, the try/catch could stay as a safety wrapper.

[jashkenas]

A couple of thoughts in no particular order:

• You shouldn't be running an app via coffee. Compile your files into JS first, then run them with node. Everything is much easier to work with if you have actual JS files to look at when you have runtime problems.

• If this is an emergency priority for your project right now, then by all means please add the try/catch in your fork of CoffeeScript. That's why it's open source — you can do what you like.

• The implementation of Error.prepareStackTrace is so poor that the best thing to do might just be to remove it entirely until someone has the time to implement it the right way.

• If the error didn't occur in 1.1.0, but does in 1.11 — it would be helpful, @murrayju, if you could run a git bisect search to chase down the specific commit which introduced the error. That might lead us to a more satisfying fix.

Even after we refactor it so that it’s not compiling code, the try/catch could stay as a safety wrapper.

That's just a (pretend I'm using less melodramatic, emotional words here) completely insane way to write JavaScript. try/catch in JS is already an extremely blunt, slow, difficult tool. In general, errors should only be thrown for truly exceptional situations where you want to crash the program. They should only be caught when you know how to handle them. We should absolutely never be catching them as superstition.

[murrayju]

You shouldn't be running an app via coffee

My production code does not run coffee. It is the build process that is broken by this.

in your fork of CoffeeScript. That's why it's open source — you can do what you like

Yes I do have a fork, but when pulling dependencies with npm, this involves the painful process of publishing a package with a different name... all just to add a few lines of code.

best thing to do might just be to remove it entirely until someone has the time to implement it the right way

I am 100% in favor of this. Please do this.

melodramatic, emotional words

I mostly agree with this sentiment in general, but in this particular case I'm just trying to be pragmatic. We are talking about the implementation of Error.prepareStackTrace. By definition, an exception has already occurred and it was caught, and someone called e.stack to get a stack trace. Why are you at all concerned with the efficiency of this case?

errors should only be thrown for truly exceptional situations where you want to crash the program

Right, and this is a case where you should absolutely not be crashing the program. This is like throwing in a constructor... something you should never do.

[jashkenas]

It sounds like we're in agreement — let's remove prepareStackTrace and all of the hoopla surrounding it until someone has time to do it right.

[GeoffreyBooth]

@murrayju do you mind revising this pull request to remove prepareStackTrace and its tests?

[murrayju]

Sounds good, yes I can do that.

[murrayju]

@GeoffreyBooth @jashkenas, I pushed another commit to the same branch, but it didn't show up here (presumably because the pr is closed). Do you want to re-open it, or should I make a new pr?

[GeoffreyBooth]

@murrayju Please try now.

[murrayju]

@GeoffreyBooth that worked, I see the new commit here now. @jashkenas please review!

[GeoffreyBooth]

I thought we were just removing Error.prepareStackTrace and its tests?

[murrayju]

Yes, I also removed the two helper functions that were internally scoped and not otherwise referenced.

[jashkenas]

If you make this change, it's going to piss off anyone who was relying on the stack trace ... but go for it.

[murrayju]

I actually question if this code was doing anything useful. The tests that I removed were actually still passing without the code in there, and as far as I can tell in some limited testing, the output of stack traces is unchanged.

[GeoffreyBooth]

Thanks for bearing with us @murrayju.

[murrayju]

Thanks guys. Any ETA for when this will make its way onto npm?

[GeoffreyBooth]

It’s live now.

[davibe]

Even if you don't want to run coffee program.coffee in production (I have been doing it since forever) it is still very convenient to use it during development. In that case exception with correct line number is the most important thing ever. I don't understand how one can live without it.

[GeoffreyBooth]

@davibe @murrayju if anyone would like to restore stack trace line numbers in a safe way, along the lines that @jashkenas outlined above, we would be very interested.