-
Notifications
You must be signed in to change notification settings - Fork 192
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
base: master
Are you sure you want to change the base?
Conversation
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. |
I was using msgp and be confused when using with interface. |
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. |
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):
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 |
@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? |
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.
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