Skip to content

fix[codegen]: interleaved effects eval for some builtins#4156

Merged
charles-cooper merged 11 commits intovyperlang:masterfrom
charles-cooper:fix/builtin-order-of-eval
Mar 17, 2025
Merged

fix[codegen]: interleaved effects eval for some builtins#4156
charles-cooper merged 11 commits intovyperlang:masterfrom
charles-cooper:fix/builtin-order-of-eval

Conversation

@charles-cooper
Copy link
Copy Markdown
Member

@charles-cooper charles-cooper commented Jun 18, 2024

What I did

follow on to #4157. unblock the behavior, and fix the bad order of eval

fixes #4019

How I did it

How to verify it

Commit message

this is a follow-on commit to 3d9c537142fb99. it fixes the potential
for interleaved effects evaluation by copying to memory in the cases
where there is such potential.

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

charles-cooper and others added 4 commits June 18, 2024 06:08
---------

Co-authored-by: trocher <[email protected]>
---------

Co-authored-by: cyberthirst <[email protected]>
---------

Co-authored-by: trocher <[email protected]>
@charles-cooper charles-cooper marked this pull request as ready for review June 18, 2024 10:23
return IRnode.from_list(["seq", do_copy, buf], typ=typ, location=MEMORY)


# TODO: refactor ensure_in_memory to use this function
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we do this within the scope of this PR?

@charles-cooper charles-cooper added this to the v0.4.1 milestone Oct 4, 2024
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.26%. Comparing base (694f001) to head (63e246a).
Report is 81 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4156   +/-   ##
=======================================
  Coverage   92.26%   92.26%           
=======================================
  Files         123      123           
  Lines       17500    17502    +2     
  Branches     2960     2960           
=======================================
+ Hits        16146    16148    +2     
  Misses        946      946           
  Partials      408      408           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@charles-cooper charles-cooper added the release - must release blocker label Feb 17, 2025
@cyberthirst cyberthirst modified the milestones: v0.4.1, v0.4.2 Mar 10, 2025
@cyberthirst
Copy link
Copy Markdown
Collaborator

an explainer note for the fix - the arguments were cached correctly and in the correct order. however, we cached a pointer, not a value. for example, this test

def test_slice_order_of_eval2(get_contract):

changes the value at the given ptr via the extcall which eventually leads to the bug

the fix copies the value to memory, while the extcall will still work with storage

@charles-cooper charles-cooper changed the title fix[codegen]: order of eval for some builtins fix[codegen]: effects eval for some builtins Mar 17, 2025
@charles-cooper charles-cooper changed the title fix[codegen]: effects eval for some builtins fix[codegen]: interleaved effects eval for some builtins Mar 17, 2025
@charles-cooper charles-cooper enabled auto-merge (squash) March 17, 2025 12:59
@charles-cooper charles-cooper merged commit fc2473c into vyperlang:master Mar 17, 2025
159 checks passed
@charles-cooper charles-cooper deleted the fix/builtin-order-of-eval branch April 8, 2025 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release - must release blocker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

right-to-left evaluation in certain cases

3 participants