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

Test case fails on julia 1.10 #39

Closed
NeroBlackstone opened this issue May 1, 2024 · 9 comments · Fixed by #40
Closed

Test case fails on julia 1.10 #39

NeroBlackstone opened this issue May 1, 2024 · 9 comments · Fixed by #40
Assignees

Comments

@NeroBlackstone
Copy link
Contributor

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

Serde.jl/test/deser.jl

Lines 501 to 504 in 6140ed0

@test_throws "WrongType: for 'Foo35_3' value '100' has wrong type 'value::Int64', must be 'value::Missing'" Serde.deser(
Foo35_3,
Dict{String,Any}("value" => 100),
)

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:

Serde.jl/src/De/Deser.jl

Lines 249 to 251 in 6140ed0

function deser(::Type{T}, data::D)::T where {T<:Missing,D<:Any}
return data
end

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.

Serde.jl/src/De/Deser.jl

Lines 494 to 515 in 6140ed0

function eldeser(
structtype::Type,
elmtype::Type,
key::K,
val::V,
) where {K<:Union{AbstractString,Symbol},V<:Any}
return try
if val isa elmtype
val
else
deser(structtype, elmtype, val)
end
catch e
if isnothing(val)
throw(ParamError("$(key)::$(elmtype)"))
elseif (e isa MethodError) || (e isa InexactError) || (e isa ArgumentError)
throw(WrongType(structtype, key, val, typeof(val), elmtype))
else
rethrow(e)
end
end
end

Question

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

@NeroBlackstone
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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

@NeroBlackstone
Copy link
Contributor Author

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
Copy link
Contributor Author

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 dmitrii-doronin self-assigned this May 1, 2024
@dmitrii-doronin
Copy link
Contributor

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 added a commit that referenced this issue May 1, 2024
gryumov added a commit that referenced this issue May 1, 2024
@gryumov gryumov mentioned this issue May 1, 2024
4 tasks
@gryumov
Copy link
Member

gryumov commented May 1, 2024

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

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 a pull request may close this issue.

3 participants