fix[parser]: fix source-map end positions - #5209
Conversation
Gas ChangesNo changes detected. Summary
|
📊 Bytecode Size Changes (venom)No changes detected. Full bytecode sizes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de2e6d1f59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| adj = adjustments.get((node.end_lineno, node.end_col_offset), 0) | ||
| node.end_col_offset += adj | ||
| node.col_offset += self._pre_parser.shift_for(node.lineno, node.col_offset) | ||
| node.end_col_offset += self._pre_parser.shift_for(node.end_lineno, node.end_col_offset) |
There was a problem hiding this comment.
When the final source line contains a rewritten keyword such as staticcall or extcall, the module node's end_col_offset is already set from the original source in visit_Module (len(self.source_lines[-1])) before generic_visit runs. Applying shift_for here treats that original EOF column as a reformatted Python column, so a file ending with return staticcall ... adds the rewrite delta again and exports an out-of-range module src span, which can be used as the pc 0 fallback in build_source_map_output. Skip this adjustment for coordinates that were synthesized from the original source, or set the module end after the generic shift path.
Useful? React with 👍 / 👎.
| node.col_offset += self._pre_parser.shift_for(node.lineno, node.col_offset) | ||
| node.end_col_offset += self._pre_parser.shift_for(node.end_lineno, node.end_col_offset) |
There was a problem hiding this comment.
For AST nodes that do not carry their own coordinates, such as operator and context singleton nodes, generic_visit copies the location from self._parents[-1]; at that point the parent has already passed through these shift lines. With a keyword rewrite earlier on the same line, e.g. return staticcall Vault(v).convertToAssets(10 ** d), the Pow/Load children inherit original-source coordinates and then shift_for adds the same delta again, so annotated_ast_dict and any diagnostics or source-map entries that use those children point several columns too far right. Track whether a coordinate came from the raw Python parser before applying the shift, or avoid reapplying it to inherited fields.
Useful? React with 👍 / 👎.
| return 0 | ||
| # bisect_right on col: the last shift point with col_key <= col is at | ||
| # index i - 1 (0 means no shift point applies yet). | ||
| i = bisect.bisect_right(pts, (col, float("inf"))) |
There was a problem hiding this comment.
why are we bisecting here?
There was a problem hiding this comment.
We're finding the element of pts which has the biggest col_key that is smaller than col
Equivalent to something like:
res: int
for (col_key, shift) in pts:
if (col_key <= col):
res = shift
else:
break
return res
There was a problem hiding this comment.
In other words, we're looking to find which shift applies at this location
There was a problem hiding this comment.
Maybe I didn't read carefully enough but why wouldn't the line just be in the dict?
There was a problem hiding this comment.
The most complete example is this:
x = extcall foo(extcall bar())
Which will be pre-parsed to this:
x = await foo(await bar())
And so:
x =should not be shifted- the first
awaitshould have its start not shifted, but its end shifted by 2 foo(should be shifted by 2- the second
awaitshould have it's start shifted by 2, and it's end shifted by 4 bar())should be shifted by 4
So a single line has actually 3 different shifts
There was a problem hiding this comment.
(oh and in this case we could replace extcall by await and avoid having to shift entirely, but there are cases where the shift goes the other way around, so that wouldn't work)
2 participants
What I did
a ** b(runtime exponent) used as a call argument #5027 (token end positions are not offset properly)get_contract-based test sanity-check the produced source mapsHow I did it
This fixes the issue since now end positions are updated correctly (previously not recorded in the dict)
get_contractis calledHow to verify it
pytest(new tests in filetests/unit/compiler/test_source_map.py)Also can check that
test_source_map_no_negative_length_after_keyword_rewritefails on masterCommit message
Description for the changelog
Fix exported source map sometimes being invalid
Cute Animal Picture