Refactor error reporting

This commit introduces ErrorReporter, a utility class which consolidates
all of the error logging functionality into a common set of functions.
It also replaces all direct interactions with an ErrorList with calls to
an ErrorReporter.

This commit resolves issue #2209

libsolidity/inlineasm/AsmAnalysis.cpp

@@ -46,7 +46,7 @@ set<string> const builtinTypes{"bool", "u8", "s8", "u32", "s32", "u64", "s64", "
 
 bool AsmAnalyzer::analyze(Block const& _block)
 {
-	if (!(ScopeFiller(m_info, m_errors))(_block))
+	if (!(ScopeFiller(m_info, m_errorReporter))(_block))
 		return false;
 
 	return (*this)(_block);
@@ -74,11 +74,10 @@ bool AsmAnalyzer::operator()(assembly::Literal const& _literal)
 	++m_stackHeight;
 	if (_literal.kind == assembly::LiteralKind::String && _literal.value.size() > 32)
 	{
-		m_errors.push_back(make_shared<Error>(
-			Error::Type::TypeError,
-			"String literal too long (" + boost::lexical_cast<std::string>(_literal.value.size()) + " > 32)",
-			_literal.location
-		));
+		m_errorReporter.typeError(
+			_literal.location,
+			"String literal too long (" + boost::lexical_cast<std::string>(_literal.value.size()) + " > 32)"
+		);
 		return false;
 	}
 	m_info.stackHeightInfo[&_literal] = m_stackHeight;
@@ -87,18 +86,17 @@ bool AsmAnalyzer::operator()(assembly::Literal const& _literal)
 
 bool AsmAnalyzer::operator()(assembly::Identifier const& _identifier)
 {
-	size_t numErrorsBefore = m_errors.size();
+	size_t numErrorsBefore = m_errorReporter.errors().size();
 	bool success = true;
 	if (m_currentScope->lookup(_identifier.name, Scope::Visitor(
 		[&](Scope::Variable const& _var)
 		{
 			if (!_var.active)
 			{
-				m_errors.push_back(make_shared<Error>(
-					Error::Type::DeclarationError,
-					"Variable " + _identifier.name + " used before it was declared.",
-					_identifier.location
-				));
+				m_errorReporter.declarationError(
+					_identifier.location,
+					"Variable " + _identifier.name + " used before it was declared."
+				);
 				success = false;
 			}
 			++m_stackHeight;
@@ -109,11 +107,10 @@ bool AsmAnalyzer::operator()(assembly::Identifier const& _identifier)
 		},
 		[&](Scope::Function const&)
 		{
-			m_errors.push_back(make_shared<Error>(
-				Error::Type::TypeError,
-				"Function " + _identifier.name + " used without being called.",
-				_identifier.location
-			));
+			m_errorReporter.typeError(
+				_identifier.location,
+				"Function " + _identifier.name + " used without being called."
+			);
 			success = false;
 		}
 	)))
@@ -127,12 +124,8 @@ bool AsmAnalyzer::operator()(assembly::Identifier const& _identifier)
 		if (stackSize == size_t(-1))
 		{
 			// Only add an error message if the callback did not do it.
-			if (numErrorsBefore == m_errors.size())
-				m_errors.push_back(make_shared<Error>(
-					Error::Type::DeclarationError,
-					"Identifier not found.",
-					_identifier.location
-				));
+			if (numErrorsBefore == m_errorReporter.errors().size())
+				m_errorReporter.declarationError(_identifier.location, "Identifier not found.");
 			success = false;
 		}
 		m_stackHeight += stackSize == size_t(-1) ? 1 : stackSize;
@@ -187,11 +180,7 @@ bool AsmAnalyzer::operator()(assembly::VariableDeclaration const& _varDecl)
 	bool success = boost::apply_visitor(*this, *_varDecl.value);
 	if ((m_stackHeight - stackHeight) != expectedItems)
 	{
-		m_errors.push_back(make_shared<Error>(
-			Error::Type::DeclarationError,
-			"Variable count mismatch.",
-			_varDecl.location
-		));
+		m_errorReporter.declarationError(_varDecl.location, "Variable count mismatch.");
 		return false;
 	}
 
@@ -233,20 +222,18 @@ bool AsmAnalyzer::operator()(assembly::FunctionCall const& _funCall)
 	if (!m_currentScope->lookup(_funCall.functionName.name, Scope::Visitor(
 		[&](Scope::Variable const&)
 		{
-			m_errors.push_back(make_shared<Error>(
-				Error::Type::TypeError,
-				"Attempt to call variable instead of function.",
-				_funCall.functionName.location
-			));
+			m_errorReporter.typeError(
+				_funCall.functionName.location,
+				"Attempt to call variable instead of function."
+			);
 			success = false;
 		},
 		[&](Scope::Label const&)
 		{
-			m_errors.push_back(make_shared<Error>(
-				Error::Type::TypeError,
-				"Attempt to call label instead of function.",
-				_funCall.functionName.location
-				));
+			m_errorReporter.typeError(
+				_funCall.functionName.location,
+				"Attempt to call label instead of function."
+			);
 			success = false;
 		},
 		[&](Scope::Function const& _fun)
@@ -257,26 +244,18 @@ bool AsmAnalyzer::operator()(assembly::FunctionCall const& _funCall)
 		}
 	)))
 	{
-		m_errors.push_back(make_shared<Error>(
-			Error::Type::DeclarationError,
-			"Function not found.",
-			_funCall.functionName.location
-		));
+		m_errorReporter.declarationError(_funCall.functionName.location, "Function not found.");
 		success = false;
 	}
 	if (success)
 	{
 		if (_funCall.arguments.size() != arguments)
 		{
-			m_errors.push_back(make_shared<Error>(
-				Error::Type::TypeError,
-				"Expected " +
-				boost::lexical_cast<string>(arguments) +
-				" arguments but got " +
-				boost::lexical_cast<string>(_funCall.arguments.size()) +
-				".",
-				_funCall.functionName.location
-				));
+			m_errorReporter.typeError(
+				_funCall.functionName.location,
+				"Expected " + boost::lexical_cast<string>(arguments) + " arguments but got " +
+				boost::lexical_cast<string>(_funCall.arguments.size()) + "."
+			);
 			success = false;
 		}
 	}
@@ -317,11 +296,10 @@ bool AsmAnalyzer::operator()(Switch const& _switch)
 			auto val = make_tuple(_case.value->kind, _case.value->value);
 			if (!cases.insert(val).second)
 			{
-				m_errors.push_back(make_shared<Error>(
-					Error::Type::DeclarationError,
-					"Duplicate case defined",
-					_case.location
-				));
+				m_errorReporter.declarationError(
+					_case.location,
+					"Duplicate case defined"
+				);
 				success = false;
 			}
 		}
@@ -354,16 +332,15 @@ bool AsmAnalyzer::operator()(Block const& _block)
 	int const stackDiff = m_stackHeight - initialStackHeight;
 	if (stackDiff != 0)
 	{
-		m_errors.push_back(make_shared<Error>(
-			Error::Type::DeclarationError,
+		m_errorReporter.declarationError(
+			_block.location,
 			"Unbalanced stack at the end of a block: " +
 			(
 				stackDiff > 0 ?
 				to_string(stackDiff) + string(" surplus item(s).") :
 				to_string(-stackDiff) + string(" missing item(s).")
-			),
-			_block.location
-		));
+			)
+		);
 		success = false;
 	}
 
@@ -375,27 +352,22 @@ bool AsmAnalyzer::operator()(Block const& _block)
 bool AsmAnalyzer::checkAssignment(assembly::Identifier const& _variable, size_t _valueSize)
 {
 	bool success = true;
-	size_t numErrorsBefore = m_errors.size();
+	size_t numErrorsBefore = m_errorReporter.errors().size();
 	size_t variableSize(-1);
 	if (Scope::Identifier const* var = m_currentScope->lookup(_variable.name))
 	{
 		// Check that it is a variable
 		if (var->type() != typeid(Scope::Variable))
 		{
-			m_errors.push_back(make_shared<Error>(
-				Error::Type::TypeError,
-				"Assignment requires variable.",
-				_variable.location
-			));
+			m_errorReporter.typeError(_variable.location, "Assignment requires variable.");
 			success = false;
 		}
 		else if (!boost::get<Scope::Variable>(*var).active)
 		{
-			m_errors.push_back(make_shared<Error>(
-				Error::Type::DeclarationError,
-				"Variable " + _variable.name + " used before it was declared.",
-				_variable.location
-			));
+			m_errorReporter.declarationError(
+				_variable.location,
+				"Variable " + _variable.name + " used before it was declared."
+			);
 			success = false;
 		}
 		variableSize = 1;
@@ -405,12 +377,8 @@ bool AsmAnalyzer::checkAssignment(assembly::Identifier const& _variable, size_t
 	if (variableSize == size_t(-1))
 	{
 		// Only add message if the callback did not.
-		if (numErrorsBefore == m_errors.size())
-			m_errors.push_back(make_shared<Error>(
-				Error::Type::DeclarationError,
-				"Variable not found or variable not lvalue.",
-				_variable.location
-			));
+		if (numErrorsBefore == m_errorReporter.errors().size())
+			m_errorReporter.declarationError(_variable.location, "Variable not found or variable not lvalue.");
 		success = false;
 	}
 	if (_valueSize == size_t(-1))
@@ -420,15 +388,14 @@ bool AsmAnalyzer::checkAssignment(assembly::Identifier const& _variable, size_t
 
 	if (_valueSize != variableSize && variableSize != size_t(-1))
 	{
-		m_errors.push_back(make_shared<Error>(
-			Error::Type::TypeError,
+		m_errorReporter.typeError(
+			_variable.location,
 			"Variable size (" +
 			to_string(variableSize) +
 			") and value size (" +
 			to_string(_valueSize) +
-			") do not match.",
-			_variable.location
-		));
+			") do not match."
+		);
 		success = false;
 	}
 	return success;
@@ -439,15 +406,14 @@ bool AsmAnalyzer::expectDeposit(int const _deposit, int const _oldHeight, Source
 	int stackDiff = m_stackHeight - _oldHeight;
 	if (stackDiff != _deposit)
 	{
-		m_errors.push_back(make_shared<Error>(
-			Error::Type::TypeError,
+		m_errorReporter.typeError(
+			_location,
 			"Expected instruction(s) to deposit " +
 			boost::lexical_cast<string>(_deposit) +
 			" item(s) to the stack, but did deposit " +
 			boost::lexical_cast<string>(stackDiff) +
-			" item(s).",
-			_location
-		));
+			" item(s)."
+		);
 		return false;
 	}
 	else
@@ -468,9 +434,8 @@ void AsmAnalyzer::expectValidType(string const& type, SourceLocation const& _loc
 		return;
 
 	if (!builtinTypes.count(type))
-		m_errors.push_back(make_shared<Error>(
-			Error::Type::TypeError,
-			"\"" + type + "\" is not a valid type (user defined types are not yet supported).",
-			_location
-		));
+		m_errorReporter.typeError(
+			_location,
+			"\"" + type + "\" is not a valid type (user defined types are not yet supported)."
+		);
 }

libsolidity/inlineasm/AsmAnalysis.h

@@ -22,8 +22,6 @@
 
 #include <libsolidity/inlineasm/AsmStack.h>
 
-#include <libsolidity/interface/Exceptions.h>
-
 #include <boost/variant.hpp>
 
 #include <functional>
@@ -33,6 +31,7 @@ namespace dev
 {
 namespace solidity
 {
+class ErrorReporter;
 namespace assembly
 {
 
@@ -63,10 +62,10 @@ class AsmAnalyzer: public boost::static_visitor<bool>
 public:
 	explicit AsmAnalyzer(
 		AsmAnalysisInfo& _analysisInfo,
-		ErrorList& _errors,
+		ErrorReporter& _errorReporter,
 		bool _julia = false,
 		julia::ExternalIdentifierAccess::Resolver const& _resolver = julia::ExternalIdentifierAccess::Resolver()
-	): m_resolver(_resolver), m_info(_analysisInfo), m_errors(_errors), m_julia(_julia) {}
+	): m_resolver(_resolver), m_info(_analysisInfo), m_errorReporter(_errorReporter), m_julia(_julia) {}
 
 	bool analyze(assembly::Block const& _block);
 
@@ -95,7 +94,7 @@ private:
 	julia::ExternalIdentifierAccess::Resolver const& m_resolver;
 	Scope* m_currentScope = nullptr;
 	AsmAnalysisInfo& m_info;
-	ErrorList& m_errors;
+	ErrorReporter& m_errorReporter;
 	bool m_julia = false;
 };
 

libsolidity/inlineasm/AsmCodeGen.cpp

@@ -107,7 +107,7 @@ eth::Assembly assembly::CodeGenerator::assemble(
 {
 	eth::Assembly assembly;
 	EthAssemblyAdapter assemblyAdapter(assembly);
-	julia::CodeTransform(m_errors, assemblyAdapter, _parsedData, _analysisInfo, _identifierAccess);
+	julia::CodeTransform(m_errorReporter, assemblyAdapter, _parsedData, _analysisInfo, _identifierAccess);
 	return assembly;
 }
 
@@ -119,5 +119,5 @@ void assembly::CodeGenerator::assemble(
 )
 {
 	EthAssemblyAdapter assemblyAdapter(_assembly);
-	julia::CodeTransform(m_errors, assemblyAdapter, _parsedData, _analysisInfo, _identifierAccess);
+	julia::CodeTransform(m_errorReporter, assemblyAdapter, _parsedData, _analysisInfo, _identifierAccess);
 }

libsolidity/inlineasm/AsmCodeGen.h

@@ -23,7 +23,6 @@
 #pragma once
 
 #include <libsolidity/inlineasm/AsmAnalysis.h>
-#include <libsolidity/interface/Exceptions.h>
 
 #include <functional>
 
@@ -35,6 +34,7 @@ class Assembly;
 }
 namespace solidity
 {
+class ErrorReporter;
 namespace assembly
 {
 struct Block;
@@ -42,8 +42,8 @@ struct Block;
 class CodeGenerator
 {
 public:
-	CodeGenerator(ErrorList& _errors):
-		m_errors(_errors) {}
+	CodeGenerator(ErrorReporter& _errorReporter):
+		m_errorReporter(_errorReporter) {}
 	/// Performs code generation and @returns the result.
 	eth::Assembly assemble(
 		Block const& _parsedData,
@@ -59,7 +59,7 @@ public:
 	);
 
 private:
-	ErrorList& m_errors;
+	ErrorReporter& m_errorReporter;
 };
 
 }

libsolidity/inlineasm/AsmParser.cpp

@@ -21,10 +21,10 @@
  */
 
 #include <libsolidity/inlineasm/AsmParser.h>
+#include <libsolidity/parsing/Scanner.h>
+#include <libsolidity/interface/ErrorReporter.h>
 #include <ctype.h>
 #include <algorithm>
-#include <libsolidity/parsing/Scanner.h>
-#include <libsolidity/interface/Exceptions.h>
 
 using namespace std;
 using namespace dev;
@@ -40,7 +40,7 @@ shared_ptr<assembly::Block> Parser::parse(std::shared_ptr<Scanner> const& _scann
 	}
 	catch (FatalError const&)
 	{
-		if (m_errors.empty())
+		if (m_errorReporter.errors().empty())
 			throw; // Something is weird here, rather throw again.
 	}
 	return nullptr;

libsolidity/inlineasm/AsmParser.h

@@ -37,7 +37,7 @@ namespace assembly
 class Parser: public ParserBase
 {
 public:
-	explicit Parser(ErrorList& _errors, bool _julia = false): ParserBase(_errors), m_julia(_julia) {}
+	explicit Parser(ErrorReporter& _errorReporter, bool _julia = false): ParserBase(_errorReporter), m_julia(_julia) {}
 
 	/// Parses an inline assembly block starting with `{` and ending with `}`.
 	/// @returns an empty shared pointer on error.

libsolidity/inlineasm/AsmScopeFiller.cpp

@@ -24,7 +24,7 @@
 #include <libsolidity/inlineasm/AsmScope.h>
 #include <libsolidity/inlineasm/AsmAnalysisInfo.h>
 
-#include <libsolidity/interface/Exceptions.h>
+#include <libsolidity/interface/ErrorReporter.h>
 #include <libsolidity/interface/Utils.h>
 
 #include <boost/range/adaptor/reversed.hpp>
@@ -37,8 +37,8 @@ using namespace dev;
 using namespace dev::solidity;
 using namespace dev::solidity::assembly;
 
-ScopeFiller::ScopeFiller(AsmAnalysisInfo& _info, ErrorList& _errors):
-	m_info(_info), m_errors(_errors)
+ScopeFiller::ScopeFiller(AsmAnalysisInfo& _info, ErrorReporter& _errorReporter):
+	m_info(_info), m_errorReporter(_errorReporter)
 {
 	m_currentScope = &scope(nullptr);
 }
@@ -48,11 +48,10 @@ bool ScopeFiller::operator()(Label const& _item)
 	if (!m_currentScope->registerLabel(_item.name))
 	{
 		//@TODO secondary location
-		m_errors.push_back(make_shared<Error>(
-			Error::Type::DeclarationError,
-			"Label name " + _item.name + " already taken in this scope.",
-			_item.location
-		));
+		m_errorReporter.declarationError(
+			_item.location,
+			"Label name " + _item.name + " already taken in this scope."
+		);
 		return false;
 	}
 	return true;
@@ -78,11 +77,10 @@ bool ScopeFiller::operator()(assembly::FunctionDefinition const& _funDef)
 	if (!m_currentScope->registerFunction(_funDef.name, arguments, returns))
 	{
 		//@TODO secondary location
-		m_errors.push_back(make_shared<Error>(
-			Error::Type::DeclarationError,
-			"Function name " + _funDef.name + " already taken in this scope.",
-			_funDef.location
-		));
+		m_errorReporter.declarationError(
+			_funDef.location,
+			"Function name " + _funDef.name + " already taken in this scope."
+		);
 		success = false;
 	}
 
@@ -132,11 +130,10 @@ bool ScopeFiller::registerVariable(TypedName const& _name, SourceLocation const&
 	if (!_scope.registerVariable(_name.name, _name.type))
 	{
 		//@TODO secondary location
-		m_errors.push_back(make_shared<Error>(
-			Error::Type::DeclarationError,
-			"Variable name " + _name.name + " already taken in this scope.",
-			_location
-		));
+		m_errorReporter.declarationError(
+			_location,
+			"Variable name " + _name.name + " already taken in this scope."
+		);
 		return false;
 	}
 	return true;

libsolidity/inlineasm/AsmScopeFiller.h

@@ -20,8 +20,6 @@
 
 #pragma once
 
-#include <libsolidity/interface/Exceptions.h>
-
 #include <boost/variant.hpp>
 
 #include <functional>
@@ -29,8 +27,10 @@
 
 namespace dev
 {
+struct SourceLocation;
 namespace solidity
 {
+class ErrorReporter;
 namespace assembly
 {
 
@@ -58,7 +58,7 @@ struct AsmAnalysisInfo;
 class ScopeFiller: public boost::static_visitor<bool>
 {
 public:
-	ScopeFiller(AsmAnalysisInfo& _info, ErrorList& _errors);
+	ScopeFiller(AsmAnalysisInfo& _info, ErrorReporter& _errorReporter);
 
 	bool operator()(assembly::Instruction const&) { return true; }
 	bool operator()(assembly::Literal const&) { return true; }
@@ -84,7 +84,7 @@ private:
 
 	Scope* m_currentScope = nullptr;
 	AsmAnalysisInfo& m_info;
-	ErrorList& m_errors;
+	ErrorReporter& m_errorReporter;
 };
 
 }

libsolidity/inlineasm/AsmStack.cpp

@@ -46,14 +46,14 @@ bool InlineAssemblyStack::parse(
 )
 {
 	m_parserResult = make_shared<Block>();
-	Parser parser(m_errors);
+	Parser parser(m_errorReporter);
 	auto result = parser.parse(_scanner);
 	if (!result)
 		return false;
 
 	*m_parserResult = std::move(*result);
 	AsmAnalysisInfo analysisInfo;
-	return (AsmAnalyzer(analysisInfo, m_errors, false, _resolver)).analyze(*m_parserResult);
+	return (AsmAnalyzer(analysisInfo, m_errorReporter, false, _resolver)).analyze(*m_parserResult);
 }
 
 string InlineAssemblyStack::toString()
@@ -64,9 +64,9 @@ string InlineAssemblyStack::toString()
 eth::Assembly InlineAssemblyStack::assemble()
 {
 	AsmAnalysisInfo analysisInfo;
-	AsmAnalyzer analyzer(analysisInfo, m_errors);
+	AsmAnalyzer analyzer(analysisInfo, m_errorReporter);
 	solAssert(analyzer.analyze(*m_parserResult), "");
-	CodeGenerator codeGen(m_errors);
+	CodeGenerator codeGen(m_errorReporter);
 	return codeGen.assemble(*m_parserResult, analysisInfo);
 }
 
@@ -77,19 +77,20 @@ bool InlineAssemblyStack::parseAndAssemble(
 )
 {
 	ErrorList errors;
+	ErrorReporter errorReporter(errors);
 	auto scanner = make_shared<Scanner>(CharStream(_input), "--CODEGEN--");
-	auto parserResult = Parser(errors).parse(scanner);
-	if (!errors.empty())
+	auto parserResult = Parser(errorReporter).parse(scanner);
+	if (!errorReporter.errors().empty())
 		return false;
 	solAssert(parserResult, "");
 
 	AsmAnalysisInfo analysisInfo;
-	AsmAnalyzer analyzer(analysisInfo, errors, false, _identifierAccess.resolve);
+	AsmAnalyzer analyzer(analysisInfo, errorReporter, false, _identifierAccess.resolve);
 	solAssert(analyzer.analyze(*parserResult), "");
-	CodeGenerator(errors).assemble(*parserResult, analysisInfo, _assembly, _identifierAccess);
+	CodeGenerator(errorReporter).assemble(*parserResult, analysisInfo, _assembly, _identifierAccess);
 
 	// At this point, the assembly might be messed up, but we should throw an
 	// internal compiler error anyway.
-	return errors.empty();
+	return errorReporter.errors().empty();
 }
 

libsolidity/inlineasm/AsmStack.h

@@ -22,9 +22,8 @@
 
 #pragma once
 
-#include <libsolidity/interface/Exceptions.h>
-
 #include <libjulia/backends/evm/AbstractAssembly.h>
+#include <libsolidity/interface/ErrorReporter.h>
 
 #include <string>
 #include <functional>
@@ -46,6 +45,8 @@ struct Identifier;
 class InlineAssemblyStack
 {
 public:
+	InlineAssemblyStack():
+		m_errorReporter(m_errorList) {}
 	/// Parse the given inline assembly chunk starting with `{` and ending with the corresponding `}`.
 	/// @return false or error.
 	bool parse(
@@ -65,11 +66,12 @@ public:
 		julia::ExternalIdentifierAccess const& _identifierAccess = julia::ExternalIdentifierAccess()
 	);
 
-	ErrorList const& errors() const { return m_errors; }
+	ErrorList const& errors() const { return m_errorReporter.errors(); }
 
 private:
 	std::shared_ptr<Block> m_parserResult;
-	ErrorList m_errors;
+	ErrorList m_errorList;
+	ErrorReporter m_errorReporter;
 };
 
 }