chore: add missing pc maps to vyper_json output
#3333
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #3333 +/- ##
==========================================
- Coverage 88.93% 88.82% -0.11%
==========================================
Files 84 84
Lines 10606 10608 +2
Branches 2215 2216 +1
==========================================
- Hits 9432 9423 -9
- Misses 768 778 +10
- Partials 406 407 +1
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
vyper/ir/compile_ir.py
Outdated
| def note_breakpoint(line_number_map, item, pos): | ||
| # Record line number attached to pos. | ||
| if item == "DEBUG": | ||
| if item in ("BREAKPOINT", "DEBUG"): |
There was a problem hiding this comment.
I made this change as the breakpoints were not showing up in the output.
There was a problem hiding this comment.
i did not plan for the new breakpoint instruction to show up in the output. right now pc_breakpoints should be empty
|
Decompressed pos map is not there tho. Brb, will link what we have to suddenly do in ape-vyper once we switched to vyper-json. |
|
this is what we had to change to get back to the same data structure we were relying on: https://github.com/ApeWorX/ape-vyper/blob/main/ape_vyper/compiler.py#L241-L263 not the end of the world but i relied on brownie to figure out how to get there, and since the data was already present in the normal way of compiling, it could be added to the |
|
Understood, I will add the original |
|
@unparalleled-js I have added |
vyper/compiler/__init__.py
Outdated
| "breakpoints": output.build_breakpoints_output, | ||
| "pc_breakpoints": output.build_pc_breakpoints_output, | ||
| "source_map": output.build_source_map_output, | ||
| "pc_pos_map": output.build_source_map_output, |
There was a problem hiding this comment.
hmm why is this a dup of source_map?
There was a problem hiding this comment.
I wasn't quite sure how to handle pc_pos_map since it is technically already in the source_map output. I think what you said below makes sense, so source_map output will contain both pc_pos_map and pc_pos_map_compressed, and there is no need for a separate pc_pos_map option. Did I understand it right?
There was a problem hiding this comment.
right. i'm not sure if we can actually replace the source_map option since people might depend on it, but we could add it like source_map_full or something
There was a problem hiding this comment.
Hm, so we would still separate it into source_map -> pc_pos_map_compressed, and source_map_full -> pc_pos_map?
There was a problem hiding this comment.
i think source_map should still return the compressed pc map, but source_map_full should return everything, just like the cli -f source_map output.
charles-cooper
left a comment
charles-cooper
left a comment
There was a problem hiding this comment.
i mean i think a much better approach is just to not select down into pc_pos_map for the source_map output. i think source_map output should already have everything needed.
| "pcBreakpoints": data["pc_breakpoints"], | ||
| "sourceMap": data["source_map"]["pc_pos_map_compressed"], | ||
| "pcPosMap": data["source_map"]["pc_pos_map"], | ||
| "sourceMapFull": data["source_map"], |
There was a problem hiding this comment.
nice. i think we can remove the other new fields now, since they are contained within sourceMapFull.
There was a problem hiding this comment.
do we want the other new fields to be sorted?
4 participants
What I did
Fix #3293 by adding
breakpoints,pc_breakpointsanderror_map-pc_pos_mapis already there so I have skipped that.Additionally, fixed a minor bug with detecting breakpoints.
How I did it
Mostly replicating how it is done for
source_map.How to verify it
Commit message
Description for the changelog
Add missing pc maps to
vyper_jsonoutputCute Animal Picture