Merge pull request #5765 from ethereum/unreachableCode

Warn about unreachable code.

Author: chriseth

Changelog.md

@@ -4,6 +4,7 @@ Language Features:
 
 
 Compiler Features:
+ * Control Flow Graph: Warn about unreachable code.
 
 
 Bugfixes:

libdevcore/Algorithms.h

@@ -75,4 +75,47 @@ class CycleDetector
 	V const* m_firstCycleVertex = nullptr;
 };
 
+/**
+ * Generic breadth first search.
+ *
+ * Example: Gather all (recursive) children in a graph starting at (and including) ``root``:
+ *
+ * Node const* root = ...;
+ * std::set<Node> allNodes = BreadthFirstSearch<Node>{{root}}.run([](Node const& _node, auto&& _addChild) {
+ *   // Potentially process ``_node``.
+ *   for (Node const& _child: _node.children())
+ *     // Potentially filter the children to be visited.
+ *     _addChild(_child);
+ * }).visited;
+ *
+ * Note that the order of the traversal is *non-deterministic* (the children are stored in a std::set of pointers).
+ */
+template<typename V>
+struct BreadthFirstSearch
+{
+	/// Runs the breadth first search. The verticesToTraverse member of the struct needs to be initialized.
+	/// @param _forEachChild is a callable of the form [...](V const& _node, auto&& _addChild) { ... }
+	/// that is called for each visited node and is supposed to call _addChild(childNode) for every child
+	/// node of _node.
+	template<typename ForEachChild>
+	BreadthFirstSearch& run(ForEachChild&& _forEachChild)
+	{
+		while (!verticesToTraverse.empty())
+		{
+			V const* v = *verticesToTraverse.begin();
+			verticesToTraverse.erase(verticesToTraverse.begin());
+			visited.insert(v);
+
+			_forEachChild(*v, [this](V const& _vertex) {
+				if (!visited.count(&_vertex))
+					verticesToTraverse.insert(&_vertex);
+			});
+		}
+		return *this;
+	}
+
+	std::set<V const*> verticesToTraverse;
+	std::set<V const*> visited{};
+};
+
 }

liblangutil/SourceLocation.h

@@ -49,6 +49,26 @@ struct SourceLocation
 
 	bool isEmpty() const { return start == -1 && end == -1; }
 
+	/// @returns the smallest SourceLocation that contains both @param _a and @param _b.
+	/// Assumes that @param _a and @param _b refer to the same source (exception: if the source of either one
+	/// is unset, the source of the other will be used for the result, even if that is unset as well).
+	/// Invalid start and end positions (with value of -1) are ignored (if start or end are -1 for both @param _a and
+	/// @param _b, then start resp. end of the result will be -1 as well).
+	static SourceLocation smallestCovering(SourceLocation _a, SourceLocation const& _b)
+	{
+		if (!_a.source)
+			_a.source = _b.source;
+
+		if (_a.start < 0)
+			_a.start = _b.start;
+		else if (_b.start >= 0 && _b.start < _a.start)
+			_a.start = _b.start;
+		if (_b.end > _a.end)
+			_a.end = _b.end;
+
+		return _a;
+	}
+
 	int start = -1;
 	int end = -1;
 	std::shared_ptr<CharStream> source;

libsolidity/analysis/ControlFlowAnalyzer.cpp

@@ -18,6 +18,7 @@
 #include <libsolidity/analysis/ControlFlowAnalyzer.h>
 
 #include <liblangutil/SourceLocation.h>
+#include <libdevcore/Algorithms.h>
 #include <boost/range/algorithm/sort.hpp>
 
 using namespace std;
@@ -36,6 +37,7 @@ bool ControlFlowAnalyzer::visit(FunctionDefinition const& _function)
 	{
 		auto const& functionFlow = m_cfg.functionFlow(_function);
 		checkUninitializedAccess(functionFlow.entry, functionFlow.exit);
+		checkUnreachable(functionFlow.entry, functionFlow.exit, functionFlow.revert);
 	}
 	return false;
 }
@@ -145,3 +147,35 @@ void ControlFlowAnalyzer::checkUninitializedAccess(CFGNode const* _entry, CFGNod
 		}
 	}
 }
+
+void ControlFlowAnalyzer::checkUnreachable(CFGNode const* _entry, CFGNode const* _exit, CFGNode const* _revert) const
+{
+	// collect all nodes reachable from the entry point
+	std::set<CFGNode const*> reachable = BreadthFirstSearch<CFGNode>{{_entry}}.run(
+		[](CFGNode const& _node, auto&& _addChild) {
+			for (CFGNode const* exit: _node.exits)
+				_addChild(*exit);
+		}
+	).visited;
+
+	// traverse all paths backwards from exit and revert
+	// and extract (valid) source locations of unreachable nodes into sorted set
+	std::set<SourceLocation> unreachable;
+	BreadthFirstSearch<CFGNode>{{_exit, _revert}}.run(
+		[&](CFGNode const& _node, auto&& _addChild) {
+			if (!reachable.count(&_node) && !_node.location.isEmpty())
+				unreachable.insert(_node.location);
+			for (CFGNode const* entry: _node.entries)
+				_addChild(*entry);
+		}
+	);
+
+	for (auto it = unreachable.begin(); it != unreachable.end();)
+	{
+		SourceLocation location = *it++;
+		// Extend the location, as long as the next location overlaps (unreachable is sorted).
+		for (; it != unreachable.end() && it->start <= location.end; ++it)
+			location.end = std::max(location.end, it->end);
+		m_errorReporter.warning(location, "Unreachable code.");
+	}
+}

libsolidity/analysis/ControlFlowAnalyzer.h

@@ -38,6 +38,9 @@ class ControlFlowAnalyzer: private ASTConstVisitor
 private:
 	/// Checks for uninitialized variable accesses in the control flow between @param _entry and @param _exit.
 	void checkUninitializedAccess(CFGNode const* _entry, CFGNode const* _exit) const;
+	/// Checks for unreachable code, i.e. code ending in @param _exit or @param _revert
+	/// that can not be reached from @param _entry.
+	void checkUnreachable(CFGNode const* _entry, CFGNode const* _exit, CFGNode const* _revert) const;
 
 	CFG const& m_cfg;
 	langutil::ErrorReporter& m_errorReporter;

libsolidity/analysis/ControlFlowBuilder.cpp

@@ -18,6 +18,7 @@
 #include <libsolidity/analysis/ControlFlowBuilder.h>
 
 using namespace dev;
+using namespace langutil;
 using namespace solidity;
 using namespace std;
 
@@ -53,6 +54,7 @@ bool ControlFlowBuilder::visit(BinaryOperation const& _operation)
 		case Token::Or:
 		case Token::And:
 		{
+			visitNode(_operation);
 			appendControlFlow(_operation.leftExpression());
 
 			auto nodes = splitFlow<2>();
@@ -62,14 +64,14 @@ bool ControlFlowBuilder::visit(BinaryOperation const& _operation)
 			return false;
 		}
 		default:
-			break;
+			return ASTConstVisitor::visit(_operation);
 	}
-	return ASTConstVisitor::visit(_operation);
 }
 
 bool ControlFlowBuilder::visit(Conditional const& _conditional)
 {
 	solAssert(!!m_currentNode, "");
+	visitNode(_conditional);
 
 	_conditional.condition().accept(*this);
 
@@ -86,6 +88,7 @@ bool ControlFlowBuilder::visit(Conditional const& _conditional)
 bool ControlFlowBuilder::visit(IfStatement const& _ifStatement)
 {
 	solAssert(!!m_currentNode, "");
+	visitNode(_ifStatement);
 
 	_ifStatement.condition().accept(*this);
 
@@ -106,6 +109,7 @@ bool ControlFlowBuilder::visit(IfStatement const& _ifStatement)
 bool ControlFlowBuilder::visit(ForStatement const& _forStatement)
 {
 	solAssert(!!m_currentNode, "");
+	visitNode(_forStatement);
 
 	if (_forStatement.initializationExpression())
 		_forStatement.initializationExpression()->accept(*this);
@@ -139,6 +143,7 @@ bool ControlFlowBuilder::visit(ForStatement const& _forStatement)
 bool ControlFlowBuilder::visit(WhileStatement const& _whileStatement)
 {
 	solAssert(!!m_currentNode, "");
+	visitNode(_whileStatement);
 
 	if (_whileStatement.isDoWhile())
 	{
@@ -183,28 +188,31 @@ bool ControlFlowBuilder::visit(WhileStatement const& _whileStatement)
 	return false;
 }
 
-bool ControlFlowBuilder::visit(Break const&)
+bool ControlFlowBuilder::visit(Break const& _break)
 {
 	solAssert(!!m_currentNode, "");
 	solAssert(!!m_breakJump, "");
+	visitNode(_break);
 	connect(m_currentNode, m_breakJump);
 	m_currentNode = newLabel();
 	return false;
 }
 
-bool ControlFlowBuilder::visit(Continue const&)
+bool ControlFlowBuilder::visit(Continue const& _continue)
 {
 	solAssert(!!m_currentNode, "");
 	solAssert(!!m_continueJump, "");
+	visitNode(_continue);
 	connect(m_currentNode, m_continueJump);
 	m_currentNode = newLabel();
 	return false;
 }
 
-bool ControlFlowBuilder::visit(Throw const&)
+bool ControlFlowBuilder::visit(Throw const& _throw)
 {
 	solAssert(!!m_currentNode, "");
 	solAssert(!!m_revertNode, "");
+	visitNode(_throw);
 	connect(m_currentNode, m_revertNode);
 	m_currentNode = newLabel();
 	return false;
@@ -232,6 +240,7 @@ bool ControlFlowBuilder::visit(FunctionCall const& _functionCall)
 		{
 			case FunctionType::Kind::Revert:
 				solAssert(!!m_revertNode, "");
+				visitNode(_functionCall);
 				_functionCall.expression().accept(*this);
 				ASTNode::listAccept(_functionCall.arguments(), *this);
 				connect(m_currentNode, m_revertNode);
@@ -241,6 +250,7 @@ bool ControlFlowBuilder::visit(FunctionCall const& _functionCall)
 			case FunctionType::Kind::Assert:
 			{
 				solAssert(!!m_revertNode, "");
+				visitNode(_functionCall);
 				_functionCall.expression().accept(*this);
 				ASTNode::listAccept(_functionCall.arguments(), *this);
 				connect(m_currentNode, m_revertNode);
@@ -314,6 +324,7 @@ bool ControlFlowBuilder::visit(Return const& _return)
 {
 	solAssert(!!m_currentNode, "");
 	solAssert(!!m_returnNode, "");
+	visitNode(_return);
 	if (_return.expression())
 	{
 		appendControlFlow(*_return.expression());
@@ -327,11 +338,12 @@ bool ControlFlowBuilder::visit(Return const& _return)
 	}
 	connect(m_currentNode, m_returnNode);
 	m_currentNode = newLabel();
-	return true;
+	return false;
 }
 
-bool ControlFlowBuilder::visit(FunctionTypeName const&)
+bool ControlFlowBuilder::visit(FunctionTypeName const& _functionTypeName)
 {
+	visitNode(_functionTypeName);
 	// Do not visit the parameters and return values of a function type name.
 	// We do not want to consider them as variable declarations for the control flow graph.
 	return false;
@@ -340,6 +352,7 @@ bool ControlFlowBuilder::visit(FunctionTypeName const&)
 bool ControlFlowBuilder::visit(InlineAssembly const& _inlineAssembly)
 {
 	solAssert(!!m_currentNode, "");
+	visitNode(_inlineAssembly);
 	for (auto const& ref: _inlineAssembly.annotation().externalReferences)
 	{
 		if (auto variableDeclaration = dynamic_cast<VariableDeclaration const*>(ref.second.declaration))
@@ -355,6 +368,7 @@ bool ControlFlowBuilder::visit(InlineAssembly const& _inlineAssembly)
 bool ControlFlowBuilder::visit(VariableDeclaration const& _variableDeclaration)
 {
 	solAssert(!!m_currentNode, "");
+	visitNode(_variableDeclaration);
 
 	m_currentNode->variableOccurrences.emplace_back(
 		_variableDeclaration,
@@ -382,6 +396,7 @@ bool ControlFlowBuilder::visit(VariableDeclaration const& _variableDeclaration)
 bool ControlFlowBuilder::visit(VariableDeclarationStatement const& _variableDeclarationStatement)
 {
 	solAssert(!!m_currentNode, "");
+	visitNode(_variableDeclarationStatement);
 
 	for (auto const& var: _variableDeclarationStatement.declarations())
 		if (var)
@@ -417,6 +432,7 @@ bool ControlFlowBuilder::visit(VariableDeclarationStatement const& _variableDecl
 bool ControlFlowBuilder::visit(Identifier const& _identifier)
 {
 	solAssert(!!m_currentNode, "");
+	visitNode(_identifier);
 
 	if (auto const* variableDeclaration = dynamic_cast<VariableDeclaration const*>(_identifier.annotation().referencedDeclaration))
 		m_currentNode->variableOccurrences.emplace_back(
@@ -430,7 +446,12 @@ bool ControlFlowBuilder::visit(Identifier const& _identifier)
 	return true;
 }
 
-
+bool ControlFlowBuilder::visitNode(ASTNode const& _node)
+{
+	solAssert(!!m_currentNode, "");
+	m_currentNode->location = langutil::SourceLocation::smallestCovering(m_currentNode->location, _node.location());
+	return true;
+}
 
 void ControlFlowBuilder::appendControlFlow(ASTNode const& _node)
 {

libsolidity/analysis/ControlFlowBuilder.h

@@ -66,6 +66,11 @@ class ControlFlowBuilder: private ASTConstVisitor
 	bool visit(VariableDeclarationStatement const& _variableDeclarationStatement) override;
 	bool visit(Identifier const& _identifier) override;
 
+protected:
+	bool visitNode(ASTNode const&) override;
+
+private:
+
 	/// Appends the control flow of @a _node to the current control flow.
 	void appendControlFlow(ASTNode const& _node);
 
@@ -77,9 +82,6 @@ class ControlFlowBuilder: private ASTConstVisitor
 	/// Creates an arc from @a _from to @a _to.
 	static void connect(CFGNode* _from, CFGNode* _to);
 
-
-private:
-
 	/// Splits the control flow starting at the current node into n paths.
 	/// m_currentNode is set to nullptr and has to be set manually or
 	/// using mergeFlow later.

libsolidity/analysis/ControlFlowGraph.h

@@ -20,6 +20,7 @@
 #include <libsolidity/ast/AST.h>
 #include <libsolidity/ast/ASTVisitor.h>
 #include <liblangutil/ErrorReporter.h>
+#include <liblangutil/SourceLocation.h>
 
 #include <map>
 #include <memory>
@@ -98,6 +99,8 @@ struct CFGNode
 
 	/// Variable occurrences in the node.
 	std::vector<VariableOccurrence> variableOccurrences;
+	// Source location of this control flow block.
+	langutil::SourceLocation location;
 };
 
 /** Describes the control flow of a function. */

test/libsolidity/syntaxTests/controlFlow/storageReturn/dowhile_err.sol

@@ -46,7 +46,11 @@ contract C {
 }
 // ----
 // TypeError: (87-98): This variable is of storage pointer type and can be returned without prior assignment.
+// Warning: (146-151): Unreachable code.
+// Warning: (169-174): Unreachable code.
 // TypeError: (223-234): This variable is of storage pointer type and can be returned without prior assignment.
+// Warning: (316-321): Unreachable code.
 // TypeError: (440-451): This variable is of storage pointer type and can be returned without prior assignment.
 // TypeError: (654-665): This variable is of storage pointer type and can be returned without prior assignment.
 // TypeError: (871-882): This variable is of storage pointer type and can be returned without prior assignment.
+// Warning: (933-938): Unreachable code.

test/libsolidity/syntaxTests/controlFlow/storageReturn/dowhile_fine.sol

@@ -29,3 +29,4 @@ contract C {
     }
 }
 // ----
+// Warning: (567-572): Unreachable code.

test/libsolidity/syntaxTests/controlFlow/uninitializedAccess/always_revert.sol

@@ -5,4 +5,6 @@ contract C {
         revert();
         b;
     }
-}
+}
+// ----
+// Warning: (125-126): Unreachable code.

test/libsolidity/syntaxTests/controlFlow/uninitializedAccess/unreachable.sol

@@ -8,3 +8,4 @@ contract C {
     }
 }
 // ----
+// Warning: (112-135): Unreachable code.

test/libsolidity/syntaxTests/controlFlow/unreachableCode/comment_fine.sol

@@ -0,0 +1,6 @@
+contract C {
+    function f() public pure {
+        return;
+        // unreachable comment
+    }
+}