Fix argument evaluation order under function inlining - #8572
Conversation
Lam_beta_reduce built the bindings for non-substitutable arguments by prepending during fold_left2 (reverse parameter order) and then wrapped the body with fold_right, making the first parameter's binding innermost - so the last argument was evaluated first. Fold left instead, so the first parameter's binding is outermost and arguments evaluate in call order. The reversed order was visible in checked-in output: bs_set_int_test.mjs evaluated setEqArray's second argument before its first. The new inline_arg_order_test fixture pins the order with recursive (hence non-substitutable) effectful arguments; its checked-in JS would show the regression directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
96751a9 to
9deb399
Compare
![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: 96751a9ee6
ℹ️ 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".
| } | ||
| } | ||
|
|
||
| let _ = { |
There was a problem hiding this comment.
This fixture evaluates the call for effA/effB side effects and intentionally discards its Boolean result, so let _ = ... is precisely the side-effect-only wildcard-binding pattern prohibited by the repository guidance. Use ignore(helper(effA(0), effB(0))) (or the equivalent pipeline form) so the intent is explicit without relying on a wildcard binding.
AGENTS.md reference: AGENTS.md:L43-L44
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8572 +/- ##
=======================================
Coverage 75.84% 75.84%
=======================================
Files 475 475
Lines 62835 62835
=======================================
Hits 47655 47655
Misses 15180 15180
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8572 |
2 participants
What
Lam_beta_reduce.propagate_beta_reduce(and its_with_mapvariant) built the let-bindings for non-substitutable inlined-call arguments by prepending duringfold_left2— leaving the list in reverse parameter order — and then wrapped the body withfold_right, which makes the first parameter's binding innermost. Net effect: when the inliner fires and arguments can't be substituted directly (anything that isn't a constant or variable), the last argument is evaluated first.Repro (current master):
The reversed order is visible in checked-in output today:
bs_set_int_test.mjsevaluatessetEqArray's second argument before its first (this PR regenerates it to call order).Fix
Fold left over the reversed binding list, so the first parameter's binding is outermost and arguments evaluate in call order. Two-line change, applied to both variants.
Tests
inline_arg_order_test.resfixture: an inlinable helper applied to recursive (hence non-substitutable) effectful arguments; the checked-in JS pinseffAbeforeeffB, so a regression flips the snapshot.bs_set_int_test.mjsregenerated to source order — the only other snapshot that changes across the full suite.Found while reviewing the generated-code changes of #8570 (which widens inliner exposure and would have made this reachable more often); the bug itself predates that stack and reproduces on master.
🤖 Generated with Claude Code