DM-9846: Improve handling of error messages

doc/mainpage.dox

@@ -44,8 +44,6 @@ that support time, spectra and tables have yet been wrapped. The python wrapper
         before calling AST code.
     - After calling AST code the wrapper checks AST's error state and if invalid,
         the wrapper resets the AST status code and throws `std::runtime_error`.
-        At present the exception contain an error number, rather than useful text,
-        and useful text is printed to stderr. Work is in progress to fix this.
 - The AST functions `set`, `set<X>` and `get<X>` are hidden;
     instead each class has explicit accessors for its attributes, such as @ref Object.getID.
     @ref Mapping "Mappings" are mostly immutable, so they have getters,
@@ -111,15 +109,6 @@ To build only the documentation:
 
 See the unit tests in `tests` and the examples in `examples`
 
-## Plans
-
-- Improve error reporting so exceptions have useful text and no text is printed to stderr.
-
-Longer term desires:
-- Support AST regions
-- Add support for building with cmake.
-- Add doc strings to python wrapped objects.
-
 ## License
 
 This product includes software developed by the LSST Project (http://www.lsst.org/).

include/astshim/Frame.h

@@ -1105,7 +1105,7 @@ class Frame : public Mapping {
     axis order for output data).
     */
     void permAxes(std::vector<int> perm) {
-        detail::assertEqual(perm.size(), "perm.size()", getNaxes(), "naxes");
+        detail::assertEqual(static_cast<int>(perm.size()), "perm.size()", getNaxes(), "naxes");
         astPermAxes(getRawPtr(), perm.data());
         assertOK();
     }
@@ -1504,7 +1504,7 @@ class Frame : public Mapping {
     */
     template <typename T>
     void assertPointLength(T const &p, char const *name) const {
-        if (p.size() != getNin()) {
+        if (static_cast<int>(p.size()) != getNin()) {
             std::ostringstream os;
             os << "point " << name << " has " << p.size() << " axes, but " << getNin() << " required";
             throw std::invalid_argument(os.str());

include/astshim/base.h

@@ -22,9 +22,9 @@
 #ifndef ASTSHIM_BASE_H
 #define ASTSHIM_BASE_H
 
-#include <limits>
-#include <sstream>
+#include <string>
 #include <stdexcept>
+#include <vector>
 
 #include "ndarray.h"
 
@@ -77,22 +77,11 @@ Throw std::runtime_error if AST's state is bad
 
 @param  rawPtr1  An AST object to free if status is bad
 @param  rawPtr2  An AST object to free if status is bad
-*/
-inline void assertOK(AstObject * rawPtr1=nullptr, AstObject * rawPtr2=nullptr) {
-    if (!astOK) {
-        if (rawPtr1) {
-            astAnnul(rawPtr1);
-        }
-        if (rawPtr2) {
-            astAnnul(rawPtr2);
-        }
-        std::ostringstream os;
-        os << "failed with AST status = " << astStatus;
-        astClearStatus;
-        throw std::runtime_error(os.str());
-    }
-}
 
+@note on the first call an error handler is registered
+that saves error messages to a buffer.
+*/
+void assertOK(AstObject * rawPtr1=nullptr, AstObject * rawPtr2=nullptr);
 
 /**
 Control whether graphical escape sequences are included in strings.

src/base.cc

@@ -1,6 +1,70 @@
 #include "astshim/base.h"
 
+#include <sstream>
+#include <stdexcept>
+#include <string>
+#include <vector>
+
 namespace ast {
+namespace {
+
+    static std::ostringstream errorMsgStream;
+
+    /*
+    Write an error message to `errorMsgStream`
+
+    Intended to be registered as an error handler to AST by calling `astSetPutErr(reportError)`.
+    */
+    void reportError(int errNum, const char * errMsg) {
+        errorMsgStream << errMsg;
+    }
+
+    /*
+    Instantiate this class to register `reportError` as an AST error handler.
+    */
+    class ErrorHandler {
+    public:
+        ErrorHandler() {
+            astSetPutErr(reportError);
+        }
+
+        ErrorHandler(ErrorHandler const &) = delete;
+        ErrorHandler(ErrorHandler &&) = delete;
+        ErrorHandler & operator=(ErrorHandler const &) = delete;
+        ErrorHandler & operator=(ErrorHandler &&) = delete;
+
+        static std::string getErrMsg() {
+            auto errMsg = errorMsgStream.str();
+            // clear status bits
+            errorMsgStream.clear();
+            if (errMsg.empty()) {
+                errMsg = "Failed with AST status = " + std::to_string(astStatus);
+            } else {
+                // empty the stream
+                errorMsgStream.str("");
+            }
+            astClearStatus;
+            return errMsg;
+        }
+    };
+
+}  // <anonymous>
+
+void assertOK(AstObject * rawPtr1, AstObject * rawPtr2) {
+    // Construct ErrorHandler once, the first time this function is called.
+    // This is done to initialize `errorMsgStream` and register `reportError` as the AST error handler.
+    // See https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use
+    static ErrorHandler * errHandler = new ErrorHandler();
+    if (!astOK) {
+        if (rawPtr1) {
+            astAnnul(rawPtr1);
+        }
+        if (rawPtr2) {
+            astAnnul(rawPtr2);
+        }
+        throw std::runtime_error(errHandler->getErrMsg());
+    }
+}
 
 ConstArray2D arrayFromVector(std::vector<double> const &vec, int nAxes) {
     if (vec.size() % nAxes != 0) {

tests/test_channel.py

@@ -24,7 +24,7 @@ def test_ChannelFileStream(self):
         nobj = outchan.write(zoommap)
         self.assertEqual(nobj, 1)
 
-        with self.assertRaises(Exception):
+        with self.assertRaises(RuntimeError):
             obj = outchan.read()
 
         instream = astshim.FileStream(path1, False)

tests/test_normMap.py

@@ -34,7 +34,7 @@ def test_NormMapFrame(self):
 
     def testNormMapMap(self):
         """Check that NormMap(Mapping) is an error"""
-        with self.assertRaises(Exception):
+        with self.assertRaises(TypeError):
             astshim.NormMap(astshim.UnitMap(1))
 
     def test_NormMapSkyFrame(self):

tests/test_object.py

@@ -1,6 +1,8 @@
 from __future__ import absolute_import, division, print_function
 import unittest
 
+import numpy as np
+
 import astshim
 from astshim.test import ObjectTestCase
 
@@ -24,23 +26,6 @@ def test_attributes(self):
         self.assertEqual(obj.getIdent(), "")
         self.assertEqual(obj.getUseDefs(), True)
 
-    def test_unknown_attributes(self):
-        """Test accessing unknown attributes"""
-        obj = astshim.ZoomMap(2, 1.3)
-
-        self.assertFalse(obj.hasAttribute("NonExistentAttribute"))
-
-        with self.assertRaises(Exception):
-            obj.getC("NonExistentAttribute")
-        with self.assertRaises(Exception):
-            obj.getF("NonExistentAttribute")
-        with self.assertRaises(Exception):
-            obj.getD("NonExistentAttribute")
-        with self.assertRaises(Exception):
-            obj.getI("NonExistentAttribute")
-        with self.assertRaises(Exception):
-            obj.getL("NonExistentAttribute")
-
     def test_clear_and_test(self):
         """Test Object.clear and Object.test"""
         obj = astshim.ZoomMap(2, 1.3)
@@ -79,6 +64,36 @@ def test_copy_and_same(self):
         self.assertEqual(obj.getNobject(), initialNumObj)
         self.assertEqual(obj.getRefCount(), 1)
 
+    def test_error_handling(self):
+        """Test handling of AST errors
+        """
+        # To introduce an AST error construct a PolyMap with no inverse mapping
+        # and then try to transform in the inverse direction.
+        coeff_f = np.array([
+            [1.2, 1, 2, 0],
+            [-0.5, 1, 1, 1],
+            [1.0, 2, 0, 1],
+        ])
+        pm = astshim.PolyMap(coeff_f, 2, "IterInverse=0")
+        pin = np.array([
+            [1.0, 0.0],
+            [2.0, 1.0],
+            [3.0, 2.0],
+        ])
+
+        # make sure the error string contains "Error"
+        try:
+            pm.tranInverse(pin)
+        except RuntimeError as e:
+            self.assertEqual(e.args[0].count("Error"), 1)
+            print(e)
+
+        # cause another error and make sure the first error message was purged
+        try:
+            pm.tranInverse(pin)
+        except RuntimeError as e:
+            self.assertEqual(e.args[0].count("Error"), 1)
+
     def test_id(self):
         """Test that ID is *not* transferred to copies"""
         obj = astshim.ZoomMap(2, 1.3)