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.

Sign up for GitHub

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

Jump to bottom

VM: options.columnOffset has no effect on column number in stack trace output #26780

Closed
Eric24 opened this issue Mar 19, 2019 · 10 comments
Closed

VM: options.columnOffset has no effect on column number in stack trace output #26780

Eric24 opened this issue Mar 19, 2019 · 10 comments
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. question Issues that look for answers. vm Issues and PRs related to the vm subsystem.

Comments

@Eric24
Copy link

Eric24 commented Mar 19, 2019

  • Version: v10.3.0 and v10.15.2
  • Platform: Linux ip-172-30-3-105 4.14.42-52.37.amzn1.x86_64
  • Subsystem: vm

In vm.runInNewContext (and maybe other versions of "run", too), the options.columnOffset has no effect. I've tried a couple of intentionally-caused errors with various columnOffset values (ranging from -5 to 5), but the character position reported in the stack trace (via e.stack) is always the same. Note that lineOffset works as expected.
@devsnek
Copy link
Member

devsnek commented Mar 19, 2019

I can't reproduce this in 10.15.3

> vm.runInNewContext('throw new Error();', {}, { columnOffset: 20 })
evalmachine.<anonymous>:1
throw new Error();
^

Error
    at evalmachine.<anonymous>:1:27

@Eric24
Copy link
Author

Eric24 commented Mar 19, 2019

@devsnek : Yes, I get the same results with your example, but it appears to only work when the VM code is a single line. I've created a stripped-down version of my code that shows the one change that makes it work or not:

In this example, columnOffset is ignored:

let code = `
throw new Error();
`;
vm.runInNewContext(code, {}, {columnOffset: 20});

// RESULT (truncated):
evalmachine.<anonymous>:2
throw new Error();
^

Error
    at evalmachine.<anonymous>:2:7

But in this example, it works:

let code = `throw new Error();`;
vm.runInNewContext(code, {}, {columnOffset: 20});

// RESULT (truncated):
evalmachine.<anonymous>:1
throw new Error();
^

Error
    at evalmachine.<anonymous>:1:27

Note that the only difference is whether code has an extra newline. In my real code, the source JS is loaded from a file, and typically contains many newlines, but this example (using either a template as show or a simple string with \n characters) shows the difference. That's why it was never working for me (I hadn't tried a simple one-line code test).

PS - The "line" difference between the two examples is expected, and this is easily adjusted using lineOffiset, which works fine in both cases.

@devsnek
Copy link
Member

devsnek commented Mar 19, 2019

The column offset only affects the first line. The reason column offset exists is for script tags in html:

<body>
  <script>maybe code
code
code
code
  </script>
</body>

Every line that isn't the first script tag line has a correct column no matter what, because they start fromt the 1st column. But the source of the js in the first line is offset by 10 (" <script>"), so in order to display the correctly attribute an error to the location of the code within an html document, the offset is used here.

@devsnek devsnek added question Issues that look for answers. vm Issues and PRs related to the vm subsystem. labels Mar 19, 2019
@Eric24
Copy link
Author

Eric24 commented Mar 19, 2019

Hmmm. OK. I guess that explains what I'm seeing. If it's only supposed to affect the first line, then maybe this is just a documentation update.

@jasnell jasnell added doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. labels Jun 19, 2020
@jasnell
Copy link
Member

jasnell commented Jun 19, 2020

Marking this as a good first issue doc update.

@jasnell jasnell added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Jun 26, 2020
@harshilparmar
Copy link

@jasnell Can I do this?

@jasnell
Copy link
Member

jasnell commented Aug 4, 2020

Please feel free!

@harshilparmar
Copy link

@jasnell I am adding Note beside columnOffset definition
here https://nodejs.org/api/vm.html#vm_vm_runinnewcontext_code_contextobject_options
Is it correct? Please correct me if I am wrong!!

@swallville
Copy link

Hi @jasnell , I don't known if this doc issue update was already made, but, if not, could I give it a shot?

@jasnell
Copy link
Member

jasnell commented Dec 31, 2020

sorry, just saw the notification on this. It appears that this is still up for someone to take on!

jasnell added a commit to jasnell/node that referenced this issue Mar 1, 2021
@jasnell
doc: clarify that columnOffset applies only to the first line
60be1b6 
Fixes: nodejs#26780
@jasnell jasnell mentioned this issue Mar 1, 2021
Closed
@jasnell jasnell closed this as completed in ee58b2d Mar 5, 2021
danielleadams pushed a commit that referenced this issue Mar 16, 2021
@jasnell @danielleadams
doc: clarify that columnOffset applies only to the first line
732d8ca 
Fixes: #26780

PR-URL: #37563
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this issue May 1, 2021
@jasnell @targos
doc: clarify that columnOffset applies only to the first line
1dc7f42 
Fixes: #26780

PR-URL: #37563
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. question Issues that look for answers. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

doc: clarify that columnOffset applies only to the first line jasnell/node
5 participants
@jasnell @Eric24 @devsnek @swallville @harshilparmar