search

polydawn / refmt

Object mapping for golang.
MIT License
48 stars 14 forks source link

Allow pointers when using UseTag #32

Open Stebalien opened 6 years ago

Stebalien commented 6 years ago

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: https://github.com/ipfs/go-ipld-cbor/pull/30. We want to deserialize "cids" to *cid.Cid but can only deserialize them to cid.Cid.

warpfork commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

(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.