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

Implement polymorphism via msgp:intercept directive #194

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

shabbyrobe
Copy link
Contributor

@shabbyrobe shabbyrobe commented Aug 24, 2017

Here's an attempt at #184. I have had about 4 or 5 goes at this now, this is the only way I've found that went in even remotely cleanly. I tried doing it with extensions, then found the warning against that in #35. I tried extending ShimMode at least twice, but that was a total mess. This is the first time I've tried something that didn't feel like I was committing an act of vandalism!

Using this, we can delegate to a function to provide a stand-in that can decode, encode, marshal and unmarshal instead of calling it on the type itself. Interface types can be supported, as well as typed primitives.

There's no interface type for the mapper so we can retain type safety. You can just implement as much or as little as the code generator requires, based on what you ask it to generate.

//msgp:intercept MyIntf using:DefaultMyIntfMapper

type Thingy struct {
    Foo MyIntf
}

type MyIntf interface {}

func DefaultMyIntfMapper() myIntfMapper { return &myIntfMapper{} }

type myIntfMapper struct {}

func (p *myIntfMapper) DecodeMsg(dc *msgp.Reader) (t MyIntf, err error) {}
func (p *myIntfMapper) UnmarshalMsg(bts []byte) (t MyIntf, o []byte, err error) {}
func (p *myIntfMapper) EncodeMsg(t MyIntf, en *msgp.Writer) (err error) {}
func (p *myIntfMapper) MarshalMsg(t MyIntf, b []byte) (o []byte, err error) {}
func (p *myIntfMapper) Msgsize(t MyIntf) (s int) {}

The use is demonstrated in the _generated/intercept_defs.go and _generated/intercept_test.go files. I know I haven't got a complete set of tests here, but I need to think of more scenarios. I just thought I'd open this for discussion before I go test-case crazy just in case this approach is not one that would be acceptable.

I added a dummy element for interfaces that gets skipped by the printer but that was only to quiet the warning that gets raised: intercept_defs.go: TestUsesIntfStructProvided: Foo: non-local identifier: TestIntfStructProvided, if there's a better way to sort that out that involves fewer changes to the Elem hierarchy I'd be grateful for a tip, I didn't see one.

I know it looks like a big patch, but the overwhelming majority of it is tests!


This change is Reviewable

@shabbyrobe
Copy link
Contributor Author

Started dogfooding this in our project today, so far so good. Works seamlessly, supports all the interface types I have thrown at it, no panics. I've just realised JSON interop might require a bit more exploration though - that's not a feature we're using but it's a feature that msgp advertises so I will investigate the impact.

@TCBeliever
Copy link

I was using msgp and be confused when using with interface.
This solutions looks like good. but it still in feature branch long time.
When does this feature merge to master?

@shabbyrobe
Copy link
Contributor Author

I've had this in production for close to a year now via a fork, it worked very well for our needs but it hasn't been vetted or reviewed by Phil. I'd be happy to help polish it up if there was something that needed to be done, but in the meantime if you want to make use of this you can make your own fork like I did and apply this PR to it.

@shabbyrobe
Copy link
Contributor Author

Note that this functionality is here to essentially "punch a hole" in the serialisation/deserialisation process provided by msgp, nothing more, so it does leave a bit of work for you to do yourself to support interfaces: you'll have to implement the following functions yourself for each interface you want to support (which can't be defined in an interface due to the hopefully temporary lack of parameterised types in Go):

func (p *MyProvider) DecodeMsg(dc *msgp.Reader) (t MyInterface, err error) 
func (p *MyProvider) UnmarshalMsg(bts []byte) (t MyInterface, o []byte, err error)
func (p *MyProvider) EncodeMsg(t MyInterface, en *msgp.Writer) (err error)
func (p *MyProvider) MarshalMsg(t MyInterface, b []byte) (o []byte, err error)
func (p *MyProvider) Msgsize(t MyInterface) (s int)

You then need to settle on a way to encode the concrete type ID yourself. I think perhaps this could be made easier if I included a helper that boils down the repeated boilerplate I have in my own code into the patch, but there's no getting around it: you'll need to write at least two Big Fat Switch statements somewhere to map the type ID to the type and vice versa, and you will need to know all the concrete types ahead of time unless you use reflection.

See here for an implementation example: https://github.com/tinylib/msgp/pull/194/files#diff-9e13c876a098520e63af3201fd0cbdb6R167

@shabbyrobe
Copy link
Contributor Author

@philhofer been a long time since I followed up on this, my apologies. If the conflicts were resolved, do you see a pathway to merging this feature or should I close up my PR & fork?

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

Successfully merging this pull request may close these issues.

2 participants