[ntuple] Fail cleanly if a field value cannot be constructed

roottest/root/ntuple/CMakeLists.txt

@@ -2,4 +2,10 @@ ROOTTEST_ADD_TEST(test_Basics
                   MACRO basics.C
                   OUTREF test_basics.ref)
 
+# issue 13910: MakeField() must fail cleanly for invalid types and keep the
+# model/interpreter usable.
+ROOTTEST_ADD_TEST(test_MakeFieldInvalid
+                  MACRO makefield_invalid.C
+                  PASSREGEX "all MakeField failure checks passed")
+
 ROOTTEST_ADD_TESTDIRS()

roottest/root/ntuple/makefield_invalid.C

@@ -0,0 +1,105 @@
+#include <ROOT/RNTupleModel.hxx>
+
+#include <TSystem.h>
+
+#include <exception>
+#include <iostream>
+#include <string>
+#include <vector>
+
+// Regression test for https://github.com/root-project/root/issues/13910:
+// MakeField() must report a diagnosable error for types that RNTuple cannot
+// construct, and it must leave the model and the interpreter usable.
+
+class RNTupleTestAbstract {
+public:
+   virtual ~RNTupleTestAbstract() = default;
+   virtual void Virt() = 0;
+};
+
+class RNTupleTestDerived : public RNTupleTestAbstract {
+public:
+   int fValue = 42;
+   void Virt() final {}
+};
+
+class RNTupleTestNoDefaultCtor {
+public:
+   explicit RNTupleTestNoDefaultCtor(int value) : fValue(value) {}
+   int fValue = 0;
+};
+
+namespace {
+
+int gFailures = 0;
+
+template <typename T>
+void ExpectFieldFailure(ROOT::RNTupleModel &model, const char *label, const char *fieldName, const char *expected)
+{
+   try {
+      // Keep the returned value alive until after the failure is reported: it
+      // may point to unconstructed memory, so letting it go out of scope would
+      // crash instead of reporting a clean test failure.
+      auto value = model.MakeField<T>(fieldName);
+      (void)value;
+      std::cerr << "FAIL: MakeField<" << label << "> did not throw" << std::endl;
+      gSystem->Exit(1);
+   } catch (const std::exception &ex) {
+      const std::string msg = ex.what();
+      std::cout << "MakeField<" << label << "> threw: " << msg << std::endl;
+      if (msg.find(expected) == std::string::npos) {
+         std::cerr << "FAIL: expected the error message to contain \"" << expected << "\"" << std::endl;
+         ++gFailures;
+      }
+   } catch (...) {
+      std::cerr << "FAIL: MakeField<" << label << "> threw a non-standard exception" << std::endl;
+      ++gFailures;
+   }
+}
+
+} // namespace
+
+void makefield_invalid()
+{
+   {
+      auto model = ROOT::RNTupleModel::Create();
+      ExpectFieldFailure<RNTupleTestAbstract>(*model, "Abstract", "abstract", "abstract");
+      if (model->GetDefaultEntry().HasField("abstract")) {
+         std::cerr << "FAIL: failed field \"abstract\" was added to the default entry" << std::endl;
+         ++gFailures;
+      }
+
+      // A failed MakeField() must not leave a partial field in the default
+      // entry: further fields on the same model have to keep working.
+      try {
+         model->MakeField<std::vector<float>>("vec");
+         std::cout << "OK: model usable after abstract failure" << std::endl;
+      } catch (const std::exception &ex) {
+         std::cerr << "FAIL: model unusable after abstract failure: " << ex.what() << std::endl;
+         ++gFailures;
+      }
+   }
+
+   {
+      auto model = ROOT::RNTupleModel::Create();
+      ExpectFieldFailure<RNTupleTestNoDefaultCtor>(*model, "NoDefaultCtor", "nodefault", "default constructor");
+      if (model->GetDefaultEntry().HasField("nodefault")) {
+         std::cerr << "FAIL: failed field \"nodefault\" was added to the default entry" << std::endl;
+         ++gFailures;
+      }
+
+      try {
+         model->MakeField<RNTupleTestDerived>("derived");
+         std::cout << "OK: model usable after no-default-constructor failure" << std::endl;
+      } catch (const std::exception &ex) {
+         std::cerr << "FAIL: model unusable after no-default-constructor failure: " << ex.what() << std::endl;
+         ++gFailures;
+      }
+   }
+
+   if (gFailures != 0) {
+      std::cerr << gFailures << " check(s) failed" << std::endl;
+      gSystem->Exit(1);
+   }
+   std::cout << "all MakeField failure checks passed" << std::endl;
+}

tree/ntuple/inc/ROOT/REntry.hxx

@@ -85,9 +85,11 @@ private:
    template <typename T>
    std::shared_ptr<T> AddValue(ROOT::RField<T> &field)
    {
+      // Create the value first: if construction fails, the entry must not be
+      // left with a field name/type that has no corresponding value.
+      auto value = field.CreateValue();
       fFieldName2Token[field.GetQualifiedFieldName()] = fValues.size();
       fFieldTypes.push_back(field.GetTypeName());
-      auto value = field.CreateValue();
       fValues.emplace_back(value);
       // We know that the created RValue has the right type, skip the unnecessary check.
       return std::static_pointer_cast<T>(value.template GetPtr<void>());

tree/ntuple/src/RFieldBase.cxx

@@ -658,7 +658,15 @@ void *ROOT::RFieldBase::CreateObjectRawPtr() const
       where = operator new(GetValueSize(), std::align_val_t(GetAlignment()));
    }
    R__ASSERT(where != nullptr);
-   ConstructValue(where);
+   try {
+      ConstructValue(where);
+   } catch (...) {
+      if (align <= sizeof(std::max_align_t))
+         operator delete(where);
+      else
+         operator delete(where, std::align_val_t(align));
+      throw;
+   }
    return where;
 }
 

tree/ntuple/src/RFieldMeta.cxx

@@ -109,6 +109,24 @@ void EnsureValidUserClass(TClass *cl, const ROOT::RFieldBase &field, std::string
    }
 }
 
+/// Construct a class value without going through TClass::New() if the class
+/// cannot be instantiated. TClass::New() may JIT a constructor wrapper, and a
+/// failed compilation can leave the interpreter in an inconsistent state.
+void ConstructClassValue(TClass *cl, const ROOT::RFieldBase &field, void *where)
+{
+   const std::string typeName = field.GetTypeName();
+   if (cl->Property() & kIsAbstract) {
+      throw ROOT::RException(R__FAIL("cannot construct an object of type " + typeName + ": the class is abstract"));
+   }
+   if (!cl->HasDefaultConstructor(kTRUE)) {
+      throw ROOT::RException(
+         R__FAIL("cannot construct an object of type " + typeName + ": the class has no default constructor"));
+   }
+   if (!cl->New(where)) {
+      throw ROOT::RException(R__FAIL("cannot construct an object of type " + typeName));
+   }
+}
+
 TEnum *EnsureValidEnum(std::string_view enumName)
 {
    auto e = TEnum::GetEnum(std::string(enumName).c_str());
@@ -627,7 +645,7 @@ void ROOT::RClassField::ReconcileOnDiskField(const RNTupleDescriptor &desc)
 
 void ROOT::RClassField::ConstructValue(void *where) const
 {
-   fClass->New(where);
+   ConstructClassValue(fClass, *this, where);
 }
 
 ROOT::RClassField::RClassDeleter::RClassDeleter(TClass *cl) : RDeleter(cl->GetClassAlignment()), fClass(cl) {}
@@ -1070,7 +1088,7 @@ void ROOT::RSoAField::ReconcileOnDiskField(const RNTupleDescriptor &desc)
 
 void ROOT::RSoAField::ConstructValue(void *where) const
 {
-   fClass->New(where);
+   ConstructClassValue(fClass, *this, where);
 }
 
 ROOT::RSoAField::RSoADeleter::RSoADeleter(TClass *cl) : RDeleter(cl->GetClassAlignment()), fSoAClass(cl) {}
@@ -1714,7 +1732,7 @@ void ROOT::RStreamerField::ReconcileOnDiskField(const RNTupleDescriptor &desc)
 
 void ROOT::RStreamerField::ConstructValue(void *where) const
 {
-   fClass->New(where);
+   ConstructClassValue(fClass, *this, where);
 }
 
 ROOT::RStreamerField::RStreamerFieldDeleter::RStreamerFieldDeleter(TClass *cl)