Author: @mcy
Approved: 2023-02-13
On 2023-02-10, a CL @mcy submitted to delete google::protobuf::Reflection::SupportsUnknownEnumValue(). Oddly, this function used the containing message‘s syntax, rather than the enum field’s, to determine whether the enum was open.
It turns out we misunderstood a critical corner-case of proto3 enums. Consider the following proto files:
// enum.proto
syntax = "proto3";
package oh.no;
enum Enum {
A = 0;
B = 1;
}
// message.proto
syntax = "proto2";
package oh.no;
import "enum.proto";
message Msg {
optional Enum enum = 1;
}
If we parse the Protoscope value 1: 2 as an oh.no.Msg, and look at the value of oh.no.Msg.enum, we will find that it is not present, and that there is a VARINT of value 2 in the UnknownFieldSet.
This is because Protobuf sometimes implements the openness of an enum by its usage, not its definition.
This case is actually quite difficult to observe, because the converse doesn't work: the proto compiler rejects proto2-enum-valued fields in proto3 messages, because such enums can have nonzero defaults, which proto3 does not support due to implicit presence.
Approximately 2.99% of enum fields import enums across syntaxes and 1.77% of enums are imported across syntaxes.
6.14% of fields being enum fields, meaning 0.18% of fields are affected when used by affected languages.
This document proposes adding an additional feature to Edition Zero Features, specified as the following .proto fragment:
message Features {
// ...
optional bool legacy_treat_enum_as_closed = ??? [
retention = RUNTIME,
target = FILE,
target = FIELD
];
}
The name of this field captures the desired intent: this is a bad legacy behavior that we believe is rare and want to stamp out. Edition 2023 would set this to false by default, and proto2 would treat it as implicitly true. It also does not permit the converse: you cannot force a field to be open, because that is currently not possible and we don't want to add more special cases.
Additionally, we would like to make special dispensation in migration tooling for this field: it should not be set unconditionally when migrating from proto2 -> editions, but only on proto2 fields that are of proto3 enum type. We should also want to build an allowlist for this, like we do for required.
This option can also help in migrating enums from closed to open, since we can use it to migrate individual use-sites by marking the enum as open and all of its uses as treat-as-closed in one CL, and then deleting the treat-as-closed annotations one by one.
An open (lol) question is whether we should move is_closed from EnumDescriptor to FieldDescriptor.
Use the “define official behavior” alternative below. Given the wide variety of behavior in different languages, a singular global setting will always leave some of our languages in the lurch. As such, we will use per language features to allow each language to control its own evolution while we define the “correct” behavior.
For example, in C++ we will define:
// Determines if the given enum field is treated as closed based on legacy
// non-conformant behavior.
//
// Conformant behavior determines closedness based on the enum and
// can be queried using EnumDescriptor::is_closed().
//
// Some runtimes currently have a quirk where non-closed enums are
// treated as closed when used as the type of fields defined in a
// `syntax = proto2;` file. This quirk is not present in all runtimes; as of
// writing, we know that:
//
// - C++, Java, and C++-based Python share this quirk.
// - UPB and UPB-based Python do not.
// - PHP and Ruby treat all enums as open regardless of declaration.
//
// Care should be taken when using this function to respect the target
// runtime's enum handling quirks.
bool FieldDescriptor::legacy_enum_field_treated_as_closed() const {
return type() == TYPE_ENUM && file().syntax() == FileDescriptor::SYNTAX_PROTO2;
}
In Java, FileDescriptor.supportsUnknownEnumValue() will need to be deprecated and replaced with the above.
Define the official behavior to be “Enums open-ness should be defined by the definition of the enum.” Add a conformance test for this behavior. Use per language features to eventually converge implementations that are out of conformance. We choose to define this as “enum openness is defined by the definition” because that matches the model for almost all other proto3/proto2 properties.
Features.enum a field-level featureHere, we don't add legacy_treat_enum_as_closed and instead make closeness a bona fide property of fields, not enums.
is_closed() on EnumDescriptor rather than FieldDescriptor.IsValid, unless we want to believe that IsValid merely states whether the value has a name we know of.Features.enum on both enums and fieldsThis allows enum owners some more control without needing to introduce a strictly “legacy do not use” feature.
Features.treat_as_closed_for_migrationThis is an aesthetic choice if we feel this is a useful knob for migration, that still highlights its temporary nature.
We can simply keep the current editions enum semantics.