Merge pull request #4129 from ethereum/doWhileContinue

[BREAKING] Fix continue inside do-while.

6 files changed, 61 insertions, 24 deletions

diff --git a/Changelog.md b/Changelog.md
index 4fa6ae55..c31aced2 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -4,6 +4,7 @@
 Breaking Changes:
  * Disallow conversions between bytesX and uintY of different size.
  * Commandline interface: Require ``-`` if standard input is used as source.
+ * General: ``continue`` in a ``do...while`` loop jumps to the condition (it used to jump to the loop body). Warning: this may silently change the semantics of existing code.
  * Type Checker: Disallow arithmetic operations for Boolean variables.
 
 Features:
diff --git a/libsolidity/analysis/ControlFlowBuilder.cpp b/libsolidity/analysis/ControlFlowBuilder.cpp
index 35d7687c..5bd39da3 100644
--- a/libsolidity/analysis/ControlFlowBuilder.cpp
+++ b/libsolidity/analysis/ControlFlowBuilder.cpp
@@ -159,15 +159,14 @@ bool ControlFlowBuilder::visit(WhileStatement const& _whileStatement)
 	{
 		auto afterWhile = newLabel();
 		auto whileBody = createLabelHere();
+		auto condition = newLabel();
 
 		{
-			// Note that "continue" in this case currently indeed jumps to whileBody
-			// and not to the condition. This is inconsistent with JavaScript and C and
-			// therefore a bug. This will be fixed in the future (planned for 0.5.0)
-			// and the Control Flow Graph will have to be adjusted accordingly.
-			BreakContinueScope scope(*this, afterWhile, whileBody);
+			BreakContinueScope scope(*this, afterWhile, condition);
 			appendControlFlow(_whileStatement.body());
 		}
+
+		placeAndConnectLabel(condition);
 		appendControlFlow(_whileStatement.condition());
 
 		connect(m_currentNode, whileBody);
diff --git a/libsolidity/codegen/ContractCompiler.cpp b/libsolidity/codegen/ContractCompiler.cpp
index 0889ac7c..f195b416 100644
--- a/libsolidity/codegen/ContractCompiler.cpp
+++ b/libsolidity/codegen/ContractCompiler.cpp
@@ -666,32 +666,36 @@ bool ContractCompiler::visit(WhileStatement const& _whileStatement)
 {
 	StackHeightChecker checker(m_context);
 	CompilerContext::LocationSetter locationSetter(m_context, _whileStatement);
+
 	eth::AssemblyItem loopStart = m_context.newTag();
 	eth::AssemblyItem loopEnd = m_context.newTag();
-	m_continueTags.push_back(loopStart);
 	m_breakTags.push_back(loopEnd);
 
 	m_context << loopStart;
 
-	// While loops have the condition prepended
-	if (!_whileStatement.isDoWhile())
+	if (_whileStatement.isDoWhile())
 	{
-		compileExpression(_whileStatement.condition());
-		m_context << Instruction::ISZERO;
-		m_context.appendConditionalJumpTo(loopEnd);
-	}
+		eth::AssemblyItem condition = m_context.newTag();
+		m_continueTags.push_back(condition);
 
-	_whileStatement.body().accept(*this);
+		_whileStatement.body().accept(*this);
 
-	// Do-while loops have the condition appended
-	if (_whileStatement.isDoWhile())
+		m_context << condition;
+		compileExpression(_whileStatement.condition());
+		m_context << Instruction::ISZERO << Instruction::ISZERO;
+		m_context.appendConditionalJumpTo(loopStart);
+	}
+	else
 	{
+		m_continueTags.push_back(loopStart);
 		compileExpression(_whileStatement.condition());
 		m_context << Instruction::ISZERO;
 		m_context.appendConditionalJumpTo(loopEnd);
-	}
 
-	m_context.appendJumpTo(loopStart);
+		_whileStatement.body().accept(*this);
+
+		m_context.appendJumpTo(loopStart);
+	}
 	m_context << loopEnd;
 
 	m_continueTags.pop_back();
diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp
index 36840568..88a1756b 100644
--- a/test/libsolidity/SolidityEndToEndTest.cpp
+++ b/test/libsolidity/SolidityEndToEndTest.cpp
@@ -482,6 +482,27 @@ BOOST_AUTO_TEST_CASE(do_while_loop)
 	testContractAgainstCppOnRange("f(uint256)", do_while_loop_cpp, 0, 5);
 }
 
+BOOST_AUTO_TEST_CASE(do_while_loop_continue)
+{
+	char const* sourceCode = R"(
+		contract test {
+			function f() public pure returns(uint r) {
+				uint i = 0;
+				do
+				{
+					if (i > 0) return 0;
+					i++;
+					continue;
+				} while (false);
+				return 42;
+			}
+		}
+	)";
+	compileAndRun(sourceCode);
+
+	ABI_CHECK(callContractFunction("f()"), encodeArgs(42));
+}
+
 BOOST_AUTO_TEST_CASE(nested_loops)
 {
 	// tests that break and continue statements in nested loops jump to the correct place
diff --git a/test/libsolidity/syntaxTests/controlFlow/storageReturn/dowhile_fine.sol b/test/libsolidity/syntaxTests/controlFlow/storageReturn/dowhile_fine.sol
index 6520672c..55c5edd3 100644
--- a/test/libsolidity/syntaxTests/controlFlow/storageReturn/dowhile_fine.sol
+++ b/test/libsolidity/syntaxTests/controlFlow/storageReturn/dowhile_fine.sol
@@ -23,13 +23,8 @@ contract C {
     }
     function k() internal view returns (S storage c) {
         do {
-            if (s.f) {
-                continue;
-                break;
-            }
-            else {
-                c = s;
-            }
+            c = s;
+            continue;
         } while(false);
     }
 }
diff --git a/test/libsolidity/syntaxTests/controlFlow/storageReturn/dowhile_warn.sol b/test/libsolidity/syntaxTests/controlFlow/storageReturn/dowhile_warn.sol
index f1a92e9c..7d001c19 100644
--- a/test/libsolidity/syntaxTests/controlFlow/storageReturn/dowhile_warn.sol
+++ b/test/libsolidity/syntaxTests/controlFlow/storageReturn/dowhile_warn.sol
@@ -21,6 +21,15 @@ contract C {
         do {
             if (s.f) {
                 break;
+            }
+            else {
+                c = s;
+            }
+        } while(false);
+    }
+    function i() internal view returns (S storage c) {
+        do {
+            if (s.f) {
                 continue;
             }
             else {
@@ -28,8 +37,16 @@ contract C {
             }
         } while(false);
     }
+    function j() internal view returns (S storage c) {
+        do {
+            continue;
+            c = s;
+        } while(false);
+    }
 }
 // ----
 // Warning: (87-98): This variable is of storage pointer type and might be returned without assignment. This can cause storage corruption. Assign the variable (potentially from itself) to remove this warning.
 // Warning: (223-234): This variable is of storage pointer type and might be returned without assignment. This can cause storage corruption. Assign the variable (potentially from itself) to remove this warning.
 // Warning: (440-451): This variable is of storage pointer type and might be returned without assignment. This can cause storage corruption. Assign the variable (potentially from itself) to remove this warning.
+// Warning: (654-665): This variable is of storage pointer type and might be returned without assignment. This can cause storage corruption. Assign the variable (potentially from itself) to remove this warning.
+// Warning: (871-882): This variable is of storage pointer type and might be returned without assignment. This can cause storage corruption. Assign the variable (potentially from itself) to remove this warning.