Warn about unreachable code.

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 @@ private:
 	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 @@ public:
 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 @@ private:
 	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 @@ private:
 	/// 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
+    }
+}

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

@@ -0,0 +1,8 @@
+contract C {
+    function f() public pure {
+        if (false) {
+            return; // unreachable, but not yet detected
+        }
+        return;
+    }
+}

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

@@ -0,0 +1,12 @@
+contract C {
+    function f() public pure {
+        do {
+            uint a = 42; a;
+            continue;
+            return; // this is unreachable
+        } while(false);
+        return; // this is still reachable
+    }
+}
+// ----
+// Warning: (119-126): Unreachable code.

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

@@ -0,0 +1,8 @@
+contract C {
+    function f() public pure returns (uint) {
+        return 0;
+        return 0;
+    }
+}
+// ----
+// Warning: (85-93): Unreachable code.

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

@@ -0,0 +1,8 @@
+contract C {
+    function f() public pure {
+        revert();
+        revert();
+    }
+}
+// ----
+// Warning: (70-78): Unreachable code.

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

@@ -0,0 +1,12 @@
+contract C {
+    function f() public pure {
+        for (uint a = 0; a < 1; a++) {
+            break;
+            uint b = 42; b;
+        }
+        return;
+    }
+}
+// ----
+// Warning: (76-79): Unreachable code.
+// Warning: (114-128): Unreachable code.

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

@@ -0,0 +1,12 @@
+contract C {
+    function f(bool c) public pure {
+        if (c) {
+            return;
+        } else {
+            return;
+        }
+        return; // unreachable
+    }
+}
+// ----
+// Warning: (142-149): Unreachable code.

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

@@ -0,0 +1,8 @@
+contract C {
+    function f() public pure {
+        revert();
+        uint a = 0; a;
+    }
+}
+// ----
+// Warning: (70-83): Unreachable code.

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

@@ -0,0 +1,8 @@
+contract C {
+    function f() public pure {
+        revert();
+        for(int i = 0; i < 3; i++) { f(); }
+    }
+}
+// ----
+// Warning: (70-105): Unreachable code.

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

@@ -0,0 +1,12 @@
+contract C {
+    function f() public pure {
+        uint a = 0;
+        while (a < 100) {
+            a++;
+            break;
+            a--;
+        }
+    }
+}
+// ----
+// Warning: (138-141): Unreachable code.

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

@@ -0,0 +1,11 @@
+contract C {
+    function f() public pure {
+        while(true) {
+            continue;
+            return;
+        }
+        return; // this is unreachable as well, but currently undetected (needs to consider constant condition "true")
+    }
+}
+// ----
+// Warning: (100-107): Unreachable code.

test/libsolidity/syntaxTests/parsing/for_loop_simple_initexpr.sol

@@ -7,6 +7,8 @@ contract test {
     }
 }
 // ----
+// Warning: (103-106): Unreachable code.
+// Warning: (144-152): Unreachable code.
 // Warning: (33-42): Unused function parameter. Remove or comment out the variable name to silence this warning.
 // Warning: (122-131): Unused local variable.
 // Warning: (20-169): Function state mutability can be restricted to pure

test/libsolidity/syntaxTests/parsing/for_loop_simple_noexpr.sol

@@ -7,6 +7,7 @@ contract test {
         }
     }
 // ----
+// Warning: (144-152): Unreachable code.
 // Warning: (37-46): Unused function parameter. Remove or comment out the variable name to silence this warning.
 // Warning: (122-131): Unused local variable.
 // Warning: (24-177): Function state mutability can be restricted to pure

test/libsolidity/syntaxTests/parsing/for_loop_vardef_initexpr.sol

@@ -6,6 +6,8 @@ contract test {
     }
 }
 // ----
+// Warning: (89-92): Unreachable code.
+// Warning: (130-138): Unreachable code.
 // Warning: (33-42): Unused function parameter. Remove or comment out the variable name to silence this warning.
 // Warning: (108-117): Unused local variable.
 // Warning: (20-155): Function state mutability can be restricted to pure

test/libsolidity/syntaxTests/parsing/while_loop.sol

@@ -5,3 +5,4 @@ contract test {
     }
 }
 // ----
+// Warning: (105-113): Unreachable code.