SIMPLNX_RESULT_REQUIRE_VALID evaluates its argument 4+ times, silently re-running filter calls

[imikejackson]

Summary

SIMPLNX_RESULT_REQUIRE_VALID and SIMPLNX_RESULT_REQUIRE_INVALID expand their argument four or more times. Passing them a function-call expression therefore re-executes that call, which for a filter means re-running it against an already-mutated DataStructure.

The macros

test/UnitTestCommon/include/simplnx/UnitTest/UnitTestCommon.hpp:43-62

#define SIMPLNX_RESULT_CATCH_PRINT(result)                        \
  for(const auto& warning : (result).warnings())      /* 1 */     \
  { WARN(...); }                                                  \
  if((result).invalid())                              /* 2 */     \
  {                                                               \
    for(const auto& error : (result).errors())        /* 3 */     \
    { ... }                                                       \
  }

#define SIMPLNX_RESULT_REQUIRE_VALID(result)                      \
  SIMPLNX_RESULT_CATCH_PRINT(result);                             \
  REQUIRE((result).valid());                          /* 4 */

So result is evaluated at least 4 times. SIMPLNX_RESULT_REQUIRE_INVALID has the same shape.

Why it matters

The intended usage is a stored local, which is safe:

auto executeResult = filter.execute(dataStructure, args);
SIMPLNX_RESULT_REQUIRE_VALID(executeResult.result);   // fine

But the macro invites the terser form, which is not:

SIMPLNX_RESULT_REQUIRE_VALID(filter.execute(dataStructure, args).result);   // runs execute() 4x

That second form compiles, usually passes, and silently executes the filter four times against the same DataStructure. Failure modes include:

• filters that append to or resize an existing array producing 4x the data;
• filters that create a DataObject failing on the second run because the object already exists — reported as a confusing error from a test that "just calls execute once";
• any filter whose output depends on prior state producing results no one can reproduce by hand;
• masked non-determinism, since the assertion inspects the last run while later code inspects a DataStructure mutated four times.

It is also easy to introduce accidentally when factoring a test into a helper that returns a Result<>.

Current status

Found while reviewing PR work on the surface meshing filters. I audited every call site in the new test files added there plus the shared test helper — all pass a stored local, so nothing is currently misbehaving. This is a latent trap, not an active bug.

I have not audited the pre-existing call sites across the whole repo.

Suggested fixes

Any of these would close it; the first is the smallest:

1. Evaluate once inside the macro. Bind to a hidden local before use:

   #define SIMPLNX_RESULT_REQUIRE_VALID(result)                                   \
     do {                                                                         \
       const auto& _snx_res = (result);                                            \
       SIMPLNX_RESULT_CATCH_PRINT(_snx_res);                                       \
       REQUIRE(_snx_res.valid());                                                  \
     } while(false)

   Note the current macros are also not wrapped in a do { } while(false), so they misbehave as the body of an unbraced if. Worth fixing at the same time. Beware that binding a const auto& to a temporary is fine here (lifetime extends to the end of the full expression / enclosing block for the local), but a const auto&& or a plain auto copy may be preferable depending on what callers pass.

2. Replace the macros with function templates, which get single-evaluation and correct scoping for free. Catch2's REQUIRE needs to appear lexically in the test for good failure reporting, so this would need care — probably a helper that formats the diagnostics plus a thin macro that only wraps the REQUIRE.

3. Audit and document, if changing the macros is judged too invasive: sweep existing call sites for call expressions and add a comment on the macro warning that the argument must be a stored variable.

Reproducer sketch

A test that asserts a filter runs exactly once will fail under the current macros:

// pseudo: a filter that appends one tuple per execute()
SIMPLNX_RESULT_REQUIRE_VALID(appendOneTuple(dataStructure).result);
REQUIRE(array.getNumberOfTuples() == 1);   // observes 4