Stream: contributing

Topic: Open up DecodeError tag

Karakatiza (Apr 06 2024 at 00:30):

In https://github.com/roc-lang/roc/blob/main/crates/compiler/builtins/roc/Decode.roc#L55,
DecodeError : [TooShort] is defined as a closed tag. This prevents ergonomic use with e.g. Task.fromResult to chain with previous tasks. Is this intentional, or could be updated to DecodeError : [TooShort]e ?

Brendan Hansknecht (Apr 06 2024 at 00:34):

Tags returned from functions should automatically become open. So this shouldn't be problem in practice. Though if it is hitting issues, we definitely want it to be flexible and merge

Karakatiza (Apr 06 2024 at 00:35):

For some reason it doesn't become open for me, so I had to come up with

decodeFromBytesPartial = \a, fmt -> {
    result: Decode.fromBytesPartial a fmt
    |> .result
    |> Result.mapErr
        (\e ->
            when e is
                TooShort -> TooShort),
}

as a workaround

Karakatiza (Apr 06 2024 at 00:37):

For example, right now I have

        body <-
            { Http.defaultRequest &
                url: "$(origin)/pipelines/$(name)",
            }
            |> Http.send
            |> await
        x : Pipeline -> Pipeline
        x = \a -> a
        body |> Str.toUtf8 |> decodeFromBytesPartial json |> .result |> Result.map x |> Task.fromResult

Please let me know If I am doing something in a non-idiomatic way

Luke Boswell (Apr 06 2024 at 01:01):

As a workaround I've been adding an underscore on the end of the tag unions to make them open. So for my tasks that return an error tag I'll do something like this myTask: Task {} [SomeError]_.

Luke Boswell (Apr 06 2024 at 01:05):

I think it is a bug, and there is an issue tracking it. I haven't found it to link here yet.

Trevor Settles (Jun 25 2024 at 20:20):

I think @Karakatiza's question is a good one. I came across this as well. With the current implementation the only error you can get is TooShort. It seems reasonable to me to either:

1. Make DecodeError an open union, or
2. DecodeResult val : { result : Result val DecodeError, rest : List U8 } could be generic over the error type

Either of these solutions (I think) would allow for implementations of the Decoding ability to define their own errors, and let users see errors other than just TooShort

Decode.roc

Brendan Hansknecht (Jun 26 2024 at 00:52):

I think both encode and decode need a revamp that models them closer to inspect. I think that will open up tons of currently missing flexibility.

Brendan Hansknecht (Jun 26 2024 at 00:52):

basically, free each encoder/decoder to choose their state type and final result type.

Brendan Hansknecht (Jun 26 2024 at 00:53):

So no more DecodeError only and no more List U8 only.

Brendan Hansknecht (Jun 26 2024 at 00:53):

I think that should be a very doable medium size project if anyone is interested.

Luke Boswell (Jun 26 2024 at 01:39):

It would be good if someone has capacity to write up a proposal on this.

Brendan Hansknecht (Jun 26 2024 at 01:55):

I'll write something with more detail up

Trevor Settles (Jun 26 2024 at 01:58):

I'd be happy to help! I don't have a ton of experience with this yet, but I've been looking for a problem in roc that directly affects me, and this one certainly has

Brendan Hansknecht (Jun 26 2024 at 04:40):

#ideas > Revamped Encode and Decode

Trevor Settles (Jul 02 2024 at 03:53):

So, I've gone through and fixed errors that I could find within the roc repo itself in this PR. However, I'm at the point where when I run cargo test I'm getting error from basic_cli's EnvDecoding.roc file. How should I go about updating those tests? It seems like I can't update roc itself because the tests depend on basic-cli, but basic-cli obviously depends on roc. Any suggestions, or previous workarounds?

One solutions I could see is making a new Decode/Encode package that is updated while keeping the old one. Then migrate basic-cli and anything else to the new one, then delete the old Encode/Decode

Luke Boswell (Jul 02 2024 at 04:04):

There's a couple of other PR's in the pipeline that are in the same place. Previously Anton has upgraded basic-cli and made a pre-release, then used that to upgrade roc, and then make the basic-cli release latest once the new nightly is available.

I think we could make a paired PR in basic-cli that has the latest Encode/Decode package.

Then roll all of the breaking changes into a single pre-release, along with task as builtin and the refactor into crates PRs.

Luke Boswell (Jul 02 2024 at 04:06):

We also have EnvDecoding.roc in basic-webserver, so it would be good to stage a PR there too

Trevor Settles (Jul 03 2024 at 21:47):

@Anton I've got a draft PR up in basic-cliat https://github.com/roc-lang/basic-cli/pull/225

I assume there are more changes needed to get it ready for review, since I haven't figured out how to actually build it yet

Luke Boswell (Jul 04 2024 at 23:22):

@Trevor Settles I recommend you either rebase or merge the refactor-host branch. In that and a recent PR we removed all the glue complexity and made the process for building the platform as simple as roc build.roc

Luke Boswell (Jul 04 2024 at 23:23):

https://github.com/roc-lang/basic-cli/pull/194

Luke Boswell (Jul 04 2024 at 23:24):

Depending on when you get to this it may have merged. I think Anton is updating basic-cli release 0.12.0 today and that is what the build script needs to run nicely.

Trevor Settles (Jul 12 2024 at 03:43):

So, I've got something working (I think) for just generic tags. It works for basic-cli's example/env.roc file, but when I try and run tests in roc-json after making similar changes, I get an error of internal error: entered unreachable code: no borrow signature for LambdaName { name: Decode.fromBytesPartial, niche: Niche(Captures([])) } layout. Is this an error that is expected with this kind of change, or is this change exposing something that was already broken?

Also, when I search for DecodeResult val fmt in the main repo, (the old syntax). I found a bunch of results that are in comments above rust code. Those comments should be DecodeResult val err fmt. What is the purpose of those comments? The rust code seems related, but it wasn't obvious to me what needed to change in those spots. I know I should update those comments, but do those comments hint at me needing to make rust change there?

Brendan Hansknecht (Jul 12 2024 at 03:59):

So based on my experiments of trying to implement one of these in roc userland, I think it is likely to get block on lambdaset bugs. Not 100% sure though cause inspect works which is theoretically similar.

Brendan Hansknecht (Jul 12 2024 at 04:21):

That borrow issue is really hiding the real issue which is that the compiler is generating wrong lambdaset signatures.

Brendan Hansknecht (Jul 12 2024 at 04:22):

That or, you care calling a function with wrong arguments, but the compiler is not generating a type mismatch

Brendan Hansknecht (Jul 12 2024 at 04:22):

So it is reaching the the borrow inference pass

Trevor Settles (Jul 16 2024 at 02:40):

Thanks for the insight! I'm leaning toward compiler bug in this case. I've only changed type annotations, so that shouldn't be causing any changes in what arguments get passed to functions. As long as my understanding is correct there.

Brendan Hansknecht (Jul 16 2024 at 02:42):

If you change the annotation such that the passed in arg no longer implements and ability or something, maybe...but yeah, probably blocked on compiler bugs sadly.

Brendan Hansknecht (Jul 16 2024 at 02:43):

When someone adds erased closures, it should unblock this work. That is in the pipeline.

Last updated: Jul 05 2025 at 12:14 UTC