Merge pull request #2152 from ethereum/warnRevert

Warn about side-effect free statements.

5 files changed, 76 insertions, 17 deletions

diff --git a/Changelog.md b/Changelog.md
index 352e8374..3603d315 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -7,6 +7,7 @@ Features:
  * Commandline interface: Add the ``--standard-json`` parameter to process a Standard JSON I/O.
  * Commandline interface: Support ``--allow-paths`` to define trusted import paths. Note: the
    path(s) of the supplied source file(s) is always trusted.
+ * Static analyzer: Warn about statements without effects.
 
 Bugfixes:
  * Assembly output: Implement missing AssemblyItem types.
diff --git a/libsolidity/analysis/StaticAnalyzer.cpp b/libsolidity/analysis/StaticAnalyzer.cpp
index c39f874e..190d2420 100644
--- a/libsolidity/analysis/StaticAnalyzer.cpp
+++ b/libsolidity/analysis/StaticAnalyzer.cpp
@@ -57,6 +57,13 @@ void StaticAnalyzer::endVisit(FunctionDefinition const&)
 	m_nonPayablePublic = false;
 }
 
+bool StaticAnalyzer::visit(ExpressionStatement const& _statement)
+{
+	if (_statement.expression().annotation().isPure)
+		warning(_statement.location(), "Statement has no effect.");
+	return true;
+}
+
 bool StaticAnalyzer::visit(MemberAccess const& _memberAccess)
 {
 	if (m_nonPayablePublic && !m_library)
diff --git a/libsolidity/analysis/StaticAnalyzer.h b/libsolidity/analysis/StaticAnalyzer.h
index 0cb961bd..84342322 100644
--- a/libsolidity/analysis/StaticAnalyzer.h
+++ b/libsolidity/analysis/StaticAnalyzer.h
@@ -60,6 +60,8 @@ private:
 	virtual bool visit(FunctionDefinition const& _function) override;
 	virtual void endVisit(FunctionDefinition const& _function) override;
 
+	virtual bool visit(ExpressionStatement const& _statement) override;
+
 	virtual bool visit(MemberAccess const& _memberAccess) override;
 
 	ErrorList& m_errors;
diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp
index dc04404d..b37db7b7 100644
--- a/libsolidity/analysis/TypeChecker.cpp
+++ b/libsolidity/analysis/TypeChecker.cpp
@@ -1674,8 +1674,8 @@ bool TypeChecker::visit(Identifier const& _identifier)
 	if (auto variableDeclaration = dynamic_cast<VariableDeclaration const*>(annotation.referencedDeclaration))
 		annotation.isPure = annotation.isConstant = variableDeclaration->isConstant();
 	else if (dynamic_cast<MagicVariableDeclaration const*>(annotation.referencedDeclaration))
-		if (auto functionType = dynamic_cast<FunctionType const*>(annotation.type.get()))
-			annotation.isPure = functionType->isPure();
+		if (dynamic_cast<FunctionType const*>(annotation.type.get()))
+			annotation.isPure = true;
 	return false;
 }
 
diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp
index 1388f01b..b98c3706 100644
--- a/test/libsolidity/SolidityNameAndTypeResolution.cpp
+++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp
@@ -3653,54 +3653,56 @@ BOOST_AUTO_TEST_CASE(conditional_with_all_types)
 				// integers
 				uint x;
 				uint y;
-				true ? x : y;
+				uint g = true ? x : y;
 
 				// integer constants
-				true ? 1 : 3;
+				uint h = true ? 1 : 3;
 
 				// string literal
-				true ? "hello" : "world";
-
+				var i = true ? "hello" : "world";
+			}
+			function f2() {
 				// bool
-				true ? true : false;
+				bool j = true ? true : false;
 
 				// real is not there yet.
 
 				// array
 				byte[2] memory a;
 				byte[2] memory b;
-				true ? a : b;
+				var k = true ? a : b;
 
 				bytes memory e;
 				bytes memory f;
-				true ? e : f;
+				var l = true ? e : f;
 
 				// fixed bytes
 				bytes2 c;
 				bytes2 d;
-				true ? c : d;
-
+				var m = true ? c : d;
+			}
+			function f3() {
 				// contract doesn't fit in here
 
 				// struct
-				true ? struct_x : struct_y;
+				struct_x = true ? struct_x : struct_y;
 
 				// function
-				true ? fun_x : fun_y;
+				var r = true ? fun_x : fun_y;
 
 				// enum
 				small enum_x;
 				small enum_y;
-				true ? enum_x : enum_y;
+				enum_x = true ? enum_x : enum_y;
 
 				// tuple
-				true ? (1, 2) : (3, 4);
+				var (n, o) = true ? (1, 2) : (3, 4);
 
 				// mapping
-				true ? table1 : table2;
+				var p = true ? table1 : table2;
 
 				// typetype
-				true ? uint32(1) : uint32(2);
+				var q = true ? uint32(1) : uint32(2);
 
 				// modifier doesn't fit in here
 
@@ -5477,6 +5479,53 @@ BOOST_AUTO_TEST_CASE(using_interface_complex)
 	success(text);
 }
 
+BOOST_AUTO_TEST_CASE(bare_revert)
+{
+	char const* text = R"(
+		contract C {
+			function f(uint x) {
+				if (x > 7)
+					revert;
+			}
+		}
+	)";
+	CHECK_WARNING(text, "Statement has no effect.");
+}
+
+BOOST_AUTO_TEST_CASE(bare_others)
+{
+	CHECK_WARNING("contract C { function f() { selfdestruct; } }", "Statement has no effect.");
+	CHECK_WARNING("contract C { function f() { assert; } }", "Statement has no effect.");
+	CHECK_WARNING("contract C { function f() { require; } }", "Statement has no effect.");
+	CHECK_WARNING("contract C { function f() { suicide; } }", "Statement has no effect.");
+}
+
+BOOST_AUTO_TEST_CASE(pure_statement_in_for_loop)
+{
+	char const* text = R"(
+		contract C {
+			function f() {
+				for (uint x = 0; x < 10; true)
+					x++;
+			}
+		}
+	)";
+	CHECK_WARNING(text, "Statement has no effect.");
+}
+
+BOOST_AUTO_TEST_CASE(pure_statement_check_for_regular_for_loop)
+{
+	char const* text = R"(
+		contract C {
+			function f() {
+				for (uint x = 0; true; x++)
+				{}
+			}
+		}
+	)";
+	success(text);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 }