Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: fix error first line col offset #2867

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions doc/api/vm.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,16 @@ The options when creating a script are:

- `filename`: allows you to control the filename that shows up in any stack
traces produced from this script.
- `lineOffset`: allows you to add an offset to the line number that is
displayed in stack traces
- `columnOffset`: allows you to add an offset to the column number that is
displayed in stack traces
- `displayErrors`: whether or not to print any errors to stderr, with the
line of code that caused them highlighted, before throwing an exception.
Applies only to syntax errors compiling the code; errors while running the
code are controlled by the options to the script's methods.
- `timeout`: a number of milliseconds to execute `code` before terminating
execution. If execution is terminated, an `Error` will be thrown.

### script.runInContext(contextifiedSandbox[, options])

Expand Down Expand Up @@ -124,8 +130,14 @@ multiple times:

The options for running a script are:

- `displayErrors`: whether or not to print any runtime errors to stderr, with
the line of code that caused them highlighted, before throwing an exception.
- `filename`: allows you to control the filename that shows up in any stack
traces produced.
- `lineOffset`: allows you to add an offset to the line number that is
displayed in stack traces
- `columnOffset`: allows you to add an offset to the column number that is
displayed in stack traces
- `displayErrors`: whether or not to print any errors to stderr, with the
line of code that caused them highlighted, before throwing an exception.
Applies only to runtime errors executing the code; it is impossible to create
a `Script` instance with syntax errors, as the constructor will throw.
- `timeout`: a number of milliseconds to execute the script before terminating
Expand Down Expand Up @@ -252,6 +264,10 @@ e.g. `(0,eval)('code')`. However, it also has the following additional options:

- `filename`: allows you to control the filename that shows up in any stack
traces produced.
- `lineOffset`: allows you to add an offset to the line number that is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to also update docs on Script constructor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the docs in two places with the lineOffset and columnOffset parameters, and also added a missing filename and timeout parameter listings.

displayed in stack traces
- `columnOffset`: allows you to add an offset to the column number that is
displayed in stack traces
- `displayErrors`: whether or not to print any errors to stderr, with the
line of code that caused them highlighted, before throwing an exception.
Will capture both syntax errors from compiling `code` and runtime errors
Expand Down
4 changes: 2 additions & 2 deletions lib/module.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ Module.wrapper = NativeModule.wrapper;
Module.wrap = NativeModule.wrap;
Module._debug = util.debuglog('module');


// We use this alias for the preprocessor that filters it out
const debug = Module._debug;

Expand Down Expand Up @@ -401,7 +400,8 @@ Module.prototype._compile = function(content, filename) {
// create wrapper function
var wrapper = Module.wrap(content);

var compiledWrapper = runInThisContext(wrapper, { filename: filename });
var compiledWrapper = runInThisContext(wrapper,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the linter complain about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not

{ filename: filename, lineOffset: -1 });
if (global.v8debug) {
if (!resolvedArgv) {
// we enter the repl if we're not given a filename argument.
Expand Down
2 changes: 1 addition & 1 deletion src/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -955,7 +955,7 @@
};

NativeModule.wrapper = [
'(function (exports, require, module, __filename, __dirname) { ',
'(function (exports, require, module, __filename, __dirname) {\n',
'\n});'
];

Expand Down
37 changes: 36 additions & 1 deletion src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -504,13 +504,15 @@ class ContextifyScript : public BaseObject {
TryCatch try_catch;
Local<String> code = args[0]->ToString(env->isolate());
Local<String> filename = GetFilenameArg(args, 1);
Local<Integer> lineOffset = GetLineOffsetArg(args, 1);
Local<Integer> columnOffset = GetColumnOffsetArg(args, 1);
bool display_errors = GetDisplayErrorsArg(args, 1);
if (try_catch.HasCaught()) {
try_catch.ReThrow();
return;
}

ScriptOrigin origin(filename);
ScriptOrigin origin(filename, lineOffset, columnOffset);
ScriptCompiler::Source source(code, origin);
Local<UnboundScript> v8_script =
ScriptCompiler::CompileUnbound(env->isolate(), &source);
Expand Down Expand Up @@ -675,6 +677,39 @@ class ContextifyScript : public BaseObject {
}


static Local<Integer> GetLineOffsetArg(
const FunctionCallbackInfo<Value>& args,
const int i) {
Local<Integer> defaultLineOffset = Integer::New(args.GetIsolate(), 0);

if (!args[i]->IsObject()) {
return defaultLineOffset;
}

Local<String> key = FIXED_ONE_BYTE_STRING(args.GetIsolate(), "lineOffset");
Local<Value> value = args[i].As<Object>()->Get(key);

return value->IsUndefined() ? defaultLineOffset : value->ToInteger();
}


static Local<Integer> GetColumnOffsetArg(
const FunctionCallbackInfo<Value>& args,
const int i) {
Local<Integer> defaultColumnOffset = Integer::New(args.GetIsolate(), 0);

if (!args[i]->IsObject()) {
return defaultColumnOffset;
}

Local<String> key = FIXED_ONE_BYTE_STRING(args.GetIsolate(),
"columnOffset");
Local<Value> value = args[i].As<Object>()->Get(key);

return value->IsUndefined() ? defaultColumnOffset : value->ToInteger();
}


static bool EvalMachine(Environment* env,
const int64_t timeout,
const bool display_errors,
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/test-error-first-line-offset.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
error
12 changes: 12 additions & 0 deletions test/parallel/test-vm-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,15 @@ var ctx = {};
Object.defineProperty(ctx, 'b', { configurable: false });
ctx = vm.createContext(ctx);
assert.equal(script.runInContext(ctx), false);

// Error on the first line of a module should
// have the correct line and column number
assert.throws(function() {
vm.runInContext('throw new Error()', context, {
filename: 'expected-filename.js',
lineOffset: 32,
columnOffset: 123
});
}, function(err) {
return /expected-filename.js:33:130/.test(err.stack);
}, 'Expected appearance of proper offset in Error stack');
8 changes: 8 additions & 0 deletions test/sequential/test-module-loading.js
Original file line number Diff line number Diff line change
Expand Up @@ -279,3 +279,11 @@ process.on('exit', function() {
// #1440 Loading files with a byte order marker.
assert.equal(42, require('../fixtures/utf8-bom.js'));
assert.equal(42, require('../fixtures/utf8-bom.json'));

// Error on the first line of a module should
// have the correct line and column number
assert.throws(function() {
require('../fixtures/test-error-first-line-offset.js');
}, function(err) {
return /test-error-first-line-offset.js:1:1/.test(err.stack);
}, 'Expected appearance of proper offset in Error stack');