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

StructTypes encourage piracy #87

Open
jakobnissen opened this issue Oct 24, 2022 · 5 comments
Open

StructTypes encourage piracy #87

jakobnissen opened this issue Oct 24, 2022 · 5 comments

Comments

@jakobnissen
Copy link

Suppose I have a package, in which I need to serialize some data that contain a LongDNA{4}, but do not want to expose the private (non-documented) memory layout of LongDNA. In that case, I'd need to overwrite StructTypes.StructType(::Type{LongDNA{4}}) - or, alternatively, overwrite some JSON3 methods.

Both are type piracy, which can have rather bad consequences. For example, it's unknown to me if the user installs another package which also need to serialize LongDNA, but may choose to do it differently. In that case, my code may randomly malfunction.

I don't see a non-breaking way of getting around it, but perhaps it's worth considering for a potential breaking release in the future:

  • This package defines a struct AbstractEncoder end
  • Struct mapping is done by StructTypes.StructType(::AbstractEncoder, ::Type{MyType})
  • Default struct mapping can use ::AbstractEncoder directly.
  • Users can then subtype AbstractEncoder, and create struct mapping using their own subtype.
@quinnj
Copy link
Member

quinnj commented Oct 25, 2022

Thanks for opening an issue to discuss! I've actually wanted to implement the kind of thing you're proposing to allow custom overloads for different packages. As you've noted, it's pretty cumbersome to try and change default struct types.

I've thought that this would be non-breaking though; do you see it being definitely breaking? I guess I just need to take a stab at implementing to see how it plays out.

@quinnj
Copy link
Member

quinnj commented Oct 25, 2022

(unless you (@jakobnissen) are up for making a PR? I'd appreciate it! Just let me know and I'm happy to chat through details or questions you have)

@jakobnissen
Copy link
Author

Huh no, you're probably right it's not breaking. Of course, to preserve backwards compatiblitiy, the method StructType(::Type{T}) where T = StructType(DefaultEncoder, T) needs to be there, which mean downstream packages might still easily commit type piracy. But then they would at least have the chance not to.

Maybe I can make a PR - but I've never really peeked deeply into the codebase of this package.

@quinnj
Copy link
Member

quinnj commented Oct 27, 2022

We also recently changed the default StructType(T) to Struct instead of NoStructType, so I would think that would help avoid people needing to define as many methods for types they don't own.

The codebase is pretty simple honestly, so the AbstractEncoder would be a pretty minimal addition, and then just adding the DefaultEncoder to all the StructType definitions we have.

If you don't think you'll do it, no worries; just let me know and I'll queue it up to do sometime in the next week or two.

@twadleigh
Copy link

twadleigh commented May 2, 2023

This is something I'm thinking about, as I may soon be in the situation where I'll need/want to support (de-)serialization from two different encodings, each with its own peculiarities. I suppose I might take a stab at a PR if I get there.

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

No branches or pull requests

3 participants