Merge pull request #11946 from protocolbuffers/cp-enum-closed-comment

Document known quirks of EnumDescriptor::is_closed() when importing a…
protocolbuffers · Feb 15, 2023 · 8d7b4e6 · 8d7b4e6
2 parents b7f7171 + a594141
commit 8d7b4e6
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 14 deletions.
diff --git a/java/core/src/main/java/com/google/protobuf/Descriptors.java b/java/core/src/main/java/com/google/protobuf/Descriptors.java
@@ -303,9 +303,7 @@ public static FileDescriptor buildFrom(FileDescriptorProto proto, FileDescriptor
      *     two messages were defined with the same name.
      */
     public static FileDescriptor buildFrom(
-        FileDescriptorProto proto,
-        FileDescriptor[] dependencies,
-        boolean allowUnknownDependencies)
+        FileDescriptorProto proto, FileDescriptor[] dependencies, boolean allowUnknownDependencies)
         throws DescriptorValidationException {
       // Building descriptors involves two steps:  translating and linking.
       // In the translation step (implemented by FileDescriptor's
@@ -462,9 +460,9 @@ public static FileDescriptor internalBuildGeneratedFileFrom(
     }
 
     /**
-     * This method is to be called by generated code only. It updates the
-     * FileDescriptorProto associated with the descriptor by parsing it again with the given
-     * ExtensionRegistry. This is needed to recognize custom options.
+     * This method is to be called by generated code only. It updates the FileDescriptorProto
+     * associated with the descriptor by parsing it again with the given ExtensionRegistry. This is
+     * needed to recognize custom options.
      */
     public static void internalUpdateFileDescriptor(
         FileDescriptor descriptor, ExtensionRegistry registry) {
@@ -1778,10 +1776,23 @@ public FileDescriptor getFile() {
      * <p>Closed enum means that it:
      *
      * <ul>
-     *   <li>Has a fixed set of named values. *
+     *   <li>Has a fixed set of values, rather than being equivalent to an int32.
      *   <li>Encountering values not in this set causes them to be treated as unknown fields.
      *   <li>The first value (i.e., the default) may be nonzero.
      * </ul>
+     *
+     * <p>WARNING: 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:
+     *
+     * <ul>
+     *   <li> C++, Java, and C++-based Python share this quirk.
+     *   <li> UPB and UPB-based Python do not.
+     *   <li> PHP and Ruby treat all enums as open regardless of declaration.
+     * </ul>
+     *
+     * <p>Care should be taken when using this function to respect the target runtime's enum
+     * handling quirks.
      */
     public boolean isClosed() {
       return getFile().getSyntax() != Syntax.PROTO3;

diff --git a/python/google/protobuf/descriptor.py b/python/google/protobuf/descriptor.py
@@ -739,13 +739,25 @@ def __init__(self, name, full_name, filename, values,
 
   @property
   def is_closed(self):
-    """If the enum is closed.
+    """Returns true whether this is a "closed" enum.
 
-    closed enum means:
-      - Has a fixed set of named values.
-      - Encountering values not in this set causes them to be treated as
-        unknown fields.
-      - The first value (i.e., the default) may be nonzero.
+    This means that it:
+    - Has a fixed set of values, rather than being equivalent to an int32.
+    - Encountering values not in this set causes them to be treated as unknown
+      fields.
+    - The first value (i.e., the default) may be nonzero.
+
+    WARNING: 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.
     """
     return self.file.syntax == 'proto2'
 

diff --git a/src/google/protobuf/descriptor.h b/src/google/protobuf/descriptor.h
@@ -1151,10 +1151,22 @@ class PROTOBUF_EXPORT EnumDescriptor : private internal::SymbolBase {
   bool is_placeholder() const;
 
   // Returns true whether this is a "closed" enum, meaning that it:
-  // - Has a fixed set of named values.
+  // - Has a fixed set of values, rather than being equivalent to an int32.
   // - Encountering values not in this set causes them to be treated as unknown
   //   fields.
   // - The first value (i.e., the default) may be nonzero.
+  //
+  // WARNING: 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 is_closed() const;
 
   // Reserved fields -------------------------------------------------