Incompatible API change between v1.33.0 and v1.34.0
bobg opened this issue · 7 comments
What version of protobuf and what language are you using?
Version: v1.34.0; Go
In version 1.34.0 of the Go package google.golang.org/protobuf/reflect/protoreflect, the type EnumDescriptor has a method called IsClosed. This method did not exist in v1.33.0.
This is a backward-incompatible change that should have resulted in a new major version number of the google.golang.org/protobuf module, according to Go module versioning rules.
(Is the addition of a new method to a Go interface a breaking change? Yes. A Go program that depends on google.golang.org/protobuf@v1.33.0 and defines a type implementing the EnumDescriptor interface will break when upgrading to google.golang.org/protobuf@v1.34.0. But upgrading to new minor versions must always be safe for correct programs to do.)
Please review your version-numbering practices. For help with knowing when to bump the major vs. minor vs. patchlevel parts of a module version number, see Modver and/or Taggo.
The protoreflect documentation states:
The protobuf descriptor interfaces are not meant to be implemented by user code since they might need to be extended in the future to support additions to the protobuf language.
Descriptor interfaces, including protoreflect.EnumDescriptor, contain an method which cannot be implemented by code outside the protobuf module (https://pkg.go.dev/google.golang.org/protobuf/internal/pragma#DoNotImplement), to avoid accidental implementations.
Ah, I see! In that case, withdrawn, and thanks for the explanation.
Hm, I wonder whether I can make Modver aware of this so it doesn't complain in cases like this.
When designing the protoreflect API, I did tender the idea of a UnimplementedXXX concrete type that implemented all methods of XXX. For example, UnimplementedEnumDescriptor would implement all methods methods of EnumDescriptor with methods that return the zero value. Thus, custom types could embed UnimplementedEnumDescriptor and then implement the methods they care about and remain forwards compatible.
However, I wasn't sure how common this would be utilized, so it seemed to be more complexity that it was worth.
My main concern here is that I have projects (personally and professionally) that would like to be able to depend on the aforementioned tools, Modver and Taggo, for semantic-version management, but this issue sometimes causes them to produce false positives.
I’m not sure why Modver or Tango should even be worrying about code that is not your own, since there is nothing you can really do about it.
I’ve actually hit this very breaking change in a different package, and it was blocking our upgrades, until I could address it… but there wasn’t really anything we could do except just implement the new methods, and carry on… it’s not like the change happened because of some code within our company’s control. 🤷♀️ (I mean, it was also caused by this package returning an unexported concrete type as the single implementation of an exported interface, rather than just the concrete type itself (which can expand receiver methods without a breaking change), but this is again, not something I or we could actually address.)
I’m not sure why Modver or Tango should even be worrying about code that is not your own, since there is nothing you can really do about it.
On the contrary. Suppose you're the author of module M and it depends on module D, which you can't control. If module D introduces a breaking change, you have two choices: do not upgrade M's dependency on D; or do, but make it a new major version of M.
(This story becomes a little harder when you have indirect dependencies on D - but only a little. You may have pinned your dependency at v1.33.0 of D, but one of your other dependencies wants v1.34.2 of D, and thanks to minimal version selection that's the one you get.)
do not upgrade M's dependency on D
Yeah, we rolled back the update until I could take a swing at correcting the issue. But we cannot just keep a pinned dependency forever. We would miss critical security updates at the least. So, we had to eventually update, and that meant accommodating the breaking change.
or do, but make it a new major version of M.
We didn’t need to do this, because we fully encapsulated the functionality of the downstream service, and so users of our own corporate platform libraries were not required to meet any breaking changes. In fact, I would even go so far as to say it’s an anti-pattern for a library to leak any breakable details of any underlying library to the higher levels.
But my point still remains: You’re not responsible for this package, so whether this package has a Modver or Tango alert should be irrelevant to your own package.