From 61c9e8549240df6d77d98ee34e68fb255b1f0d0b Mon Sep 17 00:00:00 2001
From: Maarten van der Schrieck <maarten@thingsconnected.nl>
Date: Sat, 23 Dec 2023 15:44:51 +0100
Subject: [PATCH] lib/errorlogger.cpp: Improve conversion from InternalError to
 ErrorMessage

ErrorMessage::setmsg() assumes that the supplied message string is a
combination of a short message and a verbose message, separated by a newline.

However, InternalError does not use that format, and the errorMessage of
InternalError may contain an unintentional newline. Then, when converting from
InternalError to ErrorMessage, the short message will receive the first line,
and the verbose message will receive the lines that follow.

This approach may lead to obscure messages during verbose logging since the
informative first, often explaining the reason for the InternalError line is
lost.

This is solved by setting the verbose message to the complete original error
string.

A concrete example is the failure of an addon to produce output starting with
'{', which is reported as an InternalError. The clarity of this message is
also improved with this patch, as the "Failure to execute" criterion was not
clear from the message. Also, the failing output in this case is moved to the
details field of InternalError.

Unit tests were added for multi-line InternalError conversions.
---
 lib/cppcheck.cpp         |  2 +-
 lib/errorlogger.cpp      |  9 ++++++++-
 test/testerrorlogger.cpp | 26 ++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/lib/cppcheck.cpp b/lib/cppcheck.cpp
index 9f912697db8..74beef61e13 100644
--- a/lib/cppcheck.cpp
+++ b/lib/cppcheck.cpp
@@ -310,7 +310,7 @@ static std::vector<picojson::value> executeAddon(const AddonInfo &addonInfo,
             //std::cout << "addon '" << addonInfo.name <<  "' result is not a JSON" << std::endl;
 
             result.erase(result.find_last_not_of('\n') + 1, std::string::npos); // Remove trailing newlines
-            throw InternalError(nullptr, "Failed to execute '" + pythonExe + " " + args + "'. " + result);
+            throw InternalError(nullptr, "Failed to execute '" + pythonExe + " " + args + "'. Expected '{' but got '" + line[0] + "' as first character.", "Output was: '" + result + "'");
         }
 
         //std::cout << "addon '" << addonInfo.name <<  "' result is " << line << std::endl;
diff --git a/lib/errorlogger.cpp b/lib/errorlogger.cpp
index fbb635c691d..112f61d5a97 100644
--- a/lib/errorlogger.cpp
+++ b/lib/errorlogger.cpp
@@ -252,12 +252,19 @@ ErrorMessage ErrorMessage::fromInternalError(const InternalError &internalError,
             locationList.push_back(std::move(loc));
         }
     }
+    std::string prefixedMessage = (msg.empty() ? "" : (msg + ": ")) + internalError.errorMessage;
     ErrorMessage errmsg(std::move(locationList),
                         tokenList ? tokenList->getSourceFilePath() : filename,
                         Severity::error,
-                        (msg.empty() ? "" : (msg + ": ")) + internalError.errorMessage,
+                        prefixedMessage,
                         internalError.id,
                         Certainty::normal);
+
+    // ErrorMessage assumes a message looking like "short message\nverbose message".
+    // The internal error may not reflect that format, leading to obscure messages if verbose
+    // logging is requested: the first (often most informative) line will simply vanish.
+    // Solve this by setting mVerboseMessage to the complete original message:
+    errmsg.mVerboseMessage = prefixedMessage;
     // TODO: find a better way
     if (!internalError.details.empty())
         errmsg.mVerboseMessage = errmsg.mVerboseMessage + ": " + internalError.details;
diff --git a/test/testerrorlogger.cpp b/test/testerrorlogger.cpp
index ddd7d61e514..627b88ba3a2 100644
--- a/test/testerrorlogger.cpp
+++ b/test/testerrorlogger.cpp
@@ -181,6 +181,32 @@ class TestErrorLogger : public TestFixture {
             ASSERT_EQUALS("[file.cpp:0]: (error) msg: message", msg.toString(false));
             ASSERT_EQUALS("[file.cpp:0]: (error) msg: message: details", msg.toString(true));
         }
+        {
+            InternalError internalError(nullptr, "message","details1\ndetails2");
+            const auto msg = ErrorMessage::fromInternalError(internalError, nullptr, "file.cpp", "msg");
+            ASSERT_EQUALS(1, msg.callStack.size());
+            const auto &loc = *msg.callStack.cbegin();
+            ASSERT_EQUALS(0, loc.fileIndex);
+            ASSERT_EQUALS(0, loc.line);
+            ASSERT_EQUALS(0, loc.column);
+            ASSERT_EQUALS("msg: message", msg.shortMessage());
+            ASSERT_EQUALS("msg: message: details1\ndetails2", msg.verboseMessage());
+            ASSERT_EQUALS("[file.cpp:0]: (error) msg: message", msg.toString(false));
+            ASSERT_EQUALS("[file.cpp:0]: (error) msg: message: details1\ndetails2", msg.toString(true));
+        }
+        {
+            InternalError internalError(nullptr, "message\nunintended newline","details");
+            const auto msg = ErrorMessage::fromInternalError(internalError, nullptr, "file.cpp", "msg");
+            ASSERT_EQUALS(1, msg.callStack.size());
+            const auto &loc = *msg.callStack.cbegin();
+            ASSERT_EQUALS(0, loc.fileIndex);
+            ASSERT_EQUALS(0, loc.line);
+            ASSERT_EQUALS(0, loc.column);
+            ASSERT_EQUALS("msg: message", msg.shortMessage());
+            ASSERT_EQUALS("msg: message\nunintended newline: details", msg.verboseMessage());
+            ASSERT_EQUALS("[file.cpp:0]: (error) msg: message", msg.toString(false));
+            ASSERT_EQUALS("[file.cpp:0]: (error) msg: message\nunintended newline: details", msg.toString(true));
+        }
     }
 
     void CustomFormat() const {