Include file path and line number in parsing errors (#512)

* Simple line number printout for element Signed-off-by: Aaron Chong <[email protected]> * Passing source into readXml, errors in include tags Signed-off-by: Aaron Chong <[email protected]> * Added helper function for creating trace strings Signed-off-by: Aaron Chong <[email protected]> * Fixing bad calls to elements in includes Signed-off-by: Aaron Chong <[email protected]> * Fixed bad logic in file exists check Signed-off-by: Aaron Chong <[email protected]> * Pimpl-ized Error, added API for file path and line number Signed-off-by: Aaron Chong <[email protected]> * Started using new Error API, removed previous helper functions Signed-off-by: Aaron Chong <[email protected]> * Modified error stream to look like before, using << operator for cmdCheck Signed-off-by: Aaron Chong <[email protected]> * Added tests to check for file tracing and line number Signed-off-by: Aaron Chong <[email protected]> * Added Error constructor without LineNumber for missing elements Signed-off-by: Aaron Chong <[email protected]> * Added tests for parsing errors includes, test sdfs and models Signed-off-by: Aaron Chong <[email protected]> * code_check fixes Signed-off-by: Aaron Chong <[email protected]> * Removed accidental + char Signed-off-by: Aaron Chong <[email protected]> * Removed accidental + char Signed-off-by: Aaron Chong <[email protected]> * Remove file and line number reporting in warnings Signed-off-by: Aaron Chong <[email protected]> * Removed checking for front Error: tag as ign.cc uses << now Signed-off-by: Aaron Chong <[email protected]> * Removed #ifdefs Signed-off-by: Aaron Chong <[email protected]> * Using std optional for getters Signed-off-by: Aaron Chong <[email protected]> * Added line number to Error for missing required attribute Signed-off-by: Aaron Chong <[email protected]> * Removed unused declaration Signed-off-by: Aaron Chong <[email protected]> * enforceConfigurablePolicyCondition accepts Error now, warn and log grabs the filename, line number and message from the Error object Signed-off-by: Aaron Chong <[email protected]>
gazebosim · Mar 19, 2021 · 2031028 · 2031028
1 parent 936a2e3
commit 2031028
Show file tree

Hide file tree

Showing 17 changed files with 549 additions and 137 deletions.
diff --git a/include/sdf/Error.hh b/include/sdf/Error.hh
@@ -19,6 +19,8 @@
 
 #include <iostream>
 #include <string>
+#include <optional>
+#include <ignition/utils/ImplPtr.hh>
 #include <sdf/sdf_config.h>
 #include "sdf/system_util.hh"
 
@@ -145,15 +147,33 @@ namespace sdf
 
   class SDFORMAT_VISIBLE Error
   {
-    /// \brief default constructor
-    public: Error() = default;
+    /// \brief Default constructor
+    public: Error();
 
     /// \brief Constructor.
     /// \param[in] _code The error code.
     /// \param[in] _message A description of the error.
     /// \sa ErrorCode.
     public: Error(const ErrorCode _code, const std::string &_message);
 
+    /// \brief Constructor.
+    /// \param[in] _code The error code.
+    /// \param[in] _message A description of the error.
+    /// \param[in] _filePath The file path that is related to this error.
+    /// \sa ErrorCode.
+    public: Error(const ErrorCode _code, const std::string &_message,
+                  const std::string &_filePath);
+
+    /// \brief Constructor.
+    /// \param[in] _code The error code.
+    /// \param[in] _message A description of the error.
+    /// \param[in] _filePath The file path that is related to this error.
+    /// \param[in] _lineNumber The line number in the provided file path where
+    /// this error was raised.
+    /// \sa ErrorCode.
+    public: Error(const ErrorCode _code, const std::string &_message,
+                  const std::string &_filePath, int _lineNumber);
+
     /// \brief Get the error code.
     /// \return An error code.
     /// \sa ErrorCode.
@@ -163,6 +183,15 @@ namespace sdf
     /// \return Error message.
     public: std::string Message() const;
 
+    /// \brief Get the file path associated with this error.
+    /// \return Returns the path of the file that this error is related to.
+    /// nullopt otherwise.
+    public: std::optional<std::string> FilePath() const;
+
+    /// \brief Get the line number associated with this error.
+    /// \return Returns the line number. nullopt otherwise.
+    public: std::optional<int> LineNumber() const;
+
     /// \brief Safe bool conversion.
     /// \return True if this Error's Code() != NONE. In otherwords, this is
     /// true when there is an error.
@@ -184,26 +213,35 @@ namespace sdf
     public: friend std::ostream &operator<<(std::ostream &_out,
                                             const sdf::Error &_err)
     {
-      _out << "Error Code "
-        << static_cast<std::underlying_type<sdf::ErrorCode>::type>(_err.Code())
-        << " Msg: " << _err.Message();
+      if (!_err.FilePath().has_value())
+      {
+        _out << "Error Code "
+          << static_cast<std::underlying_type<sdf::ErrorCode>::type>(
+              _err.Code())
+          << " Msg: " << _err.Message();
+      }
+      else if (!_err.LineNumber().has_value())
+      {
+        _out << "Error Code "
+          << static_cast<std::underlying_type<sdf::ErrorCode>::type>(
+              _err.Code())
+          << ": [" << _err.FilePath().value() << "]: "
+          << " Msg: " << _err.Message();
+      }
+      else
+      {
+        _out << "Error Code "
+          << static_cast<std::underlying_type<sdf::ErrorCode>::type>(
+              _err.Code())
+          << ": [" << _err.FilePath().value() << ":L"
+          << std::to_string(_err.LineNumber().value()) << "]: "
+          << " Msg: " << _err.Message();
+      }
       return _out;
     }
 
-    /// \brief The error code value.
-    private: ErrorCode code = ErrorCode::NONE;
-
-#ifdef _WIN32
-  // Disable warning C4251 which is triggered by
-  // std::string
-  #pragma warning(push)
-  #pragma warning(disable: 4251)
-#endif
-    /// \brief Description of the error.
-    private: std::string message = "";
-#ifdef _WIN32
-  #pragma warning(pop)
-#endif
+    /// \brief Private data pointer.
+    IGN_UTILS_IMPL_PTR(dataPtr)
   };
   }
 }

diff --git a/src/Error.cc b/src/Error.cc
@@ -19,35 +19,89 @@
 
 using namespace sdf;
 
+class sdf::Error::Implementation
+{
+  /// \brief The error code value.
+  public: ErrorCode code = ErrorCode::NONE;
+
+  /// \brief Description of the error.
+  public: std::string message = "";
+
+  /// \brief File path where the error was raised.
+  public: std::optional<std::string> filePath = std::nullopt;
+
+  /// \brief Line number in the file path where the error was raised.
+  public: std::optional<int> lineNumber = std::nullopt;
+};
+
+/////////////////////////////////////////////////
+Error::Error()
+  : dataPtr(ignition::utils::MakeImpl<Implementation>())
+{}
+
 /////////////////////////////////////////////////
 Error::Error(const ErrorCode _code, const std::string &_message)
+  : dataPtr(ignition::utils::MakeImpl<Implementation>())
+{
+  this->dataPtr->code = _code;
+  this->dataPtr->message = _message;
+}
+
+/////////////////////////////////////////////////
+Error::Error(const ErrorCode _code, const std::string &_message,
+             const std::string &_filePath)
+  : dataPtr(ignition::utils::MakeImpl<Implementation>())
 {
-  this->code = _code;
-  this->message = _message;
+  this->dataPtr->code = _code;
+  this->dataPtr->message = _message;
+  this->dataPtr->filePath = _filePath;
+}
+
+/////////////////////////////////////////////////
+Error::Error(const ErrorCode _code, const std::string &_message,
+             const std::string &_filePath, int _lineNumber)
+  : dataPtr(ignition::utils::MakeImpl<Implementation>())
+{
+  this->dataPtr->code = _code;
+  this->dataPtr->message = _message;
+  this->dataPtr->filePath = _filePath;
+  this->dataPtr->lineNumber = _lineNumber;
 }
 
 /////////////////////////////////////////////////
 ErrorCode Error::Code() const
 {
-  return this->code;
+  return this->dataPtr->code;
 }
 
 /////////////////////////////////////////////////
 std::string Error::Message() const
 {
-  return this->message;
+  return this->dataPtr->message;
+}
+
+/////////////////////////////////////////////////
+std::optional<std::string> Error::FilePath() const
+{
+  return this->dataPtr->filePath;
+}
+
+/////////////////////////////////////////////////
+std::optional<int> Error::LineNumber() const
+{
+  return this->dataPtr->lineNumber;
 }
 
 /////////////////////////////////////////////////
 // cppcheck-suppress unusedFunction
 Error::operator bool() const
 {
-  return this->code != ErrorCode::NONE;
+  return this->dataPtr->code != ErrorCode::NONE;
 }
 
 /////////////////////////////////////////////////
 bool Error::operator==(const bool _value) const
 {
-  return ((this->code != ErrorCode::NONE) && _value) ||
-         ((this->code == ErrorCode::NONE) && !_value);
+  return ((this->dataPtr->code != ErrorCode::NONE) && _value) ||
+         ((this->dataPtr->code == ErrorCode::NONE) && !_value);
 }
diff --git a/src/Error_TEST.cc b/src/Error_TEST.cc
@@ -16,6 +16,7 @@
 */
 
 #include <gtest/gtest.h>
+#include <optional>
 #include "sdf/sdf_config.h"
 #include "sdf/Error.hh"
 
@@ -26,18 +27,40 @@ TEST(Error, DefaultConstruction)
   EXPECT_EQ(error, false);
   EXPECT_EQ(error.Code(), sdf::ErrorCode::NONE);
   EXPECT_TRUE(error.Message().empty());
+  EXPECT_FALSE(error.FilePath().has_value());
+  EXPECT_FALSE(error.LineNumber().has_value());
 
   if (error)
     FAIL();
 }
 
 /////////////////////////////////////////////////
-TEST(Error, ValueConstruction)
+TEST(Error, ValueConstructionWithoutFile)
 {
   sdf::Error error(sdf::ErrorCode::FILE_READ, "Unable to read a file");
   EXPECT_EQ(error, true);
   EXPECT_EQ(error.Code(), sdf::ErrorCode::FILE_READ);
   EXPECT_EQ(error.Message(), "Unable to read a file");
+  EXPECT_FALSE(error.FilePath().has_value());
+  EXPECT_FALSE(error.LineNumber().has_value());
+
+  if (!error)
+    FAIL();
+}
+
+/////////////////////////////////////////////////
+TEST(Error, ValueConstructionWithFile)
+{
+  const std::string emptyFilePath = "Empty/file/path";
+  sdf::Error error(sdf::ErrorCode::FILE_READ, "Unable to read a file",
+                   emptyFilePath, 10);
+  EXPECT_EQ(error, true);
+  EXPECT_EQ(error.Code(), sdf::ErrorCode::FILE_READ);
+  EXPECT_EQ(error.Message(), "Unable to read a file");
+  EXPECT_TRUE(error.FilePath().has_value());
+  EXPECT_EQ(error.FilePath().value(), emptyFilePath);
+  EXPECT_TRUE(error.LineNumber().has_value());
+  EXPECT_EQ(error.LineNumber().value(), 10);
 
   if (!error)
     FAIL();

diff --git a/src/Utils.cc b/src/Utils.cc
@@ -92,20 +92,35 @@ bool isValidFrameReference(const std::string &_name)
 
 void enforceConfigurablePolicyCondition(
   const sdf::EnforcementPolicy _policy,
-  const std::string &_message,
-  const ErrorCode _error,
+  const sdf::Error &_error,
   sdf::Errors &_errors)
 {
   switch (_policy)
   {
     case EnforcementPolicy::ERR:
-      _errors.push_back({_error, _message});
+      _errors.push_back(_error);
       break;
     case EnforcementPolicy::WARN:
-      sdfwarn << _message;
+      if (!_error.FilePath().has_value())
+        sdfwarn << _error.Message();
+      else if (!_error.LineNumber().has_value())
+        sdfwarn << "[" << _error.FilePath().value() << "]: "
+            << _error.Message();
+      else
+        sdfwarn << "[" << _error.FilePath().value() << ":L"
+            << _error.LineNumber().value() << "]: "
+            << _error.Message();
       break;
     case EnforcementPolicy::LOG:
-      sdfdbg << _message;
+      if (!_error.FilePath().has_value())
+        sdfdbg << _error.Message();
+      else if (!_error.LineNumber().has_value())
+        sdfdbg << "[" << _error.FilePath().value() << "]: "
+            << _error.Message();
+      else
+        sdfdbg << "[" << _error.FilePath().value() << ":L"
+            << _error.LineNumber().value() << "]: "
+            << _error.Message();
       break;
     default:
       throw std::runtime_error("Unhandled warning policy enum value");

diff --git a/src/Utils.hh b/src/Utils.hh
@@ -75,13 +75,11 @@ namespace sdf
   /// Based on the policy, this will either add it to an errors vector, stream
   /// it to sdfwarn, or sdfdbg.
   /// \param[in] _policy The enforcement policy to follow
-  /// \param[in] _message The message to add for this warning
-  /// \param[in] _error An error code to use if the policy is ERR
+  /// \param[in] _error An error object to use if the policy is ERR
   /// \param[out] _errors The errors to append to if the policy is ERR
   void enforceConfigurablePolicyCondition(
     const sdf::EnforcementPolicy _policy,
-    const std::string &_message,
-    const ErrorCode _error,
+    const sdf::Error &_error,
     sdf::Errors &_errors);
 
   /// \brief Load all objects of a specific sdf element type. No error

diff --git a/src/Utils_TEST.cc b/src/Utils_TEST.cc
@@ -122,3 +122,27 @@ TEST(DOMUtils, ReservedNames)
   EXPECT_TRUE(sdf::isReservedName("__world__"));
   EXPECT_TRUE(sdf::isReservedName("__anything__"));
 }
+
+/////////////////////////////////////////////////
+TEST(PolicyUtils, EnforcementPolicyErrors)
+{
+  sdf::Errors errors;
+  const std::string emptyFilePath = "Empty/file/path";
+  sdf::Error error(sdf::ErrorCode::FILE_READ, "Unable to read a file",
+                   emptyFilePath, 10);
+  ASSERT_EQ(error, true);
+  ASSERT_TRUE(errors.empty());
+
+  sdf::enforceConfigurablePolicyCondition(
+      sdf::EnforcementPolicy::ERR,
+      error,
+      errors);
+  ASSERT_FALSE(errors.empty());
+  EXPECT_EQ(errors[0], true);
+  EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::FILE_READ);
+  EXPECT_EQ(errors[0].Message(), "Unable to read a file");
+  EXPECT_TRUE(errors[0].FilePath().has_value());
+  EXPECT_EQ(errors[0].FilePath().value(), emptyFilePath);
+  EXPECT_TRUE(errors[0].LineNumber().has_value());
+  EXPECT_EQ(errors[0].LineNumber().value(), 10);
+}
diff --git a/src/ign.cc b/src/ign.cc
@@ -41,7 +41,7 @@ extern "C" SDFORMAT_VISIBLE int cmdCheck(const char *_path)
   {
     for (auto &error : errors)
     {
-      std::cerr << "Error: " << error.Message() << std::endl;
+      std::cerr << error << std::endl;
     }
     return -1;
   }