Changing an enum field to an identical type is considered breaking WIRE change

[hatstand]

Before:

message Bar {
  enum Foo {
    FOO_UNSPECIFIED = 0;
    FOO_A = 1;
  }
  Foo field = 1;
}

After:

enum Foo {
  FOO_UNSPECIFIED = 0;
  FOO_A = 1;
}

message Bar {
  Foo field = 1;
}

This is currently detected as a breaking change with a message like:

Field "1" on message "Bar" changed type from "Bar.Foo" to "Foo".

This is not actually a breaking change in the wire format as the enum values are the same even if the type has changed name.

[bufdev]

I don't think this is something we're going to support - this is the same as changing the field from from enum Foo to SomethingElse, where SomethingElse might have completely different values over time. Are you saying you'd want deep inspection to make sure that the enum values are the same?

We do document that we make some exceptions where WIRE is technically more strict than necessary:

• https://buf.build/docs/breaking-checkers#field_same_oneof
• https://buf.build/docs/breaking-checkers#field_same_type

I don't think we want to support changing the underlying enum type as an acceptable wire breaking change.

[hatstand]

Yes, I guess I would have expected it to check the structure rather than the name. I think it's a not uncommon use case to refactor a submessage into its own message type without changing its structure.

Thanks for the links to the docs, I missed those exceptions initially.

[bufdev]

We can think about it a bit - I feel pretty strongly that this should not be handled, as it's getting into the weeds - similar to how you can switch out int32, int64, uint32, uint64, bool, but it's not completely unfair. Buf's overall goal is meant to prevent users from having to think about any of this though, and having an exception for effectively changing the type of an enum (moving from nested to not is really a complete type change from the perspective of the Protobuf compiler) is probably beyond the scope of what we want.

We can sit on it for a day or two though if you'd like.

[hatstand]

As a long time user of protocol buffers, it was definitely unexpected but I understand if it's out of scope for you folks.

As to swapping out various integer types - it's not something that I'd thought about before and would have considered it a breaking change even though it's potentially not if you're increasing the size I think? In any case, it's something I would expect buf to complain about.

Also, thanks for writing a great tool and being responsive :-)

[bufdev]

Heh no worries, it's what we're here for :-)

Yea so where do you draw the line in your head is probably the question right? Against the scalar type changes, but for the enum moving...eh it's a fuzzy line, you can prob understand why we're on the fence on these as well.

Our general philosophy here is principle of least surprise, and I'd expect people to be surprised they can move some enums, or change field types for certain enums, but not others, and have to be informed about deep inspection. Does that make sense?

[hatstand]

I think, once you get your head around protobufs, you only think about tag numbers and types and so a name change being breaking is surprising to me.

I think you're probably right about where you're drawing the line though so that users don't need too deep an understanding of how it works and can use a very slightly restricted subset of non-breaking changes. I suspect a lot of people are using dynamically typed languages anyway where an enum name change is potentially painful at the source level too.

[bufdev]

Yea I think we're gonna close this one for now, but if it comes up again we'll revisit. Thanks for the input though, appreciate it.

[rspechenkin]

hello, it would be nice if you could revisit this because we're also facing this issue

[bufdev]

We're going to revisit this, along with oneofs and scalar types, as we move towards v1.0, however no promises :-)

[rspechenkin]

That would be extremely helpful, thank you for your quick response :)

[bufdev]

Depends on #90

[Fleshgrinder]

Hmmm… this would definitely need to be something that is configurable because all things that were mentioned here are breaking changes for the generated code. Switching from i32 to i64 would for instance translate to Int and Long in Kotlin or i32 and i64 in Rust, while being compatible, the code in question breaks if we are not going to regenerate, update all usages, and release. Otherwise 9223372036854775807 will overflow and end up as -1.

[bufdev]

Yea I am inclined to keep as-is with regards to scalars.

[Fleshgrinder]

The Enums have the same issue. Upgrading the Protobuf definitions from the registry would result in a broken build. I can understand that it is sometimes desirable to do these kind of changes but it should actually be done with a new v2 package. Hence, I think the behavior of buf is sensible here, just the error message is confusing because it is factually incorrect what buf outputs.

[bufdev]

Yes, but we differentiate between wire breaking changes and source breaking changes https://buf.build/docs/breaking-checkers

[Fleshgrinder]

I know and I didn't say you shouldn't nor that the feature request makes no sense. Just that this, if added to buf, should be configurable and that the error message is misleading, as reported.

[bufdev]

This is done in #400 and will go out in the next release.