Replace uses of bare `assert` with `OPENSIM_ASSERT` #3548

adamkewley · 2023-09-13T16:52:27Z

Fixes issue #3531

Brief summary of changes

Added OpenSim/Common/Assertion.h
- Defined OPENSIM_ASSERT and OPENSIM_ASSERT_FRMOBJ macros
- One difference between these macros and OPENSIM_THROW_IF is that the they cannot customize which exception is thrown. This is so that there is room to (e.g.) add a OpenSim::AssertionError exception in the future.
- Another difference is that the thing being tested must be positive in order to not throw
- And the macros are toggled via NDEBUG, but there is deliberate room to add OPENSIM_FORCE_ASSERTIONS_ON as a cmake variable, which would allow for release builds with only OpenSim-level assertions turned on
Rolled out the assertions code-base wide:
- Replaced all uses of bare assert with OPENSIM_ASSERT or OPENSIM_ASSERT_FRMOBJ if the assertion is made within an OpenSim object. The logic being that it's better for the error message to contain as much information as possible (_FRMOBJ)
- Removed now-unused assert headers (<cassert> and assert.h)
- Removed any commented-out assertions that were probably hanging around

Testing I've completed

Added testAssertion.cpp, which exercises whether the assertions trigger and, when they do, what they print

Looking for feedback on...

CHANGELOG.md (choose one)

Updated with short explanation of change+motivation

This change is

)

OpenSim/Common/Assertion.h

adamkewley · 2023-09-14T08:54:00Z

OpenSim/Common/DebugUtilities.cpp

@@ -38,8 +38,7 @@ void Fatal_Error(const char* msg, const char* function, const char* file,
                                  "line = {})", msg, function, file, line);
    log_critical(str);
    throw std::runtime_error(str);
-    assert(false);
-    exit(1);
+    std::terminate();


This is a drive-by edit, but is included here because:

The original code makes no logical sense anyway (the thread will stop on the throw)

The assert(false) is surplus, turns up in regex searches for bare asserts, and is compiler-toggleable

The exit (and my std::terminate) are also surplus, but I wasn't sure whether something funny could be going on (e.g. disabling exceptions or something unusual), so I just replaced the exit, with the more C++stdlib-friendly std::terminate()

adamkewley · 2023-09-14T08:55:13Z

OpenSim/Common/Exception.cpp

@@ -58,12 +57,6 @@ exception()
    setMessage(aMsg);
    _file = aFile;
    _line = aLine;
-


This is also a drive-by edit that was made because I was using regex to flag uses of assert, and this one is likely to be a cute developer trick that was probably used a while ago (i.e. before IDEs like Visual Studio had debugger toggles to break on throwing an exception) but has been buried here.

adamkewley · 2023-09-14T08:57:25Z

OpenSim/Simulation/Model/GeometryPath.cpp

 #include "Model.h"

+#include <OpenSim/Common/Assertion.h>


Headers additionally rearranged when Assertion.h was added so that #includes with a matching style are clustered+alphabetical

(normalizing the header usage in OpenSim is a separate, larger, issue)

adamkewley · 2023-09-14T08:59:07Z

OpenSim/Utilities/simmToOpenSim/detools.c

@@ -809,7 +809,6 @@ XForm* get_deform_xform (DeformObject* dfm, int deformMode)
      case DE_DEFORM_END_MODE:
         return &dfm->deform_end;
   }
-/*   assert(0);*/


Changes in this file are drive-bys on comments containing the regex \bassert\( (i.e. find all uses of bare assert)

adamkewley · 2023-09-14T11:58:47Z

OpenSim/Common/Test/testAssertion.cpp

+* limitations under the License.                                             *
+* -------------------------------------------------------------------------- */
+
+#include <OpenSim/Common/Assertion.h>


This test implementation isn't following OpenSim's usual patterns and, instead, uses Catch

We've discussed this previously (e.g. with @nickbianco ), but if it's problematic to start writing tests in Catch now then I can refactor

nickbianco

Overall looks great. Just had some minor comments, including when to use OPENSIM_ASSERT versus OPENSIM_ASSERT_FRMOBJ.

Reviewed 44 of 49 files at r1, 4 of 4 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @adamkewley)

OpenSim/Common/Assertion.h line 42 at r5 (raw file):

}

#define OPENSIM_ASSERT_ALWAYS(CONDITION) \

You might be planning to do this already, but these new macros would benefit from some doc comments (a la, OpenSim::Exception).

OpenSim/Common/XMLDocument.cpp line 249 at r5 (raw file):

    // Validate >=  10500 and < latest as sanity check
    OPENSIM_ASSERT(_documentVersion >= 10500 && _documentVersion <= LatestVersion);

OPENSIM_ASSERT_FRMOBJ? I'm not really sure what the best practices are for using plain THROW/ASSERT versus their FRMOBJ counterparts. I'll mark a few other places that seem inconsistent to me, but feel free to ignore.

OpenSim/Common/Test/testAssertion.cpp line 60 at r5 (raw file):

        REQUIRE_THAT(ex.what(), Catch::Matchers::Contains("SomeAssertingFunction"));
        REQUIRE_THAT(ex.what(), Catch::Matchers::Contains("testAssertion.cpp"));
        REQUIRE_THAT(ex.what(), Catch::Matchers::Contains("34"));  // sorry ;)

Is this the line the assertion fails at?

OpenSim/Moco/MocoBounds.cpp line 48 at r5 (raw file):

MocoBounds::MocoBounds(const Property<double>& p) : MocoBounds() {
    OPENSIM_ASSERT(p.size() <= 2);

OPENSIM_ASSERT_FRMOBJ

OpenSim/Moco/MocoTrajectory.cpp line 1350 at r5 (raw file):

        // Trapezoidal rule for uniform grid:
        // dt / 2 (f_0 + 2f_1 + 2f_2 + 2f_3 + ... + 2f_{N-1} + f_N)
        OPENSIM_ASSERT(numTimes > 2);

OPENSIM_ASSERT_FRMOBJ?

OpenSim/Moco/MocoCasADiSolver/MocoCasADiSolver.cpp line 133 at r5 (raw file):

            // The API should make it impossible for both guessFromFile and
            // guessFromAPI to be non-empty.
            OPENSIM_ASSERT(m_guessFromAPI.empty());

OPENSIM_ASSERT_FRMOBJ?

OpenSim/Moco/MocoGoal/MocoAccelerationTrackingGoal.cpp line 39 at r5 (raw file):

        // Should not be able to supply any two simultaneously.
        if (get_acceleration_reference_file() != "") { // acceleration ref file
            OPENSIM_ASSERT(m_acceleration_table.getNumColumns() == 0);

OPENSIM_ASSERT_FRMOBJ? (and below)

OpenSim/Moco/tropter/TropterProblem.cpp line 41 at r5 (raw file):

    const int numDerivatives =
            (int)tropSol.adjunct_names.size() - numMultipliers;
    OPENSIM_ASSERT(numDerivatives >= 0);

OPENSIM_ASSERT_FRMOBJ?

OpenSim/Simulation/Model/PathPoint.h line 57 at r5 (raw file):

    DEPRECATED_14("Use setLocation() instead.")
    void setLocationCoord(const SimTK::State&s, int aXYZ, double aValue) {
        OPENSIM_ASSERT_FRMOBJ(aXYZ>=0 && aXYZ<=2);

OPENSIM_ASSERT? (i.e., should FRMOBJ be used in headers? I guess it's fine as long as the object pointer is available)

OpenSim/Simulation/SimbodyEngine/ConstantCurvatureJoint.cpp line 59 at r5 (raw file):

                "or simulation to avoid this state.",
                pos(0), bound));
        OPENSIM_ASSERT(false);

OPENSIM_ASSERT_FRMOBJ? (and below)

OpenSim/Simulation/SimbodyEngine/Coordinate.cpp line 55 at r5 (raw file):

    SimTK::Real calcValue(const SimTK::Vector& x) const override {
        OPENSIM_ASSERT(x.size() == argumentSize);

OPENSIM_ASSERT_FRMOBJ?

OpenSim/Simulation/SimbodyEngine/TransformAxis.h line 121 at r5 (raw file):

    /** Get one component (0,1, or 2) of the axis vector. **/
    double getAxis(int which) const 
    {   OPENSIM_ASSERT_FRMOBJ(0<=which && which<=2); return getAxis()[which]; }

OPENSIM_ASSERT?

nickbianco

Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @adamkewley)

OpenSim/Common/Assertion.cpp line 41 at r5 (raw file):

        throw Exception{failingFile, failingLine, failingFunction, failingCode};
    }
}

Missing new line.

adamkewley · 2023-09-15T05:55:04Z

You might be planning to do this already, but these new macros would benefit from some doc comments (a la, OpenSim::Exception).

👍

OPENSIM_ASSERT_FRMOBJ?

I intend to use it whenever possible (incl. in headers - the only thing it changes is that it forwards this to the failure function if you use _FRMOBJ).

OPENSIM_ASSERT? (in TransformAxis)

Same logic (i.e. if _FRMOBJ works, use it, because it can potentially put more information into the error message that a user sees)

Is this the line the assertion fails at?

Yep - I'll clarify the comment. It's sorry because hard-coding a source line into a test is obviously a little bit naughty

adamkewley · 2023-09-15T07:49:01Z

OpenSim/Common/XMLDocument.cpp line 249 at r5 (raw file) - not an OpenSim::Object
OpenSim/Common/Test/testAssertion.cpp line 60 at r5 (raw file): fixed
OpenSim/Moco/MocoBounds.cpp line 48 at r5 (raw file): - changed
OpenSim/Moco/MocoTrajectory.cpp line 1350 at r5 (raw file): - not an Object
OpenSim/Moco/MocoCasADiSolver/MocoCasADiSolver.cpp line 133 at r5 (raw file) - changed
OpenSim/Moco/MocoGoal/MocoAccelerationTrackingGoal.cpp line 39 at r5 (raw file): - changed
OpenSim/Moco/tropter/TropterProblem.cpp line 41 at r5 (raw file): not an Object (tropter::Problem)
OpenSim/Simulation/Model/PathPoint.h line 57 at r5 (raw file): - should remain as OPENSIM_ASSERT_FRMOBJ, there's no downside apart from an extra this ptr to a fully-constructed base class
OpenSim/Simulation/SimbodyEngine/ConstantCurvatureJoint.cpp line 59 at r5 (raw file): - changed
OpenSim/Simulation/SimbodyEngine/Coordinate.cpp line 55 at r5 (raw file): not an Object (is a SimTK::Function_<T>)
OpenSim/Simulation/SimbodyEngine/TransformAxis.h line 121 at r5 (raw file): leave as OPENSIM_ASSERT_FRMOBJ (it's an Object)

adamkewley

Reviewable status: 40 of 50 files reviewed, 14 unresolved discussions (waiting on @nickbianco)

OpenSim/Common/Assertion.h line 42 at r5 (raw file):