Merge pull request #11484 from ethereum/fix-unreachable-code-anaysis

Fix revert pruner modifying function flows wrong

Author: chriseth

docs/control-structures.rst

@@ -44,6 +44,7 @@ this nonsensical example::
     // SPDX-License-Identifier: GPL-3.0
     pragma solidity >=0.4.22 <0.9.0;
 
+    // This will report a warning
     contract C {
         function g(uint a) public pure returns (uint ret) { return a + f(); }
         function f() internal pure returns (uint ret) { return g(7) + f(); }

libsolidity/analysis/ControlFlowBuilder.cpp

@@ -260,37 +260,51 @@ bool ControlFlowBuilder::visit(PlaceholderStatement const&)
 
 bool ControlFlowBuilder::visit(FunctionCall const& _functionCall)
 {
+	solAssert(!!m_revertNode, "");
 	solAssert(!!m_currentNode, "");
 	solAssert(!!_functionCall.expression().annotation().type, "");
 
 	if (auto functionType = dynamic_cast<FunctionType const*>(_functionCall.expression().annotation().type))
 		switch (functionType->kind())
 		{
 			case FunctionType::Kind::Revert:
-				solAssert(!!m_revertNode, "");
 				visitNode(_functionCall);
 				_functionCall.expression().accept(*this);
 				ASTNode::listAccept(_functionCall.arguments(), *this);
+
 				connect(m_currentNode, m_revertNode);
+
 				m_currentNode = newLabel();
 				return false;
 			case FunctionType::Kind::Require:
 			case FunctionType::Kind::Assert:
 			{
-				solAssert(!!m_revertNode, "");
 				visitNode(_functionCall);
 				_functionCall.expression().accept(*this);
 				ASTNode::listAccept(_functionCall.arguments(), *this);
+
 				connect(m_currentNode, m_revertNode);
+
 				auto nextNode = newLabel();
+
 				connect(m_currentNode, nextNode);
 				m_currentNode = nextNode;
 				return false;
 			}
 			case FunctionType::Kind::Internal:
 			{
+				visitNode(_functionCall);
+				_functionCall.expression().accept(*this);
+				ASTNode::listAccept(_functionCall.arguments(), *this);
+
 				m_currentNode->functionCalls.emplace_back(&_functionCall);
-				break;
+
+				auto nextNode = newLabel();
+
+				connect(m_currentNode, nextNode);
+				m_currentNode = nextNode;
+
+				return false;
 			}
 			default:
 				break;

libsolidity/analysis/ControlFlowRevertPruner.cpp

@@ -20,6 +20,8 @@
 
 #include <libsolutil/Algorithms.h>
 
+#include <range/v3/algorithm/remove.hpp>
+
 
 namespace solidity::frontend
 {
@@ -192,7 +194,11 @@ void ControlFlowRevertPruner::modifyFunctionFlows()
 							// change anymore, we treat all "unknown" states as
 							// "reverting", since they can only be caused by
 							// recursion.
+							for (CFGNode * node: _node->exits)
+								ranges::remove(node->entries, _node);
+
 							_node->exits = {functionFlow.revert};
+							functionFlow.revert->entries.push_back(_node);
 							return;
 						default:
 							break;

test/libsolidity/smtCheckerTests/functions/function_call_does_not_clear_local_vars.sol

@@ -10,3 +10,4 @@ contract C {
 // ====
 // SMTEngine: all
 // ----
+// Warning 5740: (122-136): Unreachable code.

test/libsolidity/syntaxTests/multiVariableDeclaration/multiSingleVariableDeclaration.sol

@@ -5,3 +5,4 @@ contract C {
   }
 }
 // ----
+// Warning 5740: (78-79): Unreachable code.

test/libsolidity/syntaxTests/multiVariableDeclaration/multiVariableDeclarationSimple.sol

@@ -10,4 +10,5 @@ contract C {
   function h() internal pure returns (bytes memory, string storage s) { s = s; }
 }
 // ----
+// Warning 5740: (111-115): Unreachable code.
 // Warning 6321: (250-262): Unnamed return variable can remain unassigned. Add an explicit return with value to all non-reverting code paths or name the variable.

test/libsolidity/syntaxTests/nameAndTypeResolution/001_name_references.sol

@@ -3,4 +3,5 @@ contract test {
     function f(uint256) public returns (uint out) { f(variable); test; out; }
 }
 // ----
+// Warning 5740: (103-112): Unreachable code.
 // Warning 6133: (103-107): Statement has no effect.

test/libsolidity/syntaxTests/viewPureChecker/call_internal_functions_success.sol

@@ -5,3 +5,9 @@ contract C {
     function i() payable public { i(); h(); g(); f(); }
 }
 // ----
+// Warning 5740: (102-105): Unreachable code.
+// Warning 5740: (140-143): Unreachable code.
+// Warning 5740: (145-148): Unreachable code.
+// Warning 5740: (191-194): Unreachable code.
+// Warning 5740: (196-199): Unreachable code.
+// Warning 5740: (201-204): Unreachable code.

test/libsolidity/syntaxTests/viewPureChecker/view_pure_abi_encode.sol

@@ -7,3 +7,4 @@ contract C {
     }
 }
 // ----
+// Warning 5740: (142-237): Unreachable code.