Merge pull request #1005 from ethereum/modifierbody

Require ";" after "_"

13 files changed, 69 insertions, 59 deletions

diff --git a/Changelog.md b/Changelog.md
index 4bc750f0..bd8d924f 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -17,6 +17,7 @@ Breaking Changes:
  * Function call throws if target contract does not have code
  * Modifiers are required to contain ``_`` (use ``if (false) _`` as a workaround if needed).
  * Modifiers: return does not skip part in modifier after ``_``
+ * Placeholder statement `_` in modifier now requires explicit `;`.
  * ``ecrecover`` now returns zero if the input is malformed (it previously returned garbage)
  * Removed ``--interface`` (Solidity interface) output option
  * JSON AST: General cleanup, renamed many nodes to match their C++ names.
diff --git a/docs/common-patterns.rst b/docs/common-patterns.rst
index 531a94ad..0894aae2 100644
--- a/docs/common-patterns.rst
+++ b/docs/common-patterns.rst
@@ -148,10 +148,10 @@ restrictions highly readable.
         {
             if (msg.sender != _account)
                 throw;
-            // Do not forget the "_"! It will
+            // Do not forget the "_;"! It will
             // be replaced by the actual function
             // body when the modifier is invoked.
-            _
+            _;
         }
 
         /// Make `_newOwner` the new owner of this
@@ -164,7 +164,7 @@ restrictions highly readable.
 
         modifier onlyAfter(uint _time) {
             if (now < _time) throw;
-            _
+            _;
         }
 
         /// Erase ownership information.
@@ -187,7 +187,7 @@ restrictions highly readable.
         modifier costs(uint _amount) {
             if (msg.value < _amount)
                 throw;
-            _
+            _;
             if (msg.value > _amount)
                 msg.sender.send(msg.value - _amount);
         }
@@ -286,7 +286,7 @@ function finishes.
 
         modifier atStage(Stages _stage) {
             if (stage != _stage) throw;
-            _
+            _;
         }
 
         function nextStage() internal {
@@ -304,7 +304,7 @@ function finishes.
                     now >= creationTime + 12 days)
                 nextStage();
             // The other stages transition by transaction
-            _
+            _;
         }
 
         // Order of the modifiers matters here!
@@ -328,7 +328,7 @@ function finishes.
         // automatically.
         modifier transitionNext()
         {
-            _
+            _;
             nextStage();
         }
 
diff --git a/docs/contracts.rst b/docs/contracts.rst
index a22a3544..dfa79e79 100644
--- a/docs/contracts.rst
+++ b/docs/contracts.rst
@@ -321,7 +321,7 @@ inheritable properties of contracts and may be overridden by derived contracts.
         modifier onlyOwner {
             if (msg.sender != owner)
                 throw;
-            _
+            _;
         }
     }
 
@@ -341,7 +341,7 @@ inheritable properties of contracts and may be overridden by derived contracts.
         // Modifiers can receive arguments:
         modifier costs(uint price) {
             if (msg.value >= price) {
-                _
+                _;
             }
         }
     }
@@ -367,7 +367,7 @@ inheritable properties of contracts and may be overridden by derived contracts.
         modifier noReentrancy() {
             if (locked) throw;
             locked = true;
-            _
+            _;
             locked = false;
         }
 
diff --git a/docs/solidity-by-example.rst b/docs/solidity-by-example.rst
index 8e23dafd..b8fa4a73 100644
--- a/docs/solidity-by-example.rst
+++ b/docs/solidity-by-example.rst
@@ -403,8 +403,8 @@ high or low invalid bids.
         /// functions. `onlyBefore` is applied to `bid` below:
         /// The new function body is the modifier's body where
         /// `_` is replaced by the old function body.
-        modifier onlyBefore(uint _time) { if (now >= _time) throw; _ }
-        modifier onlyAfter(uint _time) { if (now <= _time) throw; _ }
+        modifier onlyBefore(uint _time) { if (now >= _time) throw; _; }
+        modifier onlyAfter(uint _time) { if (now <= _time) throw; _; }
 
         function BlindAuction(
             uint _biddingTime,
diff --git a/libsolidity/grammar.txt b/libsolidity/grammar.txt
index 0230729a..86df3db0 100644
--- a/libsolidity/grammar.txt
+++ b/libsolidity/grammar.txt
@@ -41,8 +41,9 @@ ArrayTypeName = TypeName StorageLocation? '[' Expression? ']'
 StorageLocation = 'memory' | 'storage'
 
 Block = '{' Statement* '}'
-Statement = IfStatement | WhileStatement | ForStatement | Block | PlaceholderStatement |
-            ( Continue | Break | Return | Throw | SimpleStatement | ExpressionStatement ) ';'
+Statement = IfStatement | WhileStatement | ForStatement | Block |
+            ( PlaceholderStatement | Continue | Break | Return |
+              Throw | SimpleStatement | ExpressionStatement ) ';'
 
 ExpressionStatement = Expression | VariableDefinition
 IfStatement = 'if' '(' Expression ')' Statement ( 'else' Statement )?
diff --git a/libsolidity/parsing/Parser.cpp b/libsolidity/parsing/Parser.cpp
index b2f4a156..1bffa5d7 100644
--- a/libsolidity/parsing/Parser.cpp
+++ b/libsolidity/parsing/Parser.cpp
@@ -753,7 +753,7 @@ ASTPointer<Statement> Parser::parseStatement()
 		{
 			statement = ASTNodeFactory(*this).createNode<PlaceholderStatement>(docString);
 			m_scanner->next();
-			return statement;
+			break;
 		}
 	// fall-through
 	default:
diff --git a/test/contracts/AuctionRegistrar.cpp b/test/contracts/AuctionRegistrar.cpp
index 681caa26..9161f2ce 100644
--- a/test/contracts/AuctionRegistrar.cpp
+++ b/test/contracts/AuctionRegistrar.cpp
@@ -158,7 +158,7 @@ contract GlobalRegistrar is Registrar, AuctionSystem {
 		return bytes(_name).length < c_freeBytes;
 	}
 
-	modifier onlyrecordowner(string _name) { if (m_toRecord[_name].owner == msg.sender) _ }
+	modifier onlyrecordowner(string _name) { if (m_toRecord[_name].owner == msg.sender) _; }
 
 	function transfer(string _name, address _newOwner) onlyrecordowner(_name) {
 		m_toRecord[_name].owner = _newOwner;
diff --git a/test/contracts/FixedFeeRegistrar.cpp b/test/contracts/FixedFeeRegistrar.cpp
index fd0861f7..c37873cf 100644
--- a/test/contracts/FixedFeeRegistrar.cpp
+++ b/test/contracts/FixedFeeRegistrar.cpp
@@ -71,7 +71,7 @@ contract FixedFeeRegistrar is Registrar {
 		address owner;
 	}
 
-	modifier onlyrecordowner(string _name) { if (m_record(_name).owner == msg.sender) _ }
+	modifier onlyrecordowner(string _name) { if (m_record(_name).owner == msg.sender) _; }
 
 	function reserve(string _name) {
 		Record rec = m_record(_name);
diff --git a/test/contracts/Wallet.cpp b/test/contracts/Wallet.cpp
index 55c2e1af..d3e1b8d7 100644
--- a/test/contracts/Wallet.cpp
+++ b/test/contracts/Wallet.cpp
@@ -86,14 +86,14 @@ contract multiowned {
 	// simple single-sig function modifier.
 	modifier onlyowner {
 		if (isOwner(msg.sender))
-			_
+			_;
 	}
 	// multi-sig function modifier: the operation must have an intrinsic hash in order
 	// that later attempts can be realised as the same underlying operation and
 	// thus count as confirmations.
 	modifier onlymanyowners(bytes32 _operation) {
 		if (confirmAndCheck(_operation))
-			_
+			_;
 	}
 
 	// METHODS
@@ -281,7 +281,7 @@ contract daylimit is multiowned {
 	// simple modifier for daily limit.
 	modifier limitedDaily(uint _value) {
 		if (underLimit(_value))
-			_
+			_;
 	}
 
 	// METHODS
diff --git a/test/libsolidity/ASTJSON.cpp b/test/libsolidity/ASTJSON.cpp
index ec60b668..882496d9 100644
--- a/test/libsolidity/ASTJSON.cpp
+++ b/test/libsolidity/ASTJSON.cpp
@@ -128,7 +128,7 @@ BOOST_AUTO_TEST_CASE(enum_value)
 BOOST_AUTO_TEST_CASE(modifier_definition)
 {
 	CompilerStack c;
-	c.addSource("a", "contract C { modifier M(uint i) { _ } function F() M(1) {} }");
+	c.addSource("a", "contract C { modifier M(uint i) { _; } function F() M(1) {} }");
 	c.parse();
 	map<string, unsigned> sourceIndices;
 	sourceIndices["a"] = 1;
@@ -136,20 +136,20 @@ BOOST_AUTO_TEST_CASE(modifier_definition)
 	Json::Value modifier = astJson["children"][0]["children"][0];
 	BOOST_CHECK_EQUAL(modifier["name"], "ModifierDefinition");
 	BOOST_CHECK_EQUAL(modifier["attributes"]["name"], "M");
-	BOOST_CHECK_EQUAL(modifier["src"], "13:24:1");
+	BOOST_CHECK_EQUAL(modifier["src"], "13:25:1");
 }
 
 BOOST_AUTO_TEST_CASE(modifier_invocation)
 {
 	CompilerStack c;
-	c.addSource("a", "contract C { modifier M(uint i) { _ } function F() M(1) {} }");
+	c.addSource("a", "contract C { modifier M(uint i) { _; } function F() M(1) {} }");
 	c.parse();
 	map<string, unsigned> sourceIndices;
 	sourceIndices["a"] = 1;
 	Json::Value astJson = ASTJsonConverter(c.ast("a"), sourceIndices).json();
 	Json::Value modifier = astJson["children"][0]["children"][1]["children"][2];
 	BOOST_CHECK_EQUAL(modifier["name"], "ModifierInvocation");
-	BOOST_CHECK_EQUAL(modifier["src"], "51:4:1");
+	BOOST_CHECK_EQUAL(modifier["src"], "52:4:1");
 	BOOST_CHECK_EQUAL(modifier["children"][0]["attributes"]["type"], "modifier (uint256)");
 	BOOST_CHECK_EQUAL(modifier["children"][0]["attributes"]["value"], "M");
 	BOOST_CHECK_EQUAL(modifier["children"][1]["attributes"]["value"], "1");
@@ -185,7 +185,7 @@ BOOST_AUTO_TEST_CASE(array_type_name)
 BOOST_AUTO_TEST_CASE(placeholder_statement)
 {
 	CompilerStack c;
-	c.addSource("a", "contract C { modifier M { _ } }");
+	c.addSource("a", "contract C { modifier M { _; } }");
 	c.parse();
 	map<string, unsigned> sourceIndices;
 	sourceIndices["a"] = 1;
diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp
index 35b7078d..f3fdef15 100644
--- a/test/libsolidity/SolidityEndToEndTest.cpp
+++ b/test/libsolidity/SolidityEndToEndTest.cpp
@@ -2353,7 +2353,7 @@ BOOST_AUTO_TEST_CASE(function_modifier)
 	char const* sourceCode = R"(
 		contract C {
 			function getOne() nonFree returns (uint r) { return 1; }
-			modifier nonFree { if (msg.value > 0) _ }
+			modifier nonFree { if (msg.value > 0) _; }
 		}
 	)";
 	compileAndRun(sourceCode);
@@ -2365,8 +2365,8 @@ BOOST_AUTO_TEST_CASE(function_modifier_local_variables)
 {
 	char const* sourceCode = R"(
 		contract C {
-			modifier mod1 { var a = 1; var b = 2; _ }
-			modifier mod2(bool a) { if (a) return; else _ }
+			modifier mod1 { var a = 1; var b = 2; _; }
+			modifier mod2(bool a) { if (a) return; else _; }
 			function f(bool a) mod1 mod2(a) returns (uint r) { return 3; }
 		}
 	)";
@@ -2379,7 +2379,7 @@ BOOST_AUTO_TEST_CASE(function_modifier_loop)
 {
 	char const* sourceCode = R"(
 		contract C {
-			modifier repeat(uint count) { for (var i = 0; i < count; ++i) _ }
+			modifier repeat(uint count) { for (var i = 0; i < count; ++i) _; }
 			function f() repeat(10) returns (uint r) { r += 1; }
 		}
 	)";
@@ -2391,7 +2391,7 @@ BOOST_AUTO_TEST_CASE(function_modifier_multi_invocation)
 {
 	char const* sourceCode = R"(
 		contract C {
-			modifier repeat(bool twice) { if (twice) _ _ }
+			modifier repeat(bool twice) { if (twice) _; _; }
 			function f(bool twice) repeat(twice) returns (uint r) { r += 1; }
 		}
 	)";
@@ -2406,7 +2406,7 @@ BOOST_AUTO_TEST_CASE(function_modifier_multi_with_return)
 	// modifier code block.
 	char const* sourceCode = R"(
 		contract C {
-			modifier repeat(bool twice) { if (twice) _ _ }
+			modifier repeat(bool twice) { if (twice) _; _; }
 			function f(bool twice) repeat(twice) returns (uint r) { r += 1; return r; }
 		}
 	)";
@@ -2420,10 +2420,10 @@ BOOST_AUTO_TEST_CASE(function_modifier_overriding)
 	char const* sourceCode = R"(
 		contract A {
 			function f() mod returns (bool r) { return true; }
-			modifier mod { _ }
+			modifier mod { _; }
 		}
 		contract C is A {
-			modifier mod { if (false) _ }
+			modifier mod { if (false) _; }
 		}
 	)";
 	compileAndRun(sourceCode);
@@ -2439,12 +2439,12 @@ BOOST_AUTO_TEST_CASE(function_modifier_calling_functions_in_creation_context)
 			function f1() mod2 { data |= 0x1; }
 			function f2() { data |= 0x20; }
 			function f3() { }
-			modifier mod1 { f2(); _ }
-			modifier mod2 { f3(); if (false) _ }
+			modifier mod1 { f2(); _; }
+			modifier mod2 { f3(); if (false) _; }
 			function getData() returns (uint r) { return data; }
 		}
 		contract C is A {
-			modifier mod1 { f4(); _ }
+			modifier mod1 { f4(); _; }
 			function f3() { data |= 0x300; }
 			function f4() { data |= 0x4000; }
 		}
@@ -2459,11 +2459,11 @@ BOOST_AUTO_TEST_CASE(function_modifier_for_constructor)
 		contract A {
 			uint data;
 			function A() mod1 { data |= 2; }
-			modifier mod1 { data |= 1; _ }
+			modifier mod1 { data |= 1; _; }
 			function getData() returns (uint r) { return data; }
 		}
 		contract C is A {
-			modifier mod1 { data |= 4; _ }
+			modifier mod1 { data |= 4; _; }
 		}
 	)";
 	compileAndRun(sourceCode);
@@ -6920,7 +6920,7 @@ BOOST_AUTO_TEST_CASE(return_does_not_skip_modifier)
 		contract C {
 			uint public x;
 			modifier setsx {
-				_
+				_;
 				x = 9;
 			}
 			function f() setsx returns (uint) {
@@ -6941,7 +6941,7 @@ BOOST_AUTO_TEST_CASE(break_in_modifier)
 			uint public x;
 			modifier run() {
 				for (uint i = 0; i < 10; i++) {
-					_
+					_;
 					break;
 				}
 			}
@@ -6963,7 +6963,7 @@ BOOST_AUTO_TEST_CASE(stacked_return_with_modifiers)
 			uint public x;
 			modifier run() {
 				for (uint i = 0; i < 10; i++) {
-					_
+					_;
 					break;
 				}
 			}
@@ -6986,7 +6986,7 @@ BOOST_AUTO_TEST_CASE(mutex)
 			modifier protected {
 				if (locked) throw;
 				locked = true;
-				_
+				_;
 				locked = false;
 			}
 		}
diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp
index 2c126b65..8d4890d1 100644
--- a/test/libsolidity/SolidityNameAndTypeResolution.cpp
+++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp
@@ -859,8 +859,8 @@ BOOST_AUTO_TEST_CASE(function_modifier_invocation)
 	char const* text = R"(
 		contract B {
 			function f() mod1(2, true) mod2("0123456") { }
-			modifier mod1(uint a, bool b) { if (b) _ }
-			modifier mod2(bytes7 a) { while (a == "1234567") _ }
+			modifier mod1(uint a, bool b) { if (b) _; }
+			modifier mod2(bytes7 a) { while (a == "1234567") _; }
 		}
 	)";
 	BOOST_CHECK(success(text));
@@ -871,7 +871,7 @@ BOOST_AUTO_TEST_CASE(invalid_function_modifier_type)
 	char const* text = R"(
 		contract B {
 			function f() mod1(true) { }
-			modifier mod1(uint a) { if (a > 0) _ }
+			modifier mod1(uint a) { if (a > 0) _; }
 		}
 	)";
 	BOOST_CHECK(expectError(text) == Error::Type::TypeError);
@@ -882,8 +882,8 @@ BOOST_AUTO_TEST_CASE(function_modifier_invocation_parameters)
 	char const* text = R"(
 		contract B {
 			function f(uint8 a) mod1(a, true) mod2(r) returns (bytes7 r) { }
-			modifier mod1(uint a, bool b) { if (b) _ }
-			modifier mod2(bytes7 a) { while (a == "1234567") _ }
+			modifier mod1(uint a, bool b) { if (b) _; }
+			modifier mod2(bytes7 a) { while (a == "1234567") _; }
 		}
 	)";
 	BOOST_CHECK(success(text));
@@ -894,7 +894,7 @@ BOOST_AUTO_TEST_CASE(function_modifier_invocation_local_variables)
 	char const* text = R"(
 		contract B {
 			function f() mod(x) { uint x = 7; }
-			modifier mod(uint a) { if (a > 0) _ }
+			modifier mod(uint a) { if (a > 0) _; }
 		}
 	)";
 	BOOST_CHECK(success(text));
@@ -903,8 +903,8 @@ BOOST_AUTO_TEST_CASE(function_modifier_invocation_local_variables)
 BOOST_AUTO_TEST_CASE(legal_modifier_override)
 {
 	char const* text = R"(
-		contract A { modifier mod(uint a) { _ } }
-		contract B is A { modifier mod(uint a) { _ } }
+		contract A { modifier mod(uint a) { _; } }
+		contract B is A { modifier mod(uint a) { _; } }
 	)";
 	BOOST_CHECK(success(text));
 }
@@ -912,8 +912,8 @@ BOOST_AUTO_TEST_CASE(legal_modifier_override)
 BOOST_AUTO_TEST_CASE(illegal_modifier_override)
 {
 	char const* text = R"(
-		contract A { modifier mod(uint a) { _ } }
-		contract B is A { modifier mod(uint8 a) { _ } }
+		contract A { modifier mod(uint a) { _; } }
+		contract B is A { modifier mod(uint8 a) { _; } }
 	)";
 	BOOST_CHECK(expectError(text) == Error::Type::TypeError);
 }
@@ -921,7 +921,7 @@ BOOST_AUTO_TEST_CASE(illegal_modifier_override)
 BOOST_AUTO_TEST_CASE(modifier_overrides_function)
 {
 	char const* text = R"(
-		contract A { modifier mod(uint a) { _ } }
+		contract A { modifier mod(uint a) { _; } }
 		contract B is A { function mod(uint a) { } }
 	)";
 	BOOST_CHECK(expectError(text) == Error::Type::TypeError);
@@ -931,7 +931,7 @@ BOOST_AUTO_TEST_CASE(function_overrides_modifier)
 {
 	char const* text = R"(
 		contract A { function mod(uint a) { } }
-		contract B is A { modifier mod(uint a) { _ } }
+		contract B is A { modifier mod(uint a) { _; } }
 	)";
 	BOOST_CHECK(expectError(text) == Error::Type::TypeError);
 }
@@ -941,7 +941,7 @@ BOOST_AUTO_TEST_CASE(modifier_returns_value)
 	char const* text = R"(
 		contract A {
 			function f(uint a) mod(2) returns (uint r) { }
-			modifier mod(uint a) { _ return 7; }
+			modifier mod(uint a) { _; return 7; }
 		}
 	)";
 	BOOST_CHECK(expectError(text) == Error::Type::TypeError);
diff --git a/test/libsolidity/SolidityParser.cpp b/test/libsolidity/SolidityParser.cpp
index 92f5a142..0c0e343b 100644
--- a/test/libsolidity/SolidityParser.cpp
+++ b/test/libsolidity/SolidityParser.cpp
@@ -681,15 +681,23 @@ BOOST_AUTO_TEST_CASE(placeholder_in_function_context)
 BOOST_AUTO_TEST_CASE(modifier)
 {
 	char const* text = "contract c {\n"
-					   "  modifier mod { if (msg.sender == 0) _ }\n"
+					   "  modifier mod { if (msg.sender == 0) _; }\n"
 					   "}\n";
 	BOOST_CHECK(successParse(text));
 }
 
+BOOST_AUTO_TEST_CASE(modifier_without_semicolon)
+{
+	char const* text = "contract c {\n"
+					   "  modifier mod { if (msg.sender == 0) _ }\n"
+					   "}\n";
+	BOOST_CHECK(!successParse(text));
+}
+
 BOOST_AUTO_TEST_CASE(modifier_arguments)
 {
 	char const* text = "contract c {\n"
-					   "  modifier mod(uint a) { if (msg.sender == a) _ }\n"
+					   "  modifier mod(uint a) { if (msg.sender == a) _; }\n"
 					   "}\n";
 	BOOST_CHECK(successParse(text));
 }
@@ -697,8 +705,8 @@ BOOST_AUTO_TEST_CASE(modifier_arguments)
 BOOST_AUTO_TEST_CASE(modifier_invocation)
 {
 	char const* text = "contract c {\n"
-					   "  modifier mod1(uint a) { if (msg.sender == a) _ }\n"
-					   "  modifier mod2 { if (msg.sender == 2) _ }\n"
+					   "  modifier mod1(uint a) { if (msg.sender == a) _; }\n"
+					   "  modifier mod2 { if (msg.sender == 2) _; }\n"
 					   "  function f() mod1(7) mod2 { }\n"
 					   "}\n";
 	BOOST_CHECK(successParse(text));