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

Embedded types are not migrated #9

Open
unmultimedio opened this issue Dec 19, 2024 · 2 comments
Open

Embedded types are not migrated #9

unmultimedio opened this issue Dec 19, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@unmultimedio
Copy link

unmultimedio commented Dec 19, 2024

Hello, if I have a generated type embedded in another type, the tool does not recognize it for migration:

type MyFoo struct {
  *foopkg.GeneratedFoo // proto generated, let's say it contains field "bar"

  // some additional fields
}

I expect:

  var f MyFoo
- f.Bar = "bar"
- bar := f.Bar
+ f.SetBar("bar")
+ bar := f.GetBar()

But the tool does not recognize MyFoo as a type that needs migration.

@stapelberg stapelberg added the enhancement New feature or request label Dec 20, 2024
@stapelberg
Copy link
Collaborator

Hey @unmultimedio, thanks for filing this issue.

Yes, embedded types are currently not recognized by the open2opaque tool.

Inside Google, we recommend against embedding protobuf generated message types into a struct. It’s an error-prone pattern, because then your struct implements the proto.Message interface by forwarding to the embedded message, which only knows about part of the struct it’s embedded in. So, for example, code which uses Go Protobuf reflection to compare two messages for equality would only operate on the proto message fields, not your additional fields.

We recommend naming the struct field, i.e.:

type MyFoo struct {
  Foo *foopkg.GeneratedFoo
  // some additional fields
}

For now, I recommend doing this refactoring first and then reaching for open2opaque.

If many people encounter this issue, we can see about implementing support for it (please use the thumbs up vote on the first comment of this issue to signal you also ran into the issue).

@unmultimedio
Copy link
Author

Thanks for your reply, I'll take a look to see if we can easily do that refactor to run the migration tool again.

Quite similarly, just realized that types definitions and aliases based off generated types have this same issue:

type MyFooT foopkg.GeneratedFoo
type MyFooA = foopkg.GeneratedFoo

Then I'd expect both types to be recognized for migration, but they're not. Should this be a separate issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants