diff --git a/include/numsim-materials/core/material_context.h b/include/numsim-materials/core/material_context.h index 2b57391..ba336bd 100644 --- a/include/numsim-materials/core/material_context.h +++ b/include/numsim-materials/core/material_context.h @@ -86,8 +86,19 @@ class material_context { m_engine.update_property(material, property, exclude); } - void commit() { m_engine.commit(); } - void revert() { m_engine.revert(); } + /// Advance / roll back history. The driver owns these calls -- materials and + /// solvers never make them. Both need the same finalize() gate their siblings + /// have: the engine's history list is built by finalize(), so on an + /// unfinalized context they used to be silent no-ops rather than errors. + void commit() { + check_finalized("commit"); + m_engine.commit(); + } + + void revert() { + check_finalized("revert"); + m_engine.revert(); + } // --- Checkpoint / Restart --- diff --git a/include/numsim-materials/core/object_store.h b/include/numsim-materials/core/object_store.h index df64280..bd8311a 100644 --- a/include/numsim-materials/core/object_store.h +++ b/include/numsim-materials/core/object_store.h @@ -1,9 +1,11 @@ #ifndef OBJECT_STORE_H #define OBJECT_STORE_H +#include #include #include #include +#include #include #include #include "numsim-materials/core/material_base.h" @@ -36,6 +38,7 @@ class object_store { template Material& create(parameter_handler& params, property_handler& properties, material_handler& materials) { + check_name_available(params); auto ptr = std::make_unique(params, properties, materials); auto& ref = *ptr; adopt(std::move(ptr), materials); @@ -47,6 +50,7 @@ class object_store { parameter_handler& params, property_handler& properties, material_handler& materials) { + check_name_available(params); auto ptr = factory_type::instance().create( type_name, params, properties, materials); auto& ref = *ptr; @@ -55,8 +59,12 @@ class object_store { } /// Find by name. + /// + /// Transparent lookup: hashing a string_view directly keeps this allocation + /// free, which is what lets it be noexcept. Building a std::string for the + /// key could throw bad_alloc, and under noexcept that is std::terminate. material_interface_type* find(std::string_view name) const noexcept { - auto it = m_by_name.find(std::string(name)); + auto it = m_by_name.find(name); return it != m_by_name.end() ? it->second : nullptr; } @@ -65,6 +73,32 @@ class object_store { } private: + /// Reject a duplicate name before anything is constructed. + /// + /// Names are the only way materials address each other -- every input wire, + /// every material_ref and every property lookup resolves against one. A + /// second material under a live name used to overwrite the first in the name + /// index and in the material handler, leaving it owned by m_storage but + /// unreachable: consumers wired before the collision kept the first, + /// consumers wired after silently got the second. With mismatched property + /// types that is not a wrong answer but a wrong pointer, because lookups + /// resolve through a static_cast -- a consumer expecting a tensor writes + /// through a pointer to a double. ASan reports a heap-buffer-overflow; + /// without ASan it is silent. + /// + /// Checked here, before construction, rather than in adopt(): a material + /// registers its properties under its own name while constructing, so by the + /// time adopt() sees it the collision has already happened in the property + /// registry. + void check_name_available(const parameter_handler& params) const { + const auto& name = params.template get("name"); + if (m_by_name.contains(name)) + throw std::invalid_argument( + "object_store: a material named '" + name + "' already exists. " + "Material names must be unique -- they are what input wires, " + "material references and property lookups resolve against."); + } + /// Take ownership of a fully constructed material and publish it. /// /// Registration happens HERE and not in material_base's constructor, because @@ -84,9 +118,18 @@ class object_store { m_storage.push_back(std::move(ptr)); } + /// Lets find() look up by string_view without building a std::string. + struct transparent_string_hash { + using is_transparent = void; + std::size_t operator()(std::string_view s) const noexcept { + return std::hash{}(s); + } + }; + std::vector> m_storage; std::vector m_interfaces; - std::unordered_map m_by_name; + std::unordered_map> m_by_name; }; } // namespace numsim::materials diff --git a/include/numsim-materials/core/property_engine.h b/include/numsim-materials/core/property_engine.h index 9418daf..54abc63 100644 --- a/include/numsim-materials/core/property_engine.h +++ b/include/numsim-materials/core/property_engine.h @@ -45,7 +45,14 @@ class property_engine { void update_property(const std::string& material, const std::string& property, const std::unordered_set& exclude = {}) { auto* target = find_in_graph(material, property); - if (!target) return; + // Returning quietly here made a typo indistinguishable from a property that + // legitimately never changes: the driver asks for an update, gets no error, + // and reads a stale value for the rest of the analysis. + if (!target) + throw std::runtime_error( + "update_property(): property '" + material + "::" + property + + "' is not in the graph. Check the material and property names, and " + "that the material publishes this property."); update_property(target, exclude); } diff --git a/include/numsim-materials/materials/tensor_component_stepper.h b/include/numsim-materials/materials/tensor_component_stepper.h index abd9577..aa00091 100755 --- a/include/numsim-materials/materials/tensor_component_stepper.h +++ b/include/numsim-materials/materials/tensor_component_stepper.h @@ -1,6 +1,8 @@ #ifndef TENSOR_COMPONENT_STEPPER_H #define TENSOR_COMPONENT_STEPPER_H +#include +#include #include #include "numsim-materials/core/material_base.h" @@ -35,7 +37,9 @@ class tensor_component_stepper m_tensor(base::template add_output("strain", &tensor_component_stepper::update)), m_inc(base::template get_parameter("increment")), m_indices(base::template get_parameter("indices")) - {} + { + validate_indices(); + } /** * @brief Defines the parameters required by this class. @@ -68,6 +72,30 @@ class tensor_component_stepper } private: + /// "indices" addresses a tensor component directly, so a deck typo used to + /// land in update() as an out-of-bounds subscript: [9,9] on a 3x3 is a read + /// and a write past the end of the tensor (ASan: heap-buffer-overflow), and + /// too few entries read m_indices[1] past the end of the vector itself. + /// Neither is checked by tmech, so this is the only place it can be caught. + /// Checked once here rather than in update(), which runs per increment. + void validate_indices() const { + if (m_indices.size() != Rank) + throw std::invalid_argument( + "tensor_component_stepper ('" + this->name() + "'): \"indices\" has " + + std::to_string(m_indices.size()) + " entr" + + (m_indices.size() == 1 ? "y" : "ies") + ", but a rank-" + + std::to_string(Rank) + " tensor needs exactly " + + std::to_string(Rank) + "."); + + for (std::size_t i = 0; i < m_indices.size(); ++i) + if (m_indices[i] >= Dim) + throw std::invalid_argument( + "tensor_component_stepper ('" + this->name() + "'): \"indices\"[" + + std::to_string(i) + "] is " + std::to_string(m_indices[i]) + + ", which is out of range for a " + std::to_string(Dim) + "D tensor " + "(valid: 0.." + std::to_string(Dim - 1) + ")."); + } + /// Produced properties tensor &m_tensor; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 0caebb8..d148f04 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -10,6 +10,7 @@ macro(add_numsim_test TARGET_NAME) endmacro() add_numsim_test(test_property_graph test_property_graph.cpp) +add_numsim_test(test_model_validation test_model_validation.cpp) add_numsim_test(test_materials test_materials.cpp) add_numsim_test(test_damage test_damage.cpp) add_numsim_test(test_property_recorder test_property_recorder.cpp) diff --git a/tests/test_model_validation.cpp b/tests/test_model_validation.cpp new file mode 100644 index 0000000..c9ec28b --- /dev/null +++ b/tests/test_model_validation.cpp @@ -0,0 +1,248 @@ +/// Tests for the checks that reject a malformed model instead of running it. +/// +/// Each of these used to be silent: a duplicate material name aliased one +/// material's properties onto another (and corrupted the heap when their types +/// differed), an out-of-range "indices" subscripted a tensor past its end, an +/// update_property() typo looked like a property that never changes, and +/// commit()/revert() on an unfinalized context did nothing at all. +#include +#include +#include "numsim-materials/core/material_context.h" +#include "numsim-materials/materials/linear_elasticity.h" +#include "numsim-materials/materials/scalar_stepper.h" +#include "numsim-materials/materials/tensor_component_stepper.h" +#include "numsim-materials/default_materials.h" + +namespace { + +using policy = numsim::materials::material_policy_default; +using T = policy::value_type; +using ctx_type = numsim::materials::material_context; +using param_type = policy::ParameterHandler; + +/// Aliased because the comma in the template argument list would otherwise be +/// read as a macro argument separator by EXPECT_THROW. +using stepper2 = numsim::materials::tensor_component_stepper<2, policy>; + +/// A stepper named @p name producing "strain" at component [0,0]. +void add_stepper(ctx_type& ctx, param_type& p, const std::string& name) { + p.clear(); + p.insert("name", name); + p.insert("increment", T{0.1}); + p.insert>("indices", {0, 0}); + ctx.create(p); +} + +// --- Duplicate material names (#56) --- + +TEST(ModelValidation, ASecondMaterialWithALiveNameIsRejected) { + ctx_type ctx; + param_type p; + add_stepper(ctx, p, "stepper"); + + p.clear(); + p.insert("name", "stepper"); + p.insert("increment", T{0.5}); + p.insert>("indices", {1, 1}); + + EXPECT_THROW( + ctx.create(p), + std::invalid_argument); +} + +/// The dangerous case: the two materials publish properties of DIFFERENT +/// types under the same name. Lookups resolve through a static_cast, so +/// before the check this wired a tensor consumer to a double and wrote past +/// the end of it -- a heap-buffer-overflow under ASan, silent without it. +TEST(ModelValidation, ADuplicateNameIsRejectedEvenWhenTheTypesDiffer) { + ctx_type ctx; + param_type p; + add_stepper(ctx, p, "collide"); + + p.clear(); + p.insert("name", "collide"); + p.insert("increment", T{0.1}); + + EXPECT_THROW(ctx.create>(p), + std::invalid_argument); +} + +TEST(ModelValidation, TheDuplicateNameErrorNamesTheMaterial) { + ctx_type ctx; + param_type p; + add_stepper(ctx, p, "the_clashing_name"); + + p.clear(); + p.insert("name", "the_clashing_name"); + p.insert("increment", T{0.1}); + p.insert>("indices", {0, 0}); + + try { + ctx.create(p); + FAIL() << "a duplicate material name was accepted"; + } catch (const std::invalid_argument& e) { + EXPECT_NE(std::string(e.what()).find("the_clashing_name"), + std::string::npos) + << "the message does not name the offending material: " << e.what(); + } +} + +/// The rejection must happen before the second material is constructed, or it +/// has already overwritten the first one's entries in the property registry. +/// The surviving material must be the one that was there first. +TEST(ModelValidation, TheFirstMaterialSurvivesARejectedDuplicate) { + ctx_type ctx; + param_type p; + add_stepper(ctx, p, "stepper"); + + p.clear(); + p.insert("name", "stepper"); + p.insert("increment", T{99.0}); // would be obvious in the result + p.insert>("indices", {0, 0}); + EXPECT_THROW( + ctx.create(p), + std::invalid_argument); + + p.clear(); + p.insert("name", "elastic"); + p.insert("strain_producer_name", "stepper"); + p.insert("K", T{166.67}); + p.insert("G", T{76.92}); + ctx.create>(p); + + ctx.finalize(); + ctx.update(); + + // One increment of 0.1 from the FIRST stepper, not 99.0 from the rejected one. + const auto& strain = ctx.get>("stepper", "strain"); + EXPECT_DOUBLE_EQ(strain(0, 0), 0.1); +} + +// --- tensor_component_stepper "indices" (#57) --- + +TEST(ModelValidation, AnOutOfRangeIndexIsRejected) { + ctx_type ctx; + param_type p; + p.clear(); + p.insert("name", "stepper"); + p.insert("increment", T{0.1}); + p.insert>("indices", {9, 9}); + + EXPECT_THROW( + ctx.create(p), + std::invalid_argument); +} + +/// Dim is the last valid index plus one -- the off-by-one a range check is +/// most likely to get wrong. +TEST(ModelValidation, TheIndexRangeCheckIsInclusiveOfTheLastComponent) { + ctx_type ctx; + param_type p; + p.clear(); + p.insert("name", "edge"); + p.insert("increment", T{0.1}); + p.insert>("indices", {2, 2}); // valid for 3D + EXPECT_NO_THROW( + ctx.create(p)); + + ctx_type ctx2; + p.clear(); + p.insert("name", "past_edge"); + p.insert("increment", T{0.1}); + p.insert>("indices", {3, 0}); // one past + EXPECT_THROW( + ctx2.create(p), + std::invalid_argument); +} + +/// Too few entries read m_indices[1] past the end of the vector itself, which +/// is a different overflow from the out-of-range one above. +TEST(ModelValidation, AnIndexListOfTheWrongLengthIsRejected) { + ctx_type ctx; + param_type p; + p.clear(); + p.insert("name", "short"); + p.insert("increment", T{0.1}); + p.insert>("indices", {0}); // rank 2 needs 2 + + EXPECT_THROW( + ctx.create(p), + std::invalid_argument); + + ctx_type ctx2; + p.clear(); + p.insert("name", "long"); + p.insert("increment", T{0.1}); + p.insert>("indices", {0, 0, 0}); + EXPECT_THROW( + ctx2.create(p), + std::invalid_argument); +} + +TEST(ModelValidation, TheIndexErrorReportsTheOffendingValueAndTheRange) { + ctx_type ctx; + param_type p; + p.clear(); + p.insert("name", "stepper"); + p.insert("increment", T{0.1}); + p.insert>("indices", {0, 7}); + + try { + ctx.create(p); + FAIL() << "an out-of-range index was accepted"; + } catch (const std::invalid_argument& e) { + const std::string msg = e.what(); + EXPECT_NE(msg.find('7'), std::string::npos) + << "the message does not report the offending value: " << msg; + EXPECT_NE(msg.find("0..2"), std::string::npos) + << "the message does not report the valid range: " << msg; + } +} + +// --- update_property with a name that is not in the graph (#69) --- + +TEST(ModelValidation, UpdatePropertyWithAnUnknownNameThrows) { + ctx_type ctx; + param_type p; + add_stepper(ctx, p, "stepper"); + ctx.finalize(); + + EXPECT_THROW(ctx.update_property("stepper", "starin"), std::runtime_error); + EXPECT_THROW(ctx.update_property("steppr", "strain"), std::runtime_error); + EXPECT_NO_THROW(ctx.update_property("stepper", "strain")); +} + +TEST(ModelValidation, TheUnknownPropertyErrorNamesBothHalves) { + ctx_type ctx; + param_type p; + add_stepper(ctx, p, "stepper"); + ctx.finalize(); + + try { + ctx.update_property("stepper", "starin"); + FAIL() << "a property that is not in the graph was accepted"; + } catch (const std::runtime_error& e) { + const std::string msg = e.what(); + EXPECT_NE(msg.find("stepper"), std::string::npos) << msg; + EXPECT_NE(msg.find("starin"), std::string::npos) << msg; + } +} + +// --- commit()/revert() before finalize() (#60) --- + +TEST(ModelValidation, CommitAndRevertRequireAFinalizedContext) { + ctx_type ctx; + param_type p; + add_stepper(ctx, p, "stepper"); + + // The engine's history list is built by finalize(), so before it these + // walked an empty list and reported success. + EXPECT_THROW(ctx.commit(), std::logic_error); + EXPECT_THROW(ctx.revert(), std::logic_error); + + ctx.finalize(); + EXPECT_NO_THROW(ctx.commit()); + EXPECT_NO_THROW(ctx.revert()); +} + +} // namespace