Skip to content

Fix coloring of info messages in isoltest #14580

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

Merged
merged 4 commits into from
Oct 19, 2023
Merged

Fix coloring of info messages in isoltest #14580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0e51e57
Add Error::parseErrorType()
cameel Sep 29, 2023
2e8e1f3
Store Error::Type rather than a string in SyntaxTestError
cameel Sep 29, 2023
d4f5503
CommonSyntaxTest: Fix info messages being colored as errors
cameel Sep 28, 2023
0da44ca
Reuse color definitions between SourceReferenceFormatter and CommonSy…
cameel Sep 29, 2023
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
20 changes: 20 additions & 0 deletions liblangutil/Exceptions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,26 @@
using namespace solidity;
using namespace solidity::langutil;

std::map<Error::Type, std::string> const Error::m_errorTypeNames = {
{Error::Type::IOError, "IOError"},
{Error::Type::FatalError, "FatalError"},
{Error::Type::JSONError, "JSONError"},
{Error::Type::InternalCompilerError, "InternalCompilerError"},
{Error::Type::CompilerError, "CompilerError"},
{Error::Type::Exception, "Exception"},
{Error::Type::CodeGenerationError, "CodeGenerationError"},
{Error::Type::DeclarationError, "DeclarationError"},
{Error::Type::DocstringParsingError, "DocstringParsingError"},
{Error::Type::ParserError, "ParserError"},
{Error::Type::SyntaxError, "SyntaxError"},
{Error::Type::TypeError, "TypeError"},
{Error::Type::UnimplementedFeatureError, "UnimplementedFeatureError"},
{Error::Type::YulException, "YulException"},
{Error::Type::SMTLogicException, "SMTLogicException"},
{Error::Type::Warning, "Warning"},
{Error::Type::Info, "Info"},
};

Error::Error(
ErrorId _errorId, Error::Type _type,
std::string const& _description,
Expand Down
38 changes: 15 additions & 23 deletions liblangutil/Exceptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@
#include <boost/preprocessor/facilities/overload.hpp>
#include <boost/algorithm/string/case_conv.hpp>

#include <optional>
#include <string>
#include <utility>
#include <vector>
#include <memory>
#include <variant>
#include <vector>

namespace solidity::langutil
{
Expand Down Expand Up @@ -269,27 +269,17 @@ class Error: virtual public util::Exception

static std::string formatErrorType(Type _type)
{
switch (_type)
{
case Type::IOError: return "IOError";
case Type::FatalError: return "FatalError";
case Type::JSONError: return "JSONError";
case Type::InternalCompilerError: return "InternalCompilerError";
case Type::CompilerError: return "CompilerError";
case Type::Exception: return "Exception";
case Type::CodeGenerationError: return "CodeGenerationError";
case Type::DeclarationError: return "DeclarationError";
case Type::DocstringParsingError: return "DocstringParsingError";
case Type::ParserError: return "ParserError";
case Type::SyntaxError: return "SyntaxError";
case Type::TypeError: return "TypeError";
case Type::UnimplementedFeatureError: return "UnimplementedFeatureError";
case Type::YulException: return "YulException";
case Type::SMTLogicException: return "SMTLogicException";
case Type::Warning: return "Warning";
case Type::Info: return "Info";
}
util::unreachable();
return m_errorTypeNames.at(_type);
}

static std::optional<Type> parseErrorType(std::string _name)
{
static std::map<std::string, Error::Type> const m_errorTypesByName = util::invertMap(m_errorTypeNames);

if (m_errorTypesByName.count(_name) == 0)
return std::nullopt;

return m_errorTypesByName.at(_name);
}

static std::string formatTypeOrSeverity(std::variant<Error::Type, Error::Severity> _typeOrSeverity)
Expand All @@ -309,6 +299,8 @@ class Error: virtual public util::Exception
private:
ErrorId m_errorId;
Type m_type;

static std::map<Type, std::string> const m_errorTypeNames;
};

}
35 changes: 23 additions & 12 deletions liblangutil/SourceReferenceFormatter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,28 @@ std::string SourceReferenceFormatter::formatErrorInformation(Error const& _error
);
}

char const* SourceReferenceFormatter::errorTextColor(Error::Severity _severity)
{
switch (_severity)
{
case Error::Severity::Error: return RED;
case Error::Severity::Warning: return YELLOW;
case Error::Severity::Info: return WHITE;
}
util::unreachable();
}

char const* SourceReferenceFormatter::errorHighlightColor(Error::Severity _severity)
{
switch (_severity)
{
case Error::Severity::Error: return RED_BACKGROUND;
case Error::Severity::Warning: return ORANGE_BACKGROUND_256;
case Error::Severity::Info: return GRAY_BACKGROUND;
Copy link
Member

Choose a reason for hiding this comment

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

LGTM. Just one question, wouldn't this allows to display the text in white over gray background?

Copy link
Member

Choose a reason for hiding this comment

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

Wait a second... we do have colored backgrounds for these :-D? That's weird - I mean, that's generally horrible, isn't it? But also I've never seen that in any output myself, even though the escape actually works in my terminal...

Copy link
Member Author

Choose a reason for hiding this comment

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

LGTM. Just one question, wouldn't this allows to display the text in white over gray background?

White text actually looks fine on this background. That's why I chose this color.

Wait a second... we do have colored backgrounds for these :-D? That's weird - I mean, that's generally horrible, isn't it? But also I've never seen that in any output myself, even though the escape actually works in my terminal...

We use it in tests for highlighting parts of the source matching a particular error/warning/info. I actually find it pretty convenient - having to decode character offsets from source locations in your head sounds more horrible :) For example you can see it in DebugWarner output in new analysis when a test does not pass. It's not used for compilation errors by the compiler.

Since it's not used outside of tests, I could put it somewhere in test utils, but putting it together with foreground colors seemed like a better choice to me. If I ever needed it and were ever looking for it, I'd probably look here (or in AnsiColorized.h, but it did not seem right to make that file depend on the Error definition).

Copy link
Member

@r0qs r0qs Oct 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Right. Just mentioned because I remembered this issue: #14378. So I guess this PR would also fix that, right? At least in some tests I did here using a white background, everything seems to work fine.

Copy link
Member

@ekpyron ekpyron Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I actually missed the "in isoltest" part of this PR (not sure how :-)) and thought it was about regular compiler output, that's why I was confused :-).
So just to confirm, this is not meant to change colored solc output, but just to affect isoltest? (I could probably also read the code for an answer :-))

Copy link
Member

Choose a reason for hiding this comment

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

It uses errorColored which in turn is used by printExceptionInformation and by the CommandLineInterface to print compiler errors in the CLI. This was why I initially thought it could be solving the issue that I mentioned above (i.e. not only affecting isoltests), but it seems that the code doesn't really change anything in the errorColored logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

So just to confirm, this is not meant to change colored solc output, but just to affect isoltest? (I could probably also read the code for an answer :-))

Exactly. There are no behavior changes outside of tests. The non-test code is only refactored.

So I guess this PR would also fix that, right?

No, at least not in a significant way. The issue is not limited to test output.

BTW, not sure what the "fix" would be there. I'd assume that the terminal that changes colors of various things would do that by remapping the meaning of these colors to something else. Or are there some "themable"/relative color codes that we should be using to play nice with such terminals?

Copy link
Member

@r0qs r0qs Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

BTW, not sure what the "fix" would be there. I'd assume that the terminal that changes colors of various things would do that by remapping the meaning of these colors to something else. Or are there some "themable"/relative color codes that we should be using to play nice with such terminals?

I'm not sure either. I know that on GNU/Linux you could use tput to query for terminal-dependent information, but I don't know if a solution based on that, or on ncurses, would be reliable and cross-platform. On GNU/Linux you can do tput colors to get the maximum number of colors supported by the terminal according with the terminfo database, but it may not really reflect the current terminal capabilities. And I'm not sure you can query for specific colors either. But if there is a way to reliably detect the background color and set the default as the opposite color, then we would have a fix for it.

}
util::unreachable();
}

AnsiColorized SourceReferenceFormatter::normalColored() const
{
return AnsiColorized(m_stream, m_colored, {WHITE});
Expand All @@ -67,18 +89,7 @@ AnsiColorized SourceReferenceFormatter::frameColored() const

AnsiColorized SourceReferenceFormatter::errorColored(std::ostream& _stream, bool _colored, Error::Severity _severity)
{
// We used to color messages of any severity as errors so this seems like a good default
// for cases where severity cannot be determined.
char const* textColor = RED;

switch (_severity)
{
case Error::Severity::Error: textColor = RED; break;
case Error::Severity::Warning: textColor = YELLOW; break;
case Error::Severity::Info: textColor = WHITE; break;
}

return AnsiColorized(_stream, _colored, {BOLD, textColor});
return AnsiColorized(_stream, _colored, {BOLD, errorTextColor(_severity)});
}

AnsiColorized SourceReferenceFormatter::messageColored(std::ostream& _stream, bool _colored)
Expand Down
10 changes: 10 additions & 0 deletions liblangutil/SourceReferenceFormatter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,16 @@ class SourceReferenceFormatter
bool _withErrorIds = false
);

/// The default text color for printing error messages of a given severity in the terminal.
/// Assumes a dark background color.
static char const* errorTextColor(Error::Severity _severity);

/// The default background color for highlighting source fragments corresponding to an error
/// of a given severity in the terminal. Assumes a light text color.
/// @note This is *not* meant to be used for the same text in combination with @a errorTextColor().
/// It's an alternative way to highlight it, while preserving the original text color.
static char const* errorHighlightColor(Error::Severity _severity);

private:
util::AnsiColorized normalColored() const;
util::AnsiColorized frameColored() const;
Expand Down
1 change: 1 addition & 0 deletions libsolutil/AnsiColorized.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ static constexpr char const* BLUE_BACKGROUND = "\033[44m";
static constexpr char const* MAGENTA_BACKGROUND = "\033[45m";
static constexpr char const* CYAN_BACKGROUND = "\033[46m";
static constexpr char const* WHITE_BACKGROUND = "\033[47m";
static constexpr char const* GRAY_BACKGROUND = "\033[100m";

// 256-bit-colors (incomplete set)
static constexpr char const* RED_BACKGROUND_256 = "\033[48;5;160m";
Expand Down
40 changes: 28 additions & 12 deletions test/CommonSyntaxTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,17 @@
#include <test/CommonSyntaxTest.h>
#include <test/Common.h>
#include <test/TestCase.h>

#include <liblangutil/SourceReferenceFormatter.h>

#include <libsolutil/CommonIO.h>
#include <libsolutil/StringUtils.h>

#include <boost/algorithm/string.hpp>
#include <boost/algorithm/string/predicate.hpp>
#include <boost/test/unit_test.hpp>
#include <boost/throw_exception.hpp>

#include <fstream>
#include <memory>
#include <stdexcept>
Expand Down Expand Up @@ -115,15 +120,18 @@ void CommonSyntaxTest::printSource(ostream& _stream, string const& _linePrefix,
{
assert(static_cast<size_t>(error.locationStart) <= source.length());
assert(static_cast<size_t>(error.locationEnd) <= source.length());
bool isWarning = error.type == "Warning";
for (int i = error.locationStart; i < error.locationEnd; i++)
if (isWarning)
{
if (sourceFormatting[static_cast<size_t>(i)] == util::formatting::RESET)
sourceFormatting[static_cast<size_t>(i)] = util::formatting::ORANGE_BACKGROUND_256;
}
else
sourceFormatting[static_cast<size_t>(i)] = util::formatting::RED_BACKGROUND;
{
char const*& cellFormat = sourceFormatting[static_cast<size_t>(i)];
char const* infoBgColor = SourceReferenceFormatter::errorHighlightColor(Error::Severity::Info);

if (
(error.type != Error::Type::Warning && error.type != Error::Type::Info) ||
(error.type == Error::Type::Warning && (cellFormat == RESET || cellFormat == infoBgColor)) ||
(error.type == Error::Type::Info && cellFormat == RESET)
)
cellFormat = SourceReferenceFormatter::errorHighlightColor(Error::errorSeverity(error.type));
}
}

_stream << _linePrefix << sourceFormatting.front() << source.front();
Expand Down Expand Up @@ -190,8 +198,12 @@ void CommonSyntaxTest::printErrorList(
for (auto const& error: _errorList)
{
{
util::AnsiColorized scope(_stream, _formatted, {BOLD, (error.type == "Warning") ? YELLOW : RED});
_stream << _linePrefix << error.type;
util::AnsiColorized scope(
_stream,
_formatted,
{BOLD, SourceReferenceFormatter::errorTextColor(Error::errorSeverity(error.type))}
);
_stream << _linePrefix << Error::formatErrorType(error.type);
if (error.errorId.has_value())
_stream << ' ' << error.errorId->error;
_stream << ": ";
Expand Down Expand Up @@ -244,7 +256,11 @@ vector<SyntaxTestError> CommonSyntaxTest::parseExpectations(istream& _stream)
auto typeBegin = it;
while (it != line.end() && isalpha(*it, locale::classic()))
++it;
string errorType(typeBegin, it);

string errorTypeStr(typeBegin, it);
optional<Error::Type> errorType = Error::parseErrorType(errorTypeStr);
if (!errorType.has_value())
BOOST_THROW_EXCEPTION(runtime_error("Invalid error type: " + errorTypeStr));

skipWhitespace(it, line.end());

Expand Down Expand Up @@ -281,7 +297,7 @@ vector<SyntaxTestError> CommonSyntaxTest::parseExpectations(istream& _stream)

string errorMessage(it, line.end());
expectations.emplace_back(SyntaxTestError{
std::move(errorType),
errorType.value(),
std::move(errorId),
std::move(errorMessage),
std::move(sourceName),
Expand Down
2 changes: 1 addition & 1 deletion test/CommonSyntaxTest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ namespace solidity::test

struct SyntaxTestError
{
std::string type;
langutil::Error::Type type;
std::optional<langutil::ErrorId> errorId;
std::string message;
std::string sourceName;
Expand Down
4 changes: 2 additions & 2 deletions test/libsolidity/SyntaxTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ void SyntaxTest::parseAndAnalyze()
catch (UnimplementedFeatureError const& _e)
{
m_errorList.emplace_back(SyntaxTestError{
"UnimplementedFeatureError",
Error::Type::UnimplementedFeatureError,
std::nullopt,
errorMessage(_e),
"",
Expand Down Expand Up @@ -140,7 +140,7 @@ void SyntaxTest::filterObtainedErrors()
}
}
m_errorList.emplace_back(SyntaxTestError{
Error::formatErrorType(currentError->type()),
currentError->type(),
currentError->errorId(),
errorMessage(*currentError),
sourceName,
Expand Down
2 changes: 1 addition & 1 deletion test/libyul/SyntaxTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ void SyntaxTest::parseAndAnalyze()
}

m_errorList.emplace_back(SyntaxTestError{
Error::formatErrorType(error->type()),
error->type(),
error->errorId(),
errorMessage(*error),
name,
Expand Down