Merge pull request #2687 from ethereum/show-unimplemented-funcs

Show unimplemented function if trying to instantiate an abstract class

8 files changed, 46 insertions, 24 deletions

diff --git a/Changelog.md b/Changelog.md
index 7d3369f3..f7eb8070 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -1,6 +1,7 @@
 ### 0.4.15 (unreleased)
 
 Features:
+ * Type Checker: Show unimplemented function if trying to instantiate an abstract class.
 
 Bugfixes:
  * Code Generator: ``.delegatecall()`` should always return execution outcome.
diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp
index 5fe03aa6..ce4baa3f 100644
--- a/libsolidity/analysis/TypeChecker.cpp
+++ b/libsolidity/analysis/TypeChecker.cpp
@@ -112,8 +112,6 @@ bool TypeChecker::visit(ContractDefinition const& _contract)
 					m_errorReporter.typeError(fallbackFunction->returnParameterList()->location(), "Fallback function cannot return values.");
 			}
 		}
-		if (!function->isImplemented())
-			_contract.annotation().isFullyImplemented = false;
 	}
 
 	for (auto const& n: _contract.subNodes())
@@ -188,7 +186,6 @@ void TypeChecker::checkContractAbstractFunctions(ContractDefinition const& _cont
 	using FunTypeAndFlag = std::pair<FunctionTypePointer, bool>;
 	map<string, vector<FunTypeAndFlag>> functions;
 
-	bool allBaseConstructorsImplemented = true;
 	// Search from base to derived
 	for (ContractDefinition const* contract: boost::adaptors::reverse(_contract.annotation().linearizedBaseContracts))
 		for (FunctionDefinition const* function: contract->definedFunctions())
@@ -199,7 +196,7 @@ void TypeChecker::checkContractAbstractFunctions(ContractDefinition const& _cont
 				if (!function->isImplemented())
 					// Base contract's constructor is not fully implemented, no way to get
 					// out of this.
-					allBaseConstructorsImplemented = false;
+					_contract.annotation().unimplementedFunctions.push_back(function);
 				continue;
 			}
 			auto& overloads = functions[function->name()];
@@ -219,16 +216,15 @@ void TypeChecker::checkContractAbstractFunctions(ContractDefinition const& _cont
 				it->second = true;
 		}
 
-	if (!allBaseConstructorsImplemented)
-		_contract.annotation().isFullyImplemented = false;
-
 	// Set to not fully implemented if at least one flag is false.
 	for (auto const& it: functions)
 		for (auto const& funAndFlag: it.second)
 			if (!funAndFlag.second)
 			{
-				_contract.annotation().isFullyImplemented = false;
-				return;
+				FunctionDefinition const* function = dynamic_cast<FunctionDefinition const*>(&funAndFlag.first->declaration());
+				solAssert(function, "");
+				_contract.annotation().unimplementedFunctions.push_back(function);
+				break;
 			}
 }
 
@@ -266,7 +262,8 @@ void TypeChecker::checkContractAbstractConstructors(ContractDefinition const& _c
 		}
 	}
 	if (!argumentsNeeded.empty())
-		_contract.annotation().isFullyImplemented = false;
+		for (ContractDefinition const* contract: argumentsNeeded)
+			_contract.annotation().unimplementedFunctions.push_back(contract->constructor());
 }
 
 void TypeChecker::checkContractIllegalOverrides(ContractDefinition const& _contract)
@@ -1523,8 +1520,15 @@ void TypeChecker::endVisit(NewExpression const& _newExpression)
 
 		if (!contract)
 			m_errorReporter.fatalTypeError(_newExpression.location(), "Identifier is not a contract.");
-		if (!contract->annotation().isFullyImplemented)
-			m_errorReporter.typeError(_newExpression.location(), "Trying to create an instance of an abstract contract.");
+		if (!contract->annotation().unimplementedFunctions.empty())
+			m_errorReporter.typeError(
+				_newExpression.location(),
+				SecondarySourceLocation().append(
+					"Missing implementation:",
+					contract->annotation().unimplementedFunctions.front()->location()
+				),
+				"Trying to create an instance of an abstract contract."
+			);
 		if (!contract->constructorIsPublic())
 			m_errorReporter.typeError(_newExpression.location(), "Contract with internal constructor cannot be created directly.");
 
diff --git a/libsolidity/ast/ASTAnnotations.h b/libsolidity/ast/ASTAnnotations.h
index 45a6dd1a..ecddbe21 100644
--- a/libsolidity/ast/ASTAnnotations.h
+++ b/libsolidity/ast/ASTAnnotations.h
@@ -79,8 +79,9 @@ struct TypeDeclarationAnnotation: ASTAnnotation
 
 struct ContractDefinitionAnnotation: TypeDeclarationAnnotation, DocumentedAnnotation
 {
-	/// Whether all functions are implemented.
-	bool isFullyImplemented = true;
+	/// List of functions without a body. Can also contain functions from base classes,
+	/// especially constructors.
+	std::vector<FunctionDefinition const*> unimplementedFunctions;
 	/// List of all (direct and indirect) base contracts in order from derived to
 	/// base, including the contract itself.
 	std::vector<ContractDefinition const*> linearizedBaseContracts;
diff --git a/libsolidity/ast/ASTJsonConverter.cpp b/libsolidity/ast/ASTJsonConverter.cpp
index eda70b63..e4a602cb 100644
--- a/libsolidity/ast/ASTJsonConverter.cpp
+++ b/libsolidity/ast/ASTJsonConverter.cpp
@@ -253,7 +253,7 @@ bool ASTJsonConverter::visit(ContractDefinition const& _node)
 		make_pair("name", _node.name()),
 		make_pair("documentation", _node.documentation() ? Json::Value(*_node.documentation()) : Json::nullValue),
 		make_pair("contractKind", contractKind(_node.contractKind())),
-		make_pair("fullyImplemented", _node.annotation().isFullyImplemented),
+		make_pair("fullyImplemented", _node.annotation().unimplementedFunctions.empty()),
 		make_pair("linearizedBaseContracts", getContainerIds(_node.annotation().linearizedBaseContracts)),
 		make_pair("baseContracts", toJson(_node.baseContracts())),
 		make_pair("contractDependencies", getContainerIds(_node.annotation().contractDependencies)),
diff --git a/libsolidity/interface/CompilerStack.cpp b/libsolidity/interface/CompilerStack.cpp
index 9689b700..7c66a843 100644
--- a/libsolidity/interface/CompilerStack.cpp
+++ b/libsolidity/interface/CompilerStack.cpp
@@ -639,7 +639,7 @@ void CompilerStack::compileContract(
 {
 	if (
 		_compiledContracts.count(&_contract) ||
-		!_contract.annotation().isFullyImplemented ||
+		!_contract.annotation().unimplementedFunctions.empty() ||
 		!_contract.constructorIsPublic()
 	)
 		return;
diff --git a/libsolidity/interface/ErrorReporter.cpp b/libsolidity/interface/ErrorReporter.cpp
index f9ef4ceb..e6171756 100644
--- a/libsolidity/interface/ErrorReporter.cpp
+++ b/libsolidity/interface/ErrorReporter.cpp
@@ -151,6 +151,16 @@ void ErrorReporter::syntaxError(SourceLocation const& _location, string const& _
 	);
 }
 
+void ErrorReporter::typeError(SourceLocation const& _location, SecondarySourceLocation const& _secondaryLocation, string const& _description)
+{
+	error(
+		Error::Type::TypeError,
+		_location,
+		_secondaryLocation,
+		_description
+	);
+}
+
 void ErrorReporter::typeError(SourceLocation const& _location, string const& _description)
 {
 	error(
diff --git a/libsolidity/interface/ErrorReporter.h b/libsolidity/interface/ErrorReporter.h
index 8b066a3e..42b0c8b6 100644
--- a/libsolidity/interface/ErrorReporter.h
+++ b/libsolidity/interface/ErrorReporter.h
@@ -73,6 +73,12 @@ public:
 
 	void syntaxError(SourceLocation const& _location, std::string const& _description);
 
+	void typeError(
+		SourceLocation const& _location,
+		SecondarySourceLocation const& _secondaryLocation,
+		std::string const& _description
+	);
+
 	void typeError(SourceLocation const& _location, std::string const& _description);
 
 	void fatalTypeError(SourceLocation const& _location, std::string const& _description);
diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp
index 4d367d3a..37bbc2eb 100644
--- a/test/libsolidity/SolidityNameAndTypeResolution.cpp
+++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp
@@ -624,7 +624,7 @@ BOOST_AUTO_TEST_CASE(function_no_implementation)
 	std::vector<ASTPointer<ASTNode>> nodes = sourceUnit->nodes();
 	ContractDefinition* contract = dynamic_cast<ContractDefinition*>(nodes[1].get());
 	BOOST_REQUIRE(contract);
-	BOOST_CHECK(!contract->annotation().isFullyImplemented);
+	BOOST_CHECK(!contract->annotation().unimplementedFunctions.empty());
 	BOOST_CHECK(!contract->definedFunctions()[0]->isImplemented());
 }
 
@@ -640,10 +640,10 @@ BOOST_AUTO_TEST_CASE(abstract_contract)
 	ContractDefinition* base = dynamic_cast<ContractDefinition*>(nodes[1].get());
 	ContractDefinition* derived = dynamic_cast<ContractDefinition*>(nodes[2].get());
 	BOOST_REQUIRE(base);
-	BOOST_CHECK(!base->annotation().isFullyImplemented);
+	BOOST_CHECK(!base->annotation().unimplementedFunctions.empty());
 	BOOST_CHECK(!base->definedFunctions()[0]->isImplemented());
 	BOOST_REQUIRE(derived);
-	BOOST_CHECK(derived->annotation().isFullyImplemented);
+	BOOST_CHECK(derived->annotation().unimplementedFunctions.empty());
 	BOOST_CHECK(derived->definedFunctions()[0]->isImplemented());
 }
 
@@ -659,9 +659,9 @@ BOOST_AUTO_TEST_CASE(abstract_contract_with_overload)
 	ContractDefinition* base = dynamic_cast<ContractDefinition*>(nodes[1].get());
 	ContractDefinition* derived = dynamic_cast<ContractDefinition*>(nodes[2].get());
 	BOOST_REQUIRE(base);
-	BOOST_CHECK(!base->annotation().isFullyImplemented);
+	BOOST_CHECK(!base->annotation().unimplementedFunctions.empty());
 	BOOST_REQUIRE(derived);
-	BOOST_CHECK(!derived->annotation().isFullyImplemented);
+	BOOST_CHECK(!derived->annotation().unimplementedFunctions.empty());
 }
 
 BOOST_AUTO_TEST_CASE(create_abstract_contract)
@@ -693,7 +693,7 @@ BOOST_AUTO_TEST_CASE(abstract_contract_constructor_args_optional)
 	BOOST_CHECK_EQUAL(nodes.size(), 4);
 	ContractDefinition* derived = dynamic_cast<ContractDefinition*>(nodes[3].get());
 	BOOST_REQUIRE(derived);
-	BOOST_CHECK(!derived->annotation().isFullyImplemented);
+	BOOST_CHECK(!derived->annotation().unimplementedFunctions.empty());
 }
 
 BOOST_AUTO_TEST_CASE(abstract_contract_constructor_args_not_provided)
@@ -712,7 +712,7 @@ BOOST_AUTO_TEST_CASE(abstract_contract_constructor_args_not_provided)
 	BOOST_CHECK_EQUAL(nodes.size(), 4);
 	ContractDefinition* derived = dynamic_cast<ContractDefinition*>(nodes[3].get());
 	BOOST_REQUIRE(derived);
-	BOOST_CHECK(!derived->annotation().isFullyImplemented);
+	BOOST_CHECK(!derived->annotation().unimplementedFunctions.empty());
 }
 
 BOOST_AUTO_TEST_CASE(redeclare_implemented_abstract_function_as_abstract)
@@ -738,7 +738,7 @@ BOOST_AUTO_TEST_CASE(implement_abstract_via_constructor)
 	BOOST_CHECK_EQUAL(nodes.size(), 3);
 	ContractDefinition* derived = dynamic_cast<ContractDefinition*>(nodes[2].get());
 	BOOST_REQUIRE(derived);
-	BOOST_CHECK(!derived->annotation().isFullyImplemented);
+	BOOST_CHECK(!derived->annotation().unimplementedFunctions.empty());
 }
 
 BOOST_AUTO_TEST_CASE(function_canonical_signature)