Merge pull request #1733 from ethereum/selfReferentialConstant

Detect cyclic dependencies between constants.

6 files changed, 231 insertions, 8 deletions

diff --git a/Changelog.md b/Changelog.md
index 5eb1e401..8e11775b 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -11,6 +11,7 @@ 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: Detect cyclic dependencies between constants.
  * Type system: Disallow arrays with negative length.
  * Type system: Fix a crash related to invalid binary operators.
  * Type system: Disallow ``var`` declaration with empty tuple type.
diff --git a/libsolidity/analysis/PostTypeChecker.cpp b/libsolidity/analysis/PostTypeChecker.cpp
new file mode 100644
index 00000000..cae77c74
--- /dev/null
+++ b/libsolidity/analysis/PostTypeChecker.cpp
@@ -0,0 +1,108 @@
+/*
+	This file is part of solidity.
+
+	solidity is free software: you can redistribute it and/or modify
+	it under the terms of the GNU General Public License as published by
+	the Free Software Foundation, either version 3 of the License, or
+	(at your option) any later version.
+
+	solidity is distributed in the hope that it will be useful,
+	but WITHOUT ANY WARRANTY; without even the implied warranty of
+	MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+	GNU General Public License for more details.
+
+	You should have received a copy of the GNU General Public License
+	along with solidity.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include <libsolidity/analysis/PostTypeChecker.h>
+#include <libsolidity/ast/AST.h>
+#include <libsolidity/analysis/SemVerHandler.h>
+#include <libsolidity/interface/Version.h>
+
+#include <boost/range/adaptor/map.hpp>
+
+#include <memory>
+
+using namespace std;
+using namespace dev;
+using namespace dev::solidity;
+
+
+bool PostTypeChecker::check(ASTNode const& _astRoot)
+{
+	_astRoot.accept(*this);
+	return Error::containsOnlyWarnings(m_errors);
+}
+
+void PostTypeChecker::typeError(SourceLocation const& _location, std::string const& _description)
+{
+	auto err = make_shared<Error>(Error::Type::TypeError);
+	*err <<
+		errinfo_sourceLocation(_location) <<
+		errinfo_comment(_description);
+
+	m_errors.push_back(err);
+}
+
+bool PostTypeChecker::visit(ContractDefinition const&)
+{
+	solAssert(!m_currentConstVariable, "");
+	solAssert(m_constVariableDependencies.empty(), "");
+	return true;
+}
+
+void PostTypeChecker::endVisit(ContractDefinition const&)
+{
+	solAssert(!m_currentConstVariable, "");
+	for (auto declaration: m_constVariables)
+		if (auto identifier = findCycle(declaration))
+			typeError(declaration->location(), "The value of the constant " + declaration->name() + " has a cyclic dependency via " + identifier->name() + ".");
+}
+
+bool PostTypeChecker::visit(VariableDeclaration const& _variable)
+{
+	solAssert(!m_currentConstVariable, "");
+	if (_variable.isConstant())
+	{
+		m_currentConstVariable = &_variable;
+		m_constVariables.push_back(&_variable);
+	}
+	return true;
+}
+
+void PostTypeChecker::endVisit(VariableDeclaration const& _variable)
+{
+	if (_variable.isConstant())
+	{
+		solAssert(m_currentConstVariable == &_variable, "");
+		m_currentConstVariable = nullptr;
+	}
+}
+
+bool PostTypeChecker::visit(Identifier const& _identifier)
+{
+	if (m_currentConstVariable)
+		if (auto var = dynamic_cast<VariableDeclaration const*>(_identifier.annotation().referencedDeclaration))
+			if (var->isConstant())
+				m_constVariableDependencies[m_currentConstVariable].insert(var);
+	return true;
+}
+
+VariableDeclaration const* PostTypeChecker::findCycle(
+	VariableDeclaration const* _startingFrom,
+	set<VariableDeclaration const*> const& _seen
+)
+{
+	if (_seen.count(_startingFrom))
+		return _startingFrom;
+	else if (m_constVariableDependencies.count(_startingFrom))
+	{
+		set<VariableDeclaration const*> seen(_seen);
+		seen.insert(_startingFrom);
+		for (auto v: m_constVariableDependencies[_startingFrom])
+			if (findCycle(v, seen))
+				return v;
+	}
+	return nullptr;
+}
diff --git a/libsolidity/analysis/PostTypeChecker.h b/libsolidity/analysis/PostTypeChecker.h
new file mode 100644
index 00000000..8774f413
--- /dev/null
+++ b/libsolidity/analysis/PostTypeChecker.h
@@ -0,0 +1,69 @@
+/*
+	This file is part of solidity.
+
+	solidity is free software: you can redistribute it and/or modify
+	it under the terms of the GNU General Public License as published by
+	the Free Software Foundation, either version 3 of the License, or
+	(at your option) any later version.
+
+	solidity is distributed in the hope that it will be useful,
+	but WITHOUT ANY WARRANTY; without even the implied warranty of
+	MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+	GNU General Public License for more details.
+
+	You should have received a copy of the GNU General Public License
+	along with solidity.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#pragma once
+
+#include <libsolidity/analysis/TypeChecker.h>
+#include <libsolidity/ast/Types.h>
+#include <libsolidity/ast/ASTAnnotations.h>
+#include <libsolidity/ast/ASTForward.h>
+#include <libsolidity/ast/ASTVisitor.h>
+
+namespace dev
+{
+namespace solidity
+{
+
+/**
+ * This module performs analyses on the AST that are done after type checking and assignments of types:
+ *  - whether there are circular references in constant state variables
+ * @TODO factor out each use-case into an individual class (but do the traversal only once)
+ */
+class PostTypeChecker: private ASTConstVisitor
+{
+public:
+	/// @param _errors the reference to the list of errors and warnings to add them found during type checking.
+	PostTypeChecker(ErrorList& _errors): m_errors(_errors) {}
+
+	bool check(ASTNode const& _astRoot);
+
+private:
+	/// Adds a new error to the list of errors.
+	void typeError(SourceLocation const& _location, std::string const& _description);
+
+	virtual bool visit(ContractDefinition const& _contract) override;
+	virtual void endVisit(ContractDefinition const& _contract) override;
+
+	virtual bool visit(VariableDeclaration const& _declaration) override;
+	virtual void endVisit(VariableDeclaration const& _declaration) override;
+
+	virtual bool visit(Identifier const& _identifier) override;
+
+	VariableDeclaration const* findCycle(
+		VariableDeclaration const* _startingFrom,
+		std::set<VariableDeclaration const*> const& _seen = {}
+	);
+
+	ErrorList& m_errors;
+
+	VariableDeclaration const* m_currentConstVariable = nullptr;
+	std::vector<VariableDeclaration const*> m_constVariables; ///< Required for determinism.
+	std::map<VariableDeclaration const*, std::set<VariableDeclaration const*>> m_constVariableDependencies;
+};
+
+}
+}
diff --git a/libsolidity/analysis/StaticAnalyzer.h b/libsolidity/analysis/StaticAnalyzer.h
index b6cf783e..0cb961bd 100644
--- a/libsolidity/analysis/StaticAnalyzer.h
+++ b/libsolidity/analysis/StaticAnalyzer.h
@@ -36,6 +36,9 @@ namespace solidity
 
 /**
  * The module that performs static analysis on the AST.
+ * In this context, static analysis is anything that can produce warnings which can help
+ * programmers write cleaner code. For every warning generated eher, it has to be possible to write
+ * equivalent code that does generate the warning.
  */
 class StaticAnalyzer: private ASTConstVisitor
 {
diff --git a/libsolidity/interface/CompilerStack.cpp b/libsolidity/interface/CompilerStack.cpp
index 9d8d872f..594efb1e 100644
--- a/libsolidity/interface/CompilerStack.cpp
+++ b/libsolidity/interface/CompilerStack.cpp
@@ -34,6 +34,7 @@
 #include <libsolidity/analysis/TypeChecker.h>
 #include <libsolidity/analysis/DocStringAnalyser.h>
 #include <libsolidity/analysis/StaticAnalyzer.h>
+#include <libsolidity/analysis/PostTypeChecker.h>
 #include <libsolidity/analysis/SyntaxChecker.h>
 #include <libsolidity/codegen/Compiler.h>
 #include <libsolidity/interface/InterfaceHandler.h>
@@ -219,6 +220,14 @@ bool CompilerStack::parse()
 
 	if (noErrors)
 	{
+		PostTypeChecker postTypeChecker(m_errors);
+		for (Source const* source: m_sourceOrder)
+			if (!postTypeChecker.check(*source->ast))
+				noErrors = false;
+	}
+
+	if (noErrors)
+	{
 		StaticAnalyzer staticAnalyzer(m_errors);
 		for (Source const* source: m_sourceOrder)
 			if (!staticAnalyzer.analyze(*source->ast))
diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp
index 02d81d14..3d82fc70 100644
--- a/test/libsolidity/SolidityNameAndTypeResolution.cpp
+++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp
@@ -20,19 +20,23 @@
  * Unit tests for the name and type resolution of the solidity parser.
  */
 
-#include <string>
+#include <test/libsolidity/ErrorCheck.h>
+
+#include <test/TestHelper.h>
 
-#include <libdevcore/SHA3.h>
 #include <libsolidity/parsing/Scanner.h>
 #include <libsolidity/parsing/Parser.h>
 #include <libsolidity/analysis/NameAndTypeResolver.h>
 #include <libsolidity/analysis/StaticAnalyzer.h>
+#include <libsolidity/analysis/PostTypeChecker.h>
 #include <libsolidity/analysis/SyntaxChecker.h>
 #include <libsolidity/interface/Exceptions.h>
 #include <libsolidity/analysis/GlobalContext.h>
 #include <libsolidity/analysis/TypeChecker.h>
-#include "../TestHelper.h"
-#include "ErrorCheck.h"
+
+#include <libdevcore/SHA3.h>
+
+#include <string>
 
 using namespace std;
 
@@ -93,10 +97,11 @@ parseAnalyseAndReturnError(string const& _source, bool _reportWarnings = false,
 					BOOST_CHECK(success || !errors.empty());
 				}
 		if (success)
-		{
-			StaticAnalyzer staticAnalyzer(errors);
-			staticAnalyzer.analyze(*sourceUnit);
-		}
+			if (!PostTypeChecker(errors).check(*sourceUnit))
+				success = false;
+		if (success)
+			if (!StaticAnalyzer(errors).analyze(*sourceUnit))
+				success = false;
 		if (errors.size() > 1 && !_allowMultipleErrors)
 			BOOST_FAIL("Multiple errors found");
 		for (auto const& currentError: errors)
@@ -5180,6 +5185,34 @@ BOOST_AUTO_TEST_CASE(address_methods)
 	CHECK_SUCCESS(text);
 }
 
+BOOST_AUTO_TEST_CASE(cyclic_dependency_for_constants)
+{
+	char const* text = R"(
+		contract C {
+			uint constant a = a;
+		}
+	)";
+	CHECK_ERROR(text, TypeError, "cyclic dependency via a");
+	text = R"(
+		contract C {
+			uint constant a = b * c;
+			uint constant b = 7;
+			uint constant c = b + uint(sha3(d));
+			uint constant d = 2 + a;
+		}
+	)";
+	CHECK_ERROR_ALLOW_MULTI(text, TypeError, "a has a cyclic dependency via c");
+	text = R"(
+		contract C {
+			uint constant a = b * c;
+			uint constant b = 7;
+			uint constant c = 4 + uint(sha3(d));
+			uint constant d = 2 + b;
+		}
+	)";
+	CHECK_SUCCESS(text);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 }