From 44a27c62044b51b072944835bc40080c9a4a62aa Mon Sep 17 00:00:00 2001 From: Geoffrey Booth Date: Wed, 23 Aug 2017 06:50:46 -0700 Subject: [PATCH] Fix #4558: Stack trace line numbers for scripts that compile CoffeeScript (#4645) * Don't throw an error in the console when loading a try: URL * Handle the possibility of compiling multiple scripts with the same filename, or multiple anonymous scripts * Fix #4558: Much more robust caching of sources and source maps, more careful lookup of source maps especially for CoffeeScript code compiled within a Coffee script (. . . within a Coffee script, etc.) * Reimplement `cake release` to just use the shell to avoid the issues plaguing that command (something to do with module caching perhaps) --- Cakefile | 14 ++++--- docs/v2/index.html | 2 +- documentation/v2/docs.coffee | 2 +- lib/coffeescript/coffeescript.js | 67 ++++++++++++++++++++++++-------- src/coffeescript.coffee | 54 +++++++++++++++++-------- test/error_messages.coffee | 27 ++++++++++--- 6 files changed, 121 insertions(+), 45 deletions(-) diff --git a/Cakefile b/Cakefile index 1aeb02f0e2..0307b6bf0f 100644 --- a/Cakefile +++ b/Cakefile @@ -341,11 +341,15 @@ task 'doc:source:watch', 'watch and continually rebuild the annotated source doc task 'release', 'build and test the CoffeeScript source, and build the documentation', -> - invoke 'build:full' - invoke 'build:browser:full' - invoke 'doc:site' - invoke 'doc:test' - invoke 'doc:source' + execSync ''' + cake build:full + cake build:browser + cake test:browser + cake test:integrations + cake doc:site + cake doc:test + cake doc:source''', stdio: 'inherit' + task 'bench', 'quick benchmark of compilation time', -> {Rewriter} = require './lib/coffeescript/rewriter' diff --git a/docs/v2/index.html b/docs/v2/index.html index a04749adac..a65f43c740 100644 --- a/docs/v2/index.html +++ b/docs/v2/index.html @@ -4682,7 +4682,7 @@

else initializeScrollspyFromHash window.location.hash # Initializing the code editors might’ve thrown off our vertical scroll position - document.getElementById(window.location.hash.slice(1)).scrollIntoView() + document.getElementById(window.location.hash.slice(1).replace(/try:.*/, '')).scrollIntoView() diff --git a/documentation/v2/docs.coffee b/documentation/v2/docs.coffee index 4ed13f3539..07b2cc752a 100644 --- a/documentation/v2/docs.coffee +++ b/documentation/v2/docs.coffee @@ -125,4 +125,4 @@ $(document).ready -> else initializeScrollspyFromHash window.location.hash # Initializing the code editors might’ve thrown off our vertical scroll position - document.getElementById(window.location.hash.slice(1)).scrollIntoView() + document.getElementById(window.location.hash.slice(1).replace(/try:.*/, '')).scrollIntoView() diff --git a/lib/coffeescript/coffeescript.js b/lib/coffeescript/coffeescript.js index e5eecf4f98..6d383e9220 100644 --- a/lib/coffeescript/coffeescript.js +++ b/lib/coffeescript/coffeescript.js @@ -4,7 +4,8 @@ // on Node.js/V8, or to run CoffeeScript directly in the browser. This module // contains the main entry functions for tokenizing, parsing, and compiling // source CoffeeScript into JavaScript. - var Lexer, SourceMap, base64encode, checkShebangLine, compile, formatSourcePosition, getSourceMap, helpers, lexer, packageJson, parser, sourceMaps, sources, withPrettyErrors; + var FILE_EXTENSIONS, Lexer, SourceMap, base64encode, checkShebangLine, compile, formatSourcePosition, getSourceMap, helpers, lexer, packageJson, parser, sourceMaps, sources, withPrettyErrors, + indexOf = [].indexOf; ({Lexer} = require('./lexer')); @@ -21,7 +22,7 @@ // The current CoffeeScript version number. exports.VERSION = packageJson.version; - exports.FILE_EXTENSIONS = ['.coffee', '.litcoffee', '.coffee.md']; + exports.FILE_EXTENSIONS = FILE_EXTENSIONS = ['.coffee', '.litcoffee', '.coffee.md']; // Expose helpers for testing. exports.helpers = helpers; @@ -67,10 +68,10 @@ // a stack trace. Assuming that most of the time, code isn’t throwing // exceptions, it’s probably more efficient to compile twice only when we // need a stack trace, rather than always generating a source map even when - // it’s not likely to be used. Save in form of `filename`: `(source)` + // it’s not likely to be used. Save in form of `filename`: [`(source)`] sources = {}; - // Also save source maps if generated, in form of `filename`: `(source map)`. + // Also save source maps if generated, in form of `(source)`: [`(source map)`]. sourceMaps = {}; // Compile CoffeeScript code to JavaScript, using the Coffee/Jison compiler. @@ -93,7 +94,10 @@ generateSourceMap = options.sourceMap || options.inlineMap || (options.filename == null); filename = options.filename || ''; checkShebangLine(filename, code); - sources[filename] = code; + if (sources[filename] == null) { + sources[filename] = []; + } + sources[filename].push(code); if (generateSourceMap) { map = new SourceMap; } @@ -158,7 +162,10 @@ } if (generateSourceMap) { v3SourceMap = map.generate(options, code); - sourceMaps[filename] = map; + if (sourceMaps[filename] == null) { + sourceMaps[filename] = []; + } + sourceMaps[filename].push(map); } if (options.inlineMap) { encoded = base64encode(JSON.stringify(v3SourceMap)); @@ -311,17 +318,43 @@ } }; - getSourceMap = function(filename) { - var answer; - if (sourceMaps[filename] != null) { - return sourceMaps[filename]; - // CoffeeScript compiled in a browser may get compiled with `options.filename` - // of ``, but the browser may request the stack trace with the - // filename of the script file. + getSourceMap = function(filename, line, column) { + var answer, i, map, ref, ref1, sourceLocation; + if (!(filename === '' || (ref = filename.slice(filename.lastIndexOf('.')), indexOf.call(FILE_EXTENSIONS, ref) >= 0))) { + // Skip files that we didn’t compile, like Node system files that appear in + // the stack trace, as they never have source maps. + return null; + } + if (filename !== '' && (sourceMaps[filename] != null)) { + return sourceMaps[filename][sourceMaps[filename].length - 1]; + // CoffeeScript compiled in a browser or via `CoffeeScript.compile` or `.run` + // may get compiled with `options.filename` that’s missing, which becomes + // ``; but the runtime might request the stack trace with the + // filename of the script file. See if we have a source map cached under + // `` that matches the error. } else if (sourceMaps[''] != null) { - return sourceMaps['']; - } else if (sources[filename] != null) { - answer = compile(sources[filename], { + ref1 = sourceMaps['']; + // Work backwards from the most recent anonymous source maps, until we find + // one that works. This isn’t foolproof; there is a chance that multiple + // source maps will have line/column pairs that match. But we have no other + // way to match them. `frame.getFunction().toString()` doesn’t always work, + // and it’s not foolproof either. + for (i = ref1.length - 1; i >= 0; i += -1) { + map = ref1[i]; + sourceLocation = map.sourceLocation([line - 1, column - 1]); + if (((sourceLocation != null ? sourceLocation[0] : void 0) != null) && (sourceLocation[1] != null)) { + return map; + } + } + } + // If all else fails, recompile this source to get a source map. We need the + // previous section (for ``) despite this option, because after it + // gets compiled we will still need to look it up from + // `sourceMaps['']` in order to find and return it. That’s why we + // start searching from the end in the previous block, because most of the + // time the source map we want is the last one. + if (sources[filename] != null) { + answer = compile(sources[filename][sources[filename].length - 1], { filename: filename, sourceMap: true, literate: helpers.isLiterate(filename) @@ -340,7 +373,7 @@ var frame, frames, getSourceMapping; getSourceMapping = function(filename, line, column) { var answer, sourceMap; - sourceMap = getSourceMap(filename); + sourceMap = getSourceMap(filename, line, column); if (sourceMap != null) { answer = sourceMap.sourceLocation([line - 1, column - 1]); } diff --git a/src/coffeescript.coffee b/src/coffeescript.coffee index bc28dfa724..b7eae3ac34 100644 --- a/src/coffeescript.coffee +++ b/src/coffeescript.coffee @@ -14,7 +14,7 @@ packageJson = require '../../package.json' # The current CoffeeScript version number. exports.VERSION = packageJson.version -exports.FILE_EXTENSIONS = ['.coffee', '.litcoffee', '.coffee.md'] +exports.FILE_EXTENSIONS = FILE_EXTENSIONS = ['.coffee', '.litcoffee', '.coffee.md'] # Expose helpers for testing. exports.helpers = helpers @@ -49,9 +49,9 @@ withPrettyErrors = (fn) -> # a stack trace. Assuming that most of the time, code isn’t throwing # exceptions, it’s probably more efficient to compile twice only when we # need a stack trace, rather than always generating a source map even when -# it’s not likely to be used. Save in form of `filename`: `(source)` +# it’s not likely to be used. Save in form of `filename`: [`(source)`] sources = {} -# Also save source maps if generated, in form of `filename`: `(source map)`. +# Also save source maps if generated, in form of `(source)`: [`(source map)`]. sourceMaps = {} # Compile CoffeeScript code to JavaScript, using the Coffee/Jison compiler. @@ -75,7 +75,8 @@ exports.compile = compile = withPrettyErrors (code, options) -> checkShebangLine filename, code - sources[filename] = code + sources[filename] ?= [] + sources[filename].push code map = new SourceMap if generateSourceMap tokens = lexer.tokenize code, options @@ -124,8 +125,9 @@ exports.compile = compile = withPrettyErrors (code, options) -> js = "// #{header}\n#{js}" if generateSourceMap - v3SourceMap = map.generate(options, code) - sourceMaps[filename] = map + v3SourceMap = map.generate options, code + sourceMaps[filename] ?= [] + sourceMaps[filename].push map if options.inlineMap encoded = base64encode JSON.stringify v3SourceMap @@ -264,16 +266,36 @@ formatSourcePosition = (frame, getSourceMapping) -> else fileLocation -getSourceMap = (filename) -> - if sourceMaps[filename]? - sourceMaps[filename] - # CoffeeScript compiled in a browser may get compiled with `options.filename` - # of ``, but the browser may request the stack trace with the - # filename of the script file. +getSourceMap = (filename, line, column) -> + # Skip files that we didn’t compile, like Node system files that appear in + # the stack trace, as they never have source maps. + return null unless filename is '' or filename.slice(filename.lastIndexOf('.')) in FILE_EXTENSIONS + + if filename isnt '' and sourceMaps[filename]? + return sourceMaps[filename][sourceMaps[filename].length - 1] + # CoffeeScript compiled in a browser or via `CoffeeScript.compile` or `.run` + # may get compiled with `options.filename` that’s missing, which becomes + # ``; but the runtime might request the stack trace with the + # filename of the script file. See if we have a source map cached under + # `` that matches the error. else if sourceMaps['']? - sourceMaps[''] - else if sources[filename]? - answer = compile sources[filename], + # Work backwards from the most recent anonymous source maps, until we find + # one that works. This isn’t foolproof; there is a chance that multiple + # source maps will have line/column pairs that match. But we have no other + # way to match them. `frame.getFunction().toString()` doesn’t always work, + # and it’s not foolproof either. + for map in sourceMaps[''] by -1 + sourceLocation = map.sourceLocation [line - 1, column - 1] + return map if sourceLocation?[0]? and sourceLocation[1]? + + # If all else fails, recompile this source to get a source map. We need the + # previous section (for ``) despite this option, because after it + # gets compiled we will still need to look it up from + # `sourceMaps['']` in order to find and return it. That’s why we + # start searching from the end in the previous block, because most of the + # time the source map we want is the last one. + if sources[filename]? + answer = compile sources[filename][sources[filename].length - 1], filename: filename sourceMap: yes literate: helpers.isLiterate filename @@ -287,7 +309,7 @@ getSourceMap = (filename) -> # positions. Error.prepareStackTrace = (err, stack) -> getSourceMapping = (filename, line, column) -> - sourceMap = getSourceMap filename + sourceMap = getSourceMap filename, line, column answer = sourceMap.sourceLocation [line - 1, column - 1] if sourceMap? if answer? then [answer[0] + 1, answer[1] + 1] else null diff --git a/test/error_messages.coffee b/test/error_messages.coffee index 3aa7241c9b..d5aeda6081 100644 --- a/test/error_messages.coffee +++ b/test/error_messages.coffee @@ -75,7 +75,7 @@ if require? finally fs.unlinkSync tempFile - test "#3890 Error.prepareStackTrace doesn't throw an error if a compiled file is deleted", -> + test "#3890: Error.prepareStackTrace doesn't throw an error if a compiled file is deleted", -> # Adapted from https://github.com/atom/coffee-cash/blob/master/spec/coffee-cash-spec.coffee filePath = path.join os.tmpdir(), 'PrepareStackTraceTestFile.coffee' fs.writeFileSync filePath, "module.exports = -> throw new Error('hello world')" @@ -90,7 +90,7 @@ if require? doesNotThrow(-> error.stack) notEqual error.stack.toString().indexOf(filePath), -1 - test "#4418 stack traces for compiled files reference the correct line number", -> + test "#4418: stack traces for compiled files reference the correct line number", -> filePath = path.join os.tmpdir(), 'StackTraceLineNumberTestFile.coffee' fileContents = """ testCompiledFileStackTraceLineNumber = -> @@ -111,15 +111,15 @@ if require? eq /StackTraceLineNumberTestFile.coffee:(\d)/.exec(error.stack.toString())[1], '3' -test "#4418 stack traces for compiled strings reference the correct line number", -> +test "#4418: stack traces for compiled strings reference the correct line number", -> try - CoffeeScript.run """ + CoffeeScript.run ''' testCompiledStringStackTraceLineNumber = -> # `a` on the next line is undefined and should throw a ReferenceError console.log a if true do testCompiledStringStackTraceLineNumber - """ + ''' catch error # Make sure the line number reported is line 3 (the original Coffee source) @@ -127,6 +127,23 @@ test "#4418 stack traces for compiled strings reference the correct line number" eq /testCompiledStringStackTraceLineNumber.*:(\d):/.exec(error.stack.toString())[1], '3' +test "#4558: compiling a string inside a script doesn’t screw up stack trace line number", -> + try + CoffeeScript.run ''' + testCompilingInsideAScriptDoesntScrewUpStackTraceLineNumber = -> + if require? + CoffeeScript = require './lib/coffeescript' + CoffeeScript.compile '' + throw new Error 'Some Error' + + do testCompilingInsideAScriptDoesntScrewUpStackTraceLineNumber + ''' + catch error + + # Make sure the line number reported is line 5 (the original Coffee source) + # and not line 10 (the generated JavaScript). + eq /testCompilingInsideAScriptDoesntScrewUpStackTraceLineNumber.*:(\d):/.exec(error.stack.toString())[1], '5' + test "#1096: unexpected generated tokens", -> # Implicit ends assertErrorFormat 'a:, b', '''