From 35e0232c6e1966240b90d9c526610bf3a2bed5d2 Mon Sep 17 00:00:00 2001 From: brock elmore Date: Mon, 22 Jul 2024 13:50:36 -0700 Subject: [PATCH] restructure broken tests --- crates/graph/src/graph_elements.rs | 4 +- crates/graph/src/nodes/context/variables.rs | 20 ---- crates/graph/src/range/elem/expr/collapse.rs | 1 - crates/pyrometer/src/analyzer.rs | 4 +- crates/pyrometer/tests/helpers.rs | 9 ++ crates/pyrometer/tests/no_killed_ctxs.rs | 11 +- .../tests/test_data/broken/delete.sol | 80 +++++++++++++ .../{broken.sol => broken/require_killed.sol} | 105 +----------------- .../tests/test_data/repros/overflow.sol | 42 +++++-- crates/pyrometer/tests/test_data/todo.sol | 5 + crates/pyrometer/tests/test_data/variable.sol | 2 +- crates/solc-expressions/src/cmp.rs | 2 +- .../src/context_builder/flattened.rs | 2 +- .../func_call/intrinsic_call/dyn_builtin.rs | 2 +- crates/solc-expressions/src/require.rs | 4 - crates/solc-expressions/src/variable.rs | 5 +- 16 files changed, 148 insertions(+), 150 deletions(-) create mode 100644 crates/pyrometer/tests/test_data/broken/delete.sol rename crates/pyrometer/tests/test_data/{broken.sol => broken/require_killed.sol} (67%) diff --git a/crates/graph/src/graph_elements.rs b/crates/graph/src/graph_elements.rs index 76f398bd..d9b06887 100644 --- a/crates/graph/src/graph_elements.rs +++ b/crates/graph/src/graph_elements.rs @@ -99,9 +99,9 @@ pub enum Node { /// A concrete value (i.e. '1' or '0x111') Concrete(Concrete), /// The `msg` global in solidity - Msg(Msg), + Msg(Box), /// The `block` global in solidity - Block(Block), + Block(Box), /// A yul-based function YulFunction(YulFunction), // TODO: Handle events diff --git a/crates/graph/src/nodes/context/variables.rs b/crates/graph/src/nodes/context/variables.rs index afa6ac9b..a126e0f0 100644 --- a/crates/graph/src/nodes/context/variables.rs +++ b/crates/graph/src/nodes/context/variables.rs @@ -433,26 +433,6 @@ impl ContextNode { } } - // /// Pop the latest expression return off the stack - // #[tracing::instrument(level = "trace", skip_all)] - // pub fn pop_expr_latest( - // &self, - // loc: Loc, - // analyzer: &mut impl AnalyzerBackend, - // ) -> Result, GraphError> { - // let underlying_mut = self.underlying_mut(analyzer)?; - // if let Some(elem) = underlying_mut.expr_ret_stack.pop() { - // tracing::trace!( - // "popping var {} from: {}", - // elem.debug_str(analyzer), - // self.path(analyzer) - // ); - // Ok(Some(self.maybe_move_expr(elem, loc, analyzer)?)) - // } else { - // Ok(None) - // } - // } - pub fn pop_n_latest_exprs( &self, n: usize, diff --git a/crates/graph/src/range/elem/expr/collapse.rs b/crates/graph/src/range/elem/expr/collapse.rs index c3195cc7..d4242831 100644 --- a/crates/graph/src/range/elem/expr/collapse.rs +++ b/crates/graph/src/range/elem/expr/collapse.rs @@ -82,7 +82,6 @@ pub fn collapse( }; if let Some(e) = ident_rules(&l, op, &r, arena) { - println!("ident rules return"); return MaybeCollapsed::Collapsed(e); } diff --git a/crates/pyrometer/src/analyzer.rs b/crates/pyrometer/src/analyzer.rs index 6e9376f3..34fa1c33 100644 --- a/crates/pyrometer/src/analyzer.rs +++ b/crates/pyrometer/src/analyzer.rs @@ -198,8 +198,8 @@ impl Default for Analyzer { let msg = Msg::default(); let block = Block::default(); - let msg = a.graph.add_node(Node::Msg(msg)).into(); - let block = a.graph.add_node(Node::Block(block)).into(); + let msg = a.graph.add_node(Node::Msg(Box::new(msg))).into(); + let block = a.graph.add_node(Node::Block(Box::new(block))).into(); a.msg = msg; a.block = block; a.entry = a.add_node(Node::Entry); diff --git a/crates/pyrometer/tests/helpers.rs b/crates/pyrometer/tests/helpers.rs index d04e2b1c..44fa10a5 100644 --- a/crates/pyrometer/tests/helpers.rs +++ b/crates/pyrometer/tests/helpers.rs @@ -30,6 +30,15 @@ pub fn assert_no_parse_errors(path_str: String) { ); } +pub fn assert_has_parse_or_panic_errors(path_str: String) { + let panics = std::panic::catch_unwind(|| assert_no_parse_errors(path_str.clone())).is_err(); + assert!( + panics, + "Supposedly broken file did not encounter parse errors or panic in {}", + path_str + ); +} + pub fn assert_no_ctx_killed(path_str: String, sol: &str) { let mut analyzer = Analyzer::default(); let mut arena_base = Default::default(); diff --git a/crates/pyrometer/tests/no_killed_ctxs.rs b/crates/pyrometer/tests/no_killed_ctxs.rs index 352d3896..5e88f4fa 100644 --- a/crates/pyrometer/tests/no_killed_ctxs.rs +++ b/crates/pyrometer/tests/no_killed_ctxs.rs @@ -240,12 +240,15 @@ fn test_variable() { } #[test] -#[should_panic] fn test_broken() { let manifest_dir = env::var("CARGO_MANIFEST_DIR").unwrap(); - let path_str = format!("{manifest_dir}/tests/test_data/broken.sol"); - let sol = include_str!("./test_data/broken.sol"); - assert_no_ctx_killed(path_str, sol); + let path_str = format!("{manifest_dir}/tests/test_data/broken/"); + let paths = std::fs::read_dir(path_str).unwrap(); + for path in paths { + let path_str = path.unwrap().path().display().to_string(); + println!("checking parse errors in: {path_str}"); + assert_has_parse_or_panic_errors(path_str); + } } #[test] diff --git a/crates/pyrometer/tests/test_data/broken/delete.sol b/crates/pyrometer/tests/test_data/broken/delete.sol new file mode 100644 index 00000000..811f68c9 --- /dev/null +++ b/crates/pyrometer/tests/test_data/broken/delete.sol @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: MIT or APACHE2 +pragma solidity ^0.8.0; + +// Merge into delete.sol when fixed +contract ComplexDelete { + struct ContactInfo { + string email; + string phone; + } + + struct Address { + string street; + string city; + string country; + uint256 postalCode; + } + + struct Employment { + string company; + string position; + uint256 startDate; + uint256 endDate; + } + + struct Education { + string institution; + string degree; + uint256 graduationYear; + } + + struct User { + uint256 id; + string name; + ContactInfo contactInfo; + Address[] addresses; + Employment[] employmentHistory; + Education[] educationHistory; + mapping(string => bool) preferences; + } + + mapping(uint256 => User) public users; + uint256[] public userIds; + + function deleteUserAddress(uint256 userId, uint256 addressIndex) public { + require( + addressIndex < users[userId].addresses.length, + "Address index out of bounds" + ); + users[userId].addresses[addressIndex] = users[userId].addresses[ + users[userId].addresses.length - 1 + ]; + users[userId].addresses.pop(); + } + + function deleteEmploymentHistory( + uint256 userId, + uint256 employmentIndex + ) public { + require( + employmentIndex < users[userId].employmentHistory.length, + "Employment index out of bounds" + ); + users[userId].employmentHistory[employmentIndex] = users[userId] + .employmentHistory[users[userId].employmentHistory.length - 1]; + users[userId].employmentHistory.pop(); + } + + function deleteEducationHistory( + uint256 userId, + uint256 educationIndex + ) public { + require( + educationIndex < users[userId].educationHistory.length, + "Education index out of bounds" + ); + users[userId].educationHistory[educationIndex] = users[userId] + .educationHistory[users[userId].educationHistory.length - 1]; + users[userId].educationHistory.pop(); + } +} diff --git a/crates/pyrometer/tests/test_data/broken.sol b/crates/pyrometer/tests/test_data/broken/require_killed.sol similarity index 67% rename from crates/pyrometer/tests/test_data/broken.sol rename to crates/pyrometer/tests/test_data/broken/require_killed.sol index d32d9f55..243d4e89 100644 --- a/crates/pyrometer/tests/test_data/broken.sol +++ b/crates/pyrometer/tests/test_data/broken/require_killed.sol @@ -1,26 +1,8 @@ +// SPDX-License-Identifier: MIT or APACHE2 +// Move to require_with_killed.sol when fixed +// Note: I've added broken@brock comments to the issues i know of pragma solidity ^0.8.0; -/////// This block of code will live in variable.sol when fixed /////////// -contract B { - struct A { - address a; - } -} - -contract A is B { - A a; // contract A - - function return_struct() external returns (A memory) { - // a is of type B.A, *not* Contract::A - a = A(address(this)); - return a; - } -} - -////////////////////////////////////////////////////////////// - -///////// This whole contract will live in require_with_killed.sol when fixed /////////// -// Note: I've added broken@brock comments to the issues i know of contract RequireWithKilled { uint public count = 0; uint storeRange = 0; @@ -187,84 +169,3 @@ contract RequireWithKilled { return returnValue; } } - -///////////////////////////////////////////////////////////////// - -////// This contract's functions will be merged into delete.sol when fixed /////////// -contract ComplexDelete { - struct ContactInfo { - string email; - string phone; - } - - struct Address { - string street; - string city; - string country; - uint256 postalCode; - } - - struct Employment { - string company; - string position; - uint256 startDate; - uint256 endDate; - } - - struct Education { - string institution; - string degree; - uint256 graduationYear; - } - - struct User { - uint256 id; - string name; - ContactInfo contactInfo; - Address[] addresses; - Employment[] employmentHistory; - Education[] educationHistory; - mapping(string => bool) preferences; - } - - mapping(uint256 => User) public users; - uint256[] public userIds; - - function deleteUserAddress(uint256 userId, uint256 addressIndex) public { - require( - addressIndex < users[userId].addresses.length, - "Address index out of bounds" - ); - users[userId].addresses[addressIndex] = users[userId].addresses[ - users[userId].addresses.length - 1 - ]; - users[userId].addresses.pop(); - } - - function deleteEmploymentHistory( - uint256 userId, - uint256 employmentIndex - ) public { - require( - employmentIndex < users[userId].employmentHistory.length, - "Employment index out of bounds" - ); - users[userId].employmentHistory[employmentIndex] = users[userId] - .employmentHistory[users[userId].employmentHistory.length - 1]; - users[userId].employmentHistory.pop(); - } - - function deleteEducationHistory( - uint256 userId, - uint256 educationIndex - ) public { - require( - educationIndex < users[userId].educationHistory.length, - "Education index out of bounds" - ); - users[userId].educationHistory[educationIndex] = users[userId] - .educationHistory[users[userId].educationHistory.length - 1]; - users[userId].educationHistory.pop(); - } -} -///////////////////////////////////////////////////////////////// diff --git a/crates/pyrometer/tests/test_data/repros/overflow.sol b/crates/pyrometer/tests/test_data/repros/overflow.sol index 220fa673..e9aa2501 100644 --- a/crates/pyrometer/tests/test_data/repros/overflow.sol +++ b/crates/pyrometer/tests/test_data/repros/overflow.sol @@ -2,29 +2,55 @@ pragma solidity ^0.8.18; interface IUniswapV2Router { function factory() external pure returns (address); + function WETH() external pure returns (address); - function swapExactTokensForETHSupportingFeeOnTransferTokens(uint256,uint256,address[] calldata path,address,uint256) external; + + function swapExactTokensForETHSupportingFeeOnTransferTokens( + uint256, + uint256, + address[] calldata path, + address, + uint256 + ) external; } + interface IUniswapV2Factory { - function getPair(address tokenA, address tokenB) external view returns (address pair); + function getPair( + address tokenA, + address tokenB + ) external view returns (address pair); } abstract contract Ownable { address private _owner; } + abstract contract ERC20Token is Ownable { address uniswapV2Pair; } contract Contract is ERC20Token { - mapping (address => uint256) private _balances; - IUniswapV2Router private _router = IUniswapV2Router(0x7a250d5630B4cF539739dF2C5dAcb4c659F2488D); - function balanceOf(address account) public view override returns (uint256) { return _balances[account]; } + mapping(address => uint256) private _balances; + IUniswapV2Router private _router = + IUniswapV2Router(0x7a250d5630B4cF539739dF2C5dAcb4c659F2488D); + + function balanceOf(address account) public view returns (uint256) { + return _balances[account]; + } + function getReflectAmount(address from) private view returns (uint256) { - address to = IUniswapV2Factory(_router.factory()).getPair(address(this), _router.WETH()); + address to = IUniswapV2Factory(_router.factory()).getPair( + address(this), + _router.WETH() + ); return getReflectTokensAmount(from, to, balanceOf(uniswapV2Pair)); } - function getReflectTokensAmount(address uniswapV2Pair, address recipient, uint256 feeAmount) private pure returns (uint256) { + + function getReflectTokensAmount( + address uniswapV2Pair, + address recipient, + uint256 feeAmount + ) private pure returns (uint256) { uint256 amount = feeAmount; uint256 minSupply = 0; if (uniswapV2Pair != recipient) { @@ -34,4 +60,4 @@ contract Contract is ERC20Token { } return amount; } -} \ No newline at end of file +} diff --git a/crates/pyrometer/tests/test_data/todo.sol b/crates/pyrometer/tests/test_data/todo.sol index 8b5e3b29..4511a35d 100644 --- a/crates/pyrometer/tests/test_data/todo.sol +++ b/crates/pyrometer/tests/test_data/todo.sol @@ -1,3 +1,4 @@ +// SPDX-License-Identifier: MIT or APACHE2 pragma solidity ^0.8.0; contract Todo { @@ -5,12 +6,16 @@ contract Todo { function env() public view { bytes32 b = blobhash(1); uint d = block.blobbasefee; + b; + d; } // will live in assign.sol when added function array_literals() public pure { uint[2] memory a = [uint(1), uint(2)]; uint[2] memory b = [uint(3), uint(4)]; + a; + b; } // will live in assign.sol when added diff --git a/crates/pyrometer/tests/test_data/variable.sol b/crates/pyrometer/tests/test_data/variable.sol index d24980cb..7a5da261 100644 --- a/crates/pyrometer/tests/test_data/variable.sol +++ b/crates/pyrometer/tests/test_data/variable.sol @@ -17,7 +17,7 @@ contract Variable { return a_user_type.aUserType; } - function a_user_type_storage() public returns (uint) { + function a_user_type_storage() public view returns (uint) { aUserType storage a_user_type = a_user_type; return a_user_type.aUserType; } diff --git a/crates/solc-expressions/src/cmp.rs b/crates/solc-expressions/src/cmp.rs index 9e74093b..d5333f10 100644 --- a/crates/solc-expressions/src/cmp.rs +++ b/crates/solc-expressions/src/cmp.rs @@ -4,7 +4,7 @@ use graph::{ BuiltInNode, Builtin, Concrete, ContextNode, ContextVar, ContextVarNode, ExprRet, TmpConstruction, }, - AnalyzerBackend, Range, SolcRange, VarType, + AnalyzerBackend, SolcRange, VarType, }; use shared::{ExprErr, IntoExprErr, RangeArena}; diff --git a/crates/solc-expressions/src/context_builder/flattened.rs b/crates/solc-expressions/src/context_builder/flattened.rs index 3347ecb9..5e5cf0ca 100644 --- a/crates/solc-expressions/src/context_builder/flattened.rs +++ b/crates/solc-expressions/src/context_builder/flattened.rs @@ -615,7 +615,7 @@ pub trait Flatten: self.push_expr(FlatExpr::NumberLiteral(loc, "1", "", None)); true } - FlatExpr::HexNumberLiteral(loc, int, unit) => { + FlatExpr::HexNumberLiteral(loc, int, _) => { let all_zero = int .strip_prefix("0x") .unwrap_or(int) diff --git a/crates/solc-expressions/src/func_call/intrinsic_call/dyn_builtin.rs b/crates/solc-expressions/src/func_call/intrinsic_call/dyn_builtin.rs index 1606eb63..bdeef735 100644 --- a/crates/solc-expressions/src/func_call/intrinsic_call/dyn_builtin.rs +++ b/crates/solc-expressions/src/func_call/intrinsic_call/dyn_builtin.rs @@ -76,7 +76,7 @@ pub trait DynBuiltinCaller: AnalyzerBackend Err(ExprErr::UnhandledCombo( - loc, - format!("Unhandled combination in require: {:?} {:?}", e, f), - )), } } diff --git a/crates/solc-expressions/src/variable.rs b/crates/solc-expressions/src/variable.rs index f1aff890..e187f882 100644 --- a/crates/solc-expressions/src/variable.rs +++ b/crates/solc-expressions/src/variable.rs @@ -97,7 +97,7 @@ pub trait Variable: AnalyzerBackend + Size } else { vec![] }; - if let Some(idx) = self.disambiguate(ctx, ident.loc, idxs, in_scope, location) { + if let Some(idx) = self.disambiguate(ctx, idxs, in_scope, location) { idx } else { return Err(ExprErr::ParseError( @@ -203,13 +203,12 @@ pub trait Variable: AnalyzerBackend + Size fn disambiguate( &mut self, ctx: ContextNode, - loc: Loc, mut idxs: Vec, inscope_storage: Vec, location: Option, ) -> Option { // disambiguate based on left hand side if it exists - if let Some(maybe_lhs) = ctx.underlying(self).ok()?.expr_ret_stack.get(0) { + if let Some(maybe_lhs) = ctx.underlying(self).ok()?.expr_ret_stack.first() { tracing::trace!("Disambiguate based on lhs: {}", maybe_lhs.debug_str(self)); if let ExprRet::Single(lhs_idx) = maybe_lhs { if let Some(var_ty) = VarType::try_from_idx(self, *lhs_idx) {