Skip to content

estdlib: add lists:mapfoldr/3#2350

Open
pguyot wants to merge 1 commit into
atomvm:release-0.7from
pguyot:w27/estdlib-lists-mapfoldr
Open

estdlib: add lists:mapfoldr/3#2350
pguyot wants to merge 1 commit into
atomvm:release-0.7from
pguyot:w27/estdlib-lists-mapfoldr

Conversation

@pguyot

@pguyot pguyot commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

lists provided mapfoldl/3 but not its right-to-left counterpart mapfoldr/3. Add it (mirroring OTP semantics: the accumulator visits the list from right to left) so that, among others, Elixir list comprehensions, which translate to lists:mapfoldr/3, can be evaluated on AtomVM.

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

lists provided mapfoldl/3 but not its right-to-left counterpart
mapfoldr/3. Add it (mirroring OTP semantics: the accumulator visits
the list from right to left) so that, among others, Elixir list
comprehensions, which translate to lists:mapfoldr/3, can be evaluated
on AtomVM.

Signed-off-by: Paul Guyot <pguyot@kallisys.net>
@petermm

petermm commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

PR Review — estdlib: add lists:mapfoldr/3

Commit: 264c91156eac5c1c5129a6c91e1bfbcf62b5c2a9
Author: Paul Guyot
Files: libs/estdlib/src/lists.erl, tests/libs/estdlib/test_lists.erl, CHANGELOG.md (+29 lines)

Verdict

Correct and mergeable. The implementation matches OTP semantics for proper lists, the test is wired into the runner, and the CHANGELOG entry is well-placed. The findings below are optional improvements — none are blockers.

Verified behavior

Case This impl OTP Match
mapfoldr(fun(X,A)->{X*A,A+1} end,1,[1,2,3]) {[3,4,3],4} {[3,4,3],4}
mapfoldr(F,1,[]) {[],1} {[],1}
mapfoldr(F,1,foo) function_clause function_clause

Accumulator order is right-to-left (visits 3, then 2, then 1) exactly as OTP — no ordering bug.


Findings

1. Suggestion — body recursion vs. AtomVM's stack-safety convention

The new mapfoldr/3 is body-recursive (one stack frame per element before Fun is applied), mirroring OTP. That is fine on a desktop, but this module deliberately avoids deep body recursion for the embedded target: the adjacent foldr/3 is implemented as foldl(Fun, Acc0, reverse(List)), and mapfoldl/3 uses an accumulator + reverse. On a microcontroller with a small stack, a long input list can overflow.

A stack-safe, OTP-semantics-preserving version (reverse the input, walk it tail-recursively so Fun is still called right-to-left):

 -spec mapfoldr(fun((A, Acc) -> {B, Acc}), Acc, [A]) -> {[B], Acc}.
-mapfoldr(_Fun, Acc0, []) ->
-    {[], Acc0};
-mapfoldr(Fun, Acc0, [H | T]) ->
-    {Bs, Acc1} = mapfoldr(Fun, Acc0, T),
-    {B, Acc2} = Fun(H, Acc1),
-    {[B | Bs], Acc2}.
+mapfoldr(Fun, Acc0, List) ->
+    mapfoldr0(Fun, ?MODULE:reverse(List), [], Acc0).
+
+mapfoldr0(Fun, [H | T], Bs, Acc0) ->
+    {B, Acc1} = Fun(H, Acc0),
+    mapfoldr0(Fun, T, [B | Bs], Acc1);
+mapfoldr0(_Fun, [], Bs, Acc0) ->
+    {Bs, Acc0}.

Trade-off / caveat: AtomVM's reverse/1 NIF raises badarg on a non-list, whereas the current body-recursive version raises function_clause (see the documented divergence on reverse/1 at libs/estdlib/src/lists.erl:172-175). Adopting this means the foo test must drop the reason:

-    ?ASSERT_ERROR(lists:mapfoldr(fun(X, A) -> {X * A, A + 1} end, 1, foo), function_clause),
+    ?ASSERT_ERROR(lists:mapfoldr(fun(X, A) -> {X * A, A + 1} end, 1, foo)),

This is exactly the same badarg-vs-function_clause compromise foldr/3 already accepts, so it is consistent with the module. Judgment call: if the author prefers the closest possible OTP match (including the error reason), keeping the current body-recursive form is defensible. If matching the module's own stack-safety convention matters more, apply the diff above.

2. Nit — OTP is_function/2 guard leniency (pre-existing, not introduced here)

OTP guards the empty-list clause with when is_function(Fun, 2), so mapfoldr(foo, 1, []) raises function_clause. This impl returns {[], 1} instead. This is a genuine OTP divergence, but mapfoldl/3 already has the identical leniency, so the new code is consistent with its neighbor. Fixing it here alone would be inconsistent; fixing both is out of scope for this small commit. Flagging only for awareness — safe to leave as-is.

3. Nit — test organization

test_mapfoldr/0 is invoked from the tail of test_mapfoldl/0 rather than directly from test/0, which hides the new function's coverage alongside the others. Minor, but a direct call reads better:

 test_mapfoldl() ->
     ?ASSERT_MATCH({[], 1}, lists:mapfoldl(fun(X, A) -> {X * A, A + 1} end, 1, [])),
     ?ASSERT_MATCH(
         {[1, 4, 9], 4},
         lists:mapfoldl(fun(X, A) -> {X * A, A + 1} end, 1, [1, 2, 3])
     ),
     ?ASSERT_ERROR(lists:mapfoldl(fun(X, A) -> {X * A, A + 1} end, 1, foo), function_clause),
-    ok = test_mapfoldr(),
     ok.
     ok = test_mapfoldl(),
+    ok = test_mapfoldr(),

(Requires test_mapfoldr to be reachable from test/0 — see the export/dispatch list at the top of test_lists.erl.)


Summary

  • Correctness: no bugs. Result and accumulator order match OTP for proper lists.
  • Main call: decide whether stack safety (Finding 1) matters for the embedded target — the rest of the module suggests it might.
  • CHANGELOG: correct (### Added).
  • Everything else is cosmetic and safe to merge as-is.

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.

2 participants