diff --git a/src/sema/mutability.rs b/src/sema/mutability.rs index b38f82153..b2b29585e 100644 --- a/src/sema/mutability.rs +++ b/src/sema/mutability.rs @@ -1,19 +1,36 @@ // SPDX-License-Identifier: Apache-2.0 -use super::ast::{ - Builtin, DestructureField, Diagnostic, Expression, Function, Mutability, Namespace, Statement, - Type, +use super::{ + ast::{ + Builtin, CallTy, DestructureField, Diagnostic, Expression, Function, Mutability, Namespace, + RetrieveType, Statement, Type, + }, + yul::ast::{YulExpression, YulStatement}, + Recurse, }; -use crate::sema::ast::RetrieveType; -use crate::sema::yul::ast::{YulExpression, YulStatement}; -use crate::sema::Recurse; -use solang_parser::pt; +use solang_parser::{helpers::CodeLocation, pt}; + +#[derive(PartialEq, PartialOrd)] +enum Access { + None, + Read, + Write, + Value, +} + +impl Access { + fn increase_to(&mut self, other: Access) { + if *self < other { + *self = other; + } + } +} /// check state mutability pub fn mutability(file_no: usize, ns: &mut Namespace) { if !ns.diagnostics.any_errors() { for func in &ns.functions { - if func.loc.try_file_no() != Some(file_no) { + if func.loc.try_file_no() != Some(file_no) || func.ty == pt::FunctionTy::Modifier { continue; } @@ -27,41 +44,90 @@ pub fn mutability(file_no: usize, ns: &mut Namespace) { /// While we recurse through the AST, maintain some state struct StateCheck<'a> { diagnostics: Vec, - does_read_state: bool, - does_write_state: bool, - can_read_state: bool, - can_write_state: bool, + declared_access: Access, + required_access: Access, func: &'a Function, + modifier: Option, ns: &'a Namespace, } impl<'a> StateCheck<'a> { + fn value(&mut self, loc: &pt::Loc) { + if self.declared_access != Access::Value { + if let Some(modifier_loc) = &self.modifier { + self.diagnostics.push(Diagnostic::error_with_note( + *modifier_loc, + format!( + "function declared '{}' but modifier accesses value sent, which is only allowed for payable functions", + self.func.mutability + ), + *loc, + "access of value sent".into() + )); + } else { + self.diagnostics.push(Diagnostic::error( + *loc, + format!( + "function declared '{}' but this expression accesses value sent, which is only allowed for payable functions", + self.func.mutability + ), + )); + } + } + + self.required_access.increase_to(Access::Value); + } + fn write(&mut self, loc: &pt::Loc) { - if !self.can_write_state { - self.diagnostics.push(Diagnostic::error( - *loc, - format!( - "function declared '{}' but this expression writes to state", - self.func.mutability - ), - )); + if self.declared_access < Access::Write { + if let Some(modifier_loc) = &self.modifier { + self.diagnostics.push(Diagnostic::error_with_note( + *modifier_loc, + format!( + "function declared '{}' but modifier writes to state", + self.func.mutability + ), + *loc, + "write to state".into(), + )); + } else { + self.diagnostics.push(Diagnostic::error( + *loc, + format!( + "function declared '{}' but this expression writes to state", + self.func.mutability + ), + )); + } } - self.does_write_state = true; + self.required_access.increase_to(Access::Write); } fn read(&mut self, loc: &pt::Loc) { - if !self.can_read_state { - self.diagnostics.push(Diagnostic::error( - *loc, - format!( - "function declared '{}' but this expression reads from state", - self.func.mutability - ), - )); + if self.declared_access < Access::Read { + if let Some(modifier_loc) = &self.modifier { + self.diagnostics.push(Diagnostic::error_with_note( + *modifier_loc, + format!( + "function declared '{}' but modifier reads from state", + self.func.mutability + ), + *loc, + "read to state".into(), + )); + } else { + self.diagnostics.push(Diagnostic::error( + *loc, + format!( + "function declared '{}' but this expression reads from state", + self.func.mutability + ), + )); + } } - self.does_read_state = true; + self.required_access.increase_to(Access::Read); } } @@ -72,25 +138,18 @@ fn check_mutability(func: &Function, ns: &Namespace) -> Vec { let mut state = StateCheck { diagnostics: Vec::new(), - does_read_state: false, - does_write_state: false, - can_write_state: false, - can_read_state: false, + declared_access: match func.mutability { + Mutability::Pure(_) => Access::None, + Mutability::View(_) => Access::Read, + Mutability::Nonpayable(_) => Access::Write, + Mutability::Payable(_) => Access::Value, + }, + required_access: Access::None, func, + modifier: None, ns, }; - match func.mutability { - Mutability::Pure(_) => (), - Mutability::View(_) => { - state.can_read_state = true; - } - Mutability::Payable(_) | Mutability::Nonpayable(_) => { - state.can_read_state = true; - state.can_write_state = true; - } - }; - for arg in &func.modifiers { if let Expression::InternalFunctionCall { function, args, .. } = &arg { // check the arguments to the modifiers @@ -118,7 +177,11 @@ fn check_mutability(func: &Function, ns: &Namespace) -> Vec { // modifiers do not have mutability, bases or modifiers itself let func = &ns.functions[function_no]; + state.modifier = Some(arg.loc()); + recurse_statements(&func.body, ns, &mut state); + + state.modifier = None; } } } @@ -126,7 +189,7 @@ fn check_mutability(func: &Function, ns: &Namespace) -> Vec { recurse_statements(&func.body, ns, &mut state); if pt::FunctionTy::Function == func.ty && !func.is_accessor { - if !state.does_write_state && !state.does_read_state { + if state.required_access == Access::None { match func.mutability { Mutability::Payable(_) | Mutability::Pure(_) => (), Mutability::Nonpayable(_) => { @@ -147,7 +210,8 @@ fn check_mutability(func: &Function, ns: &Namespace) -> Vec { } } - if !state.does_write_state && state.does_read_state && func.mutability.is_default() { + // don't suggest marking payable as view (declared_access == Value) + if state.required_access == Access::Read && state.declared_access == Access::Write { state.diagnostics.push(Diagnostic::warning( func.loc, "function can be declared 'view'".to_string(), @@ -293,6 +357,19 @@ fn read_expression(expr: &Expression, state: &mut StateCheck) -> bool { kind: Builtin::PayableSend | Builtin::PayableTransfer | Builtin::SelfDestruct, .. } => state.write(loc), + Expression::Builtin { + loc, + kind: Builtin::Value, + .. + } => { + // internal/private functions cannot be declared payable, so msg.value is only checked + // as reading state in private/internal functions in solc. + if state.func.is_public() { + state.value(loc) + } else { + state.read(loc) + } + } Expression::Builtin { loc, kind: Builtin::ArrayPush | Builtin::ArrayPop, @@ -315,13 +392,10 @@ fn read_expression(expr: &Expression, state: &mut StateCheck) -> bool { } _ => unreachable!(), }, - Expression::ExternalFunctionCallRaw { loc, .. } => { - if state.ns.target.is_substrate() { - state.write(loc) - } else { - state.read(loc) - } - } + Expression::ExternalFunctionCallRaw { loc, ty, .. } => match ty { + CallTy::Static => state.read(loc), + CallTy::Delegate | CallTy::Regular => state.write(loc), + }, _ => { return true; } diff --git a/tests/contract_testcases/evm/call/call_02.sol b/tests/contract_testcases/evm/call/call_02.sol index 31cbb6a78..8d2e81f98 100644 --- a/tests/contract_testcases/evm/call/call_02.sol +++ b/tests/contract_testcases/evm/call/call_02.sol @@ -4,8 +4,7 @@ (bool s, bytes memory bs) = a.call{value: 2}(""); } } - + // ---- Expect: diagnostics ---- -// warning: 3:13-49: function can be declared 'view' // warning: 4:23-24: destructure variable 's' has never been used // warning: 4:39-41: destructure variable 'bs' has never been used diff --git a/tests/contract_testcases/evm/comment_tests.sol b/tests/contract_testcases/evm/comment_tests.sol index 2cd024249..23e94940f 100644 --- a/tests/contract_testcases/evm/comment_tests.sol +++ b/tests/contract_testcases/evm/comment_tests.sol @@ -678,9 +678,7 @@ function _approve( // }/**//**//**//**//**//**//**/// // ---- Expect: diagnostics ---- -// warning: 188:5-75: function can be declared 'view' // warning: 195:50-56: conversion truncates uint256 to uint128, as value is type uint128 on target evm -// warning: 264:5-270:37: function can be declared 'view' // warning: 268:17-25: function parameter 'weiValue' is unused // warning: 269:23-35: function parameter 'errorMessage' is unused // warning: 276:70-78: conversion truncates uint256 to uint128, as value is type uint128 on target evm diff --git a/tests/contract_testcases/substrate/builtins/msg_01.sol b/tests/contract_testcases/substrate/builtins/msg_01.sol index c67da753f..30514742b 100644 --- a/tests/contract_testcases/substrate/builtins/msg_01.sol +++ b/tests/contract_testcases/substrate/builtins/msg_01.sol @@ -1,8 +1,8 @@ - - contract bar { - function test(uint128 v) public returns (bool) { - return msg.value > v; - } - } +contract bar { + uint128 state; + function test(uint128 v) public returns (bool) { + return state > v; + } +} // ---- Expect: diagnostics ---- -// warning: 3:13-59: function can be declared 'pure' +// warning: 3:2-48: function can be declared 'view' diff --git a/tests/contract_testcases/substrate/functions/mutability_12.sol b/tests/contract_testcases/substrate/functions/mutability_12.sol new file mode 100644 index 000000000..43cb1f6fc --- /dev/null +++ b/tests/contract_testcases/substrate/functions/mutability_12.sol @@ -0,0 +1,14 @@ +contract c { + // private functions cannot be payable and solc checks msg.value as + // state read in them + function foo() private view returns (uint) { + return msg.value; + } + + function bar() public returns (uint) { + return foo(); + } +} + +// ---- Expect: diagnostics ---- +// warning: 8:2-38: function can be declared 'view' diff --git a/tests/contract_testcases/substrate/modifier/mutability_01.sol b/tests/contract_testcases/substrate/modifier/mutability_01.sol index 97956c082..6c8f84b59 100644 --- a/tests/contract_testcases/substrate/modifier/mutability_01.sol +++ b/tests/contract_testcases/substrate/modifier/mutability_01.sol @@ -1,10 +1,18 @@ +contract c { + uint128 var; + modifier foo1() { uint128 x = var; _; } + modifier foo2() { var = 2; _; } + modifier foo3() { var = msg.value; _; } - contract c { - uint64 var; - modifier foo() { uint64 x = var; _; } - - function bar() foo() public pure {} - } + function bar1() foo1() public pure {} + function bar2() foo2() public view {} + function bar3() foo3() public {} +} // ---- Expect: diagnostics ---- -// warning: 4:37-38: local variable 'x' has been assigned, but never read -// error: 4:41-44: function declared 'pure' but this expression reads from state +// warning: 3:31-32: local variable 'x' has been assigned, but never read +// error: 7:21-27: function declared 'pure' but modifier reads from state +// note 3:35-38: read to state +// error: 8:21-27: function declared 'view' but modifier writes to state +// note 4:23-26: write to state +// error: 9:21-27: function declared 'nonpayable' but modifier accesses value sent, which is only allowed for payable functions +// note 5:29-38: access of value sent diff --git a/tests/evm.rs b/tests/evm.rs index 704e44616..0783ed768 100644 --- a/tests/evm.rs +++ b/tests/evm.rs @@ -248,7 +248,7 @@ fn ethereum_solidity_tests() { }) .sum(); - assert_eq!(errors, 1048); + assert_eq!(errors, 1042); } fn set_file_contents(source: &str, path: &Path) -> (FileResolver, Vec) {