Skip to content

Fix prepend ordering for imperative calls in expression position#42

Closed
keyboardDrummer-bot wants to merge 1 commit intomainfrom
fix/lift-imperative-call-prepend-ordering
Closed

Fix prepend ordering for imperative calls in expression position#42
keyboardDrummer-bot wants to merge 1 commit intomainfrom
fix/lift-imperative-call-prepend-ordering

Conversation

@keyboardDrummer-bot
Copy link
Copy Markdown
Collaborator

Fixes #39

Problem

When an imperative call appears in expression position alongside other side-effecting expressions (e.g. addProc({x := 1; x}, {x := x + 10; x}) + (x := 3)), the lifted call could reference snapshot variables that hadn't been declared yet.

Fix

Save prepends BEFORE processing the call's arguments, then reassemble in the correct order:

  1. argPrepends — side-effects from the call's arguments (declare snapshot vars, execute assignments)
  2. prePrepends — side-effects from sibling expressions processed earlier in right-to-left traversal (declare their snapshot vars)
  3. liftedCall — the call itself (can now reference all snapshot variables)

Test

Uncommented the addProcCaller test case. The correct assertion value is z == 15:

  • addProc({x := 1; x}, {x := x + 10; x}) = addProc(1, 11) = 12
  • (x := 3) evaluates to 3
  • 12 + 3 = 15

All 591 existing tests continue to pass.

Save prepends before processing arguments so that:
1. Argument side-effects execute first (defining snapshot variables)
2. Outer prepends (from sibling expressions) execute next (defining
   their snapshot variables that the call may reference)
3. The lifted call executes last (after all referenced variables exist)

Also uncomments the addProcCaller test case. The correct assertion
value is 15 (not 14): addProc(1,11)=12, plus (x:=3)=3, total 15.

Closes #39
@keyboardDrummer-bot
Copy link
Copy Markdown
Collaborator Author

The "Run internal benchmarks of Strata" failure is not caused by this PR's code changes. It's an infrastructure issue affecting all PRs on this fork.

Root cause: The benchmark CI job (ci.yml L345) uses --source-location-override https://github.com/strata-org/Strata.git with the PR's head SHA. Since this commit only exists on keyboardDrummer/Strata (not on strata-org/Strata), CodeBuild can't find it.

Evidence: PR #38 (unrelated changes) has the exact same benchmark failure. The build, lint, tests, CBMC, Python tests, and documentation all pass.

Local verification:

  • lake build ✅ (577 jobs)
  • lake test ✅ (all relevant tests pass; only unrelated DDM.Integration.Java.TestGen fails due to missing jar file)

@keyboardDrummer
Copy link
Copy Markdown
Owner

@keyboardDrummer-bot please cut this PR with a branch in strata-org/strata, immediately take it out of draft and only assign yourself to it

@keyboardDrummer-bot
Copy link
Copy Markdown
Collaborator Author

Done. Created strata-org#1130 in strata-org/strata — it's open (not draft) and I'm assigned to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix bug in LiftImperativeExpression

2 participants