-
Notifications
You must be signed in to change notification settings - Fork 22
Spec: add algorithm for retrieving original position #195
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
base: main
Are you sure you want to change the base?
Conversation
I have some changes to this that I'll push after #194 is merged based on the recent TG4 discussion (adding a LUB variant of lookup for example). |
ba24fd2
to
976b509
Compare
I finally made another revision to this, with the PR rebased on the latest spec and changing how the operations work a bit. Instead of providing multiple versions of For For additional context, I summarized the browser devtools behavior for these two operations: Getting the original position:
Getting generated positions:
|
There's a rendered preview of the PR here. |
spec.emu
Outdated
1. If the result of performing ComparePositions(_mapping_.[[GeneratedPosition]], _generatedPosition_) is ~greater~, then | ||
1. Return _closest_. |
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.
There's some disagreement between impls here. When there are multiple mappings with the same generated position, this spec will return the last one.
- Chrome agrees, because it's
upperBound
uses>=
and moves rightward - Safari agrees for the same reason
- Mozilla agrees, but uses Rust's
binary_search_by
which states it could be any result an is subject to change- It also always returns the last match, regardless of bias.
trace-mapping
will return the first match when using glb, and the last match when using lup.- Mozilla's old JS impl would return the any match, and I wanted something deterministic regardless of adding unrelated mappings elsewhere. 😬
1. If the result of performing ComparePositions(_mapping_.[[GeneratedPosition]], _generatedPosition_) is ~greater~, then | |
1. Return _closest_. | |
1. If the result of performing ComparePositions(_mapping_.[[GeneratedPosition]], _generatedPosition_) is not ~lesser~, then | |
1. Return _closest_. |
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.
Yeah that's a good point about multiple matching mappings; I hadn't reached a conclusion on how to handle it. I wonder if the spec should just allow an arbitrary matching mapping to be returned when there are multiple.
BTW, for your suggested change, wouldn't that fail to return a matching with an equal generated position if there is one? (_closest_
would need to be updated first?).
Introduces a new section with algorithms for using Decoded Source Map Records in order to perform lookups. The section is intended to have multiple operations in the future but only has one for now. Additionally, this introduces an explicit sort of the mappings when they are put in a decoded source map record.
976b509
to
cd17d6f
Compare
I updated this PR to just add the original position operation, since that's a more obvious starting point and lets other PRs build on it. One thing that's unresolved is what to do when there are duplicate mappings at the same location. We could change the algorithm to return an arbitrary mapping when picking the closest element: Current (pick last closest):
Arbitrary:
Any opinion on this? (or other options) |
This makes more sense given the name of the function
GetOriginalPosition ( | ||
_sourceMapRecord_: a Decoded Source Map Record, | ||
_generatedPosition_: a Position Record, | ||
): an Original Position Record or *null* |
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 also changed this to return the original position record instead of the entry. Given the name of the operation it probably makes more sense to return the position record, and that also makes a bit more sense for the range mappings proposal spec text (since you just return a modified position, rather than making up a new mapping with a modified position).
This should also be ok for PR #219 which only uses the original position part.
@@ -683,6 +683,7 @@ | |||
1. Let _sources_ be DecodeSourceMapSources(_baseURL_, _sourceRootField_, _sourcesField_, _sourcesContentField_, _ignoreListField_). | |||
1. Let _namesField_ be GetOptionalListOfStrings(_json_, *"names"*). | |||
1. Let _mappings_ be DecodeMappings(_mappingsField_, _namesField_, _sources_). | |||
1. [declared="a,b"] Sort _mappings_ in ascending order, with a Decoded Mapping Record _a_ being less than a Decoded Mapping Record _b_ if ComparePositions(_a_.[[GeneratedPosition]], _b_.[[GeneratedPosition]]) is ~lesser~. |
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.
Why move this?
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.
Previously the sort was only done below in the index map algorithm to check validity. It's moved here so that decoded mappings are guaranteed to be sorted, because otherwise the search algorithm for looking up positions is slower.
This PR is an initial attempt at specifying how we can use Decoded Source Map Records to do position lookups:
Specifying something like this helps proposals like the range mapping proposal that affect or change how mappings should be interpreted.