Merge pull request #4409 from ethereum/viewPureChecker

Enforce state mutability in view pure checker.

Changelog.md

@@ -34,6 +34,7 @@ Breaking Changes:
  * Type Checker: Only accept a single ``bytes`` type for ``.call()`` (and family), ``keccak256()``, ``sha256()`` and ``ripemd160()``.
  * Remove obsolete ``std`` directory from the Solidity repository. This means accessing ``https://github.com/ethereum/soldity/blob/develop/std/*.sol`` (or ``https://github.com/ethereum/solidity/std/*.sol`` in Remix) will not be possible.
  * Syntax Checker: Named return values in function types are an error.
+ * View Pure Checker: Strictly enfore state mutability. This was already the case in the experimental 0.5.0 mode.
 
 Language Features:
  * General: Allow appending ``calldata`` keyword to types, to explicitly specify data location for arguments of external functions.

docs/contracts.rst

@@ -485,9 +485,6 @@ The following statements are considered modifying the state:
   prevent modifications to the state on the level of the EVM by adding
   ``pragma experimental "v0.5.0";``
 
-.. warning::
-  The compiler does not enforce yet that a ``view`` method is not modifying state. It raises a warning though.
-
 .. index:: ! pure function, function;pure
 
 .. _pure-functions:
@@ -529,12 +526,15 @@ In addition to the list of state modifying statements explained above, the follo
   It is a non-circumventable runtime checks done by the EVM.
 
 .. warning::
-  Before version 0.4.17 the compiler didn't enforce that ``pure`` is not reading the state.
+  Before version 0.4.17 the compiler did not enforce that ``pure`` is not reading the state.
   It is a compile-time type check, which can be circumvented doing invalid explicit conversions
   between contract types, because the compiler can verify that the type of the contract does
   not do state-changing operations, but it cannot check that the contract that will be called
   at runtime is actually of that type.
 
+.. warning::
+  Before version 0.5.0 the compiler did not enforce that ``view`` is not writing the state.
+
 .. index:: ! fallback function, function;fallback
 
 .. _fallback-function:

docs/miscellaneous.rst

@@ -397,8 +397,8 @@ Function Visibility Specifiers
 Modifiers
 =========
 
-- ``pure`` for functions: Disallows modification or access of state - this is not enforced yet.
+- ``pure`` for functions: Disallows modification or access of state.
-- ``view`` for functions: Disallows modification of state - this is not enforced yet.
+- ``view`` for functions: Disallows modification of state.
 - ``payable`` for functions: Allows them to receive Ether together with a call.
 - ``constant`` for state variables: Disallows assignment (except initialisation), does not occupy storage slot.
 - ``anonymous`` for events: Does not store event signature as topic.

libsolidity/analysis/ViewPureChecker.cpp

@@ -116,31 +116,22 @@ private:
 
 bool ViewPureChecker::check()
 {
-	// The bool means "enforce view with errors".
+	vector<ContractDefinition const*> contracts;
-	vector<pair<ContractDefinition const*, bool>> contracts;
 
 	for (auto const& node: m_ast)
 	{
 		SourceUnit const* source = dynamic_cast<SourceUnit const*>(node.get());
 		solAssert(source, "");
-		bool enforceView = source->annotation().experimentalFeatures.count(ExperimentalFeature::V050);
+		contracts += source->filteredNodes<ContractDefinition>(source->nodes());
-		for (ContractDefinition const* c: source->filteredNodes<ContractDefinition>(source->nodes()))
-			contracts.emplace_back(c, enforceView);
 	}
 
 	// Check modifiers first to infer their state mutability.
 	for (auto const& contract: contracts)
-	{
+		for (ModifierDefinition const* mod: contract->functionModifiers())
-		m_enforceViewWithError = contract.second;
-		for (ModifierDefinition const* mod: contract.first->functionModifiers())
 			mod->accept(*this);
-	}
 
 	for (auto const& contract: contracts)
-	{
+		contract->accept(*this);
-		m_enforceViewWithError = contract.second;
-		contract.first->accept(*this);
-	}
 
 	return !m_errors;
 }
@@ -232,17 +223,20 @@ void ViewPureChecker::reportMutability(StateMutability _mutability, SourceLocati
 {
 	if (m_currentFunction && m_currentFunction->stateMutability() < _mutability)
 	{
-		string text;
 		if (_mutability == StateMutability::View)
-			text =
+			m_errorReporter.typeError(
+				_location,
 				"Function declared as pure, but this expression (potentially) reads from the "
-				"environment or state and thus requires \"view\".";
+				"environment or state and thus requires \"view\"."
+			);
 		else if (_mutability == StateMutability::NonPayable)
-			text =
+			m_errorReporter.typeError(
+				_location,
 				"Function declared as " +
 				stateMutabilityToString(m_currentFunction->stateMutability()) +
 				", but this expression (potentially) modifies the state and thus "
-				"requires non-payable (the default) or payable.";
+				"requires non-payable (the default) or payable."
+			);
 		else
 			solAssert(false, "");
 
@@ -251,13 +245,7 @@ void ViewPureChecker::reportMutability(StateMutability _mutability, SourceLocati
 			m_currentFunction->stateMutability() == StateMutability::Pure,
 			""
 		);
-		if (!m_enforceViewWithError && m_currentFunction->stateMutability() == StateMutability::View)
+		m_errors = true;
-			m_errorReporter.warning(_location, text);
-		else
-		{
-			m_errors = true;
-			m_errorReporter.typeError(_location, text);
-		}
 	}
 	if (_mutability > m_currentBestMutability)
 		m_currentBestMutability = _mutability;

libsolidity/analysis/ViewPureChecker.h

@@ -71,7 +71,6 @@ private:
 	ErrorReporter& m_errorReporter;
 
 	bool m_errors = false;
-	bool m_enforceViewWithError = false;
 	StateMutability m_currentBestMutability = StateMutability::Payable;
 	FunctionDefinition const* m_currentFunction = nullptr;
 	std::map<ModifierDefinition const*, StateMutability> m_inferredMutability;

test/libsolidity/syntaxTests/viewPureChecker/assembly_jump_view_fail.sol

@@ -0,0 +1,8 @@
+contract C {
+    function k() public view {
+        assembly { jump(2) }
+    }
+}
+// ----
+// Warning: (63-70): Jump instructions and labels are low-level EVM features that can lead to incorrect stack access. Because of that they are discouraged. Please consider using "switch", "if" or "for" statements instead.
+// TypeError: (63-70): Function declared as view, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable.

test/libsolidity/syntaxTests/viewPureChecker/builtin_functions_restrict_warning.sol

@@ -0,0 +1,21 @@
+contract C {
+    function f() view public {
+        bytes32 x = keccak256("abc");
+        bytes32 y = sha256("abc");
+        address z = ecrecover(bytes32(1), uint8(2), bytes32(3), bytes32(4));
+        require(true);
+        assert(true);
+        x; y; z;
+    }
+    function g() public {
+        bytes32 x = keccak256("abc");
+        bytes32 y = sha256("abc");
+        address z = ecrecover(bytes32(1), uint8(2), bytes32(3), bytes32(4));
+        require(true);
+        assert(true);
+        x; y; z;
+    }
+}
+// ----
+// Warning: (17-261): Function state mutability can be restricted to pure
+// Warning: (266-505): Function state mutability can be restricted to pure

test/libsolidity/syntaxTests/viewPureChecker/builtin_functions_view_fail.sol

@@ -0,0 +1,23 @@
+contract C {
+    function f() view public {
+        address(this).transfer(1);
+    }
+    function g() view public {
+        require(address(this).send(2));
+    }
+    function h() view public {
+        selfdestruct(address(this));
+    }
+    function i() view public {
+        require(address(this).delegatecall(""));
+    }
+    function j() view public {
+        require(address(this).call(""));
+    }
+}
+// ----
+// TypeError: (52-77): Function declared as view, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable.
+// TypeError: (132-153): Function declared as view, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable.
+// TypeError: (201-228): Function declared as view, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable.
+// TypeError: (283-313): Function declared as view, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable.
+// TypeError: (369-391): Function declared as view, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable.

test/libsolidity/syntaxTests/viewPureChecker/call_internal_functions_fail.sol

@@ -1,7 +1,10 @@
 contract C {
+    uint x;
     function f() pure public { g(); }
-    function g() view public {}
+    function g() view public { x; }
+    function h() view public { i(); }
+    function i() public { x = 2; }
 }
 // ----
-// TypeError: (44-47): Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view".
+// TypeError: (56-59): Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view".
-// Warning: (55-82): Function state mutability can be restricted to pure
+// TypeError: (130-133): Function declared as view, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable.

test/libsolidity/syntaxTests/viewPureChecker/constant_restrict_warning.sol

@@ -0,0 +1,12 @@
+contract C {
+    uint constant x = 2;
+    function f() view public returns (uint) {
+        return x;
+    }
+    function g() public returns (uint) {
+        return x;
+    }
+}
+// ----
+// Warning: (42-107): Function state mutability can be restricted to pure
+// Warning: (112-172): Function state mutability can be restricted to pure

test/libsolidity/syntaxTests/viewPureChecker/creation_view_fail.sol

@@ -0,0 +1,6 @@
+contract D {}
+contract C {
+    function f() public view { new D(); }
+}
+// ----
+// TypeError: (58-65): Function declared as view, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable.

test/libsolidity/syntaxTests/viewPureChecker/function_types_fail.sol

@@ -0,0 +1,18 @@
+contract C {
+    function f() pure public {
+        function () external nonpayFun;
+        nonpayFun();
+    }
+    function g() pure public {
+        function () external view viewFun;
+        viewFun();
+    }
+    function h() view public {
+        function () external nonpayFun;
+        nonpayFun();
+    }
+}
+// ----
+// TypeError: (92-103): Function declared as pure, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable.
+// TypeError: (193-202): Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view".
+// TypeError: (289-300): Function declared as view, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable.

test/libsolidity/syntaxTests/viewPureChecker/local_storage_variables_fail.sol

@@ -0,0 +1,15 @@
+contract C {
+    struct S { uint a; }
+    S s;
+    function f() pure public {
+        S storage x = s;
+        x;
+    }
+    function g() view public {
+        S storage x = s;
+        x.a = 1;
+    }
+}
+// ----
+// TypeError: (100-101): Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view".
+// TypeError: (184-187): Function declared as view, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable.

test/libsolidity/syntaxTests/viewPureChecker/modifiers_fail.sol

@@ -0,0 +1,12 @@
+contract D {
+    uint x;
+    modifier viewm(uint) { uint a = x; _; a; }
+    modifier nonpayablem(uint) { x = 2; _; }
+}
+contract C is D {
+    function f() viewm(0) pure public {}
+    function g() nonpayablem(0) view public {}
+}
+// ----
+// TypeError: (154-162): Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view".
+// TypeError: (195-209): Function declared as view, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable.

test/libsolidity/syntaxTests/viewPureChecker/overriding_fail.sol

@@ -0,0 +1,16 @@
+contract D {
+    uint x;
+    function f() public view { x; }
+    function g() public pure {}
+}
+contract C1 is D {
+    function f() public {}
+    function g() public view {}
+}
+contract C2 is D {
+    function g() public {}
+}
+// ----
+// TypeError: (118-140): Overriding function changes state mutability from "view" to "nonpayable".
+// TypeError: (145-172): Overriding function changes state mutability from "pure" to "view".
+// TypeError: (198-220): Overriding function changes state mutability from "pure" to "nonpayable".

test/libsolidity/syntaxTests/viewPureChecker/read_storage_pure_fail.sol

@@ -0,0 +1,8 @@
+contract C {
+    uint x;
+    function f() public pure returns (uint) {
+        return x;
+    }
+}
+// ----
+// TypeError: (86-87): Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view".

test/libsolidity/syntaxTests/viewPureChecker/returning_structs_fail.sol

@@ -0,0 +1,13 @@
+contract C {
+    struct S { uint x; }
+    S s;
+    function f() pure internal returns (S storage) {
+        return s;
+    }
+    function g() pure public {
+        f().x;
+    }
+}
+// ----
+// TypeError: (115-116): Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view".
+// TypeError: (163-168): Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view".

test/libsolidity/syntaxTests/viewPureChecker/selector.sol

@@ -5,4 +5,8 @@ contract C {
     function g() pure public returns (bytes4) {
         return this.f.selector ^ this.x.selector;
     }
+    function h() view public returns (bytes4) {
+        x;
+        return this.f.selector ^ this.x.selector;
+    }
 }

test/libsolidity/syntaxTests/viewPureChecker/write_storage_fail.sol

@@ -3,4 +3,4 @@ contract C {
     function f() view public { x = 2; }
 }
 // ----
-// Warning: (56-57): Function declared as view, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable.
+// TypeError: (56-57): Function declared as view, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable.

test/libsolidity/syntaxTests/viewPureChecker/write_storage_fail_v050.sol

@@ -1,7 +0,0 @@
-pragma experimental "v0.5.0";
-contract C {
-    uint x;
-    function f() view public { x = 2; }
-}
-// ----
-// TypeError: (86-87): Function declared as view, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable.