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

findSourceMap/findEntry provides confusing/misleading line and column numbers #47770

Closed
isaacs opened this issue Apr 28, 2023 · 7 comments · Fixed by #47790
Closed

findSourceMap/findEntry provides confusing/misleading line and column numbers #47770

isaacs opened this issue Apr 28, 2023 · 7 comments · Fixed by #47790
Labels
source maps Issues and PRs related to source map support.

Comments

@isaacs
Copy link
Contributor

isaacs commented Apr 28, 2023

Version

20.0.0, 18.16.0

Platform

Darwin moxy.lan 22.4.0 Darwin Kernel Version 22.4.0: Mon Mar 6 20:59:28 PST 2023; root:xnu-8796.101.5~3/RELEASE_ARM64_T6000 arm64

Subsystem

No response

What steps will reproduce the bug?

Run the code provided in this gist, as shown in the first comment: https://gist.github.com/isaacs/ba57fca9ec16152256cba4b9b544750c

Verified with VSCode Source Map Visualizer that the mappings are correct. Note that:

  • The generatedLine and generatedColumn from the source map payload do not consistently match the line/column from the error object. (It seems like they always should?)
  • The column number is never correct, even when the lineNumber is.
  • Frequently the column number is far longer than the line referenced, which should be impossible.

How often does it reproduce? Is there a required condition?

100% of the time.

It seems like node's --enable-source-maps is just broken, or the SourceMap object is not doing the right thing.

What is the expected behavior? Why is that the expected behavior?

Expected behavior would be:

  • generatedLine, generatedColumn should always match the call site line/number specified. I don't see how it could be different, it's supposed to be looking up the mapping for that generated result, so how could the generated result be different from the thing being looked up?
  • originalLine and originalColumn should accurately reflect the origin specified in the sourcemap. For example, the callsite lineNumber 51 columnNumber 11 should result in an originalLine of 64, and originalColumn of 9.

What do you see instead?

Confusing and random errors in every line/column reported.

Origin locations that do not match the origin location provided.

Additional information

No response

@isaacs
Copy link
Contributor Author

isaacs commented Apr 28, 2023

Ok, looking at this a bit deeper, it seems like maybe the originalLine, originalColumn, generatedLine, and generatedColumn are just confusingly named?

If I'm reading this correctly, they're not the original and generated "line" and "column", they're the start of the range offsets within the map that covers the line/column in question.

However, treating them as ranges and then calculating the relevant offsets doesn't consistently yield the correct values either.

@VoltrexKeyva VoltrexKeyva added the source maps Issues and PRs related to source map support. label Apr 28, 2023
@isaacs
Copy link
Contributor Author

isaacs commented Apr 28, 2023

Ok, figured out how to get the actual line/col mapping. This is exceptionally non-obvious.

They aren't lines and columns. They are zero-indexed mapping offsets.

So, if you have a line and column from a callsite, you have to subtract 1 from each. The returned payload gives you the zero-indexed line and zero-indexed column of the start of the range. So, to get the actual line and column in the source file, you have to subtract the generatedLine and generatedColumn values from the line/col from the callsite to get the offset of the reported line/col from the start of the map, then add that difference to the originalLine and originalColumn to get the actual line and column in the source file.

Is there interest in a PR to just pass in a 1-indexed callsite line/column, and get back the 1-indexed line/column in the source? I have to write it anyway, may as well provide it in core as well.

@isaacs isaacs changed the title --enable-source-maps provides incorrect line and column numbers findSourceMap/findEntry provides confusing/misleading line and column numbers Apr 28, 2023
@cjihrig
Copy link
Contributor

cjihrig commented Apr 28, 2023

I think I'm in favor of having that in core. I haven't followed any of the source maps in core work closely, but I think that might be changing soon. The node:test test runner's code coverage does not yet support source maps, and I would like to add that support.

@isaacs
Copy link
Contributor Author

isaacs commented Apr 29, 2023

I think along with SourceMap.findRange, there could be a SourceMap.findLocation that just does the required math. Would be pretty easy, and save the next person having to learn how source maps work :)

@isaacs
Copy link
Contributor Author

isaacs commented Apr 29, 2023

Or maybe findOrigin?

@cjihrig
Copy link
Contributor

cjihrig commented Apr 29, 2023

I don't mind findLocation(). Maybe we should be a little more verbose so it's clear that it's the original location - findOriginalLocation(), getOriginalLocation(), etc. The bikeshedding possibilities are endless 😄

@isaacs
Copy link
Contributor Author

isaacs commented Apr 29, 2023

Yeah, "origin" just occurred to me because it mirrors the evalOrigin name in CallSite.

But the spelling doesn't matter too much to me, as long as it's there and documented clearly.

isaacs added a commit to isaacs/node that referenced this issue May 1, 2023
This adds the `SourceMap.findOrigin(lineNumber, columnNumber)` method,
for finding the origin source file and 1-indexed line and column numbers
corresponding to the 1-indexed line and column numbers from a call site
in generated source code.

Fix: nodejs#47770
nodejs-github-bot pushed a commit that referenced this issue Jun 23, 2023
This adds the `SourceMap.findOrigin(lineNumber, columnNumber)` method,
for finding the origin source file and 1-indexed line and column numbers
corresponding to the 1-indexed line and column numbers from a call site
in generated source code.

Fix: #47770
PR-URL: #47790
Fixes: #47770
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
RafaelGSS pushed a commit that referenced this issue Jul 3, 2023
This adds the `SourceMap.findOrigin(lineNumber, columnNumber)` method,
for finding the origin source file and 1-indexed line and column numbers
corresponding to the 1-indexed line and column numbers from a call site
in generated source code.

Fix: #47770
PR-URL: #47790
Fixes: #47770
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
This adds the `SourceMap.findOrigin(lineNumber, columnNumber)` method,
for finding the origin source file and 1-indexed line and column numbers
corresponding to the 1-indexed line and column numbers from a call site
in generated source code.

Fix: nodejs#47770
PR-URL: nodejs#47790
Fixes: nodejs#47770
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
This adds the `SourceMap.findOrigin(lineNumber, columnNumber)` method,
for finding the origin source file and 1-indexed line and column numbers
corresponding to the 1-indexed line and column numbers from a call site
in generated source code.

Fix: nodejs#47770
PR-URL: nodejs#47790
Fixes: nodejs#47770
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
ruyadorno pushed a commit that referenced this issue Sep 10, 2023
This adds the `SourceMap.findOrigin(lineNumber, columnNumber)` method,
for finding the origin source file and 1-indexed line and column numbers
corresponding to the 1-indexed line and column numbers from a call site
in generated source code.

Fix: #47770
PR-URL: #47790
Fixes: #47770
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
ruyadorno pushed a commit that referenced this issue Sep 13, 2023
This adds the `SourceMap.findOrigin(lineNumber, columnNumber)` method,
for finding the origin source file and 1-indexed line and column numbers
corresponding to the 1-indexed line and column numbers from a call site
in generated source code.

Fix: #47770
PR-URL: #47790
Fixes: #47770
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
source maps Issues and PRs related to source map support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants