Merge pull request #1701 from ethereum/fixFatalErrors

Fix early exits for fatal errors.

Changelog.md

@@ -9,6 +9,7 @@ Features:
 Bugfixes:
  * Commandline interface: Always escape filenames (replace ``/``, ``:`` and ``.`` with ``_``).
  * Commandline interface: Do not try creating paths ``.`` and ``..``.
+ * Type system: Fix a crash caused by continuing on fatal errors in the code.
  * Type system: Disallow arrays with negative length.
 
 ### 0.4.9 (2017-01-31)

libsolidity/analysis/NameAndTypeResolver.cpp

@@ -21,6 +21,7 @@
  */
 
 #include <libsolidity/analysis/NameAndTypeResolver.h>
+
 #include <libsolidity/ast/AST.h>
 #include <libsolidity/analysis/TypeChecker.h>
 #include <libsolidity/interface/Exceptions.h>
@@ -130,62 +131,9 @@ bool NameAndTypeResolver::performImports(SourceUnit& _sourceUnit, map<string, So
 
 bool NameAndTypeResolver::resolveNamesAndTypes(ASTNode& _node, bool _resolveInsideCode)
 {
-	bool success = true;
 	try
 	{
-		if (ContractDefinition* contract = dynamic_cast<ContractDefinition*>(&_node))
+		return resolveNamesAndTypesInternal(_node, _resolveInsideCode);
-		{
-			m_currentScope = m_scopes[contract->scope()].get();
-			solAssert(!!m_currentScope, "");
-
-			for (ASTPointer<InheritanceSpecifier> const& baseContract: contract->baseContracts())
-				if (!resolveNamesAndTypes(*baseContract, true))
-					success = false;
-
-			m_currentScope = m_scopes[contract].get();
-
-			if (success)
-			{
-				linearizeBaseContracts(*contract);
-				vector<ContractDefinition const*> properBases(
-					++contract->annotation().linearizedBaseContracts.begin(),
-					contract->annotation().linearizedBaseContracts.end()
-				);
-
-				for (ContractDefinition const* base: properBases)
-					importInheritedScope(*base);
-			}
-
-			// these can contain code, only resolve parameters for now
-			for (ASTPointer<ASTNode> const& node: contract->subNodes())
-			{
-				m_currentScope = m_scopes[contract].get();
-				if (!resolveNamesAndTypes(*node, false))
-					success = false;
-			}
-
-			if (!success)
-				return false;
-
-			if (!_resolveInsideCode)
-				return success;
-
-			m_currentScope = m_scopes[contract].get();
-
-			// now resolve references inside the code
-			for (ASTPointer<ASTNode> const& node: contract->subNodes())
-			{
-				m_currentScope = m_scopes[contract].get();
-				if (!resolveNamesAndTypes(*node, true))
-					success = false;
-			}
-		}
-		else
-		{
-			if (m_scopes.count(&_node))
-				m_currentScope = m_scopes[&_node].get();
-			return ReferencesResolver(m_errors, *this, _resolveInsideCode).resolve(_node);
-		}
 	}
 	catch (FatalError const&)
 	{
@@ -193,7 +141,6 @@ bool NameAndTypeResolver::resolveNamesAndTypes(ASTNode& _node, bool _resolveInsi
 			throw; // Something is weird here, rather throw again.
 		return false;
 	}
-	return success;
 }
 
 bool NameAndTypeResolver::updateDeclaration(Declaration const& _declaration)
@@ -249,21 +196,25 @@ vector<Declaration const*> NameAndTypeResolver::cleanedDeclarations(
 	solAssert(_declarations.size() > 1, "");
 	vector<Declaration const*> uniqueFunctions;
 
-	for (auto it = _declarations.begin(); it != _declarations.end(); ++it)
+	for (Declaration const* declaration: _declarations)
 	{
-		solAssert(*it, "");
+		solAssert(declaration, "");
 		// the declaration is functionDefinition, eventDefinition or a VariableDeclaration while declarations > 1
-		solAssert(dynamic_cast<FunctionDefinition const*>(*it) || dynamic_cast<EventDefinition const*>(*it) || dynamic_cast<VariableDeclaration const*>(*it),
+		solAssert(
-			"Found overloading involving something not a function or a variable");
+			dynamic_cast<FunctionDefinition const*>(declaration) ||
+			dynamic_cast<EventDefinition const*>(declaration) ||
+			dynamic_cast<VariableDeclaration const*>(declaration),
+			"Found overloading involving something not a function or a variable."
+		);
 
-		shared_ptr<FunctionType const> functionType { (*it)->functionType(false) };
+		FunctionTypePointer functionType { declaration->functionType(false) };
 		if (!functionType)
-			functionType = (*it)->functionType(true);
+			functionType = declaration->functionType(true);
-		solAssert(functionType, "failed to determine the function type of the overloaded");
+		solAssert(functionType, "Failed to determine the function type of the overloaded.");
 
 		for (auto parameter: functionType->parameterTypes() + functionType->returnParameterTypes())
 			if (!parameter)
-				reportFatalDeclarationError(_identifier.location(), "Function type can not be used in this context");
+				reportFatalDeclarationError(_identifier.location(), "Function type can not be used in this context.");
 
 		if (uniqueFunctions.end() == find_if(
 			uniqueFunctions.begin(),
@@ -276,11 +227,73 @@ vector<Declaration const*> NameAndTypeResolver::cleanedDeclarations(
 				return newFunctionType && functionType->hasEqualArgumentTypes(*newFunctionType);
 			}
 		))
-			uniqueFunctions.push_back(*it);
+			uniqueFunctions.push_back(declaration);
 	}
 	return uniqueFunctions;
 }
 
+bool NameAndTypeResolver::resolveNamesAndTypesInternal(ASTNode& _node, bool _resolveInsideCode)
+{
+	if (ContractDefinition* contract = dynamic_cast<ContractDefinition*>(&_node))
+	{
+		bool success = true;
+		m_currentScope = m_scopes[contract->scope()].get();
+		solAssert(!!m_currentScope, "");
+
+		for (ASTPointer<InheritanceSpecifier> const& baseContract: contract->baseContracts())
+			if (!resolveNamesAndTypes(*baseContract, true))
+				success = false;
+
+		m_currentScope = m_scopes[contract].get();
+
+		if (success)
+		{
+			linearizeBaseContracts(*contract);
+			vector<ContractDefinition const*> properBases(
+				++contract->annotation().linearizedBaseContracts.begin(),
+				contract->annotation().linearizedBaseContracts.end()
+			);
+
+			for (ContractDefinition const* base: properBases)
+				importInheritedScope(*base);
+		}
+
+		// these can contain code, only resolve parameters for now
+		for (ASTPointer<ASTNode> const& node: contract->subNodes())
+		{
+			m_currentScope = m_scopes[contract].get();
+			if (!resolveNamesAndTypes(*node, false))
+			{
+				success = false;
+				break;
+			}
+		}
+
+		if (!success)
+			return false;
+
+		if (!_resolveInsideCode)
+			return success;
+
+		m_currentScope = m_scopes[contract].get();
+
+		// now resolve references inside the code
+		for (ASTPointer<ASTNode> const& node: contract->subNodes())
+		{
+			m_currentScope = m_scopes[contract].get();
+			if (!resolveNamesAndTypes(*node, true))
+				success = false;
+		}
+		return success;
+	}
+	else
+	{
+		if (m_scopes.count(&_node))
+			m_currentScope = m_scopes[&_node].get();
+		return ReferencesResolver(m_errors, *this, _resolveInsideCode).resolve(_node);
+	}
+}
+
 void NameAndTypeResolver::importInheritedScope(ContractDefinition const& _base)
 {
 	auto iterator = m_scopes.find(&_base);

libsolidity/analysis/NameAndTypeResolver.h

@@ -89,6 +89,9 @@ public:
 	);
 
 private:
+	/// Internal version of @a resolveNamesAndTypes (called from there) throws exceptions on fatal errors.
+	bool resolveNamesAndTypesInternal(ASTNode& _node, bool _resolveInsideCode = true);
+
 	/// Imports all members declared directly in the given contract (i.e. does not import inherited members)
 	/// into the current scope if they are not present already.
 	void importInheritedScope(ContractDefinition const& _base);

libsolidity/analysis/ReferencesResolver.cpp

@@ -35,14 +35,7 @@ using namespace dev::solidity;
 
 bool ReferencesResolver::resolve(ASTNode const& _root)
 {
-	try
+	_root.accept(*this);
-	{
-		_root.accept(*this);
-	}
-	catch (FatalError const&)
-	{
-		solAssert(m_errorOccurred, "");
-	}
 	return !m_errorOccurred;
 }
 

libsolidity/analysis/ReferencesResolver.h

@@ -52,7 +52,7 @@ public:
 		m_resolveInsideCode(_resolveInsideCode)
 	{}
 
-	/// @returns true if no errors during resolving
+	/// @returns true if no errors during resolving and throws exceptions on fatal errors.
 	bool resolve(ASTNode const& _root);
 
 private:

libsolidity/ast/Types.cpp

@@ -1652,6 +1652,7 @@ MemberList::MemberMap StructType::nativeMembers(ContractDefinition const*) const
 	for (ASTPointer<VariableDeclaration> const& variable: m_struct.members())
 	{
 		TypePointer type = variable->annotation().type;
+		solAssert(type, "");
 		// Skip all mapping members if we are not in storage.
 		if (location() != DataLocation::Storage && !type->canLiveOutsideStorage())
 			continue;
@@ -1964,6 +1965,8 @@ FunctionType::FunctionType(VariableDeclaration const& _varDecl):
 	if (auto structType = dynamic_cast<StructType const*>(returnType.get()))
 	{
 		for (auto const& member: structType->members(nullptr))
+		{
+			solAssert(member.type, "");
 			if (member.type->category() != Category::Mapping)
 			{
 				if (auto arrayType = dynamic_cast<ArrayType const*>(member.type.get()))
@@ -1972,6 +1975,7 @@ FunctionType::FunctionType(VariableDeclaration const& _varDecl):
 				retParams.push_back(member.type);
 				retParamNames.push_back(member.name);
 			}
+		}
 	}
 	else
 	{

test/libsolidity/SolidityNameAndTypeResolution.cpp

@@ -5079,6 +5079,22 @@ BOOST_AUTO_TEST_CASE(invalid_address_length)
 	CHECK_WARNING(text, "checksum");
 }
 
+BOOST_AUTO_TEST_CASE(early_exit_on_fatal_errors)
+{
+	// This tests a crash that occured because we did not stop for fatal errors.
+	char const* text = R"(
+		contract C {
+			struct S {
+				ftring a;
+			}
+			S public s;
+			function s() s {
+			}
+		}
+	)";
+	CHECK_ERROR(text, DeclarationError, "Identifier not found or not unique");
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 }