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

StaticArray serialization broken #15450

Open
wolfgang371 opened this issue Feb 11, 2025 · 7 comments
Open

StaticArray serialization broken #15450

wolfgang371 opened this issue Feb 11, 2025 · 7 comments

Comments

@wolfgang371
Copy link

Seems like serialization of StaticArray is broken.
Just manually including the modules doesn't fix it.

require "yaml"
require "json"

# monkey patching (w/o it doesn't even compile)
# BTW struct StaticArray(T, N) doc says:
# - "Instance methods inherited from class Object: [...] to_json, [...] to_yaml, [...]"
# - "Class methods inherited from class Object: from_json, from_yaml"
struct StaticArray(T, N)
    include JSON::Serializable
    include YAML::Serializable
end

class X
    include JSON::Serializable
    include YAML::Serializable
    @x : StaticArray(Int32, 4)
    property x = StaticArray[1,2,3,4]
    def initialize
    end
end

x = X.new
p X.from_yaml(x.to_yaml) # #<X:0x7f0a047e3fa0 @x=StaticArray[1, 0, 0, 0]>
p X.from_json(x.to_json) # Unhandled exception: Expected BeginObject but was BeginArray at line 1, column 7; parsing StaticArray(Int32, 4) at line 1, column 6 [...]

Using Crystal 1.15.1 [89944bf17] (2025-02-04), LLVM: 18.1.6, Default target: x86_64-unknown-linux-gnu

@wolfgang371 wolfgang371 added the kind:bug A bug in the code. Does not apply to documentation, specs, etc. label Feb 11, 2025
@Blacksmoke16 Blacksmoke16 added kind:feature topic:stdlib:serialization and removed kind:bug A bug in the code. Does not apply to documentation, specs, etc. labels Feb 11, 2025
@Blacksmoke16
Copy link
Member

I think this is more of a feature because as you found out you can't currently deserialize into a StaticArray. But is also TBD if that's something we even want to allow. I recall a previous discussion around this but can't seem to find it...

@HertzDevil
Copy link
Contributor

*::Serializable is for your own classes or structs, primitive types always define the (de-)serialization methods directly, e.g. src/json/from_json.cr and src/yaml/to_yaml.cr

@straight-shoota
Copy link
Member

Unfortunately, the #to_json and #to_yaml methods are defined on Object and thus inherited by every type. That causes confusion. See #5695
A type actually provides serialization only if it implements #to_json(JSON::Builder) and #to_yaml(YAML::Nodes::Builder), respectively.

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Feb 20, 2025

I suppose we could serialize a StaticArray(T, N) for any N size. This is transforming the static array to a dynamic array.

sa = StaticArray(Int32, 2).new { |i| i }
json = sa.to_json # => "[0,1]"

We could deserialize to a StaticArray(T, N) since N is known; though that's a fixed size, so we'd have to raise if the actual number of items isn't exactly N (no undefined values on underflow + no buffer overflow).

StaticArray(Int32, 2).from_json("[0,1]")   # OK
StaticArray(Int32, 2).from_json("[0]")     # ERROR
StaticArray(Int32, 2).from_json("[0,1,2]") # ERROR

@straight-shoota
Copy link
Member

straight-shoota commented Feb 20, 2025

Serialization and specifically deserialization for StaticArray into JSON seems a bit of an odd feature.
Before looking into how we could implement the, I think we should consider if that's even very useful.

@wolfgang371 Do you have a practical use case for this?

@wolfgang371
Copy link
Author

@straight-shoota Well, I expected Crystal to have serialization to work out of the box for all internal types, including StaticArray (all the while knowing that value based objects can bring you trouble when being used in a nested way).
My use case was having sort of a fingerprint stored inside a class, where StaticArray seemed most natural to me.

@straight-shoota
Copy link
Member

Yeah I guess it makes sense to have it. And it is possible to implement. I suppose there's some similarity to a char(n) etc. in SQL Vs varchar and text.

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

No branches or pull requests

5 participants