Fix #4558: Stack trace line numbers for scripts that compile CoffeeScript

[GeoffreyBooth]

Fixes #4558. Also fixes the cake release command, which broke somewhere around when Node 8 came out.

Feedback welcome. So basically the issue in #4558 is that when you have a CoffeeScript file that itself has CoffeeScript = require 'coffeescript' and then something like CoffeeScript.compile, there’s basically a script within a script here and we need to preserve the source maps within the source maps, so that stack trace line numbers work correctly. Unfortunately the child scripts are all cached as <anonymous>, and the runtime always throws errors referencing the filename of the top-level script, so there’s some dark arts involved in picking which source map to use for patching the error stack trace. The method I’ve chosen isn’t foolproof, if you have multiple source maps that happen to have identical code or mappings, but it should work most of the time. I don’t see how to get a foolproof method without the runtime providing the full source of the “file” throwing the error, which it doesn’t (as far as I can tell). There’s more gory detail in the comments of the PR.

[lydell]

So the change of this PR is basically:

• Handle <anonymous> differently.
• Allow to fallback to recompiling in more cases.

I guess that's pretty harmless? Not sure if the approach is fully correct, but if it works better than before I guess it's fine.

I'm not sure I understand completely the issue, the obstacles and the solution, but I don't think there's any harm merging this.

[jashkenas]

Yeesh this looks a little hacky. Can we not correctly associate the scripts with the maps by maintaining a correct lookup somehow?

[GeoffreyBooth]

That’s kind of what’s happening now. The issue is that there are multiple maps per file when a file runs CoffeeScript.compile or run etc. For example if this is test.coffee:

console.log 'Hello!'

CoffeeScript.run '''
    CoffeeScript = require './lib/coffeescript
    CoffeeScript.compile ''
    throw new Error 'Some Error'
  '''

This generates three source maps, all under test.coffee: the entire file, the string passed to .run, and the string passed to .compile. Hence the sources and sourceMaps objects now are in the form where the key is test.coffee and the value is an array of these three sources/source maps. The issue is that when an exception is thrown, the browser just tells us that it’s from test.coffee—there’s no way to know which one of these three the browser refers to.

There is a method called frame.getFunction().toString() that gives us the source of the function containing the exception, but it’s not always available. I thought about using it to get the source and searching through the test.coffee sources array to find the first one that had the source of the function in question, which still wouldn’t be foolproof if the “child scripts” had the same source code for that function, but I found getFunction too unreliably available to use that as a solution.

So instead of that, I search through the array of source maps to find the first one that has a matching source map for the exception. This is potentially even less foolproof, as source maps could easily have overlapping matches (especially 1:1) but it was the best option I could find. There doesn’t seem to be a rule that can be followed reliably like ”exceptions are always from the newest source/source map at the end of the array.”

If you can think of a better solution, please suggest it. I agree that a hacky, non-foolproof matching of source maps line/column pairs is far from ideal, but it’s better than returning NaN and it seems to be the best we can do as far as I can tell.

[GeoffreyBooth]

So what do we think? @jashkenas or @lydell, is this okay to merge in?

If you think of a better solution for the line numbers problem than this one, we can always submit a new PR later.

[lydell]

• Does this cause a risk that code that doesn't have something like CoffeeScript.compile in it (which is the by far common case) get wrong numbers?
• According to CLI: Line numbers shown as NaN in stack trace when compiling CoffeeScript inside CoffeeScript #4558 the numbers used to be correct. Do we know why they became NaN? If they could be correct without hacks before, why can't they now?

I don't doubt that you've found the best solution that can be thought of for the moment, but it would be nice to clear any confusion up before merging :)

[GeoffreyBooth]

Does this cause a risk that code that doesn’t have something like CoffeeScript.compile in it (which is the by far common case) get wrong numbers?

I hope not! We do have tests for that, though.

If they could be correct without hacks before, why can’t they now?

They weren’t always correct before. If the exception was thrown in the topmost script (i.e. not a string passed to compile or run etc.) then yes, in 1.12.1 there were line numbers and after #4428 there weren’t. But if a child or grandchild etc. script (a string passed to compile etc.) threw an exception, its exception line numbers would be incorrect. After this PR both should be correct.

Do we know why they became NaN?

So there’s a pretty long, tortured history.

• In 1.12.1 and earlier, we patched Error.prepareStackTrace to return line numbers based on the CoffeeScript source, but the implementation was problematic. Specifically, if an exception was thrown, the compiler took the filename from the exception and used fs to reopen the file, recompiled it to get a source map, and then used that source map to revise the stack trace. This didn’t work at all in a browser environment, and had other issues. Hence . . .
• In 1.12.2 we removed our patched Error.prepareStackTrace entirely. Line numbers in stack traces corresponded to the original runtime line numbers, e.g. the JavaScript source line numbers.
• In 1.12.3 we restored our patched Error.prepareStackTrace, per Fix stack trace #4428. This version added caching for sources and source maps, to avoid the file reopen on exception. However the caching had an edge case issue discussed above, in that multiple files with the same filename (or multiple files called <anonymous>) would overwrite each other’s cached sources/source maps, which led to the issue CLI: Line numbers shown as NaN in stack trace when compiling CoffeeScript inside CoffeeScript #4558 that this PR is meant to fix. When a source map couldn’t be found in a cache, or it didn’t match the stack trace, NaN was the result. (This is the simplified version. You can read through the diffs of Fix stack trace #4428 and this PR for the details.)

[lydell]

IMO, we're good to go then.

[GeoffreyBooth]

If we want to replicate the 1.12.1 behavior but keep the caching, we could cache the first appearance of each filename but ignore any subsequent appearances. That would be equivalent to the file-open approach, but it would share the bug with that version of incorrect line numbers of child scripts. This bug is more important than it might seem, because lots of our own tests are compilations of child scripts.

[jashkenas]

Just throwing out thoughts here -- but what if we stopped using the filename as the key to the source map lookup, and instead used something more robust: like the SHA of the source code to-be-compiled?

[GeoffreyBooth]

I thought of that. Unfortunately the issue is that when the runtime throws the exception, all it supplies is the filename, not the source. We have only a filename to go on to find the source/source map.

[jashkenas]

Ah, I see. In that case your fix looks perfect.

That said, it seems like with any engine providing source map support, there really ought to be a way to link the specific JS to the map directly.

[GeoffreyBooth]

Yes. It seems pretty clear that the designers of the JavaScript stack trace never imagined that there might be multiple different sources with the same filename (or that someone might be rewriting their stack trace, for that matter).