fix: register type with prefix #52
Conversation
|
|
| gob.Register(&MapType{}) | ||
| gob.Register(&NamedType{}) | ||
| gob.Register(&PointerType{}) | ||
| gob.RegisterName(pkgPath+".ArrayType", &ArrayType{}) |
There was a problem hiding this comment.
Should the registered named be prefixed with a "*"? From my understanding gob.Register is only misbehaving (i.e. not fully qualifying) for named pointer types.
There was a problem hiding this comment.
From gob's implementation, the registered type will be firstly reflected for the actual type (userType), then stored in two map, with <name, type> and <type, name> pairs: https://cs.opensource.google/go/go/+/refs/tags/go1.21.0:src/encoding/gob/type.go;l=836-844;drc=f80e270babcddd28e29a52c1cb834ffed427bd8a
In the encode part, encoder will use the same userType again to reflect the actual type, and look up the name: https://cs.opensource.google/go/go/+/refs/tags/go1.21.0:src/encoding/gob/encode.go;l=400-405;drc=6a51c000def084b820756f066397fd2505e75e25
In the decode part, a similar but reverse process will be used for looking up type by name: https://cs.opensource.google/go/go/+/refs/tags/go1.21.0:src/encoding/gob/decode.go;l=699-704;drc=f62c9701b4bc61da6a5f4db8ef81d816f112430e
The default gob.Register call infers the type name with this: https://cs.opensource.google/go/go/+/refs/tags/go1.21.0:src/encoding/gob/type.go;l=854-856;drc=f80e270babcddd28e29a52c1cb834ffed427bd8a (without calling user type etc) . For instance, giving a pointer to MapType from two different packages, the name here will be inferred as the same value (yes it will have a * prefix as it's a pointer type). As a result, we will see panic in later register when using like issue #51 .
The fix here is to add a qualified prefix to the value type, so we can avoid the panic on register. But to gob, it's just an unique identifier. Or, we can define it as something like {pkgPath}.*MapType. Let me know your thought!
| gob.RegisterName(pkgPath+".FuncType", &FuncType{}) | ||
| gob.RegisterName(pkgPath+".MapType", &MapType{}) | ||
| gob.RegisterName(pkgPath+".NamedType", &NamedType{}) | ||
| gob.RegisterName(pkgPath+".PointerType", &PointerType{}) |
There was a problem hiding this comment.
Can we add a comment why we are using RegisterName ?
There was a problem hiding this comment.
Added a comment, @moisesvega / @sywhang PTAL
There was a problem hiding this comment.
Thanks for fixing this @bcho ! This looks good to me, other than @moisesvega 's suggestion on adding a brief comment on top of init explaining that this is needed for projects that are using both golang/mock and uber/mock.
|
@sywhang thanks for merging this, any plan to create a new release? Our team needs this to migrate from golang/mock to uber/mock :) |
+1 |
fix #51