Fix Hermes get original function name#106
Conversation
|
|
||
| assert_eq!( | ||
| sm.lookup_token(0, 11947).unwrap().to_tuple(), | ||
| ("module.js", 1, 4, None) |
There was a problem hiding this comment.
This basically points to the throw token here (which is at line 2:5)
| sm.lookup_token(0, 11947).unwrap().to_tuple(), | ||
| ("module.js", 1, 4, None) | ||
| ); | ||
| assert_eq!(sm.get_original_function_name(11947), Some("foo")); |
There was a problem hiding this comment.
Without the fix, this was returning <global> instead of foo as it thought it was outside the function scope.
src/hermes.rs
Outdated
| // https://github.com/facebook/metro/blob/f2d80cebe66d3c64742f67259f41da26e83a0d8d/packages/metro/src/Server/symbolicate.js#L58-L60 | ||
| let (_mapping_idx, mapping) = | ||
| greatest_lower_bound(&function_map.mappings, &token.get_src(), |o| { | ||
| greatest_lower_bound(&function_map.mappings, &(token.get_src_line() + 1, token.get_src_col()), |o| { |
There was a problem hiding this comment.
I could either do this, or change the mapping function to do (o.line - 1, o.column), let me know what do you prefer
There was a problem hiding this comment.
I think doing it this way is fine.
|
Hi, thank you very much for the contribution! |
|
@loewenheim thanks for the approval! I just fixed the lint error that was getting |
|
@loewenheim Can we run the workflow to see if the linter and tests pass? |
|
@loewenheim There were 2 |
|
@pablomatiasgomez Apologies, I didn't realize those lint errors weren't part of your PR. Thank you for taking care of them! |
|
@loewenheim no worries! I just fixed another one 😅 |
|
@loewenheim What is the process to get this merged? No rush on my end, just want to know if there's anything left on my end to do 😄 |
|
Just merged it :) |
2 participants
The utility to get the original function name of a given token is slightly incorrect.
The mappings included in the map file, for whatever reason are 1-based indexed for the lines, and 0-based for the columns.
Given that we were doing the lookup based on both 0-based indexes, sometimes the lookup was incorrect as we are pointing to line before, and in cases where the token is the very first line of the function, it may return an invalid function name (only when the column ends up being also before the
functionkeyword)I added a test case based on the testdata that we have today that points to the
throwtoken of the module.js file, which is before thefunctionin terms of the column, and that showed the error.Also, here we can see that facebook uses 1-based for lines and 0-based for cols: https://github.com/facebook/metro/blob/f2d80cebe66d3c64742f67259f41da26e83a0d8d/packages/metro/src/Server/symbolicate.js#L58-L60