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

Add a generic proto.Clone #1594

Open
akshayjshah opened this issue Mar 4, 2024 · 1 comment
Open

Add a generic proto.Clone #1594

akshayjshah opened this issue Mar 4, 2024 · 1 comment

Comments

@akshayjshah
Copy link

akshayjshah commented Mar 4, 2024

Is your feature request related to a problem? Please describe.

Use of proto.Clone is awkward because it returns a proto.Message, even though the caller often has a concrete type in hand. In those cases, working with the cloned message requires a type assertion and (more importantly) error handling. For example:

now := timestamppb.Now()
clone, ok := proto.Clone(now).(*timestamppb.Timestamp)
if !ok {
  return errors.New("boom")
}
fmt.Println(clone.AsTime())

The error-handling path is unnecessary - Clone always returns a *timestamppb.Timestamp, but that isn't visible to the type system. Of course, the caller can instead panic if the type assertion fails...but modern Go offers a more palatable option.

Describe the solution you'd like

Now that generics are available in all supported Go versions, it'd be nice to have a type-safe, generic variant of proto.Clone:

func Clone2[T proto.Message](msg T) T {
  // implementation
}

Then our example code becomes:

now := timestamppb.Now()
clone := proto.Clone2(now)
fmt.Println(clone.AsTime())

Virtually all implementations of proto.Message are pointers to structs and have the same gcshape, so use of this generic function shouldn't significantly affect binary size (at least, with the current generics implementation).

The new function could be called DeepCopy, CloneMessage, CloneT, or whatever else seems appropriate. DeepCopy feels the most natural to me, since it's how the current GoDoc describe's Clone's behavior.

Describe alternatives you've considered

  • Keep the package as-is, and continue to have callers of Clone use type assertions. This is fine, but why not offer a nicer API now that we can?
  • Use generics more extensively, probably by adding DeepMerge and DeepEqual as generic equivalents of Merge and Equal. I'm in favor of this, but perhaps we should discuss in a separate issue?

Additional context
I'm happy to open a CL for this if we can agree on naming.

@znkr
Copy link

znkr commented Mar 5, 2024

We discussed this internally and more or less aligned on

// CloneOf returns a deep copy of m. If the top-level message is invalid, 
// it returns an invalid message as well.
func CloneOf[M Message](m M) M {
        return proto.Clone(m).(M)
}

We decided to not move forward with it because at the time some of the tooling we use internally wasn't ready for generics. The remaining difficulty is that we are still supporting go 1.17, but we can work around that with build tags.

We're happy to accept a contribution. :)

Regarding Merge and Equal a separate proposal would be good.

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

No branches or pull requests

2 participants