bhftbootcamp / Serde.jl

Serde is a Julia library for (de)serializing data to/from various formats. The library offers a simple and concise API for defining custom (de)serialization behavior for user-defined types
Apache License 2.0
31 stars 7 forks source link

Test case fails on julia 1.10 #39

Closed NeroBlackstone closed 2 months ago

NeroBlackstone commented 2 months ago

In test/deser.jl, test case No.30:

https://github.com/bhftbootcamp/Serde.jl/blob/6140ed063b0c4b2e73d31a86152c6febbcdeba18/test/deser.jl#L501-L504

In julia 1.10, it will fail:

Case №30: Deserializing missing and nothing: Test Failed at /home/nero/github/Serde.jl/test/deser.jl:501
  Expression: Serde.deser(Foo35_3, Dict{String, Any}("value" => 100))
    Expected: "WrongType: for 'Foo35_3' value '100' has wrong type 'value::Int64', must be 'value::Missing'"
     Message: "cannot convert a value to missing for assignment"

Stacktrace:
 [1] macro expansion
   @ ~/github/Serde.jl/test/deser.jl:501 [inlined]
 [2] macro expansion
   @ ~/.julia/juliaup/julia-1.10.3+0.x64.linux.gnu/share/julia/stdlib/v1.10/Test/src/Test.jl:1577 [inlined]
 [3] macro expansion
   @ ~/github/Serde.jl/test/deser.jl:472 [inlined]
 [4] macro expansion
   @ ~/.julia/juliaup/julia-1.10.3+0.x64.linux.gnu/share/julia/stdlib/v1.10/Test/src/Test.jl:1577 [inlined]
 [5] top-level scope
   @ ~/github/Serde.jl/test/deser.jl:7

This is because Julia 1.10 throw a different Error in src/de/deser.jl:

https://github.com/bhftbootcamp/Serde.jl/blob/6140ed063b0c4b2e73d31a86152c6febbcdeba18/src/De/Deser.jl#L249-L251

If we test this function:

deser(Missing,100)

Julia 1.10 will throw ERROR: cannot convert a value to missing for assignment

Julia 1.8 will throw ERROR: MethodError: convert(::Type{Union{}}, ::Int64) is ambiguous. Candidates

And in src/de/deser.jl, we are not handling ErrorException.

https://github.com/bhftbootcamp/Serde.jl/blob/6140ed063b0c4b2e73d31a86152c6febbcdeba18/src/De/Deser.jl#L494-L515

Question

How do you think it can be improved better? I can open a PR.

NeroBlackstone commented 2 months ago

A similar julia 1.10 test fail is No.37 in same test file.

Case №37: Deserialization Number to Number: Test Failed at /home/nero/github/Serde.jl/test/deser.jl:662
  Expression: Serde.deser_json(Message{Nothing}, exp_str)
    Expected: "WrongType: for 'Message{Nothing}' value 'Dict{String, Any}()' has wrong type 'payload::Dict{String, Any}', must be 'payload::Nothing'"
     Message: "cannot convert a value to nothing for assignment"

Stacktrace:
 [1] macro expansion
   @ ~/github/Serde.jl/test/deser.jl:662 [inlined]
 [2] macro expansion
   @ ~/.julia/juliaup/julia-1.10.3+0.x64.linux.gnu/share/julia/stdlib/v1.10/Test/src/Test.jl:1577 [inlined]
 [3] macro expansion
   @ ~/github/Serde.jl/test/deser.jl:655 [inlined]
 [4] macro expansion
   @ ~/.julia/juliaup/julia-1.10.3+0.x64.linux.gnu/share/julia/stdlib/v1.10/Test/src/Test.jl:1577 [inlined]
 [5] top-level scope
   @ ~/github/Serde.jl/test/deser.jl:7
dmitrii-doronin commented 2 months ago

Hi, @NeroBlackstone! I've seen this issue before and I have not been able to pinpoint the root cause. If you do plain conversion, outside of the package, it will work just fine. This behaviour sprung up after 1.10 release.

Currently it tries to convert after return (there's a specific syntax for that). I think, the solution would be to switch to explicit conversion within functions before return.

NeroBlackstone commented 2 months ago

I find the reason, convert() throws inconsistent errors in Julia 1.10.

I think the simplest solution is:

elseif (e isa MethodError) || (e isa InexactError) || (e isa ArgumentError) || (e isa ErrorException)

I have tested it and all passed, what do you think?

dmitrii-doronin commented 2 months ago

Have you tried explicit conversions? I think it might take more time but be more sustainable in the long run.

Also I'm baffled by the policy. It's not julia 2.0 to break existing error behaviours)

dmitrii-doronin commented 2 months ago

Also, @gryumov what's your stand on this? Apparently, there's an issue already on the official page.

NeroBlackstone commented 2 months ago

Also I'm baffled by the policy. It's not julia 2.0 to break existing error behaviours)

“Error types, unless explicitly documented (like DomainError or Base.IOError), are subject to change is the official policy”

Looks like they don't think this is a break change.😅

I don’t know if subsequent Julia versions will maintain this behavior.

NeroBlackstone commented 2 months ago

I think, the solution would be to switch to explicit conversion within functions before return.

Sorry, I don't quite understand what you are describing here. Do you mean to explicitly call the convert function to convert Any type to Missing before the derse function returns?

Or define a new convert function to throw MethodError ?

dmitrii-doronin commented 2 months ago

Okay, I see where the issue is. I'm taking a look. You can comment out these tests if you're actively developing. They don't affect the core functionality.

gryumov commented 2 months ago

Hello everyone! @dmitrii-doronin it might seem strange #40, but we need to deal with multiple dispatch, let's create issues for this:

https://github.com/bhftbootcamp/Serde.jl/actions/runs/8912634371/job/24476466918?pr=40