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

Conversation

tflanagan
Copy link
Contributor

Rehash of nodejs/node-v0.x-archive#25342 with noted corrections.

Fixes #2860, nodejs/node-v0.x-archive#9445

I've also created another variant of this solution by adding a "\n" to the wrapper and setting the lineOffset property to -1, re: tflanagan@9d8e6a9

@kenany
Copy link
Contributor

kenany commented Sep 14, 2015

I feel like @brendan0powers's authorship should be preserved here.

@tflanagan
Copy link
Contributor Author

👍

Just looking for things to do (this seemed rather important)

@brendanashworth
Copy link
Contributor

Are there any side effects of using the second commit (tflanagan@9d8e6a9)? It seems way simpler.

@@ -41,6 +41,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.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 16, 2015

I agree with @brendanashworth that the other commit looks much simpler. Does it cause any issues on Windows?

@tflanagan
Copy link
Contributor Author

bump for LTS? Don't want this PR to end up like @brendan0powers's

@brendanashworth
Copy link
Contributor

@tflanagan does tflanagan@489b5b4 cause any issues (perhaps windows)? I think this should be pushed instead of the other bigger commits. But this would be a candidate for LTS back-porting anyways.

@tflanagan
Copy link
Contributor Author

I've beening running it on windows for over 3 weeks without issues, granted any issue would only show if an error occured

@brendanashworth
Copy link
Contributor

@tflanagan could you push only that commit to this PR then? Then we can review and run the CI.

@tflanagan
Copy link
Contributor Author

@brendanashworth okay, i've rebased/amended/pushed it down to one commit with the NL approach.

$ vcbuild test nosign passed all tests

@tflanagan
Copy link
Contributor Author

poke

edit: poke

@@ -60,3 +60,23 @@ var ctx = {};
Object.defineProperty(ctx, 'b', { configurable: false });
ctx = vm.createContext(ctx);
assert.equal(script.runInContext(ctx), false);

// Issue GH-2860
Copy link
Contributor

Choose a reason for hiding this comment

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

The first line of this comment can probably be removed.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 20, 2015

@tflanagan I'm interested in getting this landed. Would you mind rebasing?

@tflanagan
Copy link
Contributor Author

@cjihrig Rebased, running new tests now, will force push shortly

@tflanagan
Copy link
Contributor Author

@cjihrig Done.

Besides for those pesky flaky Buffer tests, it passes local tests fwiw

Edit: Fixed the docs

@tflanagan tflanagan force-pushed the module-fix-error-offsets branch 2 times, most recently from b28d69a to 1505c73 Compare November 20, 2015 19:57
@cjihrig
Copy link
Contributor

cjihrig commented Nov 20, 2015

@tflanagan
Copy link
Contributor Author

@cjihrig Looks like it passed! :) 💯

@cjihrig
Copy link
Contributor

cjihrig commented Nov 20, 2015

💚 from the CI. Nice. Landing.

cjihrig pushed a commit that referenced this pull request Nov 20, 2015
Because Node modules are wrapped, errors on the first line
of a file leak the wrapper to the user and report the wrong
column number. This commit adds a line break to the module
wrapper so that the first line is treated the same as all
other lines. To compensate for the additional line, a line
offset of -1 is also applied to errors.

Fixes: #2860
PR-URL: #2867
Reviewed-By: Colin Ihrig <[email protected]>
@cjihrig
Copy link
Contributor

cjihrig commented Nov 20, 2015

Thanks! Landed in dfee4e3

@cjihrig cjihrig closed this Nov 20, 2015
@tflanagan
Copy link
Contributor Author

Thank you! 👍

@tflanagan tflanagan deleted the module-fix-error-offsets branch November 20, 2015 21:43
rvagg pushed a commit that referenced this pull request Dec 5, 2015
Because Node modules are wrapped, errors on the first line
of a file leak the wrapper to the user and report the wrong
column number. This commit adds a line break to the module
wrapper so that the first line is treated the same as all
other lines. To compensate for the additional line, a line
offset of -1 is also applied to errors.

Fixes: #2860
PR-URL: #2867
Reviewed-By: Colin Ihrig <[email protected]>
cjihrig added a commit to cjihrig/node that referenced this pull request Dec 14, 2015
In dfee4e3, the module wrapper
and line offset used when wrapping module code was changed to
better report errors on the first line of modules. However, that
commit did not update the runInThisContext() call used to
execute the core modules, so their error line numbers have been
off by one. This commit provides the correct lineOffset for core
modules.

Refs: nodejs#2867
PR-URL: nodejs#4254
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
cjihrig added a commit that referenced this pull request Dec 14, 2015
In dfee4e3, the module wrapper
and line offset used when wrapping module code was changed to
better report errors on the first line of modules. However, that
commit did not update the runInThisContext() call used to
execute the core modules, so their error line numbers have been
off by one. This commit provides the correct lineOffset for core
modules.

Refs: #2867
PR-URL: #4254
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
cjihrig added a commit that referenced this pull request Dec 15, 2015
In dfee4e3, the module wrapper
and line offset used when wrapping module code was changed to
better report errors on the first line of modules. However, that
commit did not update the runInThisContext() call used to
execute the core modules, so their error line numbers have been
off by one. This commit provides the correct lineOffset for core
modules.

Refs: #2867
PR-URL: #4254
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@rvagg rvagg mentioned this pull request Dec 17, 2015
MylesBorins pushed a commit that referenced this pull request Dec 29, 2015
Because Node modules are wrapped, errors on the first line
of a file leak the wrapper to the user and report the wrong
column number. This commit adds a line break to the module
wrapper so that the first line is treated the same as all
other lines. To compensate for the additional line, a line
offset of -1 is also applied to errors.

Fixes: #2860
PR-URL: #2867
Reviewed-By: Colin Ihrig <[email protected]>
cjihrig added a commit that referenced this pull request Dec 30, 2015
In dfee4e3, the module wrapper
and line offset used when wrapping module code was changed to
better report errors on the first line of modules. However, that
commit did not update the runInThisContext() call used to
execute the core modules, so their error line numbers have been
off by one. This commit provides the correct lineOffset for core
modules.

Refs: #2867
PR-URL: #4254
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
Because Node modules are wrapped, errors on the first line
of a file leak the wrapper to the user and report the wrong
column number. This commit adds a line break to the module
wrapper so that the first line is treated the same as all
other lines. To compensate for the additional line, a line
offset of -1 is also applied to errors.

Fixes: #2860
PR-URL: #2867
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
In dfee4e3, the module wrapper
and line offset used when wrapping module code was changed to
better report errors on the first line of modules. However, that
commit did not update the runInThisContext() call used to
execute the core modules, so their error line numbers have been
off by one. This commit provides the correct lineOffset for core
modules.

Refs: #2867
PR-URL: #4254
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
In dfee4e3, the module wrapper
and line offset used when wrapping module code was changed to
better report errors on the first line of modules. However, that
commit did not update the runInThisContext() call used to
execute the core modules, so their error line numbers have been
off by one. This commit provides the correct lineOffset for core
modules.

Refs: nodejs#2867
PR-URL: nodejs#4254
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants