Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decoding of records with missing fields. #6587

Merged
merged 23 commits into from
Apr 15, 2024

Conversation

faldor20
Copy link
Collaborator

@faldor20 faldor20 commented Mar 15, 2024

This is a test implementation of decoding records where some fields may not exist in the encoded data.

Before giving up decoding it will attempt to run the decoder for any missing field with a 0 byte input: Decode.decodeWith [] Decode.decoder fmt and if the decoder returns a success it will put that in the field.

This allows us to define types that have specific behavior when decoding nothing. eg:

Option val := [None, Some val]
#...
optionDecode = Decode.custom \bytes, fmt ->
    if bytes |> List.len == 0 then
        { result: Ok (@Option (None)), rest: [] }
    else
        when bytes |> Decode.decodeWith (Decode.decoder) fmt is
            { result: Ok res, rest } -> { result: Ok (@Option (Some res)), rest }
            { result: Err a, rest } -> { result: Err a, rest }

As we can see, if no bytes are given we just output a "None" value

A full code example:
Option val := [None, Some val]
    implements [
        Eq {
            isEq: optionEq,
        },
        Decoding {
            decoder: optionDecode,
        },
    ]

none = \{} -> @Option None
some = \a -> @Option (Some a)
isNone = \@Option opt ->
    when opt is
        None -> Bool.true
        _ -> Bool.false

optionEq = \@Option a, @Option b ->
    when (a, b) is
        (Some a1, Some b1) -> a1 == b1
        (None, None) -> Bool.true
        _ -> Bool.false

optionDecode = Decode.custom \bytes, fmt ->
    if bytes |> List.len == 0 then
        { result: Ok (@Option (None)), rest: [] }
    else
        when bytes |> Decode.decodeWith (Decode.decoder) fmt is
            { result: Ok res, rest } -> { result: Ok (@Option (Some res)), rest }
            { result: Err a, rest } -> { result: Err a, rest }

# Now I can try to modify the json decoding to try decoding every type with a zero byte buffer and see if that will decode my field
OptionTest : { y : U8, maybe : Option U8 }
expect
    decoded : Result OptionTest _
    decoded = "{\"y\":1}" |> Str.toUtf8 |> Decode.fromBytes TotallyNotJson.json
    dbg "hil"

    expected = Ok ({ y: 1u8, maybe: none {} })
    isGood =
        when (decoded, expected) is
            (Ok a, Ok b) ->
                a == b

            _ -> Bool.false
    isGood == Bool.true
OptionTest2 : { maybe : Option U8 }
expect
    decoded : Result OptionTest2 _
    decoded =
        """
        {"maybe":1}
        """
        |> Str.toUtf8
        |> Decode.fromBytes TotallyNotJson.json
    dbg "hil"

    expected = Ok ({ maybe: some 1u8 })
    expected == decoded

@lukewilliamboswell
Copy link
Collaborator

Testing this PR with the below example and a minor update to roc-json to pass arguments to the finaliser.

# switch to PR branch
cargo run -- run optional-decoding.roc
(Ok {count: 12, optional64: (@Option None)})
app "optional-decode"
    packages {
        cli: "../basic-cli/platform/main.roc",
        json: "../roc-json/package/main.roc",
    }
    imports [cli.Stdout, json.Core.{ json }]
    provides [main] to cli

main =

    input = "{ \"count\": 12 }" |> Str.toUtf8
    
    decoded : DecodeResult { optional64: Option U64, count: U32  }
    decoded = Decode.fromBytesPartial input json

    decoded.result
    |> Inspect.toStr 
    |> Stdout.line

Option val := [None, Some val] implements [
    Decoding { decoder: optionDecode },
    Inspect, # auto derive
]

optionDecode : Decoder (Option val) fmt where val implements Decoding, fmt implements DecoderFormatting
optionDecode = Decode.custom \bytes, fmt ->
    if bytes |> List.isEmpty then
        { result: Ok (@Option (None)), rest: [] }
    else
        when bytes |> Decode.decodeWith (Decode.decoder) fmt is
            { result: Ok res, rest } -> { result: Ok (@Option (Some res)), rest }
            { result: Err a, rest } -> { result: Err a, rest }

@faldor20 faldor20 force-pushed the optional-decoding-works branch 2 times, most recently from 3fc93ad to 2565580 Compare March 17, 2024 06:19
@faldor20
Copy link
Collaborator Author

Tests are currently failing because the basic-cli has an "Env" decoder in it and needs to have the type annotation for it updated to reflect the fact that finalize has 2 params now

@faldor20 faldor20 marked this pull request as ready for review March 17, 2024 10:30
@Anton-4
Copy link
Contributor

Anton-4 commented Mar 22, 2024

Are you ok with this change in general @rtfeldman, see also zulip? If so, I'll do the full review of this, do a new basic-cli release, etc. .

@rtfeldman
Copy link
Sponsor Contributor

I'm ok with it if @ayazhafiz is ok with it! 👍

Copy link
Sponsor Member

@ayazhafiz ayazhafiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach overall looks good. I would really prefer to have the implementation all in one file. It would make the diff easier to review and I don't think there is any benefit in splitting up the derivation across multiple files.

crates/compiler/derive/src/decoding/record/finalizer.rs Outdated Show resolved Hide resolved
crates/compiler/derive/src/decoding/record/finalizer.rs Outdated Show resolved Hide resolved
crates/compiler/derive/src/decoding/record/finalizer.rs Outdated Show resolved Hide resolved
\#Derived.stateRecord ->
when #Derived.stateRecord.first is
\#Derived.stateRecord, #Derived.fmt ->
when when #Derived.stateRecord.first is
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not important for this PR but this formatting should be fixed.

@faldor20
Copy link
Collaborator Author

faldor20 commented Mar 23, 2024

This approach overall looks good. I would really prefer to have the implementation all in one file. It would make the diff easier to review and I don't think there is any benefit in splitting up the derivation across multiple files.

I find enormous single files make it annoying to navigate through and find the parts I want, and I like how breaking things up makes it more clear when things are isolated from each other or dependent.

But yeah it does make reviewing harder. I did do the splitting at the very end so I'm very happy to recombine if you like, or I can do the splitting in another PR. Just let me know :)

@Anton-4
Copy link
Contributor

Anton-4 commented Mar 30, 2024

What do you think of @faldor20's suggestions @ayazhafiz?

Anton-4
Anton-4 previously approved these changes Apr 8, 2024
Copy link
Contributor

@Anton-4 Anton-4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @faldor20! I'll get started on a new basic-cli release.

Anton-4 added a commit to roc-lang/basic-cli that referenced this pull request Apr 12, 2024
 EnvDecoding type matches changes to decoding from here: roc-lang/roc#6587
@Anton-4 Anton-4 changed the title Decoding of records with missing fields. WIP Decoding of records with missing fields. Apr 13, 2024
@Anton-4 Anton-4 changed the title WIP Decoding of records with missing fields. Decoding of records with missing fields. Apr 13, 2024
Anton-4
Anton-4 previously approved these changes Apr 13, 2024
Copy link
Contributor

@Anton-4 Anton-4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @faldor20 and everyone else involved ❤️

@Anton-4 Anton-4 changed the title Decoding of records with missing fields. WIP Decoding of records with missing fields. Apr 13, 2024
@Anton-4
Copy link
Contributor

Anton-4 commented Apr 13, 2024

I'm going to merge this Monday morning, because the tutorial in this PR is already updated for basic-cli 0.9.0, but that won't work unless you have Monday's nightly.

@Anton-4 Anton-4 changed the title WIP Decoding of records with missing fields. DO NOT MERGE Decoding of records with missing fields. Apr 13, 2024
Anton-4 added a commit to roc-lang/basic-webserver that referenced this pull request Apr 13, 2024
@Anton-4 Anton-4 changed the title DO NOT MERGE Decoding of records with missing fields. Decoding of records with missing fields. Apr 15, 2024
@Anton-4 Anton-4 merged commit 972b254 into roc-lang:main Apr 15, 2024
17 checks passed
@faldor20
Copy link
Collaborator Author

@Anton-4 Thanks a lot for all your work getting this deployed everywhere :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants