roc test crash · bugs · Zulip Chat Archive

$ roc test Aoc/Year2023/Day09.roc
thread 'main' panicked at crates/compiler/load_internal/src/file.rs:373:85:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
^C

drew (Oct 22 2024 at 01:41):

drew (Oct 22 2024 at 01:42):

$ roc test
0 failed and 5 passed in 423 ms.

Luke Boswell (Oct 22 2024 at 02:03):

I'm guessing a bug in the implementation of that feature for roc test to take a file path as an argument

Luke Boswell (Oct 22 2024 at 02:04):

drew (Oct 22 2024 at 02:04):

drew (Nov 26 2024 at 15:46):

any updates on https://github.com/roc-lang/roc/issues/7175? i'd love to try roc for aoc this year, but the LSP constantly crashes when opening files in my multi-file project. happy to wait until next year if it's not the right time.

drew (Nov 26 2024 at 15:47):

Anton (Nov 26 2024 at 16:10):

I can take a look at it after I'm done with the breaking changes update, although others are welcome to investigate this high priority issue

Anthony Bullard (Nov 28 2024 at 14:28):

Looks like it may be an issue with something holding an extra reference to an Arc that contains the module ids. When i finish up the snake_case work I can take a look

Anthony Bullard (Nov 28 2024 at 14:47):

Any pointers you can give on tracking down these sort of references would be helpful.

Anton (Nov 28 2024 at 14:52):

Looking at the authors of the surrounding code, it's possible that @Ayaz Hafiz, @Richard Feldman or @Folkert de Vries have some good tips.

Anthony Bullard (Nov 28 2024 at 15:13):

Ok, checking this locally it looks like I'm seeing a different error with the latest. Now the issue is that there is a dependency that hasn't been solved when type checking completes.

Step(pf.PlatformTasks, SolveTypes): NotStarted,
Step(pf., SolveTypes): NotStarted,
Step(pf.Stderr, SolveTypes): NotStarted,
Step(pf., CanonicalizeAndConstrain): NotStarted,
Step(pf.Stderr, CanonicalizeAndConstrain): NotStarted,
Step(pf.PlatformTasks, CanonicalizeAndConstrain): Pending,
Step(pf.InternalHttp, SolveTypes): Pending,
Step(pf.InternalCommand, SolveTypes): Pending,
Step(pf.InternalPath, SolveTypes): Pending,

Anthony Bullard (Nov 28 2024 at 16:56):

It seems that it is adding the platform modules to be solved, but since they are never used in checking this leaf module, we never typecheck. It seems that we should only add a Canonicalize/SolveTypes Steps when the module is required.

Anthony Bullard (Nov 30 2024 at 12:58):

I'm really struggling with this. It seems that the use case is valid (single file checking), but when we decide to add the msgs for the platform modules, we don't know if the file we are checking even needs them. So the solutions I see are:

I'd like to get some thoughts from some of the core team on this. @Anton @Brendan Hansknecht @Richard Feldman (Sorry, I don't know all who's on the core team).

Anthony Bullard (Nov 30 2024 at 16:58):

Regardless, I'd probably need a little walk through to understand all the things I need to think about regarding this

Brendan Hansknecht (Nov 30 2024 at 17:37):

So the issue is that to check the validity of a single roc file, we also have to check the platform in order to fully solve the type information?

Anthony Bullard (Nov 30 2024 at 17:43):

So in file.rs::update when we determine that have finished all type checking because the root_id equals the module_id and some other things, the platform modules have not been solved and we hit an assert that checks that and blows up.

thread 'main' panicked at crates/compiler/load_internal/src/file.rs:2532:17:
assertion failed: state.dependencies.solved_all()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

If you ignore this like I'm doing right now when the root type is a module, you get the original issue where there are outstanding references (Arc clones) to state.derived module.

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>
Outstanding references to the derived module
Location: crates/compiler/load_internal/src/file.rs:3325:29

Anthony Bullard (Nov 30 2024 at 17:44):

We don't need to (from a functional standpoint) check the platform since it's not used. But this assert makes us have to (probably to ensure all check errors are returned).

Anthony Bullard (Nov 30 2024 at 17:46):

It seems like the check is there to prevent this latter message from happening (clearing all the work will cause all of the clones to be dropped).

Anthony Bullard (Nov 30 2024 at 17:46):

It's a lot of different concepts to swim through when I get about 30mins at a time to look into it.

drew (Nov 30 2024 at 23:21):

$ roc --version
roc nightly pre-release, built from commit d72da8eb0fb on Fri Nov 29 09:25:13 UTC 2024

Anthony Bullard (Dec 01 2024 at 00:06):

It sounds silly but add an import for any module from your platform to the file and it’s good to go

drew (Dec 01 2024 at 00:07):

drew (Dec 01 2024 at 00:11):

drew (Dec 01 2024 at 00:12):

drew (Dec 01 2024 at 00:16):

i'd be more than happy to do a workaround for now if i can have working LSP for advent of code :smile:

drew (Dec 01 2024 at 00:21):

ok weird, this did work for the LSP! but the command line roc check still fails

Anthony Bullard (Dec 01 2024 at 00:26):

That’s weird. The roc check will fail now because you have an unused import, but it shouldn’t crash

drew (Dec 01 2024 at 00:27):

$ roc check Aoc/Year2023/Day09.roc
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>
Outstanding references to the derived module
Location: crates/compiler/load_internal/src/file.rs:3311:29

drew (Dec 01 2024 at 00:27):

$ head Aoc/Year2023/Day09.roc
module [part1, part2]

import pf.Stdout

nextVal : List I64 -> I64
nextVal = \l ->
    if List.all l \n -> n == 0 then
        0
    else
        diffs = List.map2 l (List.dropFirst l 1) \a, b -> b - a

Anthony Bullard (Dec 01 2024 at 00:27):

drew (Dec 01 2024 at 00:27):

Anthony Bullard (Dec 01 2024 at 00:27):

Anthony Bullard (Dec 01 2024 at 00:28):

drew (Dec 01 2024 at 00:28):

Anthony Bullard (Dec 01 2024 at 00:28):

drew (Dec 01 2024 at 00:28):

Anthony Bullard (Dec 01 2024 at 00:29):

Anthony Bullard (Dec 01 2024 at 00:30):

Anthony Bullard (Dec 01 2024 at 00:55):

I doubt I’ll have much time tonight to work on this, but if I have a commit with a fix, I’ll share the SHA here

Anthony Bullard (Dec 01 2024 at 01:58):

Anthony Bullard (Dec 01 2024 at 02:01):

Don't have time for that, here's a patch that you can git apply if you have a local repo:

diff --git a/crates/compiler/load_internal/src/file.rs b/crates/compiler/load_internal/src/file.rs
index ff7ce3a50c..ba8c47c95f 100644
--- a/crates/compiler/load_internal/src/file.rs
+++ b/crates/compiler/load_internal/src/file.rs
@@ -658,7 +658,7 @@ struct CanAndCon {
     module_docs: Option<ModuleDocumentation>,
 }

-#[derive(Debug)]
+#[derive(Debug, PartialEq, Eq)]
 enum PlatformPath<'a> {
     NotSpecified,
     Valid(To<'a>),
@@ -1141,7 +1141,6 @@ impl<'a> LoadStart<'a> {

         // Load the root module synchronously; we can't proceed until we have its id.
         let root_start_time = Instant::now();
-
         let load_result = load_filename(
             arena,
             filename.clone(),
@@ -1286,7 +1285,6 @@ fn handle_root_type<'a>(
                 if let (Some(main_path), Some(cache_dir)) = (main_path.clone(), cache_dir) {
                     let mut messages = Vec::with_capacity(4);
                     messages.push(header_output.msg);
-
                     load_packages_from_main(
                         arena,
                         src_dir.clone(),
@@ -2245,7 +2243,9 @@ fn update<'a>(

                     // If we're building an app module, and this was the platform
                     // specified in its header's `to` field, record it as our platform.
-                    if state.opt_platform_shorthand == Some(config_shorthand) {
+                    if state.opt_platform_shorthand == Some(config_shorthand)
+                        || state.platform_path == PlatformPath::RootIsModule
+                    {
                         debug_assert!(state.platform_data.is_none());

                         state.platform_data = Some(PlatformData {
@@ -2339,7 +2339,6 @@ fn update<'a>(
                 extend_module_with_builtin_import(parsed, ModuleId::INSPECT);
                 extend_module_with_builtin_import(parsed, ModuleId::TASK);
             }
-
             state
                 .module_cache
                 .imports

Anthony Bullard (Dec 01 2024 at 02:05):

Anthony Bullard (Dec 01 2024 at 02:06):

drew (Dec 01 2024 at 02:07):

Anthony Bullard (Dec 01 2024 at 02:07):

I'm going to assume I've made a mistake here and some tests will fail, or it will fail on a larger test case.

Anthony Bullard (Dec 01 2024 at 02:08):

@Luke Boswell If you have a moment to take a look at that PR and see if there isn't something obvious I'm going to mess up here?

Luke Boswell (Dec 01 2024 at 02:09):

Luke Boswell (Dec 01 2024 at 02:10):

Anthony Bullard (Dec 01 2024 at 02:12):

Anthony Bullard (Dec 01 2024 at 02:13):

Luke Boswell (Dec 01 2024 at 02:13):