JuliaData / StructTypes.jl

Abstract definitions and convenience methods for describing, processing, and constructing Julia objects
https://juliadata.github.io/StructTypes.jl/stable/
MIT License
80 stars 22 forks source link

Default to `StructTypes.Struct()`? #61

Closed ericphanson closed 2 years ago

ericphanson commented 3 years ago

I wonder if StructType should default to StructTypes.Struct() since that seems to be the most common option. Then probably in more cases JSON serialization will "just work". Though maybe it will be less discoverable how to customize it since you won't get the ArgumentError: X doesn't have a definedStructTypes.StructType` error.

jakobnissen commented 3 years ago

This issue comes from a thread on Slack with the following problem: Suppose you make a package that uses a type from a foreign package, and you want to serialize it to JSON. As far as I can tell, this is only possible either by modifying the package to add StructType(::Type{TheirType}) = ..., or by engaging in type piracy.

For most use cases, the type piracy can be avoided if StructTypes has a default representation for unknown objects.

quinnj commented 3 years ago

Yeah, this came up before, but we thought it may lead to more problems than it solves. For example, for "interface" types, if the default is Struct, then it can make things really weird; an example would be if a Dict got serialized as Struct, where you have the slots, keys, worldage etc fields.

So you could end up in a scenario where things "work", like a type serializes, but then deserialization doesn't work because they may not have a constructor with all these internal fields. And then you have to dig down in a stack of types until you find one that is being serialized via Struct and causing problems. As opposed to just getting an error that this nested type doesn't have a StructType defined.

It's a tradeoff. I wonder if we could make it configurable, like maybe we could allow JSON3.write(x; default=StructTypes.Struct) and keep the default JSON3.write(x; default=StructTypes.NoStruct).

ericphanson commented 3 years ago

Hm, yeah. I was thinking the dispatches for things like AbstractDict might catch many of the common cases, but I didn’t think of the non-deserialisability angle for when something isn’t caught. That does sound pretty bad.

The default sounds okay though. At least opt-in means maybe the user will read the doc string first and can read a warning or such.

If the main goal is to allow customisation without piracy, maybe one needs to be able to define StructTypes options without creating new methods for the global one? One option could be to have a macro that creates a brand new set of functions in the users module that fallback to calling the usual StructTypes ones but the user could add methods to without affecting the StructTypes functions. In other words, automated shadowing like

f(args…) = StructTypes.f(args…)

so the user has their own f to add methods to. That might not play well with more nuanced dispatch though, since you’d only hit the predefined methods on the generic fallback. And then there’d have to be some way to get JSON etc to actually use these functions instead of the real ones…

quinnj commented 3 years ago

Yeah, I've certainly considered that it would be nice if there was a way to dispatch on a Module as a type/val, then we could have fallback definitions like: StructTypes.StructType(M, ::Type{T}) where {T} = StructTypes.StructType(T), but I could overload the struct type for specific types in my own package like StructTypes.StructType(::JSON3, ::Type{T}) where {T} = # a different struct type. But last time I thought about this, there wasn't an obvious way to dispatch on a module other than just doing like a Val{:JSON3}, which can still be pirated/spoofed.

jpsamaroo commented 2 years ago

Has there been any progress on this? Or has anyone come up with a workaround that works for arbitrary structs?

quinnj commented 2 years ago

I'm in favor of changing teh default to Struct if someone wants to make a PR.

Wynand commented 2 years ago

I haven't seen much movement on #67, so I made a PR which includes those changes and more that I think should handle all the types in Core, Base, as well as any types or subtypes that can be defined: https://github.com/JuliaData/StructTypes.jl/pull/70

I think follow up work would be to define a better meaning for NoStructType to distinguish user defined structs from things like Functions (Maybe CustomStruct would be best there?)

quinnj commented 2 years ago

Implemented in https://github.com/JuliaData/StructTypes.jl/pull/70