[ntuple] fixes to subfield name handling

tree/ntuple/inc/ROOT/RField.hxx

@@ -69,14 +69,18 @@ class RFieldZero final : public RFieldBase {
    /// This flag is reset on Clone().
    bool fAllowFieldSubstitutions = false;
 
+   std::unordered_set<std::string> fSubFieldNames; ///< Efficient detection of duplicate field names
+
 protected:
    std::unique_ptr<RFieldBase> CloneImpl(std::string_view newName) const final;
    void ConstructValue(void *) const final {}
 
 public:
    RFieldZero() : RFieldBase("", "", ROOT::ENTupleStructure::kRecord, false /* isSimple */) {}
 
-   using RFieldBase::Attach;
+   /// A public version of the Attach method that allows piece-wise construction of the zero field.
+   /// Will throw on duplicate subfield names.
+   void Attach(std::unique_ptr<RFieldBase> child);
    size_t GetValueSize() const final { return 0; }
    size_t GetAlignment() const final { return 0; }
 

tree/ntuple/src/RField.cxx

@@ -38,11 +38,22 @@ void ROOT::Internal::SetAllowFieldSubstitutions(RFieldZero &fieldZero, bool val)
    fieldZero.fAllowFieldSubstitutions = val;
 }
 
+void ROOT::RFieldZero::Attach(std::unique_ptr<RFieldBase> child)
+{
+   const std::string childName = child->GetFieldName();
+   if (fSubFieldNames.count(childName) > 0)
+      throw RException(R__FAIL("duplicate field name: " + childName));
+   RFieldBase::Attach(std::move(child), "");
+   fSubFieldNames.insert(childName);
+}
+
 std::unique_ptr<ROOT::RFieldBase> ROOT::RFieldZero::CloneImpl(std::string_view /*newName*/) const
 {
    auto result = std::make_unique<RFieldZero>();
-   for (auto &f : fSubfields)
+   for (auto &f : fSubfields) {
       result->Attach(f->Clone(f->GetFieldName()));
+      result->fSubFieldNames.insert(f->GetFieldName());
+   }
    return result;
 }
 
@@ -603,13 +614,19 @@ ROOT::RRecordField::RRecordField(std::string_view fieldName, std::vector<std::un
 {
    fTraits |= kTraitTrivialType;
    fOffsets.reserve(itemFields.size());
+   std::unordered_set<std::string> fieldNames;
    for (auto &item : itemFields) {
+      const auto itemName = item->GetFieldName();
+      if (fieldNames.count(itemName) > 0) {
+         throw RException(R__FAIL("duplicate field name: " + itemName));
+      }
       fSize += GetItemPadding(fSize, item->GetAlignment());
       fOffsets.push_back(fSize);
       fMaxAlignment = std::max(fMaxAlignment, item->GetAlignment());
       fSize += item->GetValueSize();
       fTraits &= item->GetTraits();
       Attach(std::move(item));
+      fieldNames.insert(itemName);
    }
    fTraits |= !emulatedFromType.empty() * kTraitEmulatedField;
    // Trailing padding: although this is implementation-dependent, most add enough padding to comply with the

tree/ntuple/test/ntuple_field_name.cxx

@@ -51,3 +51,20 @@ TEST(RNTuple, SubFieldName)
       EXPECT_EQ("_1", itr->GetFieldName());
    }
 }
+
+TEST(RNTuple, FieldNameCollision)
+{
+   ROOT::RFieldZero zero;
+   zero.Attach(std::make_unique<RField<char>>("x"));
+   EXPECT_THROW(zero.Attach(std::make_unique<RField<char>>("x")), ROOT::RException);
+
+   std::vector<std::unique_ptr<ROOT::RFieldBase>> items;
+   items.emplace_back(std::make_unique<RField<char>>("x"));
+   items.emplace_back(std::make_unique<RField<char>>("x"));
+   try {
+      ROOT::RRecordField("f", std::move(items));
+      FAIL() << "duplicate field names should throw";
+   } catch (const ROOT::RException &err) {
+      EXPECT_THAT(err.what(), testing::HasSubstr("duplicate field name"));
+   }
+}