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

Allow pointers when using UseTag #32

Open
Stebalien opened this issue Jul 17, 2018 · 3 comments
Open

Allow pointers when using UseTag #32

Stebalien opened this issue Jul 17, 2018 · 3 comments

Comments

@Stebalien
Copy link
Contributor

Currently, when decoding into a blank interface (interface{}), some formats (e.g., CBOR), support picking the atlas entry by tag. Unfortunately, atlas entries can't be built for types behind pointers. Normally, this isn't an issue as refmt will just "do the right thing" however, in this case, refmt doesn't have enough information to do that.

It would be nice if there were some way to tell refmt to deserialize to a pointer.

Possible solutions:

  1. Add a method to the atlas builder.
  2. Allow types behind pointers in atlas.BuildEntry. refmt would still "do the right thing" but it would now have enough information to pick the right "default form" (behind pointer or not).

Context: ipfs/go-ipld-cbor#30. We want to deserialize "cids" to *cid.Cid but can only deserialize them to cid.Cid.

@warpfork
Copy link
Member

warpfork commented Sep 16, 2018

I just want to leave a note here that I found another place where this comes up: if using unions, you'll also get non-pointer types back from an unmarshal, and there doesn't really seem to be a way to configure that to behave otherwise.

It's a similar "refmt doesn't have enough information to do that" situation -- unions also have the unmarshal into an interface, and when the target is an interface, there's no hint for the desired level of pointer indirections.

I'm backing away from getting my other project hung up on this case for unions, because on further code review in that other context, I decided having pointers inside the interface was silly in all my use cases (basically, things are effectively pointers anyway since they're referenced via the interface; but I definitely don't want them to be nils of their concrete type; so declaring them as pointer types just added noise and no gain).

(Speaking of, since I notice your use case with "cids" has also shifted to non-pointer types for unrelated reasons, I'm probably not prioritizing this at all until some other use case pops up.)

I'm definitely open to propositions for how this syntax should look for specifying things though. Tentatively, I don't think BuildEntry should accept pointer types, because that tends towards being confusing in every other situation than this, i.e. the majority of the time. Maybe another method on the builder? Or maybe it should be totally different and there's a site-specific way to indicate "hey this underconstrained interface here, use pointers for this one's contents, whatever they are"?

@warpfork
Copy link
Member

I was briefly considering the idea of adding some Cheneyian optionals to the Unmarshal call itself as a solution to the original issue described, but now that it's clear that unions and other nested interfaces can experience the same unclarity, doing something like this just at the topmost level seems pretty limited and probably the wrong way to go.

@Stebalien
Copy link
Contributor Author

(Speaking of, since I notice your use case with "cids" has also shifted to non-pointer types for unrelated reasons, I'm probably not prioritizing this at all until some other use case pops up.)

It wasn't entirely unrelated. This issue was the final nail in the coffin for that switch.

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

2 participants