Add builtin `List.walkWithIndexUntil` #6258

sandprickle · 2023-12-12T04:07:42Z

First contribution to builtins. Open to any and all feedback!

(Contribution suggested here: https://roc.zulipchat.com/#narrow/stream/231634-beginners/topic/Sudoku.20Solver/near/407095970)

bhansconnect

Looks good. You will probably need to run ui and mono tests, but we'll see if CI passes or not.

If they fail just run cargo test -p test_mono -p uitest --no-fail-fast and commit the changes.

sandprickle · 2023-12-12T13:12:24Z

Hmm, won't have time to troubleshoot failing tests until this evening.

sandprickle · 2023-12-12T23:18:43Z

roc_repl_expect test lookup_clone_result is failing on my branch and on main for me. I seem to recall something similar happening when I was working on the abilities syntax change, but I can't remember how I fixed it.

sandprickle · 2023-12-13T12:06:09Z

cli_run::hello_gui Is failing in CI, but passing (in release mode) on both of my machines. Not sure how to troubleshoot.

The test that was failing CI before took a while to track down, because I wasn't using release mode initially, and there was another test that was failing in dev mode.

Anton-4 · 2023-12-13T16:15:35Z

This time it's not a flake:

$ cargo test-gen-llvm-wasm --locked --release
 ---- gen_list::list_walk_with_index_until_sum stdout ----
thread 'gen_list::list_walk_with_index_until_sum' panicked at 'assertion failed: `(left == right)`
  left: `676405809512461`,
 right: `13`: Wasm test failed', crates/compiler/test_gen/src/gen_list.rs:996:5

@brian-carroll do you have any ideas how 676405809512461could have been the output for:

List.walkWithIndexUntil [5, 7, 2, 3] 0 (\state, elem, index ->
            if elem % 2 == 0 then
                Break state
            else
                Continue (elem + index + state)

brian-carroll · 2023-12-14T07:45:13Z

Nope!

Anton-4 · 2023-12-15T12:54:42Z

For anyone willing to debug; I recommend:

Make sure the failing test reproduces locally, so cargo test-gen-llvm-wasm --locked --release gen_list::list_walk_with_index_until_sum
If it reproduces, try to simplify the test to find at what point we start getting wrong results.

sandprickle · 2023-12-15T16:33:24Z

I plan to start poking at the problem this weekend unless someone beats me to it.

sandprickle · 2023-12-16T16:53:15Z

Either a) omitting index from the sum:

List.walkWithIndexUntil [5, 7, 2, 3] 0 (\state, elem, index ->
    if elem % 2 == 0 then
        Break state
    else
        Continue (elem + state)

Or b) converting all ints to I64:

List.walkWithIndexUntil [5, 7, 2, 3] 0 (\state, elem, index ->
    if elem % 2 == 0 then
        Break state
    else
        a = Num.toI64 elem
        b = Num.toI64 state
        c = Num.toI64 index
        Continue (a + b + c)

result in a passing test.

So it appears there is some issue with adding an Int * to an Int Natural in wasm.

sandprickle · 2023-12-16T17:08:53Z

Hmm, this fails:

#[test]
#[cfg(any(feature = "gen-llvm", feature = "gen-wasm", feature = "gen-dev"))]
fn int_addition() {
    assert_evals_to!(
        r#"
        foo : Int Natural # !!
        foo = 1

        bar : Int *
        bar = 1

        foo + bar
        "#, 2, i64);
}

But this passes:

#[test]
#[cfg(any(feature = "gen-llvm", feature = "gen-wasm", feature = "gen-dev"))]
fn int_addition() {
    assert_evals_to!(
        r#"
        foo : Nat # !!
        foo = 1

        bar : Int *
        bar = 1

        foo + bar
        "#, 2, i64);
}

Anton-4 · 2023-12-16T17:13:34Z

Nice reduction, what's the result for foo + bar in the first case?

sandprickle · 2023-12-16T20:19:41Z

Oops, yeah that would be helpful wouldn't it 😅

---- gen_list::int_addition stdout ----
An internal compiler expectation was broken.
This is definitely a compiler bug.
Please file an issue here: https://github.com/roc-lang/roc/issues/new/choose
thread 'gen_list::int_addition' panicked at 'Alias `4.IdentId(83)` not registered in delayed aliases! [(`15.IdentId(35)`, Index(20), DelayedAliasVariables { start: 0, type_variables_len: 0, lambda_set_variables_len: 0, recursion_variables_len: 0, infer_ext_in_output_variables_len: 0 }, Opaque), (`13.IdentId(1)`, Index(22), DelayedAliasVariables { start: 0, type_variables_len: 1, lambda_set_variables_len: 0, recursion_variables_len: 0, infer_ext_in_output_variables_len: 2 }, Structural), (`9.IdentId(31)`, Index(32), DelayedAliasVariables { start: 3, type_variables_len: 0, lambda_set_variables_len: 0, recursion_variables_len: 0, infer_ext_in_output_variables_len: 0 }, Structural), (`9.IdentId(0)`, Index(35), DelayedAliasVariables { start: 3, type_variables_len: 2, lambda_set_variables_len: 0, recursion_variables_len: 0, infer_ext_in_output_variables_len: 0 }, Opaque), (`15.IdentId(4)`, Index(48), DelayedAliasVariables { start: 5, type_variables_len: 4, lambda_set_variables_len: 2, recursion_variables_len: 0, infer_ext_in_output_variables_len: 0 }, Structural), (`13.IdentId(2)`, Index(59), DelayedAliasVariables { start: 11, type_variables_len: 2, lambda_set_variables_len: 1, recursion_variables_len: 0, infer_ext_in_output_variables_len: 2 }, Opaque), (`9.IdentId(32)`, Index(78), DelayedAliasVariables { start: 16, type_variables_len: 0, lambda_set_variables_len: 0, recursion_variables_len: 0, infer_ext_in_output_variables_len: 0 }, Opaque), (`15.IdentId(1)`, Index(79), DelayedAliasVariables { start: 16, type_variables_len: 1, lambda_set_variables_len: 1, recursion_variables_len: 0, infer_ext_in_output_variables_len: 0 }, Opaque), (`13.IdentId(0)`, Index(83), DelayedAliasVariables { start: 18, type_variables_len: 0, lambda_set_variables_len: 0, recursion_variables_len: 0, infer_ext_in_output_variables_len: 1 }, Structural), (`9.IdentId(33)`, Index(85), DelayedAliasVariables { start: 19, type_variables_len: 0, lambda_set_variables_len: 0, recursion_variables_len: 0, infer_ext_in_output_variables_len: 0 }, Opaque), (`12.IdentId(0)`, Index(88), DelayedAliasVariables { start: 19, type_variables_len: 1, lambda_set_variables_len: 1, recursion_variables_len: 0, infer_ext_in_output_variables_len: 0 }, Opaque), (`15.IdentId(3)`, Index(95), DelayedAliasVariables { start: 21, type_variables_len: 3, lambda_set_variables_len: 2, recursion_variables_len: 0, infer_ext_in_output_variables_len: 0 }, Structural), (`10.IdentId(0)`, Index(105), DelayedAliasVariables { start: 26, type_variables_len: 1, lambda_set_variables_len: 0, recursion_variables_len: 0, infer_ext_in_output_variables_len: 0 }, Opaque)]', crates/compiler/solve/src/aliases.rs:250:25
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

backtrace:

stack backtrace:
   0: rust_begin_unwind
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/panicking.rs:593:5
   1: core::panicking::panic_fmt
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/core/src/panicking.rs:67:14
   2: roc_solve::aliases::Aliases::instantiate_real_var
   3: roc_solve::to_var::type_to_var_help
   4: roc_solve::to_var::type_to_var
   5: roc_solve::solve::solve
   6: roc_solve::solve::run
   7: roc_solve::module::run_solve
   8: roc_load_internal::file::run_solve_solve
   9: roc_load_internal::file::run_task
  10: roc_load_internal::file::load_single_threaded
  11: roc_load_internal::file::load
  12: roc_load::load_and_monomorphize_from_str
  13: test_gen::helpers::llvm::create_llvm_module
  14: test_gen::helpers::llvm::compile_to_wasm_bytes
  15: test_gen::helpers::llvm::assert_wasm_evals_to_help
  16: core::ops::function::FnOnce::call_once
  17: core::ops::function::FnOnce::call_once
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

sandprickle · 2023-12-16T20:34:15Z

Actually,

foo : Int Natural
foo = 1 
foo

Is enough to cause an error, and not just in wasm.

sandprickle · 2023-12-16T20:44:27Z

For what it's worth, doing a similar test for List.walkWithIndex fails in a similar way:

 List.walkWithIndex [5, 7, 2, 3] 0 (\state, elem, index ->
            state + elem + index
        )

result:

---- gen_list::list_walk_with_index_sum stdout ----
thread 'gen_list::list_walk_with_index_sum' panicked at 'assertion failed: `(left == right)`
  left: `676268370558999`,
 right: `23`: Wasm test failed', crates/compiler/test_gen/src/gen_list.rs:1016:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

So it looks like this issue is probably unrelated to the new function walkWithIndexUntil?

Anton-4 · 2023-12-18T15:16:24Z

So it looks like this issue is probably unrelated to the new function walkWithIndexUntil?

Yes indeed, I'd be ok with modifying the tests to avoid them failing using I64 for example and merging this in.

Can you make issues for the problems you encountered @sandprickle?

sandprickle · 2023-12-18T20:00:03Z

Put a bandaid on the test. @Anton-4 I'll try to get those issues filed tonight after work. Thanks!

Anton-4

Thanks @sandprickle!

sandprickle added 2 commits December 11, 2023 23:04

Add builtin List.walkWithIndexUntil

59eb28e

Merge branch 'main' into list-walk-with-index-until

89febf5

bhansconnect previously approved these changes Dec 12, 2023

View reviewed changes

run test_mono

80dd0b6

sandprickle dismissed bhansconnect’s stale review via 80dd0b6 December 12, 2023 12:42

sandprickle added 2 commits December 12, 2023 07:54

Merge branch 'main' into list-walk-with-index-until

2db4cae

re-run test-mono

8dbdd30

sandprickle enabled auto-merge December 12, 2023 13:00

sandprickle disabled auto-merge December 12, 2023 13:08

Merge branch 'main' into list-walk-with-index-until

fdd6bb6

don't run test for gen_wasm

99e596c

sandprickle requested a review from bhansconnect December 13, 2023 12:07

skip flaky failures

ec8467f

sandprickle added 2 commits December 18, 2023 11:24

modify test to sidestep wasm gen bug

12d1d73

Merge branch 'main' into list-walk-with-index-until

a710dd1

sandprickle enabled auto-merge December 19, 2023 02:36

Anton-4 approved these changes Dec 19, 2023

View reviewed changes

sandprickle merged commit 11e2fb5 into main Dec 19, 2023
17 checks passed

sandprickle deleted the list-walk-with-index-until branch December 19, 2023 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add builtin `List.walkWithIndexUntil` #6258

Add builtin `List.walkWithIndexUntil` #6258

sandprickle commented Dec 12, 2023

bhansconnect left a comment

sandprickle commented Dec 12, 2023

sandprickle commented Dec 12, 2023

sandprickle commented Dec 13, 2023

Anton-4 commented Dec 13, 2023

brian-carroll commented Dec 14, 2023

Anton-4 commented Dec 15, 2023

sandprickle commented Dec 15, 2023

sandprickle commented Dec 16, 2023 •

edited

Loading

sandprickle commented Dec 16, 2023

Anton-4 commented Dec 16, 2023

sandprickle commented Dec 16, 2023 •

edited

Loading

sandprickle commented Dec 16, 2023 •

edited

Loading

sandprickle commented Dec 16, 2023

Anton-4 commented Dec 18, 2023

sandprickle commented Dec 18, 2023

Anton-4 left a comment

Add builtin List.walkWithIndexUntil #6258

Add builtin List.walkWithIndexUntil #6258

Conversation

sandprickle commented Dec 12, 2023

bhansconnect left a comment

Choose a reason for hiding this comment

sandprickle commented Dec 12, 2023

sandprickle commented Dec 12, 2023

sandprickle commented Dec 13, 2023

Anton-4 commented Dec 13, 2023

brian-carroll commented Dec 14, 2023

Anton-4 commented Dec 15, 2023

sandprickle commented Dec 15, 2023

sandprickle commented Dec 16, 2023 • edited Loading

sandprickle commented Dec 16, 2023

Anton-4 commented Dec 16, 2023

sandprickle commented Dec 16, 2023 • edited Loading

sandprickle commented Dec 16, 2023 • edited Loading

sandprickle commented Dec 16, 2023

Anton-4 commented Dec 18, 2023

sandprickle commented Dec 18, 2023

Anton-4 left a comment

Choose a reason for hiding this comment

Add builtin `List.walkWithIndexUntil` #6258

Add builtin `List.walkWithIndexUntil` #6258

sandprickle commented Dec 16, 2023 •

edited

Loading

sandprickle commented Dec 16, 2023 •

edited

Loading

sandprickle commented Dec 16, 2023 •

edited

Loading