Skip to content

Commit

Permalink
Fix #4558: Stack trace line numbers for scripts that compile CoffeeSc…
Browse files Browse the repository at this point in the history
…ript (#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)
  • Loading branch information
GeoffreyBooth authored Aug 23, 2017
1 parent 4623bf5 commit 44a27c6
Show file tree
Hide file tree
Showing 6 changed files with 121 additions and 45 deletions.
14 changes: 9 additions & 5 deletions Cakefile
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
2 changes: 1 addition & 1 deletion docs/v2/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -4682,7 +4682,7 @@ <h2 class="header">
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()

</script>

Expand Down
2 changes: 1 addition & 1 deletion documentation/v2/docs.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -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()
67 changes: 50 additions & 17 deletions lib/coffeescript/coffeescript.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

54 changes: 38 additions & 16 deletions src/coffeescript.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 `<anonymous>`, 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 '<anonymous>' or filename.slice(filename.lastIndexOf('.')) in FILE_EXTENSIONS

if filename isnt '<anonymous>' 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
# `<anonymous>`; but the runtime might request the stack trace with the
# filename of the script file. See if we have a source map cached under
# `<anonymous>` that matches the error.
else if sourceMaps['<anonymous>']?
sourceMaps['<anonymous>']
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['<anonymous>'] 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 `<anonymous>`) despite this option, because after it
# gets compiled we will still need to look it up from
# `sourceMaps['<anonymous>']` 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
Expand All @@ -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

Expand Down
27 changes: 22 additions & 5 deletions test/error_messages.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -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')"
Expand All @@ -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 = ->
Expand All @@ -111,22 +111,39 @@ 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)
# and not line 6 (the generated JavaScript).
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', '''
Expand Down

0 comments on commit 44a27c6

Please sign in to comment.