Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

module: fix column offset on first line stack trace #25342

Closed
wants to merge 1 commit into from

Conversation

brendan0powers
Copy link

When modules are loaded by require(), they are wrapped in
a function. This added text causes errors on the first line
of a file to be offset by the length of the function
declaration. For example, an error on the first character
of a module would appear as if it were on column 63.

To fix this problem I added line and column offset support
to the ContextifyScript object and added options to the
runInContext(and friends) function. I then added a column
offset to module._compile that compensates for the function
wrapper.

Fixes #9445

When modules are loaded by require(), they are wrapped in
a function. This added text causes errors on the first line
of a file to be offset by the length of the function
declaration. For example, an error on the first character
of a module would appear as if it were on column 63.

To fix this problem I added line and column offset support
to the ContextifyScript object and added options to the
runInContext(and friends) function. I then added a column
offset to module._compile that compensates for the function
wrapper.

Fixes nodejs#9445
if (args[i]->IsInt32()) {
return args[i].As<Integer>();
}
if (!args[i]->IsObject()) {
Copy link

Choose a reason for hiding this comment

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

I think this is the only check you actually need here. The IsUndefined() and IsInt32() checks above can go. Same comment applies for your GetColumnOffsetArg() function.

@cjihrig
Copy link

cjihrig commented May 28, 2015

@joyent/node-coreteam what do you think of this? It does seem to solve the problem. If we take this, I'm not sure we should necessarily document it as part of the public API.

Also, if we go this route, an alternative would be to change the module wrapper from:

'(function (exports, require, module, __filename, __dirname) { '

to

'(function (exports, require, module, __filename, __dirname) {\n'

And just use a lineOffset of -1. Would that work on Windows too? It honestly scares me a little to mess with the module wrapper though.

@@ -441,8 +441,10 @@ Module.prototype._compile = function(content, filename) {

// create wrapper function
var wrapper = Module.wrap(content);
var wrapperOffset = Module.wrapper[0].length;
Copy link

Choose a reason for hiding this comment

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

This can be pulled out of the function to avoid calculating every time.

@jasnell
Copy link
Member

jasnell commented Aug 15, 2015

@brendan0powers ... how would you like to proceed on this? @cjihrig raised a number of issues that would need to be addressed. If you'd like to pursue, updating the PR and re-opening it against http://github.com/nodejs/node would likely be the best approach.

@brendanashworth
Copy link

It looks like this is being followed up in nodejs/node#2867. I'll close this for now, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants