Allow registered serializable types to be pointers. #457
Conversation
runtime/codegen/automarshal.go
Outdated
| } | ||
|
|
||
| // canEncodeInterface returns true if the concrete type of value is registered | ||
| // as a serializable type and can therefore be sent using Encoder.Interface. | ||
| func canEncodeInterface(value any) bool { | ||
| if _, ok := value.(AutoMarshal); !ok { |
There was a problem hiding this comment.
If I'm understanding correctly, if value is of type T, T implements error, but *T implements AutoMarshal instead of T, then we return false here? Is it possible for us to encode value by taking a pointer to it?
I noticed that values of type codegen.customTestError return false here and end up being decoded as codegen.decodedError, but I would have expected them be decoded into a codegen.customTestError.
There was a problem hiding this comment.
Good catch. Fixed.
runtime/codegen/encoder.go
Outdated
| // We allow either value, or a pointer to value to implement AutoMarshal. | ||
| am, ok := value.(AutoMarshal) | ||
| if !ok { | ||
| // Try using a pointer to value. |
There was a problem hiding this comment.
Can we use the trick you used in automarshal.go here?
if !ok {
value = &value
am, ok = value.(AutoMarshal)
}There was a problem hiding this comment.
That was a good idea. Until I realized that the "a = &value" trick was only working because value had the generic type T. Here, value has type any, and so "&value" does not have the type we want.
runtime/codegen/automarshal.go
Outdated
| // (since AutoMarshal methods have pointer receivers, but T itself may be a | ||
| // struct). | ||
| func RegisterSerializable[T any, PT autoMarshalPointer[T]]() { | ||
| // REQUIRES: T can be serialized (either T or a pointer to T must implement |
There was a problem hiding this comment.
I believe right now, we always generate AutoMarshal methods on *T, and I don't think users are supposed to implement AutoMarshal methods manually. Does it simplify things if we always require *T to implement AutoMarshal?
There was a problem hiding this comment.
That's a good point. Maybe I should abandon this change.
There was a problem hiding this comment.
Oh, maybe I misunderstood, but I thought most of the changes here were still needed to support errors where the Error method has a pointer receiver?
There was a problem hiding this comment.
You are correct. I was wondering if we need to support Error() with a pointer receiver.
There was a problem hiding this comment.
Ah, I'm in favor of supporting it.
There was a problem hiding this comment.
Ack. Just to clarify, yes the AutoMarshal methods are always defined on a pointer to a struct S.
The code here allows T to be either S or *S. This way, the user is free to place their Error() method on either S or *S.
The type T passed to RegisterSerializable[T] is now always AutoMarshal. However the error interface may be implemented by either T or *T. There are now two encoding tags for custom error values: one tag if the error itself is AutoMarshal, another tag if a pointer to the error is AutoMarshal. errors.Is etc. do not handle cyclic errors well, so make a separate test for cyclic errors that just verifies that we can encode and decode cyclic errors.
|
PTAL |
Remove type constraint from RegisterSerializable[T] and instead check at runtime that either T or *T implements AutoMarshal.