diff options
24 files changed, 183 insertions, 43 deletions
diff --git a/Changelog.md b/Changelog.md index b36bec2e..64207c56 100644 --- a/Changelog.md +++ b/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. diff --git a/docs/contracts.rst b/docs/contracts.rst index 41240a9c..53e50656 100644 --- a/docs/contracts.rst +++ b/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: diff --git a/docs/miscellaneous.rst b/docs/miscellaneous.rst index c19c8c59..1d5add9a 100644 --- a/docs/miscellaneous.rst +++ b/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. -- ``view`` for functions: Disallows modification of state - this is not enforced yet. +- ``pure`` for functions: Disallows modification or access of state. +- ``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. diff --git a/libsolidity/analysis/ViewPureChecker.cpp b/libsolidity/analysis/ViewPureChecker.cpp index 107eb3aa..18c642c3 100644 --- a/libsolidity/analysis/ViewPureChecker.cpp +++ b/libsolidity/analysis/ViewPureChecker.cpp @@ -116,31 +116,22 @@ private: bool ViewPureChecker::check() { - // The bool means "enforce view with errors". - vector<pair<ContractDefinition const*, bool>> contracts; + vector<ContractDefinition const*> 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); - for (ContractDefinition const* c: source->filteredNodes<ContractDefinition>(source->nodes())) - contracts.emplace_back(c, enforceView); + contracts += source->filteredNodes<ContractDefinition>(source->nodes()); } // Check modifiers first to infer their state mutability. for (auto const& contract: contracts) - { - m_enforceViewWithError = contract.second; - for (ModifierDefinition const* mod: contract.first->functionModifiers()) + for (ModifierDefinition const* mod: contract->functionModifiers()) mod->accept(*this); - } for (auto const& contract: contracts) - { - m_enforceViewWithError = contract.second; - contract.first->accept(*this); - } + contract->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_errorReporter.warning(_location, text); - else - { - m_errors = true; - m_errorReporter.typeError(_location, text); - } + m_errors = true; } if (_mutability > m_currentBestMutability) m_currentBestMutability = _mutability; diff --git a/libsolidity/analysis/ViewPureChecker.h b/libsolidity/analysis/ViewPureChecker.h index 0b882cd8..3db52e7e 100644 --- a/libsolidity/analysis/ViewPureChecker.h +++ b/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; diff --git a/test/libsolidity/syntaxTests/viewPureChecker/assembly_jump.sol b/test/libsolidity/syntaxTests/viewPureChecker/assembly_jump_no_restrict_warning.sol index 418be561..418be561 100644 --- a/test/libsolidity/syntaxTests/viewPureChecker/assembly_jump.sol +++ b/test/libsolidity/syntaxTests/viewPureChecker/assembly_jump_no_restrict_warning.sol diff --git a/test/libsolidity/syntaxTests/viewPureChecker/assembly_jump_view_fail.sol b/test/libsolidity/syntaxTests/viewPureChecker/assembly_jump_view_fail.sol new file mode 100644 index 00000000..c1729db7 --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/viewPureChecker/builtin_functions_restrict_warning.sol b/test/libsolidity/syntaxTests/viewPureChecker/builtin_functions_restrict_warning.sol new file mode 100644 index 00000000..0b834022 --- /dev/null +++ b/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 diff --git a/test/libsolidity/syntaxTests/viewPureChecker/builtin_functions_view_fail.sol b/test/libsolidity/syntaxTests/viewPureChecker/builtin_functions_view_fail.sol new file mode 100644 index 00000000..9b00fd6d --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/viewPureChecker/call_internal_functions_fail.sol b/test/libsolidity/syntaxTests/viewPureChecker/call_internal_functions_fail.sol index 22855c34..e21037bd 100644 --- a/test/libsolidity/syntaxTests/viewPureChecker/call_internal_functions_fail.sol +++ b/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". -// Warning: (55-82): Function state mutability can be restricted to pure +// TypeError: (56-59): Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view". +// TypeError: (130-133): Function declared as view, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable. diff --git a/test/libsolidity/syntaxTests/viewPureChecker/constant_restrict_warning.sol b/test/libsolidity/syntaxTests/viewPureChecker/constant_restrict_warning.sol new file mode 100644 index 00000000..a4b4a353 --- /dev/null +++ b/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 diff --git a/test/libsolidity/syntaxTests/viewPureChecker/creation.sol b/test/libsolidity/syntaxTests/viewPureChecker/creation_no_restrict_warning.sol index d80edd1b..d80edd1b 100644 --- a/test/libsolidity/syntaxTests/viewPureChecker/creation.sol +++ b/test/libsolidity/syntaxTests/viewPureChecker/creation_no_restrict_warning.sol diff --git a/test/libsolidity/syntaxTests/viewPureChecker/creation_view_fail.sol b/test/libsolidity/syntaxTests/viewPureChecker/creation_view_fail.sol new file mode 100644 index 00000000..08e45ea1 --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/viewPureChecker/function_types_fail.sol b/test/libsolidity/syntaxTests/viewPureChecker/function_types_fail.sol new file mode 100644 index 00000000..d00f65c9 --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/viewPureChecker/local_storage_variables_fail.sol b/test/libsolidity/syntaxTests/viewPureChecker/local_storage_variables_fail.sol new file mode 100644 index 00000000..0ff1ac24 --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/viewPureChecker/modifiers_fail.sol b/test/libsolidity/syntaxTests/viewPureChecker/modifiers_fail.sol new file mode 100644 index 00000000..513850f7 --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/viewPureChecker/overriding_fail.sol b/test/libsolidity/syntaxTests/viewPureChecker/overriding_fail.sol new file mode 100644 index 00000000..61702495 --- /dev/null +++ b/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". diff --git a/test/libsolidity/syntaxTests/viewPureChecker/overriding.sol b/test/libsolidity/syntaxTests/viewPureChecker/overriding_no_restrict_warning.sol index c82c7908..c82c7908 100644 --- a/test/libsolidity/syntaxTests/viewPureChecker/overriding.sol +++ b/test/libsolidity/syntaxTests/viewPureChecker/overriding_no_restrict_warning.sol diff --git a/test/libsolidity/syntaxTests/viewPureChecker/read_storage_pure_fail.sol b/test/libsolidity/syntaxTests/viewPureChecker/read_storage_pure_fail.sol new file mode 100644 index 00000000..785656b9 --- /dev/null +++ b/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". diff --git a/test/libsolidity/syntaxTests/viewPureChecker/returning_structs_fail.sol b/test/libsolidity/syntaxTests/viewPureChecker/returning_structs_fail.sol new file mode 100644 index 00000000..e04d0825 --- /dev/null +++ b/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". diff --git a/test/libsolidity/syntaxTests/viewPureChecker/returning_structs.sol b/test/libsolidity/syntaxTests/viewPureChecker/returning_structs_no_restrict_warning.sol index 9b4eb466..9b4eb466 100644 --- a/test/libsolidity/syntaxTests/viewPureChecker/returning_structs.sol +++ b/test/libsolidity/syntaxTests/viewPureChecker/returning_structs_no_restrict_warning.sol diff --git a/test/libsolidity/syntaxTests/viewPureChecker/selector.sol b/test/libsolidity/syntaxTests/viewPureChecker/selector.sol index 2ad4518d..c4e30075 100644 --- a/test/libsolidity/syntaxTests/viewPureChecker/selector.sol +++ b/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; + } } diff --git a/test/libsolidity/syntaxTests/viewPureChecker/write_storage_fail.sol b/test/libsolidity/syntaxTests/viewPureChecker/write_storage_fail.sol index 2a8bba31..3fed4d29 100644 --- a/test/libsolidity/syntaxTests/viewPureChecker/write_storage_fail.sol +++ b/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. diff --git a/test/libsolidity/syntaxTests/viewPureChecker/write_storage_fail_v050.sol b/test/libsolidity/syntaxTests/viewPureChecker/write_storage_fail_v050.sol deleted file mode 100644 index b85078ed..00000000 --- a/test/libsolidity/syntaxTests/viewPureChecker/write_storage_fail_v050.sol +++ /dev/null @@ -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. |