-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: Add SourceMap.findOrigin #47790
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -169,28 +169,28 @@ class SourceMap { | |
}; | ||
|
||
/** | ||
* @param {number} lineNumber in compiled resource | ||
* @param {number} columnNumber in compiled resource | ||
* @return {?Array} | ||
* @param {number} lineOffset 0-indexed line offset in compiled resource | ||
* @param {number} columnOffset 0-indexed column offset in compiled resource | ||
* @return {object} representing start of range if found, or empty object | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The For example: /**
* @returns {{
* foo: string;
* bar?: number;
* }}
*/ |
||
*/ | ||
findEntry(lineNumber, columnNumber) { | ||
findEntry(lineOffset, columnOffset) { | ||
let first = 0; | ||
let count = this.#mappings.length; | ||
while (count > 1) { | ||
const step = count >> 1; | ||
const middle = first + step; | ||
const mapping = this.#mappings[middle]; | ||
if (lineNumber < mapping[0] || | ||
(lineNumber === mapping[0] && columnNumber < mapping[1])) { | ||
if (lineOffset < mapping[0] || | ||
(lineOffset === mapping[0] && columnOffset < mapping[1])) { | ||
count = step; | ||
} else { | ||
first = middle; | ||
count -= step; | ||
} | ||
} | ||
const entry = this.#mappings[first]; | ||
if (!first && entry && (lineNumber < entry[0] || | ||
(lineNumber === entry[0] && columnNumber < entry[1]))) { | ||
if (!first && entry && (lineOffset < entry[0] || | ||
(lineOffset === entry[0] && columnOffset < entry[1]))) { | ||
return {}; | ||
} else if (!entry) { | ||
return {}; | ||
|
@@ -205,6 +205,32 @@ class SourceMap { | |
}; | ||
} | ||
|
||
/** | ||
* @param {number} lineNumber 1-indexed line number in compiled resource call site | ||
* @param {number} columnNumber 1-indexed column number in compiled resource call site | ||
* @return {object} representing origin call site if found, or empty object | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||
*/ | ||
findOrigin(lineNumber, columnNumber) { | ||
const range = this.findEntry(lineNumber - 1, columnNumber - 1); | ||
if ( | ||
range.originalSource === undefined || | ||
range.originalLine === undefined || | ||
range.originalColumn === undefined || | ||
range.generatedLine === undefined || | ||
range.generatedColumn === undefined | ||
) { | ||
return {}; | ||
} | ||
const lineOffset = lineNumber - range.generatedLine; | ||
const columnOffset = columnNumber - range.generatedColumn; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried this PR out locally and noticed that if you create a source map from an existing coverage report some inputs return negative column numbers. Is this function expected to work with the source map data generated via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh that's weird. No, the column number should be at minimum 1. Can you share the steps you used to get to a negative column number? Maybe you're passing it offsets instead of column numbers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But yes, I'm using this now in tap on coverage map data produced by V8, and it seems to work fine, so if you found a weird edge case, I'm very interested. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @isaacs - I'll have to see if I still have the code on a branch, but the gist of it was: Given a line in the generated file, I was trying to find all of the corresponding lines in the original source file. I created a I think this is related to the fact that V8 ranges can start with whitespace on a non-zero column (for example around the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would assume that What it's doing is calculating the row and column deltas from the start of the generated line/column offsets to the 1-indexed line/column numbers you give it, then applying that same delta to the source line/column values from the map entry. So if you told it your origin is I'm not sure just looking for As I describe this, I'm realizing that this strategy might only work because TypeScript doesn't change token lengths, though. Like, let's say you had a source like const someLongName = otherLongName.x() And that got minified to const _l=_o.x() And let's say the error being thrown is that Lines are the same, so no issue there. But the range will start at
If we add 13 to the sourceColumn, that isn't going to point at the right spot, and you'll get an output like:
It'll work if the minifier is smart enough to make a new range where the two start matching, then it's fine, but I think the heuristic might have to be a little bit smarter, take into account the end of the range as well, and idk, say "It's probably somewhere in the general vicinity of these characters, good luck". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, so the problem is with The delta from the range Similarly, the range offset I think that Also, just to un-XY this problem, if your goal is "find all the lines in the origin source that map to a given line in the generated source", this strategy of checking column 1 and column Infinity is not ever going to work reliably. Any line in the generated source may be composed of an arbitrary number of entries from an arbitrary set of locations within the origin source, so if you only test the first and last in the line, you'll miss everything in the middle, and there's no guarantee that they line up. Consider, for example, a munger that shuffles functions around, or a minifier that puts every line in the origin into a single line in the output. What you need to do is get the set of offset ranges from the sourcemap entries that have a line offset of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ie, suggesting this change to findEntry, to have it return an empty range if the supplied offsets are not within the entry it finds (haven't tested, this might break other stuff, idk) diff --git a/lib/internal/source_map/source_map.js b/lib/internal/source_map/source_map.js
index 3112a026b64..08ad65b919a 100644
--- a/lib/internal/source_map/source_map.js
+++ b/lib/internal/source_map/source_map.js
@@ -189,10 +189,7 @@ class SourceMap {
}
}
const entry = this.#mappings[first];
- if (!first && entry && (lineOffset < entry[0] ||
- (lineOffset === entry[0] && columnOffset < entry[1]))) {
- return {};
- } else if (!entry) {
+ if (!entry || lineOffset !== entry[0] || columnOffset < entry[1]) {
return {};
}
return { With that, it's much more clear that the approach of testing column 1 and column Infinity is invalid, because you walked off the source map. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, that's only half of the fix, because we're not tracking the end of the range in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So this was just me doing some random testing, not an actual goal. I agree that it won't work for anything useful - a generated file can be a single line, which would then map to every line in the original source, making it pretty useless for something like code coverage. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can definitely see some potential for something like "from |
||
return { | ||
name: range.name, | ||
fileName: range.originalSource, | ||
lineNumber: range.originalLine + lineOffset, | ||
columnNumber: range.originalColumn + columnOffset, | ||
}; | ||
} | ||
|
||
/** | ||
* @override | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The column limit is 130 now, I think. You could use more space to make this more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes the most sense to follow the conventions in the file when adding new content. It's a bit weird to have some of the docs wrapped at 130, and others at 80, no? And even worse to have a commit that touches the whole file when only a single section was added/edited.
Better to reformat markdown as a subsequent PR, or better yet, reformat all the markdown in the project in one atomic commit that does nothing else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it’s more readable when wrapped at 80 chars, at least it is on the GitHub web UI because they wrap suggestions at 80 chars 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearly, what we need is more input on this important matter. 😅