Kilian Vounckx (May 28 2023 at 15:01): Kilian Vounckx (May 28 2023 at 15:01):As far as I know, when you want to update a single item in a List at the moment, you have to first get it with List.get, then update it somehow, and afterwards set it again with List.set. You could do this in a single expression, but this gets really verbose. Especially because List.get returns a Result.
I propose a new function List.updateAt. The name could be changed of course (maybe without the 'At'?). Its signature would be updateAt : List a, Nat, (a -> a) -> List a. The second argument is the index for the item you want to update. If it is out of bounds, the list remains unchanged like in List.set. The third argument obviously specifies how to update the item.
I think this should be in the standard library. What do you all think?
Brendan Hansknecht (May 28 2023 at 15:47):It was mentioned recently in another thread. I definitely am for it. Slightly question if it would be more surprising if it doesn't update. As in, should it give a result. Also, I think just List.update should be the name.
Though I do think whatever we pick, we should consider the API or Dict.update. I would assume they should have a similar API, but maybe that isn't necessary.
Kilian Vounckx (May 28 2023 at 15:53):Just took a look at Dict.update. It seems to take a function with signature [Missing, Present a] -> [Missing, Present a]. I don't think this can be translated to List.update, because the equivalent of Missing is Err OutOfBounds. Doing anything in this case does not make sense.
Thinking about having it List.update return an error does make sense. I don't know which is best. I just thought having it do nothing in case of out of bounds would be similar to List.set.
Brendan Hansknecht (May 28 2023 at 16:06):Ah, yeah, i guess you can't do anything if missing
Brendan Hansknecht (May 28 2023 at 16:06):That's fair
Brendan Hansknecht (May 28 2023 at 16:06):So yeah, API sounds good
Kilian Vounckx (May 28 2023 at 16:10):should this be implemented in rust/zig, or in roc? It can be done in roc using the combination of List.get and List.set, but then it would do two bounds checks I think
Brendan Hansknecht (May 28 2023 at 16:11):I think llvm should optimize that away.
Brendan Hansknecht (May 28 2023 at 16:12):Can always be updated to zig later if it is measurable on perf.
Kilian Vounckx (May 28 2023 at 16:12):I looked at the file where List.get and List.set are implemented. I think if you use getUnsafe it can be done in roc without the bounds check. Is this correct? Or does getUnsafe do something else?
Kilian Vounckx (May 28 2023 at 16:12):Or do you think using the safe versions is still better?
Brendan Hansknecht (May 28 2023 at 16:14):Oh yeah, get unsafe does that, but i think you need set unsafe
Brendan Hansknecht (May 28 2023 at 16:14):Cause you would get, on failure return the original list. On success run the function and call set unsafe
Kilian Vounckx (May 28 2023 at 16:14):there seems to be no setUnsafe, but there is replaceUnsafe
Kilian Vounckx (May 28 2023 at 16:15):This increments the reference count of the returned value though which is not ideal I think
Brendan Hansknecht (May 28 2023 at 16:15):Yeah, replace is our primitive, so that should do what you need.
Kilian Vounckx (May 28 2023 at 16:16):Alright, I will tackle this tonight and submit a PR
Kilian Vounckx (May 28 2023 at 16:16):Thanks for the info I needed
Brendan Hansknecht (May 28 2023 at 16:16):Hmm. That's an interesting note on replace, i wonder if that is affecting perf
Brendan Hansknecht (May 28 2023 at 16:16):Since it is used to build set which then would just decrement the refcojnt
Kilian Vounckx (May 28 2023 at 16:17):I honestly have no idea, but I guess it could affect it. Only benchmarks could show it. But thinking about it, it would increment the count, just to decrement it directly because the value is not used. LLVM should optimize this away as well?
Brendan Hansknecht (May 28 2023 at 16:25):Llvm is terrible at optimizing stuff like that due to touching memory. Also, refcount update and decrements are (at least currently) recursive. I actually may have a good app to test if this is measurable. Been messing with lists with expensive refcount updates in an interpreter recently. I guess I should test this theory and see what the affect it.
Brendan Hansknecht (May 28 2023 at 16:26):Anyway, don't worry about it for your change
Kilian Vounckx (May 28 2023 at 17:51):Starting now. For tests, should I do them in the roc file itself with expect, in rust in crates/compiler/test_gen/src/gen_list.rs, or in both?
Brendan Hansknecht (May 28 2023 at 18:01):Expect should be fine
Brendan Hansknecht (May 28 2023 at 18:02):The only advantage of gen_list is that it runs on wasm. Not important for something like this.
Kilian Vounckx (May 28 2023 at 18:04):I ran cargo test without changing anything first. I get this error:
thread 'gen_tags::issue_5162_recast_nested_nullable_unwrapped_layout' has overflowed its stack
fatal runtime error: stack overflow
error: test failed, to rerun pass `-p test_gen --test test_gen`
Should I worry about it?
Kilian Vounckx (May 28 2023 at 18:40):Pull request made: https://github.com/roc-lang/roc/pull/5466
Kilian Vounckx (May 28 2023 at 18:43):Just as a bit of feedback. I made two PR's a few months ago. Since then the CONTRIBUTING.md file seems to have been updated. It has really helped me! This link in particular was awesome. Roc is the first open source project I contributed to so having this stuff for beginners really helps!
Brendan Hansknecht (May 28 2023 at 19:01):I am gonna guess that that test needs to be run in release mode and we just don't have the attributes to ignore it in debug mode
Brendan Hansknecht (May 28 2023 at 19:01):It's probably safe to ignore
Kilian Vounckx (Jun 01 2023 at 07:38):I see the pull request has been waiting for approval for two days?
Is there something that still needs to be done for it?
I totally get it if there are other priorities for this. I just wanted to make sure you are not waiting for me to do something still
Ayaz Hafiz (Jun 01 2023 at 12:37):The changes look good, but can you address the CI failures?
Brendan Hansknecht (Jun 01 2023 at 14:20):Sorry, i just didn't check the PR again. Left a comment.
Kilian Vounckx (Jun 01 2023 at 16:00):No problem! will look at it. I thought I ran the tests, but not properly apperently
Brendan Hansknecht (Jun 01 2023 at 16:01):Maybe merging main caused them to change more. Not sure.
Kilian Vounckx (Jun 01 2023 at 16:01):Could be, I am running them again
Kilian Vounckx (Jun 01 2023 at 16:02):I am merging main with the sync button on github. Could that be a problem? Or should I better merge manually in the terminal?
Brendan Hansknecht (Jun 01 2023 at 16:03):Shouldn't make a difference. Just if something related to list tests was merged in, it would change the mono test numbers more.
Kilian Vounckx (Jun 01 2023 at 16:04):I get this error now: thread 'record_update' panicked at 'Output changed: resolve conflicts and git add the file.', crates/compiler/test_mono/src/tests.rs:232:9. Didn't get it the first time
Kilian Vounckx (Jun 01 2023 at 16:04):How do I resolve the conflicts?
Brendan Hansknecht (Jun 01 2023 at 16:38):Just git add the changed file
Brendan Hansknecht (Jun 01 2023 at 16:38):These changes are expected
Brendan Hansknecht (Jun 01 2023 at 16:39):After committing the new changes, the test should pass.
Kilian Vounckx (Jun 01 2023 at 16:40):Oh yeah, even after just adding (not committing) it works.
I didn't want to try this yet in case I had to revert afterwards
Kilian Vounckx (Jun 01 2023 at 16:41):So now it is 25 commits behind. Do I just sync with main again?
Brendan Hansknecht (Jun 01 2023 at 16:53):Don't worry about it. If the tests pass, it should merge fine
Brendan Hansknecht (Jun 01 2023 at 16:53):I think those changes are totally unrelated to this and from dev backend stuff.
Kilian Vounckx (Jun 01 2023 at 16:54):Alright, thanks for all the help! Hopefully next time will be smoother
Last updated: Jun 16 2026 at 16:19 UTC