-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix source map with multibyte characters #5988
Conversation
😳 has this been the problem with misaligned source maps the whole time?! awesome find... thank you for figuring this out! |
packages/source-map-utils/index.js
Outdated
@@ -17,21 +17,25 @@ var SourceMapUtils = { | |||
var column = 0; | |||
|
|||
source.forEach(function (character) { | |||
let loc; |
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.
If loc
is declared and initited, there's no need for the else clause below.
let loc = { line, column };
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.
Tested and looks good to me! I'd recommend waiting for @haltman-at to comment before merging.
error - observed
debug(localhost:8545:0xfb602772...)> b AggregationRouterV5.sol:2849
Breakpoint added at line 2849 in AggregationRouterV5.sol.
debug(localhost:8545:0xfb602772...)> c
AggregationRouterV5.sol:
2847:
2848: /// @inheritdoc IUniswapV3SwapCallback
2849: function uniswapV3SwapCallback(
^^^^^^^^^^
fix - observed
debug(localhost:8545:0xfb602772...)> b AggregationRouterV5.sol:2849
Breakpoint added at line 2849 in AggregationRouterV5.sol.
debug(localhost:8545:0xfb602772...)> c
AggregationRouterV5.sol:
2847:
2848: /// @inheritdoc IUniswapV3SwapCallback
2849: function uniswapV3SwapCallback(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
debug(localhost:8545:0xfb602772...)>
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.
Thanks for catching this! My only note is to suggest adding an explanatory comment. On first read I wasn't convinced this was an appropriate solution. I had to look it over a bit more to realize that yes this was actually a good way of handling this. So, I'd like to request adding a comment to explain.
} | ||
|
||
for (let i = 0; i < Buffer.from(character).length; i++) { |
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'd suggest adding a comment on this loop. Something like, "because our offsets are given in bytes rather than in characters, we pad out the line/column map as per the UTF-8 length of the character". I think that will make this clearer for future maintainers.
Actually, I just added the comment myself, so you don't need to do anything. I'll merge as soon as this passes CI! |
PR description
Make
getCharacterOffsetToLineAndColumnMapping()
handle multibyte characters properly. Previously, it counts all characters as 1 byte.Addresses #5914
Testing instructions
Run
truffle debug 0xfb6027725ccff056d8fe4065227fecd335fe123e8ab4da3a85a71377f7e9c6f9 --fetch-external
. With this bugfix, source mapping seems correctSteps
ganache --fork
truffle debug 0xfb6027725ccff056d8fe4065227fecd335fe123e8ab4da3a85a71377f7e9c6f9 --fetch-external --url http://localhost:8545