Print better error message when registering an extension with a dupli…

…cate number.

Before, the upb_ExtensionRegistry_AddArray API would just return a boolean
indicating whether the operation succeeded or failed. This is not descriptive
enough in some cases and made the error output confusing.

Specifically, when trying to register an extension with a duplicate extension
number, AddArray first performs a map lookup before inserting the extension
entry into the registry. The code handled lookup failure (due to duplicates)
the same way as insertion failure (due to OOM), and printed an error message
that showed OOM when there is a duplicate array entry.

This was acknolwedged in a TODO in the AddArray code comment -- which is now
fixed. :)

PiperOrigin-RevId: 700764584

hpb/extension.h

@@ -134,8 +134,9 @@ class ExtensionRegistry {
   void AddExtension(const ExtensionIdentifier& id) {
     if (registry_) {
       auto* extension = id.mini_table_ext();
-      bool success = upb_ExtensionRegistry_AddArray(registry_, &extension, 1);
-      if (!success) {
+      upb_ExtensionRegistryStatus status =
+          upb_ExtensionRegistry_AddArray(registry_, &extension, 1);
+      if (status != kUpb_ExtensionRegistryStatus_Ok) {
         registry_ = nullptr;
       }
     }

python/google/protobuf/internal/message_factory_test.py

@@ -179,14 +179,19 @@ def testDuplicateExtensionNumber(self):
         type_name='Duplicate',
         extendee='Container',
     )
+
     if api_implementation.Type() == 'upb':
       with self.assertRaisesRegex(
           TypeError,
-          # TODO - b/380939902: Figure out why this fails with an OOM message.
-          "Couldn't build proto file into descriptor pool: out of memory",
+          "Couldn't build proto file into descriptor pool: "
+          'duplicate extension entry',
       ):
         pool.Add(f)
     else:
+      # TODO: b/381131694 - Ensure conformance between upb/c++/python.
+      # C++ and pure Python implementations should raise an error when adding a
+      # duplicate extension number. There doesn't seem to be a benefit to failing
+      # only when GetMessageClassesForFiles is called.
       pool.Add(f)
 
       with self.assertRaises(Exception) as cm:

upb/mini_table/extension_registry.c

@@ -11,6 +11,7 @@
 #include <stdint.h>
 #include <string.h>
 
+#include "upb/hash/common.h"
 #include "upb/hash/str_table.h"
 #include "upb/mem/arena.h"
 #include "upb/mini_table/extension.h"
@@ -39,24 +40,32 @@ upb_ExtensionRegistry* upb_ExtensionRegistry_New(upb_Arena* arena) {
   return r;
 }
 
-UPB_API bool upb_ExtensionRegistry_Add(upb_ExtensionRegistry* r,
-                                       const upb_MiniTableExtension* e) {
+UPB_API upb_ExtensionRegistryStatus upb_ExtensionRegistry_Add(
+    upb_ExtensionRegistry* r, const upb_MiniTableExtension* e) {
   char buf[EXTREG_KEY_SIZE];
   extreg_key(buf, e->UPB_PRIVATE(extendee), upb_MiniTableExtension_Number(e));
-  if (upb_strtable_lookup2(&r->exts, buf, EXTREG_KEY_SIZE, NULL)) return false;
-  return upb_strtable_insert(&r->exts, buf, EXTREG_KEY_SIZE,
-                             upb_value_constptr(e), r->arena);
+
+  if (upb_strtable_lookup2(&r->exts, buf, EXTREG_KEY_SIZE, NULL)) {
+    return kUpb_ExtensionRegistryStatus_DuplicateEntry;
+  }
+
+  if (!upb_strtable_insert(&r->exts, buf, EXTREG_KEY_SIZE,
+                           upb_value_constptr(e), r->arena)) {
+    return kUpb_ExtensionRegistryStatus_OutOfMemory;
+  }
+  return kUpb_ExtensionRegistryStatus_Ok;
 }
 
-bool upb_ExtensionRegistry_AddArray(upb_ExtensionRegistry* r,
-                                    const upb_MiniTableExtension** e,
-                                    size_t count) {
+upb_ExtensionRegistryStatus upb_ExtensionRegistry_AddArray(
+    upb_ExtensionRegistry* r, const upb_MiniTableExtension** e, size_t count) {
   const upb_MiniTableExtension** start = e;
   const upb_MiniTableExtension** end = UPB_PTRADD(e, count);
+  upb_ExtensionRegistryStatus status = kUpb_ExtensionRegistryStatus_Ok;
   for (; e < end; e++) {
-    if (!upb_ExtensionRegistry_Add(r, *e)) goto failure;
+    status = upb_ExtensionRegistry_Add(r, *e);
+    if (status != kUpb_ExtensionRegistryStatus_Ok) goto failure;
   }
-  return true;
+  return kUpb_ExtensionRegistryStatus_Ok;
 
 failure:
   // Back out the entries previously added.
@@ -67,7 +76,8 @@ bool upb_ExtensionRegistry_AddArray(upb_ExtensionRegistry* r,
                upb_MiniTableExtension_Number(ext));
     upb_strtable_remove2(&r->exts, buf, EXTREG_KEY_SIZE, NULL);
   }
-  return false;
+  UPB_ASSERT(status != kUpb_ExtensionRegistryStatus_Ok);
+  return status;
 }
 
 #ifdef UPB_LINKARR_DECLARE
@@ -80,7 +90,9 @@ bool upb_ExtensionRegistry_AddAllLinkedExtensions(upb_ExtensionRegistry* r) {
   for (const upb_MiniTableExtension* p = start; p < stop; p++) {
     // Windows can introduce zero padding, so we have to skip zeroes.
     if (upb_MiniTableExtension_Number(p) != 0) {
-      if (!upb_ExtensionRegistry_Add(r, p)) return false;
+      if (upb_ExtensionRegistry_Add(r, p) != kUpb_ExtensionRegistryStatus_Ok) {
+        return false;
+      }
     }
   }
   return true;

upb/mini_table/extension_registry.h

@@ -8,6 +8,9 @@
 #ifndef UPB_MINI_TABLE_EXTENSION_REGISTRY_H_
 #define UPB_MINI_TABLE_EXTENSION_REGISTRY_H_
 
+#include <stddef.h>
+#include <stdint.h>
+
 #include "upb/mem/arena.h"
 #include "upb/mini_table/extension.h"
 #include "upb/mini_table/message.h"
@@ -55,21 +58,25 @@ extern "C" {
 
 typedef struct upb_ExtensionRegistry upb_ExtensionRegistry;
 
+typedef enum {
+  kUpb_ExtensionRegistryStatus_Ok = 0,
+  kUpb_ExtensionRegistryStatus_DuplicateEntry = 1,
+  kUpb_ExtensionRegistryStatus_OutOfMemory = 2,
+} upb_ExtensionRegistryStatus;
+
 // Creates a upb_ExtensionRegistry in the given arena.
 // The arena must outlive any use of the extreg.
 UPB_API upb_ExtensionRegistry* upb_ExtensionRegistry_New(upb_Arena* arena);
 
-UPB_API bool upb_ExtensionRegistry_Add(upb_ExtensionRegistry* r,
-                                       const upb_MiniTableExtension* e);
+UPB_API upb_ExtensionRegistryStatus upb_ExtensionRegistry_Add(
+    upb_ExtensionRegistry* r, const upb_MiniTableExtension* e);
 
 // Adds the given extension info for the array |e| of size |count| into the
 // registry. If there are any errors, the entire array is backed out.
 // The extensions must outlive the registry.
 // Possible errors include OOM or an extension number that already exists.
-// TODO: There is currently no way to know the exact reason for failure.
-bool upb_ExtensionRegistry_AddArray(upb_ExtensionRegistry* r,
-                                    const upb_MiniTableExtension** e,
-                                    size_t count);
+upb_ExtensionRegistryStatus upb_ExtensionRegistry_AddArray(
+    upb_ExtensionRegistry* r, const upb_MiniTableExtension** e, size_t count);
 
 #ifdef UPB_LINKARR_DECLARE
 

upb/reflection/file_def.c

@@ -453,8 +453,15 @@ void _upb_FileDef_Create(upb_DefBuilder* ctx,
   }
 
   if (file->ext_count) {
-    bool ok = upb_ExtensionRegistry_AddArray(
+    upb_ExtensionRegistryStatus status = upb_ExtensionRegistry_AddArray(
         _upb_DefPool_ExtReg(ctx->symtab), file->ext_layouts, file->ext_count);
-    if (!ok) _upb_DefBuilder_OomErr(ctx);
+    if (status != kUpb_ExtensionRegistryStatus_Ok) {
+      if (status == kUpb_ExtensionRegistryStatus_OutOfMemory) {
+        _upb_DefBuilder_OomErr(ctx);
+      }
+
+      UPB_ASSERT(status == kUpb_ExtensionRegistryStatus_DuplicateEntry);
+      _upb_DefBuilder_Errf(ctx, "duplicate extension entry");
+    }
   }
 }