Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lib/errorlogger.cpp: Improve conversion from InternalError to ErrorMessage #5804

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

lib/errorlogger.cpp: Improve conversion from InternalError to ErrorMessage #5804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/cppcheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 + "'");
Copy link
Owner

@danmar danmar Dec 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so we should have written "line" instead of "result" here. I understand the output was cryptic!

Could we fix the bug here and replace the result with line. I am not against that we add a verbose output that contains all results but it would be good to point out the bad line..

Something like:

short message:

Failed to execute '...'. Invalid line: abcd

verbose message:

Failed to execute '...'. Expected '{' but got 'a' as first character. Invalid line: abcd
Results:
 ....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to not include all of the result but just the offending line. However in line with the InternalError logic I would suggest the following:

The throwing:

throw InternalError(nullptr,
   "Unexpected output from '" + pythonExe + " " + args + "'. Invalid line '"+line+"'",
   "Expected '{' but got '" + line[0] + "' as first character."
);

(I changed "failed to execute" because it actually did execute - we just don't like what it produced.)

The resulting short message:

Unexpected output from '...'. Invalid line 'abcd'

The resulting verbose message:

Unexpected output from '...'. Invalid line 'abcd': Expected '{' but got 'a' as first character.

Still then, the question is what to do with InternalError messages containing \n? Or can we be confident that they never occur again..?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still then, the question is what to do with InternalError messages containing \n? Or can we be confident that they never occur again..?

maybe your approach is fine.. however I do feel that there should really not be newlines.. ideally I would like to see some assertion error or internal error message that just complains that there is a newline..

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I changed "failed to execute" because it actually did execute - we just don't like what it produced.)

I like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still then, the question is what to do with InternalError messages containing \n? Or can we be confident that they never occur again..?

maybe your approach is fine.. however I do feel that there should really not be newlines.. ideally I would like to see some assertion error or internal error message that just complains that there is a newline..

I kind of agree, but imagine getting "an internal error occurred processing an internal error" - technically very accurate, but helpful... not really. Not for a new user, not for the bug report they will submit. Compile-time options seem very limited, short from enforce the short message of InternalError to be a string literal, and putting all dynamic/formatted/concatenated stuff in the details/verbose field. Can we simply take the first N characters while replacing \n with spaces?

Copy link
Owner

@danmar danmar Dec 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if a developer is debugging cppcheck I think a assertion error that terminates would be good.. but:

  1. we don't currently have assertions that only terminates during debugging.. I guess we would need to update the build scripts to set the NDEBUG macro or something..
  2. what if the user runs into the problem instead. we need a solution that is acceptable for the user.

so well I think ignore the assertion for now.. feel free to look into that later..

Can we simply take the first N characters while replacing \n with spaces?

sure

}

//std::cout << "addon '" << addonInfo.name << "' result is " << line << std::endl;
Expand Down
9 changes: 8 additions & 1 deletion lib/errorlogger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
26 changes: 26 additions & 0 deletions test/testerrorlogger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading