Report errors properly from --eval and stdin

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> 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);
     }

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);
   }
 

src/node_script.cc

@@ -417,6 +417,7 @@ Handle<Value> 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 {

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();

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.<anonymous> ([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.<anonymous> ([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.<anonymous> ([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.<anonymous> ([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

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) {