Luke Boswell (Apr 23 2024 at 11:14): Luke Boswell (Apr 23 2024 at 11:14):I have been thinking about how we could get an intermediate step towards Task as builtin.
What if we just made InternalTask a builtin. We could assume that all platforms will have an Effects.roc file.
This would mean that users could write main = Stdout.line! "foo" without importing pf.Task, and also the platform main wouldn't need to import Task either I think.
interface InternalTask
exposes [Task, fromEffect, toEffect, ok, err]
imports [Effect.{ Effect }]
Task ok err := Effect (Result ok err)
ok : a -> Task a *
ok = \a -> @Task (Effect.always (Ok a))
err : a -> Task * a
err = \a -> @Task (Effect.always (Err a))
fromEffect : Effect (Result ok err) -> Task ok err
fromEffect = \effect -> @Task effect
toEffect : Task ok err -> Effect (Result ok err)
toEffect = \@Task effect -> effect
I think we'd need to name it Task rather than InternalTask, but yeah that could work!
Richard Feldman (Apr 23 2024 at 11:15):(otherwise if you write Task.mapErr it wouldn't work)
Luke Boswell (Apr 23 2024 at 11:16):Ahk, in that case we would need to include all the things... like loop, mapErr etc and this would be common accross all platforms.
Richard Feldman (Apr 23 2024 at 11:55):yeah that'll happen with the transition to builtin Task anyway :big_smile:
Isaac Van Doren (May 16 2024 at 02:28):I was taking a look at what is necessary to implement Task as a builtin going off of this message:
Richard Feldman said:
yeah basically task as builtin (on its own) just requires:
- add the
Taskmodule to builtins- instead of generating
Effectinhostedmodules we generateTask- hosts have to update their effect implementations accordingly (basically now they'll always be dealing with a
Result)
If I'm understanding correctly, in the platforms we need to update Effect.roc to generate Task instead of Effect. This is just renaming things right?
Right now the Task module uses Effect and InternalTask from the platform. What I don't understand is how this will work once the Task module no longer has access to the platform. Won't it need to know about the Task type that is defined in Effect.roc because it is opaque?
I would appreciate other details about what needs to be done also :smiley:
Ayaz Hafiz (May 17 2024 at 19:52):Is any generation of effectful code/tasks in hosted modules required if task is a builtin? I would have guessed it isn't but maybe im missing something
Agus Zubiaga (May 17 2024 at 23:13):Yeah, I think the idea is that hosted modules wouldn’t generate any types, and their members could return Task directly.
Ayaz Hafiz (May 17 2024 at 23:36):As for actually implementing this, yeah you could copy over the implementation of task into the standard library. You'll also want to define the after and so on methods in Roc source, which you can find as generated by the compiler today here https://github.com/roc-lang/roc/blob/main/crates/compiler/can/src/effect_module.rs
Isaac Van Doren (May 20 2024 at 00:45):Okay cool, that makes sense! I got nerd sniped by another project, but once I finish that I might try to implement this. If anyone else wants to do it before I get to it, go for it
Isaac Van Doren (Jun 12 2024 at 02:13):What would be a good way to call the functions created in effect_module.rs from the builtins? Is there already infrastructure setup to do this or will new stuff need to be put in place?
Luke Boswell (Aug 29 2024 at 23:42):@Brendan Hansknecht I think we need to add you Heap implementation to the False interpreter host. It's so we can handle refcounting on the FileHandle correctly as I suspect that is causing our issue there.
The added benefit is that if we remove the basic-cli tests from roc-lang/roc, at least we will have a good way to continue testing that and keep it in sync with roc.
Luke Boswell (Aug 29 2024 at 23:43):There's probably a way to do it without all that extra ceremony, i.e. convert a *mut BufReader<File> to a RocBox<()>... but my rust foo isn't good enough yet.
Sam Mohr (Aug 29 2024 at 23:47):I'll try it
Luke Boswell (Aug 30 2024 at 00:04):This hasn't really helped -- https://github.com/roc-lang/roc/commit/fc8c46beee6c9b6ae7f54b50a322c2e6bab90390
$ cargo test -p roc_cli cli_run::false_interpreter
Finished test [unoptimized + debuginfo] target(s) in 0.51s
Running unittests src/lib.rs (target/debug/deps/roc_cli-7e72f520dc480795)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 4 filtered out; finished in 0.00s
Running tests/cli_run.rs (target/debug/deps/cli_run-f57f6c0a8cfd68c7)
running 1 test
test cli_run::false_interpreter ... FAILED
failures:
---- cli_run::false_interpreter stdout ----
thread 'cli_run::false_interpreter' panicked at crates/cli/tests/cli_run.rs:314:17:
> expected output to end with:
1414
> but instead got:
> stderr was:
[src/lib.rs:256:11] File::open(string) = Ok(
File {
fd: 3,
path: "/Users/luke/Documents/GitHub/roc/examples/cli/false-interpreter/examples/sqrt.false",
read: true,
write: false,
},
)
[src/lib.rs:83:5] &format!("Panic: {}", &*msg) = "Panic: unreachable: File.open"
Application hit a panic: unreachable: File.open
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
At this point, I should probably figure out cargo and make that a proper rust psckage
Brendan Hansknecht (Aug 30 2024 at 00:08):Or at least make it downloadable from a github url
Luke Boswell (Aug 30 2024 at 00:08):We could add it as a crate in roc-lang/roc, and then it's easy to use from there
Luke Boswell (Aug 30 2024 at 00:09):Makes sense, because it's specific to RocBox.
Sam Mohr (Aug 30 2024 at 00:11):That would work!
Brendan Hansknecht (Aug 30 2024 at 00:34):Oh yeah. That sounds like a good idea
Luke Boswell (Aug 30 2024 at 00:35):I'm an a branch off from Task as builtin, I'll move it into a crate.
I have fixed a couple of things up in the False platform, but haven't resolved the main issue.
Luke Boswell (Aug 30 2024 at 00:36):I'm at the stage where I think I need to inspect the LLVM IR to see what roc is actually generating for the effect roc_fx_openFile, though I dont' fully know how to do that properly.
Brendan Hansknecht (Aug 30 2024 at 00:37):Should just be build --emit-llvm-ir then search for roc_fx_openFile
Luke Boswell (Aug 30 2024 at 00:38):Oh yeah, I've got that part ok... it's the knowing how to interpret the IR
Luke Boswell (Aug 30 2024 at 00:38)::sweat_smile:
Brendan Hansknecht (Aug 30 2024 at 00:40):Paste the function signature here and we can help guess
Luke Boswell (Aug 30 2024 at 00:43):FileHandle := Box {}
openFile : Str -> Task FileHandle {}
lib.rspub extern "C" fn roc_fx_openFile(name: &RocStr) -> RocResult<RocBox<()>, ()>
declare { [0 x i64], [1 x i64], i8, [7 x i8] } @roc_fx_openFile(ptr)
define internal fastcc void @roc_fx_openFile_fastcc_wrapper(ptr %0, ptr %1)
entry:
%tmp = call { [0 x i64], [1 x i64], i8, [7 x i8] } @roc_fx_openFile(ptr %0), !dbg !117
store { [0 x i64], [1 x i64], i8, [7 x i8] } %tmp, ptr %1, align 8, !dbg !117
ret void, !dbg !117
}
define internal fastcc void @PlatformTasks_task_closure_openFile_e93bf924107d8e1718c9a20bb89a13fa86198933bf2155d2785223e43180({} %"48", ptr %closure_arg_openFile_0, ptr %0) !dbg !116 {
entry:
%result_value = alloca { [0 x i64], [1 x i64], i8, [7 x i8] }, align 8
call fastcc void @roc_fx_openFile_fastcc_wrapper(ptr %closure_arg_openFile_0, ptr nonnull %result_value), !dbg !117
call fastcc void @"#Attr_#dec_7"(ptr %closure_arg_openFile_0), !dbg !117
call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 8 dereferenceable(16) %0, ptr noundef nonnull align 8 dereferenceable(16) %result_value, i64 16, i1 false), !dbg !117
ret void, !dbg !117
}
That looks correct to me
Luke Boswell (Aug 30 2024 at 01:18):And this looks ok too right?
Handle := PlatformTasks.FileHandle
open : Str -> Task Handle *
open = \path ->
PlatformTasks.openFile path
|> Task.map @Handle
|> Task.mapErr \{} -> crash "unreachable: File.open"
I'm running out of ideas here... I'm thinking the problem must be inside roc at this point.
Luke Boswell (Aug 30 2024 at 01:31):Ok I've pushed the investigation stuff to https://github.com/roc-lang/roc/tree/builtin-task-wip
Luke Boswell (Aug 30 2024 at 01:31):I didn't want to force push onto Sam's branch in case we decide we don't want to go this path.
Luke Boswell (Aug 30 2024 at 01:31):Anton had reverted Sam's last change, and hasn't had a chance to look at this since then.
Sam Mohr (Aug 30 2024 at 01:32):Okay, so I might have it working??
Sam Mohr (Aug 30 2024 at 01:32):I did the AtomicU64 trick for file handle keys in a static hashmap
Sam Mohr (Aug 30 2024 at 01:33):But when I return Task U64 {}, there's a segfault. When I return Task {} {}, it works.
Luke Boswell (Aug 30 2024 at 01:33):Interesting... checkout the changes I made to main on :point_up: I fixed an issue around calling main in there
Sam Mohr (Aug 30 2024 at 01:33):So I just have to figure this last issue out
Luke Boswell (Aug 30 2024 at 01:34):you could git cherry-pick e60450dc9617465e8b1223a266d59e266c362914 probably, I put it all in that one commit
Sam Mohr (Aug 30 2024 at 01:38):Doing that now!
Luke Boswell (Aug 30 2024 at 01:48):Ok, Sam's latest (commit 74f9b993887a4a8f5096b70394a0446faf61805c) on my linux x64 dev machine native (not nix)
Just the false interpreter failure.
$ cargo test -p roc_cli
---- cli_run::false_interpreter stdout ----
thread 'cli_run::false_interpreter' panicked at crates/cli/tests/cli_run.rs:261:29:
`valgrind` exited with no exit code. valgrind stdout was: ""
valgrind stderr was: "==42266== If you believe this happened as a result of a stack
==42266== overflow in your program's main thread (unlikely but
==42266== possible), you can try to increase the size of the
==42266== main thread stack using the --main-stacksize= flag.
==42266== The main thread stack size used in this run was 8388608.
"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
cli_run::false_interpreter
test result: FAILED. 57 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 37.77s
I'll run the full test suite now, just to check everything else is ok
Brendan Hansknecht (Aug 30 2024 at 01:50):Oh, is this a new issue?
Brendan Hansknecht (Aug 30 2024 at 01:50):Like different than the one above
Sam Mohr (Aug 30 2024 at 01:50):Yep
Brendan Hansknecht (Aug 30 2024 at 01:50):on which branch?
Sam Mohr (Aug 30 2024 at 01:50):Not sure how so
Sam Mohr (Aug 30 2024 at 01:50):https://github.com/smores56/roc/tree/builtin-task
Sam Mohr (Aug 30 2024 at 01:50):The PR's branch
Luke Boswell (Aug 30 2024 at 01:54):I think it's the same issue. I scrolled back up through my shell history and it looks the same as where we started today.
Sam Mohr (Aug 30 2024 at 01:54):Welp
Sam Mohr (Aug 30 2024 at 01:55):I presumed it was different because of the changes I made, I was not confident in it being different.
Luke Boswell (Aug 30 2024 at 01:56):Well, this is consistent behaviour though. We now have two branches, with two different approaches to the platform side implementaiton and both kind of suggest an alignment bug inside roc
Sam Mohr (Aug 30 2024 at 01:57):I haven't merged in main personally in a while
Sam Mohr (Aug 30 2024 at 01:57):This might be the bug that Brendan already fixed for us, unless someone else has merged in for me
Sam Mohr (Aug 30 2024 at 01:57):Let me try that
Luke Boswell (Aug 30 2024 at 02:01):It didn't help on my other branch... I've got failures in two other test platforms though. Looks like this.
$ cargo test -p roc_cli interactive_effects
<---- snipped --->
Roc standard library crashed with message
voided tag constructor is unreachable
What's interesting about this is that the effects platform is a zig platform. But it also currently has mainForHost : Task {} []. I'm going to update that and see it it changes the error message at all.
Luke Boswell (Aug 30 2024 at 02:03):I don't exactly know how to handle roc giving the host a refcounted RocResult in zig. But I'll give it a go
Sam Mohr (Aug 30 2024 at 02:04):Okay, I got the right crash back. Which isn't great, but it should be more fixable
Sam Mohr (Aug 30 2024 at 02:04):failures:
---- cli_run::false_interpreter stdout ----
thread 'cli_run::false_interpreter' panicked at crates/cli/tests/cli_run.rs:314:17:
> expected output to end with:
1414
> but instead got:
> stderr was:
Application hit a panic: unreachable File.open
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Pushed that version to the builtin-task branch
Brendan Hansknecht (Aug 30 2024 at 02:06):Yeah, this crash is really confusing me. At a quick glance to the llvm ir, it feels like the ir is correct
Brendan Hansknecht (Aug 30 2024 at 02:06):But nothing seems wrong with the platform or any of the types
Luke Boswell (Aug 30 2024 at 02:07):Linux x64 native commit 9263986433531cb4c0f2d78729dbdbb706c6a5d5
$ cargo test -p roc_cli
<--- snip---->
---- cli_run::false_interpreter stdout ----
thread 'cli_run::false_interpreter' panicked at crates/cli/tests/cli_run.rs:261:29:
`valgrind` exited with no exit code. valgrind stdout was: ""
valgrind stderr was: "thread panicked while processing panic. aborting.
"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
cli_run::false_interpreter
test result: FAILED. 57 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 38.66s
Luke, are you on my latest commit?
Sam Mohr (Aug 30 2024 at 02:09):The answer is yes
Sam Mohr (Aug 30 2024 at 02:10):---- cli_run::false_interpreter stdout ----
thread 'cli_run::false_interpreter' panicked at crates/cli/tests/cli_run.rs:314:17:
> expected output to end with:
1414
> but instead got:
handle id: (Err {})
> stderr was:
Application hit a panic: unreachable File.open
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Currently, if I print out the returned Result U64 {} value, it returns Err {}
Sam Mohr (Aug 30 2024 at 02:12):And the Rust side calls panic! on failure, so it's not being returned
Sam Mohr (Aug 30 2024 at 02:12):dbg! verifies this
Brendan Hansknecht (Aug 30 2024 at 02:15):Yeah, feels like it should either be a bad mapping for the return type leading to roc reading the wrong memory as the returned result or a bug in roc leading to incorrect handling of the task after it is loaded
Sam Mohr (Aug 30 2024 at 02:15):Could be the latter, but in most of my testing of this feature it was the former
Luke Boswell (Aug 30 2024 at 02:15):Valgrind is angry https://gist.github.com/lukewilliamboswell/086f5cb00b923a6269fc16a2ea662104
Brendan Hansknecht (Aug 30 2024 at 02:16):can you show the result from an optimized build as well?
Luke Boswell (Aug 30 2024 at 02:18):Is this normal??
lb-dev@lb-dev:~/github/roc$ valgrind examples/cli/false-interpreter/False examples/cli/false-interpreter/examples/sqrt.false &> out2.txt
Aborted (core dumped)
lb-dev@lb-dev:~/github/roc$ head out2.txt
==71529== Memcheck, a memory error detector
==71529== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==71529== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==71529== Command: examples/cli/false-interpreter/False examples/cli/false-interpreter/examples/sqrt.false
==71529==
==71529== Conditional jump or move depends on uninitialised value(s)
==71529== at 0x14DAA9: ??? (in /home/lb-dev/github/roc/examples/cli/false-interpreter/False)
==71529== by 0x14C585: ??? (in /home/lb-dev/github/roc/examples/cli/false-interpreter/False)
==71529== by 0x4887B17: ??? (in /usr/lib/x86_64-linux-gnu/libc.so.6)
==71529== by 0x1FFEFFFBEF: ???
Valgrind crashing?
Brendan Hansknecht (Aug 30 2024 at 02:19):Normal? no. Happens? on occasion
Luke Boswell (Aug 30 2024 at 02:19):Here's an optimized version https://gist.github.com/lukewilliamboswell/ac614630e785ac6592497432031697d2
Brendan Hansknecht (Aug 30 2024 at 02:23):So probably one of the things making valgrind angry is roc reading the wrong memory at some point leading to it seeing Err {} instead of the correct result from the file task
Brendan Hansknecht (Aug 30 2024 at 02:24):Is valgrind happy with all of the other examples? I would guess so cause you said this is the only failing test case?
Brendan Hansknecht (Aug 30 2024 at 02:26):I would guess this is some sort of regression with deeply nested lambdasets cause that is what makes false special (terible code with tons of copies, allocations, and lambdasets).
Brendan Hansknecht (Aug 30 2024 at 02:26):but not really sure at this piont
Brendan Hansknecht (Aug 30 2024 at 02:27):Also... :tear: [1] 1329912 IOT instruction valgrind examples/cli/false-interpreter/False &> out.txt I can't even run valgrind on the false intrpreter from the builtin-task-wip branch
Luke Boswell (Aug 30 2024 at 02:28):That's my other branch...
Luke Boswell (Aug 30 2024 at 02:28):I've also got issues with other test platforms on that one
Brendan Hansknecht (Aug 30 2024 at 02:30):I'll get the proper branch, just more effort cause it is on a fork
Brendan Hansknecht (Aug 30 2024 at 02:38):Same on the proper branch....
Sam Mohr (Aug 30 2024 at 02:38):Hmmmmm
Brendan Hansknecht (Aug 30 2024 at 02:38):let me fully clean...maybe a cached artifact or something
Sam Mohr (Aug 30 2024 at 02:39):I think your theory of false-interpreter -> unification issue could be it
Brendan Hansknecht (Aug 30 2024 at 02:41):In general, I feel like this is a real bug and not something false interpreter specific, but I also think that it may not be worth fixing (for this pr). It might get fixed with the future lambdaset cleanup work.
Brendan Hansknecht (Aug 30 2024 at 02:41):It might be a super big time sync....hard to say.
Brendan Hansknecht (Aug 30 2024 at 02:42):Do we have an extra level of closures in tasks now due to this change? Or is the builtin type still the same as with effect?
Sam Mohr (Aug 30 2024 at 02:43):Literally a copy of Effect
Sam Mohr (Aug 30 2024 at 02:43):The only difference being that we've changed to assume everything is a Result, instead of arbitrary types.
Sam Mohr (Aug 30 2024 at 02:43):So not literally a copy of Effect, but literally a copy of Effect, if you catch my drift.
Sam Mohr (Aug 30 2024 at 02:44):Yes, I'd like to see if fixing the lambda set issues will fix this, it feels unrelated.
Brendan Hansknecht (Aug 30 2024 at 02:47):If it is the only blocking test, I say table it. :fingers_crossed: this isn't something common in the wild when this goes out.
Sam Mohr (Aug 30 2024 at 02:47):Okay, I'll disable it and let Anton know in the comments of the PR
Brendan Hansknecht (Aug 30 2024 at 02:48):My guess is we are holding lambdasets a little bit differently and they are breaking. There are a lot of captures and any one could be messed up. So let's look at it again when we fix up lambdasets
Luke Boswell (Aug 30 2024 at 04:24):I'm afraid this may be related to Task as Builtin... so I'm posting here.
I've been working on a PR to add support for Json Web Tokens to basic-webserver
https://github.com/roc-lang/basic-webserver/pull/72
I'm getting a random segfault using the builtin task branch.
The builtin task PR is a branch on my fork, but I've made this PR on roc-lang/basic-webserver so it's easier to see -- but it's the last 2 commits and based off the builtin-task branch.
Luke Boswell (Aug 30 2024 at 04:28):It doesn't do anything yet, just sets up the interface between roc and the host and stubs out the impl.
Luke Boswell (Aug 30 2024 at 04:32):Yeah, I'm really hoping I've done something obviously wrong...
Luke Boswell (Aug 30 2024 at 04:34):Ok, I might be doing something strange... built a clean version on linux and it's happy. So I must have a cache or something strange on mac. :fingers_crossed:
Luke Boswell (Aug 30 2024 at 04:36):Yeah false alarm :sweat_smile: cleaned out my cargo cache and rebuilt everything and it's looking fine.
Brendan Hansknecht (Aug 30 2024 at 04:37):Yay
Sam Mohr (Aug 30 2024 at 04:38):My heart
Luke Boswell (Aug 30 2024 at 04:38):Sorry to do this to you guys.
Sam Mohr (Aug 30 2024 at 04:39):Luckily you've already bled for this feature change, so I can't be mad
Luke Boswell (Aug 30 2024 at 04:39):I don't think I'll be able to sleep until Anton hit's merge on that PR... :pray:
Luke Boswell (Aug 30 2024 at 04:40):I don't know why I keep testing things... I'm bound to find an issue.
Luke Boswell (Aug 30 2024 at 06:40):There is something really messed up with our cargo setup for basic-webserver.
I've been trying to add a single test that I can run using cargo test and it's complicated. I haven't been able to achieve it yet. :sad:
Luke Boswell (Aug 30 2024 at 06:41):I was wanting to write a unit test for each of the JWT algorithms, but I think I'm limited to doing it all in a roc example for now which isn't ideal.
Sam Mohr (Aug 30 2024 at 16:50):@Anton for the basic-webserver task branch, there's a TODO to use a proper basic-cli task release for scripts/testing. Do you want to wait to prepare a release, or am I good to just cleanup everything, leave the 0.15-testing release, and get ready for merge after approval?
Anton (Aug 30 2024 at 16:51):I was just about to start building basic-cli 0.15 :)
Sam Mohr (Aug 30 2024 at 16:53):Well then!
Sam Mohr (Aug 30 2024 at 16:53):I'll still do everything else, we can update the final download link at that point
Last updated: Jul 06 2025 at 12:14 UTC