roc-lang / roc

A fast, friendly, functional language.
https://roc-lang.org
Universal Permissive License v1.0
3.86k stars 284 forks source link

Crashed with Malformed Suffix #6838

Closed fwgreen closed 4 days ago

fwgreen commented 1 week ago
app [main] {
    pf: platform "https://github.com/roc-lang/basic-cli/releases/download/0.10.0/vNe6s9hWzoTZtFmNkvEICPErI9ptji_ySjicO6CkucY.tar.br",
}

import pf.Stdout
import pf.Task
import pf.Utc

Option a : [Some a, None]

main =
    start = Utc.toMillisSinceEpoch Utc.now! 

    list = List.range { start: At 0i64, end: At 1_000_000i64 }

    results = List.map list \i -> binarySearch list i

    dbg List.len results

    dbg (Utc.toMillisSinceEpoch Utc.now!) - start

    Stdout.line! "Done"

binarySearch : List I64, I64 -> Option I64 
binarySearch = \list, value ->
    find : I64, I64 -> Option I64
    find = \lowerBound, upperBound ->
        if lowerBound < upperBound then 
            None 
        else
            midPoint = (upperBound + lowerBound) // 2
            when List.get list (Num.toU64 midPoint) is 
                Ok x ->
                    when Num.compare x value is
                        LT -> find lowerBound (midPoint - 1)
                        GT -> find (midPoint + 1) upperBound
                        EQ -> Some midPoint
                _ -> None

    find 0 (Num.toI64 (List.len list) - 1)
── UNUSED DEFINITION in main.roc ───────────────────────────────────────────────

binarySearch is not used anywhere in your code.

27│  binarySearch = \list, value ->
     ^^^^^^^^^^^^

If you didn't intend on using binarySearch then remove it so future
readers of your code don't wonder why it is there.

── UNUSED IMPORT in main.roc ───────────────────────────────────────────────────

Stdout is imported but not used.

6│  import pf.Stdout
    ^^^^^^^^^^^^^^^^

Since Stdout isn't used, you don't need to import it.

── UNUSED IMPORT in main.roc ───────────────────────────────────────────────────

Utc is imported but not used.

8│  import pf.Utc
    ^^^^^^^^^^^^^

Since Utc isn't used, you don't need to import it.

────────────────────────────────────────────────────────────────────────────────

0 errors and 3 warnings found in 254 ms
.

Running program…

────────────────────────────────────────────────────────────────────────────────
Roc crashed with:

        MalformedSuffixed(@309-585)

Here is the call stack that led to the crash:

        roc.panic
        app.main
        .mainForHost
        rust.main

Optimizations can make this list inaccurate! If it looks wrong, try running without `--optimize` and with `--linker=legacy`

roc nightly pre-release, built from commit d47a073634e on Tue Jun 25 09:12:30 UTC 2024 on macOs 14.5

Anton-4 commented 1 week ago

Thanks for reporting this @fwgreen!

In case it may help, this is a workaround:

dbg List.len results

end = Utc.toMillisSinceEpoch Utc.now!

dbg end - start
Anton-4 commented 1 week ago

Minimal reproduction for future debugging:

app [main] { pf: platform "https://github.com/roc-lang/basic-cli/releases/download/0.10.0/vNe6s9hWzoTZtFmNkvEICPErI9ptji_ySjicO6CkucY.tar.br" }

import pf.Stdout
import pf.Task
import pf.Utc

main =
    dbg Utc.now!

    Stdout.line! "Done"
kdziamura commented 1 week ago

The corresponding place in compiler:

https://github.com/roc-lang/roc/blob/7e609bfdbf37b51dd8ae576462a99b7ff404ca63/crates/compiler/can/src/suffixed.rs#L133-L137

It was probably done to not mess with control flow, but I can't say precisely. @lukewilliamboswell any insights? Should we add better logging here or make dbg work with the bang?

lukewilliamboswell commented 1 week ago

It's an error. Dbg shouldn't be used with a bang suffix.

We should improve the error message. I think we just left it as something basic like MalformedSuffix and haven't added any actual error reports yet.

Anton-4 commented 6 days ago

Dbg shouldn't be used with a bang suffix.

Can you expand on that @lukewilliamboswell? It seems like we can make it work and I think it's a feature users would like to have.

lukewilliamboswell commented 6 days ago

I think I need to defer to @rtfeldman here. It may be possible to unwrap it, but I think it also may be a deliberate design choice.

For example


# this could in theory
main =
    dbg Utc.now!

    Stdout.line! "Done"

# unwrap to
main =
    Task.await Utc.now \answer -> 
        dbg answer

        Stdout.line! "Done"

But do we want this?

kdziamura commented 6 days ago

Yeah, as I mentioned, it leads to control flow change. I think it makes sense that dbg is expected to be transparent for control flow. But dbg with bang requires the change only to make dbg work.

Upd. However, the alternative is to introduce the await manually

rtfeldman commented 5 days ago

But do we want this?

I think we want it! 👍

kdziamura commented 5 days ago

Ok, I'll add the fix

kdziamura commented 4 days ago

@rtfeldman I'd assume expect shouldn't support bangs inside? My intuition is if you await something for expect, you intend to use it later. But maybe there are special cases?