Merge pull request #836 from chriseth/unusedunderscore

BREAKING: Require modifiers to contain "_".

libsolidity/analysis/SyntaxChecker.cpp

@@ -40,13 +40,26 @@ void SyntaxChecker::syntaxError(SourceLocation const& _location, std::string con
 	m_errors.push_back(err);
 }
 
+bool SyntaxChecker::visit(ModifierDefinition const&)
+{
+	m_placeholderFound = false;
+	return true;
+}
+
+void SyntaxChecker::endVisit(ModifierDefinition const& _modifier)
+{
+	if (!m_placeholderFound)
+		syntaxError(_modifier.body().location(), "Modifier body does not contain '_'.");
+	m_placeholderFound = false;
+}
+
 bool SyntaxChecker::visit(WhileStatement const&)
 {
 	m_inLoopDepth++;
 	return true;
 }
 
-void SyntaxChecker::endVisit(WhileStatement const&)
+void SyntaxChecker::endVisit(WhileStatement const&	)
 {
 	m_inLoopDepth--;
 }
@@ -78,3 +91,9 @@ bool SyntaxChecker::visit(Break const& _breakStatement)
 	return true;
 }
 
+bool SyntaxChecker::visit(const PlaceholderStatement&)
+{
+	m_placeholderFound = true;
+	return true;
+}
+

libsolidity/analysis/SyntaxChecker.h

@@ -31,6 +31,7 @@ namespace solidity
 /**
  * The module that performs syntax analysis on the AST:
  *  - whether continue/break is in a for/while loop.
+ *  - whether a modifier contains at least one '_'
  */
 class SyntaxChecker: private ASTConstVisitor
 {
@@ -44,6 +45,9 @@ private:
 	/// Adds a new error to the list of errors.
 	void syntaxError(SourceLocation const& _location, std::string const& _description);
 
+	virtual bool visit(ModifierDefinition const& _modifier) override;
+	virtual void endVisit(ModifierDefinition const& _modifier) override;
+
 	virtual bool visit(WhileStatement const& _whileStatement) override;
 	virtual void endVisit(WhileStatement const& _whileStatement) override;
 	virtual bool visit(ForStatement const& _forStatement) override;
@@ -52,8 +56,13 @@ private:
 	virtual bool visit(Continue const& _continueStatement) override;
 	virtual bool visit(Break const& _breakStatement) override;
 
+	virtual bool visit(PlaceholderStatement const& _placeholderStatement) override;
+
 	ErrorList& m_errors;
 
+	/// Flag that indicates whether a function modifier actually contains '_'.
+	bool m_placeholderFound = false;
+
 	int m_inLoopDepth = 0;
 };
 

test/libsolidity/SolidityEndToEndTest.cpp

@@ -2410,7 +2410,7 @@ BOOST_AUTO_TEST_CASE(function_modifier_overriding)
 			modifier mod { _ }
 		}
 		contract C is A {
-			modifier mod { }
+			modifier mod { if (false) _ }
 		}
 	)";
 	compileAndRun(sourceCode);
@@ -2427,7 +2427,7 @@ BOOST_AUTO_TEST_CASE(function_modifier_calling_functions_in_creation_context)
 			function f2() { data |= 0x20; }
 			function f3() { }
 			modifier mod1 { f2(); _ }
-			modifier mod2 { f3(); }
+			modifier mod2 { f3(); if (false) _ }
 			function getData() returns (uint r) { return data; }
 		}
 		contract C is A {

test/libsolidity/SolidityNameAndTypeResolution.cpp

@@ -901,8 +901,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 A { modifier mod(uint a) { _ } }
-		contract B is A { modifier mod(uint a) {} }
+		contract B is A { modifier mod(uint a) { _ } }
 	)";
 	BOOST_CHECK(success(text));
 }
@@ -910,8 +910,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 A { modifier mod(uint a) { _ } }
-		contract B is A { modifier mod(uint8 a) {} }
+		contract B is A { modifier mod(uint8 a) { _ } }
 	)";
 	BOOST_CHECK(expectError(text) == Error::Type::TypeError);
 }
@@ -919,8 +919,8 @@ 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) {} }
+		contract B is A { function mod(uint a) { } }
 	)";
 	BOOST_CHECK(expectError(text) == Error::Type::TypeError);
 }
@@ -928,8 +928,8 @@ BOOST_AUTO_TEST_CASE(modifier_overrides_function)
 BOOST_AUTO_TEST_CASE(function_overrides_modifier)
 {
 	char const* text = R"(
-		contract A { function mod(uint a) {} }
+		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);
 }
@@ -938,8 +938,8 @@ BOOST_AUTO_TEST_CASE(modifier_returns_value)
 {
 	char const* text = R"(
 		contract A {
-			function f(uint a) mod(2) returns (uint r) {}
+			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);
@@ -3823,6 +3823,16 @@ BOOST_AUTO_TEST_CASE(unused_return_value_delegatecall)
 	BOOST_CHECK(expectError(text, true) == Error::Type::Warning);
 }
 
+BOOST_AUTO_TEST_CASE(modifier_without_underscore)
+{
+	char const* text = R"(
+		contract test {
+			modifier m() {}
+		}
+	)";
+	BOOST_CHECK(expectError(text, true) == Error::Type::SyntaxError);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 }