Skip to content

fix(eliminate): skip inlining when substitution site is inside a loop body#58

Merged
NikolaRHristov merged 7 commits into
Currentfrom
fix/eliminate-loop-body-inline
May 25, 2026
Merged

fix(eliminate): skip inlining when substitution site is inside a loop body#58
NikolaRHristov merged 7 commits into
Currentfrom
fix/eliminate-loop-body-inline

Conversation

@NikolaRHristov
Copy link
Copy Markdown
Member

Problem

A binding used only inside a for/while/loop body has RefCount=1 and passes every existing check, so it gets inlined. The initialiser is then placed inside the loop and runs on every iteration instead of once. For side-effecting or expensive initialisers this changes observable behaviour. For move-only types it causes E0382 on the second iteration.

// before - initialiser runs once
let X = expensive();
for item in &collection { process(item, X); }

// after eliminate (wrong) - initialiser runs N times
for item in &collection { process(item, expensive()); }

Fix

Count.rs

CountReferences now returns (usize, bool, bool): the third value is InLoop.

ExprCounter gains an InsideLoop: bool context flag and overrides visit_expr_for_loop, visit_expr_while, and visit_expr_loop to set it before descending. Any reference to Target found while InsideLoop is true sets InLoop = true on the counter.

The flag is saved and restored around each loop variant so nested loops compose correctly.

Inline.rs

EliminateBlock destructures the new three-tuple and adds || InLoop to the skip condition alongside InClosure.

Tests added

  • Count::Tests::LoopBodySetsInLoop - for loop sets InLoop
  • Count::Tests::WhileBodySetsInLoop - while loop sets InLoop
  • Count::Tests::LoopExprBodySetsInLoop - bare loop sets InLoop
  • Count::Tests::OutsideLoopNotFlagged - usage outside loop leaves InLoop false
  • Count::Tests::UsedInsideAndOutsideLoop - mixed: count=2 and InLoop=true
  • Inline::Tests::LoopBodyBindingKept - for loop integration
  • Inline::Tests::WhileBodyBindingKept - while loop integration
  • Inline::Tests::LoopExprBindingKept - bare loop integration
  • Inline::Tests::OutsideLoopStillInlined - no regression for normal inline

… body

Addresses finding C from issue #55.

Before this change, a binding used only inside a for/while/loop body was
counted as RefCount=1 and inlined, moving the initialiser expression
inside the loop. This changes evaluation semantics: the initialiser now
runs on every iteration instead of once before the loop. For
side-effecting or expensive initialisers this is a behavioural change.
For move-only types it causes a compile error on the second iteration.

CountReferences now returns a third value InLoop alongside InClosure.
InLoop is true when the single reference to Target is found inside the
body of a for, while, or loop expression. EliminateBlock skips the
candidate when InLoop is true.

The existing LoopBodyCountedAsOne test is updated to document the new
behaviour: count remains 1 (we do not hoist the loop counter) but
InLoop is now true, which prevents inlining.
…od.rs

PR #57 added RunPreserve() to Transform/mod.rs on Current. PR #58 branched
before that merge, so its mod.rs is the pre-#57 version and conflicts.

Forward-port Current's mod.rs (Run + RunPreserve, no CountReferences calls)
onto this branch. Count.rs and Inline.rs already have the correct 3-tuple
CountReferences signature and InLoop guard — no changes needed there.

After this commit the branch compiles cleanly against Current.
…nLoop guard

Current's Inline.rs (post-#57) added IsFreeVarSafe/FindSubstSite which
#58's branch did not have. #58's branch added the InLoop guard and 3-tuple
CountReferences destructure which Current did not have.

This commit is the true three-way merge:
- Keeps #57's full structure: section banners, FindSubstSite, IsFreeVarSafe,
  all test sections including free-variable-move-safety tests.
- Applies #58's changes: (RefCount, InClosure, InLoop) 3-tuple destructure,
  `|| InLoop` guard in EliminateBlock, loop-body tests.
- Updates FindSubstSite's internal CountReferences call to discard the two
  new booleans with (Count, _, _) to match #58's Count.rs signature.
- Adds #58's loop-body tests alongside #57's free-var tests.
Current gained tests/Eliminate/{Integration.rs,Syntactic.rs} and
tests/Eliminate/Integration/*.rs via commits ad03561 (test split) and
eaa6a54 (batch-2 test expansion) after this branch was created.
Adding all of them verbatim so GitHub's merge machinery no longer
reports conflicts caused by missing files.
Four conflict hunks resolved in favour of the PR branch:
1. Comment: keep 'used-in-loop-body' in the algorithm description.
2. EliminateBlock: keep 3-tuple (RefCount, InClosure, InLoop) and remove
   the duplicated SubstSiteOffset/StmtsBetween block that crept in during
   the conflict.
3. FindSubstSite: keep 3-tuple destructure (Count, _, _) matching the
   updated CountReferences signature.
4. Unit tests: keep LoopBodyBindingKept / WhileBodyBindingKept /
   LoopExprBindingKept / OutsideLoopStillInlined - these are the core
   regression tests for this PR.
Signed-off-by: Nikola Hristov <Nikola@PlayForm.Cloud>
@NikolaRHristov NikolaRHristov merged commit 62ad5a3 into Current May 25, 2026
2 checks passed
@NikolaRHristov NikolaRHristov deleted the fix/eliminate-loop-body-inline branch May 25, 2026 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant