Merge pull request #1795 from LianaHus/sol_SecondaryLocationsForErrors

Solidity: Secondary locations for errors

libsolidity/AST.cpp

@@ -96,8 +96,8 @@ void ContractDefinition::checkTypeRequirements()
 		FixedHash<4> const& hash = it.first;
 		if (hashes.count(hash))
 			BOOST_THROW_EXCEPTION(createTypeError(
-									  std::string("Function signature hash collision for ") +
-									  it.second->externalSignature()));
+				string("Function signature hash collision for ") + it.second->externalSignature()
+			));
 		hashes.insert(hash);
 	}
 }
@@ -140,12 +140,22 @@ void ContractDefinition::checkDuplicateFunctions() const
 	map<string, vector<FunctionDefinition const*>> functions;
 	for (ASTPointer<FunctionDefinition> const& function: getDefinedFunctions())
 		functions[function->getName()].push_back(function.get());
+
 	if (functions[getName()].size() > 1)
+	{
+		SecondarySourceLocation ssl;
+		auto it = functions[getName()].begin();
+		++it;
+		for (; it != functions[getName()].end(); ++it)
+			ssl.append("Another declaration is here:", (*it)->getLocation());
+
 		BOOST_THROW_EXCEPTION(
 			DeclarationError() <<
-			errinfo_sourceLocation(getLocation()) <<
-			errinfo_comment("More than one constructor defined.")
+			errinfo_sourceLocation(functions[getName()].front()->getLocation()) <<
+			errinfo_comment("More than one constructor defined.") <<
+			errinfo_secondarySourceLocation(ssl)
 		);
+	}
 	for (auto const& it: functions)
 	{
 		vector<FunctionDefinition const*> const& overloads = it.second;
@@ -155,7 +165,9 @@ void ContractDefinition::checkDuplicateFunctions() const
 					BOOST_THROW_EXCEPTION(
 						DeclarationError() <<
 						errinfo_sourceLocation(overloads[j]->getLocation()) <<
-						errinfo_comment("Function with same name and arguments already defined.")
+						errinfo_comment("Function with same name and arguments defined twice.") <<
+						errinfo_secondarySourceLocation(SecondarySourceLocation().append(
+							"Other declaration is here:", overloads[i]->getLocation()))
 					);
 	}
 }
@@ -299,12 +311,12 @@ void ContractDefinition::checkExternalTypeClashes() const
 					));
 }
 
-std::vector<ASTPointer<EventDefinition>> const& ContractDefinition::getInterfaceEvents() const
+vector<ASTPointer<EventDefinition>> const& ContractDefinition::getInterfaceEvents() const
 {
 	if (!m_interfaceEvents)
 	{
 		set<string> eventsSeen;
-		m_interfaceEvents.reset(new std::vector<ASTPointer<EventDefinition>>());
+		m_interfaceEvents.reset(new vector<ASTPointer<EventDefinition>>());
 		for (ContractDefinition const* contract: getLinearizedBaseContracts())
 			for (ASTPointer<EventDefinition> const& e: contract->getEvents())
 				if (eventsSeen.count(e->getName()) == 0)
@@ -944,7 +956,7 @@ void Identifier::overloadResolution(TypePointers const& _argumentTypes)
 	solAssert(!m_referencedDeclaration, "Referenced declaration should be null before overload resolution.");
 	solAssert(!m_overloadedDeclarations.empty(), "No candidates for overload resolution found.");
 
-	std::vector<Declaration const*> possibles;
+	vector<Declaration const*> possibles;
 	if (m_overloadedDeclarations.size() == 1)
 		m_referencedDeclaration = *m_overloadedDeclarations.begin();
 

libsolidity/DeclarationContainer.cpp

@@ -28,6 +28,28 @@ using namespace std;
 using namespace dev;
 using namespace dev::solidity;
 
+Declaration const* DeclarationContainer::conflictingDeclaration(Declaration const& _declaration) const
+{
+	ASTString const& name(_declaration.getName());
+	solAssert(!name.empty(), "");
+	vector<Declaration const*> declarations;
+	if (m_declarations.count(name))
+		declarations += m_declarations.at(name);
+	if (m_invisibleDeclarations.count(name))
+		declarations += m_invisibleDeclarations.at(name);
+	if (dynamic_cast<FunctionDefinition const*>(&_declaration))
+	{
+		// check that all other declarations with the same name are functions
+		for (Declaration const* declaration: declarations)
+			if (!dynamic_cast<FunctionDefinition const*>(declaration))
+				return declaration;
+	}
+	else if (!declarations.empty())
+		return declarations.front();
+
+	return nullptr;
+}
+
 bool DeclarationContainer::registerDeclaration(Declaration const& _declaration, bool _invisible, bool _update)
 {
 	ASTString const& name(_declaration.getName());
@@ -40,24 +62,8 @@ bool DeclarationContainer::registerDeclaration(Declaration const& _declaration,
 		m_declarations.erase(name);
 		m_invisibleDeclarations.erase(name);
 	}
-	else
-	{
-		vector<Declaration const*> declarations;
-		if (m_declarations.count(name))
-			declarations += m_declarations.at(name);
-		if (m_invisibleDeclarations.count(name))
-			declarations += m_invisibleDeclarations.at(name);
-		if (dynamic_cast<FunctionDefinition const*>(&_declaration))
-		{
-			// check that all other declarations with the same name are functions
-
-			for (Declaration const* declaration: declarations)
-				if (!dynamic_cast<FunctionDefinition const*>(declaration))
-					return false;
-		}
-		else if (!declarations.empty())
-			return false;
-	}
+	else if (conflictingDeclaration(_declaration))
+		return false;
 
 	if (_invisible)
 		m_invisibleDeclarations[name].insert(&_declaration);

libsolidity/DeclarationContainer.h

@@ -34,7 +34,7 @@ namespace solidity
 {
 
 /**
- * Container that stores mappings betwee names and declarations. It also contains a link to the
+ * Container that stores mappings between names and declarations. It also contains a link to the
  * enclosing scope.
  */
 class DeclarationContainer
@@ -51,6 +51,8 @@ public:
 	std::set<Declaration const*> resolveName(ASTString const& _name, bool _recursive = false) const;
 	Declaration const* getEnclosingDeclaration() const { return m_enclosingDeclaration; }
 	std::map<ASTString, std::set<Declaration const*>> const& getDeclarations() const { return m_declarations; }
+	/// @returns whether declaration is valid, and if not also returns previous declaration.
+	Declaration const* conflictingDeclaration(Declaration const& _declaration) const;
 
 private:
 	Declaration const* m_enclosingDeclaration;

libsolidity/Exceptions.h

@@ -23,6 +23,7 @@
 #pragma once
 
 #include <string>
+#include <utility>
 #include <libdevcore/Exceptions.h>
 #include <libevmcore/SourceLocation.h>
 
@@ -38,7 +39,22 @@ struct CompilerError: virtual Exception {};
 struct InternalCompilerError: virtual Exception {};
 struct DocstringParsingError: virtual Exception {};
 
+using errorSourceLocationInfo = std::pair<std::string, SourceLocation>;
+
+class SecondarySourceLocation
+{
+public:
+	SecondarySourceLocation& append(std::string const& _errMsg, SourceLocation const& _sourceLocation)
+	{
+		infos.push_back(std::make_pair(_errMsg, _sourceLocation));
+		return *this;
+	}
+
+	std::vector<errorSourceLocationInfo> infos;
+};
+
 using errinfo_sourceLocation = boost::error_info<struct tag_sourceLocation, SourceLocation>;
+using errinfo_secondarySourceLocation = boost::error_info<struct tag_secondarySourceLocation, SecondarySourceLocation>;
 
 }
 }

libsolidity/NameAndTypeResolver.cpp

@@ -354,9 +354,33 @@ void DeclarationRegistrationHelper::closeCurrentScope()
 void DeclarationRegistrationHelper::registerDeclaration(Declaration& _declaration, bool _opensScope)
 {
 	if (!m_scopes[m_currentScope].registerDeclaration(_declaration, !_declaration.isVisibleInContract()))
-		BOOST_THROW_EXCEPTION(DeclarationError() << errinfo_sourceLocation(_declaration.getLocation())
-												 << errinfo_comment("Identifier already declared."));
-	//@todo the exception should also contain the location of the first declaration
+	{
+		SourceLocation firstDeclarationLocation;
+		SourceLocation secondDeclarationLocation;
+		Declaration const* conflictingDeclaration = m_scopes[m_currentScope].conflictingDeclaration(_declaration);
+		solAssert(conflictingDeclaration, "");
+
+		if (_declaration.getLocation().start < conflictingDeclaration->getLocation().start)
+		{
+			firstDeclarationLocation = _declaration.getLocation();
+			secondDeclarationLocation = conflictingDeclaration->getLocation();
+		}
+		else
+		{
+			firstDeclarationLocation = conflictingDeclaration->getLocation();
+			secondDeclarationLocation = _declaration.getLocation();
+		}
+
+		BOOST_THROW_EXCEPTION(
+			DeclarationError() <<
+			errinfo_sourceLocation(secondDeclarationLocation) <<
+			errinfo_comment("Identifier already declared.") <<
+			errinfo_secondarySourceLocation(
+				SecondarySourceLocation().append("The previous declaration is here:", firstDeclarationLocation)
+			)
+		);
+	}
+
 	_declaration.setScope(m_currentScope);
 	if (_opensScope)
 		enterNewSubScope(_declaration);
@@ -435,8 +459,11 @@ bool ReferencesResolver::visit(Identifier& _identifier)
 {
 	auto declarations = m_resolver.getNameFromCurrentScope(_identifier.getName());
 	if (declarations.empty())
-		BOOST_THROW_EXCEPTION(DeclarationError() << errinfo_sourceLocation(_identifier.getLocation())
-												 << errinfo_comment("Undeclared identifier."));
+		BOOST_THROW_EXCEPTION(
+			DeclarationError() <<
+			errinfo_sourceLocation(_identifier.getLocation()) <<
+			errinfo_comment("Undeclared identifier.")
+		);
 	else if (declarations.size() == 1)
 		_identifier.setReferencedDeclaration(**declarations.begin(), m_currentContract);
 	else

libsolidity/SourceReferenceFormatter.cpp

@@ -32,9 +32,11 @@ namespace dev
 namespace solidity
 {
 
-void SourceReferenceFormatter::printSourceLocation(ostream& _stream,
-												   SourceLocation const& _location,
-												   Scanner const& _scanner)
+void SourceReferenceFormatter::printSourceLocation(
+	ostream& _stream,
+	SourceLocation const& _location,
+	Scanner const& _scanner
+)
 {
 	int startLine;
 	int startColumn;
@@ -46,11 +48,11 @@ void SourceReferenceFormatter::printSourceLocation(ostream& _stream,
 	{
 		string line = _scanner.getLineAtPosition(_location.start);
 		_stream << line << endl;
-		std::for_each(line.cbegin(), line.cbegin() + startColumn,
-			[&_stream](char const& ch)
-			{
-				_stream << (ch == '\t' ? '\t' : ' ');
-			});
+		for_each(
+			line.cbegin(),
+			line.cbegin() + startColumn,
+			[&_stream](char const& ch) { _stream << (ch == '\t' ? '\t' : ' '); }
+		);
 		_stream << "^";
 		if (endColumn > startColumn + 2)
 			_stream << string(endColumn - startColumn - 2, '-');
@@ -59,33 +61,65 @@ void SourceReferenceFormatter::printSourceLocation(ostream& _stream,
 		_stream << endl;
 	}
 	else
-		_stream << _scanner.getLineAtPosition(_location.start) << endl
-				<< string(startColumn, ' ') << "^\n"
-				<< "Spanning multiple lines.\n";
+		_stream <<
+			_scanner.getLineAtPosition(_location.start) <<
+			endl <<
+			string(startColumn, ' ') <<
+			"^\n" <<
+			"Spanning multiple lines.\n";
 }
 
-void SourceReferenceFormatter::printExceptionInformation(ostream& _stream,
-														 Exception const& _exception,
-														 string const& _name,
-														 CompilerStack const& _compiler)
+void SourceReferenceFormatter::printSourceName(
+	ostream& _stream,
+	SourceLocation const& _location,
+	Scanner const& _scanner
+)
+{
+	int startLine;
+	int startColumn;
+	tie(startLine, startColumn) = _scanner.translatePositionToLineColumn(_location.start);
+	_stream << *_location.sourceName << ":" << (startLine + 1) << ":" << (startColumn + 1) << ": ";
+}
+
+void SourceReferenceFormatter::printExceptionInformation(
+	ostream& _stream,
+	Exception const& _exception,
+	string const& _name,
+	CompilerStack const& _compiler
+)
 {
 	SourceLocation const* location = boost::get_error_info<errinfo_sourceLocation>(_exception);
-	Scanner const* scanner;
+	auto secondarylocation = boost::get_error_info<errinfo_secondarySourceLocation>(_exception);
+	Scanner const* scanner = nullptr;
 
 	if (location)
 	{
 		scanner = &_compiler.getScanner(*location->sourceName);
-		int startLine;
-		int startColumn;
-		tie(startLine, startColumn) = scanner->translatePositionToLineColumn(location->start);
-		_stream << *location->sourceName << ":" << (startLine + 1) << ":" << (startColumn + 1) << ": ";
+		printSourceName(_stream, *location, *scanner);
 	}
+
 	_stream << _name;
 	if (string const* description = boost::get_error_info<errinfo_comment>(_exception))
 		_stream << ": " << *description << endl;
 
 	if (location)
+	{
+		scanner = &_compiler.getScanner(*location->sourceName);
 		printSourceLocation(_stream, *location, *scanner);
+	}
+
+	if (secondarylocation && !secondarylocation->infos.empty())
+	{
+		for (auto info: secondarylocation->infos)
+		{
+			scanner = &_compiler.getScanner(*info.second.sourceName);
+			_stream << info.first << " ";
+			printSourceName(_stream, info.second, *scanner);
+			_stream << endl;
+			printSourceLocation(_stream, info.second, *scanner);
+		}
+		_stream << endl;
+	}
 }
 
 }

libsolidity/SourceReferenceFormatter.h

@@ -40,8 +40,14 @@ struct SourceReferenceFormatter
 {
 public:
 	static void printSourceLocation(std::ostream& _stream, SourceLocation const& _location, Scanner const& _scanner);
-	static void printExceptionInformation(std::ostream& _stream, Exception const& _exception,
-										  std::string const& _name, CompilerStack const& _compiler);
+	static void printExceptionInformation(
+		std::ostream& _stream,
+		Exception const& _exception,
+		std::string const& _name,
+		CompilerStack const& _compiler
+	);
+private:
+	static void printSourceName(std::ostream& _stream, SourceLocation const& _location, Scanner const& _scanner);
 };
 
 }