From b3cf3f35fcf192c717e7a393fb984bf6f879d0cc Mon Sep 17 00:00:00 2001 From: isaacs Date: Sat, 28 Jul 2012 14:00:27 -0700 Subject: [PATCH] Report errors properly from --eval and stdin --- src/node.cc | 49 +++++++++++++++++----------- src/node.js | 16 ++++++++-- src/node_script.cc | 1 + test/message/eval_messages.js | 53 +++++++++++++++++++++++++++++++ test/message/eval_messages.out | 58 ++++++++++++++++++++++++++++++++++ test/simple/test-cli-eval.js | 13 ++++---- 6 files changed, 163 insertions(+), 27 deletions(-) create mode 100644 test/message/eval_messages.js create mode 100644 test/message/eval_messages.out diff --git a/src/node.cc b/src/node.cc index 257b96e9eb..a1e9fd8475 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1214,8 +1214,15 @@ ssize_t DecodeWrite(char *buf, return buflen; } - void DisplayExceptionLine (TryCatch &try_catch) { + // Prevent re-entry into this function. For example, if there is + // a throw from a program in vm.runInThisContext(code, filename, true), + // then we want to show the original failure, not the secondary one. + static bool displayed_error = false; + + if (displayed_error) return; + displayed_error = true; + HandleScope scope; Handle message = try_catch.Message(); @@ -1234,33 +1241,37 @@ void DisplayExceptionLine (TryCatch &try_catch) { String::Utf8Value sourceline(message->GetSourceLine()); const char* sourceline_string = *sourceline; - // HACK HACK HACK - // - // FIXME - // - // Because of how CommonJS modules work, all scripts are wrapped with a - // "function (function (exports, __filename, ...) {" + // Because of how node modules work, all scripts are wrapped with a + // "function (module, exports, __filename, ...) {" // to provide script local variables. // // When reporting errors on the first line of a script, this wrapper - // function is leaked to the user. This HACK is to remove it. The length - // of the wrapper is 62. That wrapper is defined in src/node.js + // function is leaked to the user. There used to be a hack here to + // truncate off the first 62 characters, but it caused numerous other + // problems when vm.runIn*Context() methods were used for non-module + // code. + // + // If we ever decide to re-instate such a hack, the following steps + // must be taken: // - // If that wrapper is ever changed, then this number also has to be - // updated. Or - someone could clean this up so that the two peices - // don't need to be changed. + // 1. Pass a flag around to say "this code was wrapped" + // 2. Update the stack frame output so that it is also correct. // - // Even better would be to get support into V8 for wrappers that - // shouldn't be reported to users. - int offset = linenum == 1 ? 62 : 0; + // It would probably be simpler to add a line rather than add some + // number of characters to the first line, since V8 truncates the + // sourceline to 78 characters, and we end up not providing very much + // useful debugging info to the user if we remove 62 characters. - fprintf(stderr, "%s\n", sourceline_string + offset); - // Print wavy underline (GetUnderline is deprecated). int start = message->GetStartColumn(); - for (int i = offset; i < start; i++) { + int end = message->GetEndColumn(); + + // fprintf(stderr, "---\nsourceline:%s\noffset:%d\nstart:%d\nend:%d\n---\n", sourceline_string, start, end); + + fprintf(stderr, "%s\n", sourceline_string); + // Print wavy underline (GetUnderline is deprecated). + for (int i = 0; i < start; i++) { fputc((sourceline_string[i] == '\t') ? '\t' : ' ', stderr); } - int end = message->GetEndColumn(); for (int i = start; i < end; i++) { fputc('^', stderr); } diff --git a/src/node.js b/src/node.js index eb31b4b332..071016e0d0 100644 --- a/src/node.js +++ b/src/node.js @@ -73,7 +73,7 @@ } else if (process._eval != null) { // User passed '-e' or '--eval' arguments to Node. - evalScript('eval'); + evalScript('[eval]'); } else if (process.argv[1]) { // make process.argv[1] into a full path var path = NativeModule.require('path'); @@ -267,7 +267,19 @@ var module = new Module(name); module.filename = path.join(cwd, name); module.paths = Module._nodeModulePaths(cwd); - var result = module._compile('return eval(process._eval)', name); + var script = process._eval; + if (!Module._contextLoad) { + var body = script; + script = 'global.__filename = ' + JSON.stringify(name) + ';\n' + + 'global.exports = exports;\n' + + 'global.module = module;\n' + + 'global.__dirname = __dirname;\n' + + 'global.require = require;\n' + + 'return require("vm").runInThisContext(' + + JSON.stringify(body) + ', ' + + JSON.stringify(name) + ', true);\n'; + } + var result = module._compile(script, name + '-wrapper'); if (process._print_eval) console.log(result); } diff --git a/src/node_script.cc b/src/node_script.cc index 22693dc9b2..15c56122c7 100644 --- a/src/node_script.cc +++ b/src/node_script.cc @@ -417,6 +417,7 @@ Handle WrappedScript::EvalMachine(const Arguments& args) { if (output_flag == returnResult) { result = script->Run(); if (result.IsEmpty()) { + if (display_error) DisplayExceptionLine(try_catch); return try_catch.ReThrow(); } } else { diff --git a/test/message/eval_messages.js b/test/message/eval_messages.js new file mode 100644 index 0000000000..d03c0eb1b1 --- /dev/null +++ b/test/message/eval_messages.js @@ -0,0 +1,53 @@ + +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +var common = require('../common'); +var assert = require('assert'); + +var spawn = require('child_process').spawn; + +function run(cmd, strict, cb) { + var args = []; + if (strict) args.push('--use_strict'); + args.push('-pe', cmd); + var child = spawn(process.execPath, args); + child.stdout.pipe(process.stdout); + child.stderr.pipe(process.stdout); + child.on('close', cb); +} + +var queue = + [ 'with(this){__filename}', + '42', + 'throw new Error("hello")', + 'var x = 100; y = x;', + 'var ______________________________________________; throw 10' ]; + +function go() { + var c = queue.shift(); + if (!c) return console.log('done'); + run(c, false, function() { + run(c, true, go); + }); +} + +go(); diff --git a/test/message/eval_messages.out b/test/message/eval_messages.out new file mode 100644 index 0000000000..f56bbdbddb --- /dev/null +++ b/test/message/eval_messages.out @@ -0,0 +1,58 @@ +[eval] + +[eval]:1 +with(this){__filename} +^^^^ +SyntaxError: Strict mode code may not include a with statement + at Object. ([eval]-wrapper:6:22) + at Module._compile (module.js:449:26) + at evalScript (node.js:282:25) + at startup (node.js:76:7) + at node.js:623:3 +42 +42 + +[eval]:1 +throw new Error("hello") + ^ +Error: hello + at [eval]:1:7 + at Object. ([eval]-wrapper:6:22) + at Module._compile (module.js:449:26) + at evalScript (node.js:282:25) + at startup (node.js:76:7) + at node.js:623:3 + +[eval]:1 +throw new Error("hello") + ^ +Error: hello + at [eval]:1:7 + at Object. ([eval]-wrapper:6:22) + at Module._compile (module.js:449:26) + at evalScript (node.js:282:25) + at startup (node.js:76:7) + at node.js:623:3 +100 + +[eval]:1 +var x = 100; y = x; + ^ +ReferenceError: y is not defined + at [eval]:1:16 + at Object. ([eval]-wrapper:6:22) + at Module._compile (module.js:449:26) + at evalScript (node.js:282:25) + at startup (node.js:76:7) + at node.js:623:3 + +[eval]:1 +var ______________________________________________; throw 10 + ^ +10 + +[eval]:1 +var ______________________________________________; throw 10 + ^ +10 +done diff --git a/test/simple/test-cli-eval.js b/test/simple/test-cli-eval.js index b47cc141ff..7e3e0cd6e3 100644 --- a/test/simple/test-cli-eval.js +++ b/test/simple/test-cli-eval.js @@ -19,21 +19,22 @@ // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE // USE OR OTHER DEALINGS IN THE SOFTWARE. +if (module.parent) { + // signal we've been loaded as a module + console.log('Loaded as a module, exiting with status code 42.'); + process.exit(42); +} + var common = require('../common.js'), assert = require('assert'), child = require('child_process'), nodejs = '"' + process.execPath + '"'; + // replace \ by / because windows uses backslashes in paths, but they're still // interpreted as the escape character when put between quotes. var filename = __filename.replace(/\\/g, '/'); -if (module.parent) { - // signal we've been loaded as a module - console.log('Loaded as a module, exiting with status code 42.'); - process.exit(42); -} - // assert that nothing is written to stdout child.exec(nodejs + ' --eval 42', function(err, stdout, stderr) {