Verify closed enums rather than fallback to eager parsing.

PiperOrigin-RevId: 567666569

python/google/protobuf/internal/descriptor_test.py

@@ -37,6 +37,7 @@ message NestedMessage {
     FOREIGN_FOO = 4;
     FOREIGN_BAR = 5;
     FOREIGN_BAZ = 6;
+    FOREIGN_BAX = 32;
   }
   optional int32 bb = 1;
 }
@@ -72,6 +73,7 @@ class DescriptorTest(unittest.TestCase):
     enum_proto.value.add(name='FOREIGN_FOO', number=4)
     enum_proto.value.add(name='FOREIGN_BAR', number=5)
     enum_proto.value.add(name='FOREIGN_BAZ', number=6)
+    enum_proto.value.add(name='FOREIGN_BAX', number=32)
 
     file_proto.message_type.add(name='ResponseMessage')
     service_proto = file_proto.service.add(name='DescriptorTestService')
@@ -871,6 +873,10 @@ class DescriptorCopyToProtoTest(unittest.TestCase):
         name: 'FOREIGN_BAZ'
         number: 6
       >
+      value: <
+        name: 'FOREIGN_BAX'
+        number: 32
+      >
       """
 
     self._InternalTestCopyToProto(

python/google/protobuf/internal/reflection_test.py

@@ -469,7 +469,7 @@ class ReflectionTest(unittest.TestCase):
     self.assertRaises(TypeError, proto.repeated_string.__setitem__, 0, 10)
 
     # Repeated enums tests.
-    #proto.repeated_nested_enum.append(0)
+    # proto.repeated_nested_enum.append(0)
 
   def testSingleScalarGettersAndSetters(self, message_module):
     proto = message_module.TestAllTypes()
@@ -594,9 +594,14 @@ class ReflectionTest(unittest.TestCase):
 
   def testEnum_KeysAndValues(self, message_module):
     if message_module == unittest_pb2:
-      keys = ['FOREIGN_FOO', 'FOREIGN_BAR', 'FOREIGN_BAZ']
-      values = [4, 5, 6]
-      items = [('FOREIGN_FOO', 4), ('FOREIGN_BAR', 5), ('FOREIGN_BAZ', 6)]
+      keys = ['FOREIGN_FOO', 'FOREIGN_BAR', 'FOREIGN_BAZ', 'FOREIGN_BAX']
+      values = [4, 5, 6, 32]
+      items = [
+          ('FOREIGN_FOO', 4),
+          ('FOREIGN_BAR', 5),
+          ('FOREIGN_BAZ', 6),
+          ('FOREIGN_BAX', 32),
+      ]
     else:
       keys = ['FOREIGN_ZERO', 'FOREIGN_FOO', 'FOREIGN_BAR', 'FOREIGN_BAZ']
       values = [0, 4, 5, 6]

src/google/protobuf/compiler/cpp/file_unittest.cc

@@ -7,10 +7,10 @@
 
 #include "google/protobuf/compiler/cpp/file.h"
 
+#include <algorithm>
 #include <cstddef>
 #include <vector>
 
-#include "google/protobuf/testing/googletest.h"
 #include <gtest/gtest.h>
 #include "absl/strings/match.h"
 #include "absl/strings/string_view.h"
@@ -49,6 +49,9 @@ TEST(FileTest, TopologicallyOrderedDescriptors) {
       "TestUnpackedExtensions",
       "TestReservedFields",
       "TestRequiredOneof.NestedMessage",
+      "TestRequiredNoMaskMulti",
+      "TestRequiredEnumNoMask",
+      "TestRequiredEnumMulti",
       "TestRequiredEnum",
       "TestRepeatedString",
       "TestRepeatedScalarDifferentTagSizes",
@@ -193,16 +196,18 @@ TEST(FileTest, TopologicallyOrderedDescriptors) {
   EXPECT_TRUE(kExpectedDescriptorCount == actual_descriptor_order.size())
       << "Expected: " << kExpectedDescriptorCount
       << ", got: " << actual_descriptor_order.size();
-  for (size_t i = 0; i < actual_descriptor_order.size(); ++i) {
-    bool found = false;
+
+  auto limit =
+      std::min(kExpectedDescriptorCount, actual_descriptor_order.size());
+  for (auto i = 0u; i < limit; ++i) {
     const Descriptor* desc = actual_descriptor_order[i];
-    for (size_t j = 0; j < kExpectedDescriptorCount; ++j) {
-      if (absl::EndsWith(desc->full_name(), kExpectedDescriptorOrder[j])) {
-        found = true;
-        break;
-      }
+    bool match = absl::EndsWith(desc->full_name(), kExpectedDescriptorOrder[i]);
+    EXPECT_TRUE(match) << "failed to match; expected "
+                       << kExpectedDescriptorOrder[i] << ", got "
+                       << desc->full_name();
+    if (!match) {
+      break;
     }
-    EXPECT_TRUE(found) << "Descriptor " << desc->full_name() << " not found!";
   }
 }
 

src/google/protobuf/compiler/cpp/unittest.inc

@@ -1023,8 +1023,8 @@ TEST(GENERATED_ENUM_TEST_NAME, MinAndMax) {
   EXPECT_EQ(4, UNITTEST::TestAllTypes::NestedEnum_ARRAYSIZE);
 
   EXPECT_EQ(UNITTEST::FOREIGN_FOO, UNITTEST::ForeignEnum_MIN);
-  EXPECT_EQ(UNITTEST::FOREIGN_BAZ, UNITTEST::ForeignEnum_MAX);
-  EXPECT_EQ(7, UNITTEST::ForeignEnum_ARRAYSIZE);
+  EXPECT_EQ(UNITTEST::FOREIGN_BAX, UNITTEST::ForeignEnum_MAX);
+  EXPECT_EQ(33, UNITTEST::ForeignEnum_ARRAYSIZE);
 
   EXPECT_EQ(1, UNITTEST::TestEnumWithDupValue_MIN);
   EXPECT_EQ(3, UNITTEST::TestEnumWithDupValue_MAX);

src/google/protobuf/message_unittest.inc

@@ -70,6 +70,7 @@ using google::protobuf::io::win32::open;
 #endif
 #endif
 
+
 TEST(MESSAGE_TEST_NAME, SerializeHelpers) {
   // TODO:  Test more helpers?  They're all two-liners so it seems
   //   like a waste of time.
@@ -743,6 +744,7 @@ inline bool IsOptimizeForCodeSize(const Descriptor* descriptor) {
   return descriptor->file()->options().optimize_for() == FileOptions::CODE_SIZE;
 }
 
+
 TEST(MESSAGE_TEST_NAME, Swap) {
   UNITTEST::NestedTestAllTypes o;
   constexpr int kDepth = 5;

src/google/protobuf/unittest.proto

@@ -194,6 +194,7 @@ enum ForeignEnum {
   FOREIGN_FOO = 4;
   FOREIGN_BAR = 5;
   FOREIGN_BAZ = 6;
+  FOREIGN_BAX = 32;  // (1 << 32) to generate a 64b bitmask would be incorrect.
 }
 
 message TestReservedFields {
@@ -410,12 +411,64 @@ message TestNestedChildExtensionData {
 // Required and closed enum fields are considered unknown fields if the value is
 // not valid. We need to make sure it functions as expected.
 message TestRequiredEnum {
-  // Required closed enum results in missing required fields.
   required ForeignEnum required_enum = 1;
+
+  // A dummy optional field.
+  optional int32 a = 2;
+}
+
+// TestRequiredEnum + using enum values that won't fit to 64 bitmask.
+message TestRequiredEnumNoMask {
+  enum NestedEnum {
+    UNSPECIFIED = 0;
+    FOO = 2;
+    BAR = 100;
+    BAZ = -1;  // Intentionally negative.
+  }
+
+  required NestedEnum required_enum = 1;
+
   // A dummy optional field.
   optional int32 a = 2;
 }
 
+message TestRequiredEnumMulti {
+  enum NestedEnum {
+    UNSPECIFIED = 0;
+    FOO = 1;
+    BAR = 2;
+    BAZ = 100;
+  }
+
+  // Intentionally placed in descending field number to force sorting in closed
+  // enum verification.
+  required NestedEnum required_enum_4 = 4;
+  optional int32 a_3 = 3;
+  required NestedEnum required_enum_2 = 2;
+  required ForeignEnum required_enum_1 = 1;
+}
+
+message TestRequiredNoMaskMulti {
+  enum NestedEnum {
+    UNSPECIFIED = 0;
+    FOO = 1;
+    BAR = 2;
+    BAZ = 100;
+  }
+
+  // Intentionally placed in descending field number to force sorting in closed
+  // enum verification. Also, using large field numbers to use tag only
+  // matching for required fields.
+  required fixed32 required_fixed32_80 = 80;
+  required fixed32 required_fixed32_70 = 70;
+
+  required NestedEnum required_enum_64 = 64;
+  required NestedEnum required_enum_4 = 4;
+  optional int32 a_3 = 3;
+  required NestedEnum required_enum_2 = 2;
+  required ForeignEnum required_enum_1 = 1;
+}
+
 // We have separate messages for testing required fields because it's
 // annoying to have to fill in required fields in TestProto in order to
 // do anything with it.  Note that we don't need to test every type of
@@ -490,6 +543,9 @@ message TestNestedRequiredForeign {
   optional int32 dummy = 3;
   // optional message to test required closed enum.
   optional TestRequiredEnum required_enum = 5;
+  optional TestRequiredEnumNoMask required_enum_no_mask = 6;
+  optional TestRequiredEnumMulti required_enum_multi = 7;
+  optional TestRequiredNoMaskMulti required_no_mask = 9;
 }
 
 // Test that we can use NestedMessage from outside TestAllTypes.