cppcheck

cppcheck copied to clipboard

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; 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 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.

EDIT: Removed, I misunderstood what the changes could do..

I am confused why CI accepted this.

hmm.. somehow it would be nice with a test for this.. I would assume a unit test can test ErrorMessage::fromInternalError directly and check the result..

I am confused why CI accepted this.

hmm.. somehow it would be nice with a test for this.. I would assume a unit test can test ErrorMessage::fromInternalError directly and check the result..

The unit test does not cover multi-line scenarios. The offending line (part of this commit) throws InternalError(nullptr, "Failed to execute ..." + result); where result is not trusted, and may contain newlines.

I added a test case InternalError internalError(nullptr, "message\nmultiline1\nmultiline2"); under ErrorMessageFromInternalError() with the following output:

test/testerrorlogger.cpp:193(TestErrorLogger::ErrorMessageFromInternalError): Assertion failed. 
Expected: 
msg: message: details

Actual: 
multiline1\n
multiline2

Here the string "message" is indeed lost.

I think there are two problems to fix. First, the error should not contain a newline in the first place, so I could push:

throw InternalError(nullptr, "Failed to execute '" + pythonExe + " " + args + "'. Expected '{' but got '" + line[0] + "' as first character.",
   "Output was: '" + result + "'");

This would make sense, as the output of the command is the detailed context of the reported issue.

Second, if the error does contain a newline, the first line should not get lost. This is already addressed by the current commit.

The following test cases succeed with the patch as it is:

        {
            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, "message1\nmessage2","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: message1", msg.shortMessage());
            ASSERT_EQUALS("msg: message1\nmessage2: details", msg.verboseMessage());
            ASSERT_EQUALS("[file.cpp:0]: (error) msg: message1", msg.toString(false));
            ASSERT_EQUALS("[file.cpp:0]: (error) msg: message1\nmessage2: details", msg.toString(true));
        }

Is this acceptable?

There may be a few more places where newlines may accidentally end up in the message, e.g. Tokenizer::syntaxError().

I think at least the assertions in the first {} is OK.

In the second {} it is a bit more questionable.. but as you say it's more likely that it's unintended bugs in cppcheck. I have the feeling that such newlines are a mistake rather than intentional.. I wonder if we can even assert that the message does not contain a newline.

In the second {} it is a bit more questionable.. but as you say it's more likely that it's unintended bugs in cppcheck. I have the feeling that such newlines are a mistake rather than intentional.. I wonder if we can even assert that the message does not contain a newline.

A lot of InternalError construction is done using concatenated strings, so anything can happen. One option is to replace \n with spaces in the constructor, but that does not necessarily make things more understandable in practice.

My thinking is: newlines should not occur there, but if they do, the error reporting code should not make matters more confusing than they already are, for someone stumbling on an InternalError. The second {} asserts that at least the first line will always be visible, which is the line that contains the explanation for the internal error. My experience making namingng.py suitable for --cli would have been better if it had worked this way.