From e86c778192fcd5a7cbf7dc71f5fc1a7c7044f763 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Thu, 28 Mar 2024 04:10:59 +0400 Subject: [PATCH 01/10] fix: coverage for internal libraries --- crates/evm/coverage/src/analysis.rs | 186 ++++------------------------ crates/evm/coverage/src/anchors.rs | 40 +++--- crates/forge/bin/cmd/coverage.rs | 8 +- 3 files changed, 42 insertions(+), 192 deletions(-) diff --git a/crates/evm/coverage/src/analysis.rs b/crates/evm/coverage/src/analysis.rs index 1926d2b4ed20..0c77063d400c 100644 --- a/crates/evm/coverage/src/analysis.rs +++ b/crates/evm/coverage/src/analysis.rs @@ -1,9 +1,8 @@ use super::{ContractId, CoverageItem, CoverageItemKind, SourceLocation}; -use foundry_common::TestFunctionExt; use foundry_compilers::artifacts::ast::{self, Ast, Node, NodeType}; use rustc_hash::FxHashMap; use semver::Version; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; /// A visitor that walks the AST of a single contract and finds coverage items. #[derive(Clone, Debug)] @@ -23,36 +22,14 @@ pub struct ContractVisitor<'a> { /// Coverage items pub items: Vec, - - /// Node IDs of this contract's base contracts, as well as IDs for referenced contracts such as - /// libraries - pub base_contract_node_ids: HashSet, } impl<'a> ContractVisitor<'a> { pub fn new(source_id: usize, source: &'a str, contract_name: String) -> Self { - Self { - source_id, - source, - contract_name, - branch_id: 0, - last_line: 0, - items: Vec::new(), - base_contract_node_ids: HashSet::new(), - } + Self { source_id, source, contract_name, branch_id: 0, last_line: 0, items: Vec::new() } } pub fn visit(mut self, contract_ast: Node) -> eyre::Result { - let linearized_base_contracts: Vec = - contract_ast.attribute("linearizedBaseContracts").ok_or_else(|| { - eyre::eyre!( - "The contract's AST node is missing a list of linearized base contracts" - ) - })?; - - // We skip the first ID because that's the ID of the contract itself - self.base_contract_node_ids.extend(&linearized_base_contracts[1..]); - // Find all functions and walk their AST for node in contract_ast.nodes { if node.node_type == NodeType::FunctionDefinition { @@ -327,15 +304,6 @@ impl<'a> ContractVisitor<'a> { self.branch_id += 1; } } - // Might be a call to a library - Some(NodeType::MemberAccess) => { - let referenced_declaration_id = expr - .and_then(|expr| expr.attribute("expression")) - .and_then(|subexpr: Node| subexpr.attribute("referencedDeclaration")); - if let Some(id) = referenced_declaration_id { - self.base_contract_node_ids.insert(id); - } - } _ => (), } @@ -435,9 +403,6 @@ impl<'a> ContractVisitor<'a> { pub struct SourceAnalysis { /// A collection of coverage items. pub items: Vec, - /// A mapping of contract IDs to item IDs relevant to the contract (including items in base - /// contracts). - pub contract_items: HashMap>, } /// Analyzes a set of sources to find coverage items. @@ -451,10 +416,6 @@ pub struct SourceAnalyzer { contracts: HashMap, /// A collection of coverage items. items: Vec, - /// A map of contract IDs to item IDs. - contract_items: HashMap>, - /// A map of contracts to their base contracts - contract_bases: HashMap>, } impl SourceAnalyzer { @@ -497,11 +458,7 @@ impl SourceAnalyzer { /// /// Coverage items are found by: /// - Walking the AST of each contract (except interfaces) - /// - Recording the items and base contracts of each contract - /// - /// Finally, the item IDs of each contract and its base contracts are flattened, and the return - /// value represents all coverage items in the project, along with a mapping of contract IDs to - /// item IDs. + /// - Recording the items of each contract /// /// Each coverage item contains relevant information to find opcodes corresponding to them: the /// source ID the item is in, the source code range of the item, and the contract name the item @@ -511,127 +468,26 @@ impl SourceAnalyzer { /// two different solc versions will produce overlapping source IDs if the compiler version is /// not taken into account. pub fn analyze(mut self) -> eyre::Result { - // Analyze the contracts - self.analyze_contracts()?; - - // Flatten the data - let mut flattened: HashMap> = HashMap::new(); - for contract_id in self.contract_items.keys() { - let mut item_ids: Vec = Vec::new(); - - // - // for a specific contract (id == contract_id): - // - // self.contract_bases.get(contract_id) includes the following contracts: - // 1. all the ancestors of this contract (including parent, grandparent, ... - // contracts) - // 2. the libraries **directly** used by this contract - // - // The missing contracts are: - // 1. libraries used in ancestors of this contracts - // 2. libraries used in libraries (i.e libs indirectly used by this contract) - // - // We want to find out all the above contracts and libraries related to this contract. - - for contract_or_lib in { - // A set of contracts and libraries related to this contract (will include "this" - // contract itself) - let mut contracts_libraries: HashSet<&ContractId> = HashSet::new(); - - // we use a stack for depth-first search. - let mut stack: Vec<&ContractId> = Vec::new(); - - // push "this" contract onto the stack - stack.push(contract_id); - - while let Some(contract_or_lib) = stack.pop() { - // whenever a contract_or_lib is removed from the stack, it is added to the set - contracts_libraries.insert(contract_or_lib); - - // push all ancestors of contract_or_lib and libraries used by contract_or_lib - // onto the stack - if let Some(bases) = self.contract_bases.get(contract_or_lib) { - stack.extend( - bases.iter().filter(|base| !contracts_libraries.contains(base)), - ); - } - } - - contracts_libraries - } { - // get items of each contract or library - if let Some(items) = self.contract_items.get(contract_or_lib) { - item_ids.extend(items.iter()); - } - } - - // If there are no items for this contract, then it was most likely filtered - if !item_ids.is_empty() { - flattened.insert(contract_id.clone(), item_ids); - } - } - - Ok(SourceAnalysis { items: self.items.clone(), contract_items: flattened }) - } - - fn analyze_contracts(&mut self) -> eyre::Result<()> { for contract_id in self.contracts.keys() { - // Find this contract's coverage items if we haven't already - if !self.contract_items.contains_key(contract_id) { - let ContractVisitor { items, base_contract_node_ids, .. } = ContractVisitor::new( - contract_id.source_id, - self.sources.get(&contract_id.source_id).unwrap_or_else(|| { - panic!( - "We should have the source code for source ID {}", - contract_id.source_id - ) - }), - contract_id.contract_name.clone(), - ) - .visit( - self.contracts - .get(contract_id) - .unwrap_or_else(|| { - panic!("We should have the AST of contract: {contract_id:?}") - }) - .clone(), - )?; - - let is_test = items.iter().any(|item| { - if let CoverageItemKind::Function { name } = &item.kind { - name.is_test() - } else { - false - } - }); - - // Record this contract's base contracts - // We don't do this for test contracts because we don't care about them - if !is_test { - self.contract_bases.insert( - contract_id.clone(), - base_contract_node_ids - .iter() - .filter_map(|base_contract_node_id| { - self.contract_ids.get(base_contract_node_id).cloned() - }) - .collect(), - ); - } - - // For tests and contracts with no items we still record an empty Vec so we don't - // end up here again - if items.is_empty() || is_test { - self.contract_items.insert(contract_id.clone(), Vec::new()); - } else { - let item_ids: Vec = - (self.items.len()..self.items.len() + items.len()).collect(); - self.items.extend(items); - self.contract_items.insert(contract_id.clone(), item_ids.clone()); - } - } + let ContractVisitor { items, .. } = ContractVisitor::new( + contract_id.source_id, + self.sources.get(&contract_id.source_id).unwrap_or_else(|| { + panic!("We should have the source code for source ID {}", contract_id.source_id) + }), + contract_id.contract_name.clone(), + ) + .visit( + self.contracts + .get(contract_id) + .unwrap_or_else(|| { + panic!("We should have the AST of contract: {contract_id:?}") + }) + .clone(), + )?; + + self.items.extend(items); } - Ok(()) + Ok(SourceAnalysis { items: self.items }) } } diff --git a/crates/evm/coverage/src/anchors.rs b/crates/evm/coverage/src/anchors.rs index 237d941d1a3d..704471299f17 100644 --- a/crates/evm/coverage/src/anchors.rs +++ b/crates/evm/coverage/src/anchors.rs @@ -12,36 +12,32 @@ pub fn find_anchors( bytecode: &Bytes, source_map: &SourceMap, ic_pc_map: &IcPcMap, - item_ids: &[usize], items: &[CoverageItem], ) -> Vec { - item_ids + items .iter() - .filter_map(|item_id| { - let item = items.get(*item_id)?; - - match item.kind { - CoverageItemKind::Branch { path_id, .. } => { - match find_anchor_branch(bytecode, source_map, *item_id, &item.loc) { - Ok(anchors) => match path_id { - 0 => Some(anchors.0), - 1 => Some(anchors.1), - _ => panic!("Too many paths for branch"), - }, - Err(e) => { - warn!("Could not find anchor for item: {}, error: {e}", item); - None - } - } - } - _ => match find_anchor_simple(source_map, ic_pc_map, *item_id, &item.loc) { - Ok(anchor) => Some(anchor), + .enumerate() + .filter_map(|(item_id, item)| match item.kind { + CoverageItemKind::Branch { path_id, .. } => { + match find_anchor_branch(bytecode, source_map, item_id, &item.loc) { + Ok(anchors) => match path_id { + 0 => Some(anchors.0), + 1 => Some(anchors.1), + _ => panic!("Too many paths for branch"), + }, Err(e) => { warn!("Could not find anchor for item: {}, error: {e}", item); None } - }, + } } + _ => match find_anchor_simple(source_map, ic_pc_map, item_id, &item.loc) { + Ok(anchor) => Some(anchor), + Err(e) => { + warn!("Could not find anchor for item: {}, error: {e}", item); + None + } + }, }) .collect() } diff --git a/crates/forge/bin/cmd/coverage.rs b/crates/forge/bin/cmd/coverage.rs index e140b8436aaf..7c1427a049d6 100644 --- a/crates/forge/bin/cmd/coverage.rs +++ b/crates/forge/bin/cmd/coverage.rs @@ -257,18 +257,16 @@ impl CoverageArgs { })?, )? .analyze()?; - let anchors: HashMap> = source_analysis - .contract_items + let anchors: HashMap> = source_maps .iter() - .filter_map(|(contract_id, item_ids)| { + .filter_map(|(contract_id, (_, deployed_source_map))| { // TODO: Creation source map/bytecode as well Some(( contract_id.clone(), find_anchors( &bytecodes.get(contract_id)?.1, - &source_maps.get(contract_id)?.1, + deployed_source_map, &ic_pc_maps.get(contract_id)?.1, - item_ids, &source_analysis.items, ), )) From 8906c4e1f6afedbfcb319af19aa7c9daab0219ca Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Thu, 28 Mar 2024 05:51:53 +0400 Subject: [PATCH 02/10] optimize --- crates/evm/coverage/src/anchors.rs | 59 +++++++++++++++++++++--------- crates/forge/bin/cmd/coverage.rs | 1 + 2 files changed, 42 insertions(+), 18 deletions(-) diff --git a/crates/evm/coverage/src/anchors.rs b/crates/evm/coverage/src/anchors.rs index 704471299f17..b752fdb1555f 100644 --- a/crates/evm/coverage/src/anchors.rs +++ b/crates/evm/coverage/src/anchors.rs @@ -1,10 +1,12 @@ +use std::collections::HashMap; + use super::{CoverageItem, CoverageItemKind, ItemAnchor, SourceLocation}; use alloy_primitives::Bytes; use foundry_compilers::sourcemap::{SourceElement, SourceMap}; use foundry_evm_core::utils::IcPcMap; use revm::{ interpreter::opcode::{self, spec_opcode_gas}, - primitives::SpecId, + primitives::{HashSet, SpecId}, }; /// Attempts to find anchors for the given items using the given source map and bytecode. @@ -14,30 +16,51 @@ pub fn find_anchors( ic_pc_map: &IcPcMap, items: &[CoverageItem], ) -> Vec { - items + // Build helper mapping + // source_id -> [item_id] + let items_map = items .iter() .enumerate() - .filter_map(|(item_id, item)| match item.kind { - CoverageItemKind::Branch { path_id, .. } => { - match find_anchor_branch(bytecode, source_map, item_id, &item.loc) { - Ok(anchors) => match path_id { - 0 => Some(anchors.0), - 1 => Some(anchors.1), - _ => panic!("Too many paths for branch"), - }, + .map(|(item_id, item)| (item.loc.source_id, item_id)) + .fold(HashMap::new(), |mut map, (source_id, item_id)| { + map.entry(source_id).or_insert_with(Vec::new).push(item_id); + map + }); + + // Prepare coverage items from all sources referenced in the source map + let potential_item_ids = source_map + .iter() + .filter_map(|element| Some(items_map.get(&(element.index? as usize))?)) + .flatten() + .collect::>(); + + potential_item_ids + .into_iter() + .filter_map(|item_id| { + let item = &items[*item_id]; + + match item.kind { + CoverageItemKind::Branch { path_id, .. } => { + match find_anchor_branch(bytecode, source_map, *item_id, &item.loc) { + Ok(anchors) => match path_id { + 0 => Some(anchors.0), + 1 => Some(anchors.1), + _ => panic!("Too many paths for branch"), + }, + Err(e) => { + warn!("Could not find anchor for item: {}, error: {e}", item); + None + } + } + } + _ => match find_anchor_simple(source_map, ic_pc_map, *item_id, &item.loc) { + Ok(anchor) => Some(anchor), Err(e) => { warn!("Could not find anchor for item: {}, error: {e}", item); None } - } + }, } - _ => match find_anchor_simple(source_map, ic_pc_map, item_id, &item.loc) { - Ok(anchor) => Some(anchor), - Err(e) => { - warn!("Could not find anchor for item: {}, error: {e}", item); - None - } - }, }) .collect() } diff --git a/crates/forge/bin/cmd/coverage.rs b/crates/forge/bin/cmd/coverage.rs index 7c1427a049d6..0dd17f545ed3 100644 --- a/crates/forge/bin/cmd/coverage.rs +++ b/crates/forge/bin/cmd/coverage.rs @@ -257,6 +257,7 @@ impl CoverageArgs { })?, )? .analyze()?; + let anchors: HashMap> = source_maps .iter() .filter_map(|(contract_id, (_, deployed_source_map))| { From 6e5fdb62ff5d88f06fd1fef8c720b0ed1fbfdf11 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Thu, 28 Mar 2024 05:55:32 +0400 Subject: [PATCH 03/10] optimize --- crates/evm/coverage/src/anchors.rs | 14 ++------------ crates/forge/bin/cmd/coverage.rs | 13 +++++++++++++ 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/crates/evm/coverage/src/anchors.rs b/crates/evm/coverage/src/anchors.rs index b752fdb1555f..5e05f2520c01 100644 --- a/crates/evm/coverage/src/anchors.rs +++ b/crates/evm/coverage/src/anchors.rs @@ -15,22 +15,12 @@ pub fn find_anchors( source_map: &SourceMap, ic_pc_map: &IcPcMap, items: &[CoverageItem], + items_by_source_id: &HashMap>, ) -> Vec { - // Build helper mapping - // source_id -> [item_id] - let items_map = items - .iter() - .enumerate() - .map(|(item_id, item)| (item.loc.source_id, item_id)) - .fold(HashMap::new(), |mut map, (source_id, item_id)| { - map.entry(source_id).or_insert_with(Vec::new).push(item_id); - map - }); - // Prepare coverage items from all sources referenced in the source map let potential_item_ids = source_map .iter() - .filter_map(|element| Some(items_map.get(&(element.index? as usize))?)) + .filter_map(|element| Some(items_by_source_id.get(&(element.index? as usize))?)) .flatten() .collect::>(); diff --git a/crates/forge/bin/cmd/coverage.rs b/crates/forge/bin/cmd/coverage.rs index 0dd17f545ed3..cf9db39a9a18 100644 --- a/crates/forge/bin/cmd/coverage.rs +++ b/crates/forge/bin/cmd/coverage.rs @@ -258,6 +258,18 @@ impl CoverageArgs { )? .analyze()?; + // Build helper mapping + // source_id -> [item_id] + let items_by_source_id = source_analysis + .items + .iter() + .enumerate() + .map(|(item_id, item)| (item.loc.source_id, item_id)) + .fold(HashMap::new(), |mut map, (source_id, item_id)| { + map.entry(source_id).or_insert_with(Vec::new).push(item_id); + map + }); + let anchors: HashMap> = source_maps .iter() .filter_map(|(contract_id, (_, deployed_source_map))| { @@ -269,6 +281,7 @@ impl CoverageArgs { deployed_source_map, &ic_pc_maps.get(contract_id)?.1, &source_analysis.items, + &items_by_source_id, ), )) }) From 2ef0c2e87026cdc2d18586a01a1ec0496037d41e Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Thu, 28 Mar 2024 05:59:51 +0400 Subject: [PATCH 04/10] doc --- crates/forge/bin/cmd/coverage.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/forge/bin/cmd/coverage.rs b/crates/forge/bin/cmd/coverage.rs index cf9db39a9a18..c552ef1e869d 100644 --- a/crates/forge/bin/cmd/coverage.rs +++ b/crates/forge/bin/cmd/coverage.rs @@ -258,8 +258,7 @@ impl CoverageArgs { )? .analyze()?; - // Build helper mapping - // source_id -> [item_id] + // Build helper mapping used by `find_anchors` let items_by_source_id = source_analysis .items .iter() From 34f9923838b7bd6c62a7dd6bf76bf2e7e9ae8207 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Thu, 28 Mar 2024 06:29:54 +0400 Subject: [PATCH 05/10] rm tests --- crates/evm/coverage/src/analysis.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/crates/evm/coverage/src/analysis.rs b/crates/evm/coverage/src/analysis.rs index 0c77063d400c..fd2ecf1325b3 100644 --- a/crates/evm/coverage/src/analysis.rs +++ b/crates/evm/coverage/src/analysis.rs @@ -1,4 +1,5 @@ use super::{ContractId, CoverageItem, CoverageItemKind, SourceLocation}; +use foundry_common::TestFunctionExt; use foundry_compilers::artifacts::ast::{self, Ast, Node, NodeType}; use rustc_hash::FxHashMap; use semver::Version; @@ -485,7 +486,17 @@ impl SourceAnalyzer { .clone(), )?; - self.items.extend(items); + let is_test = items.iter().any(|item| { + if let CoverageItemKind::Function { name } = &item.kind { + name.is_test() + } else { + false + } + }); + + if !is_test { + self.items.extend(items); + } } Ok(SourceAnalysis { items: self.items }) From c9dfe41e6b192c8d6f54283709a64f839a29e93a Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Thu, 28 Mar 2024 06:51:22 +0400 Subject: [PATCH 06/10] clippy --- crates/forge/bin/cmd/coverage.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/forge/bin/cmd/coverage.rs b/crates/forge/bin/cmd/coverage.rs index c552ef1e869d..f55260b712f4 100644 --- a/crates/forge/bin/cmd/coverage.rs +++ b/crates/forge/bin/cmd/coverage.rs @@ -264,8 +264,8 @@ impl CoverageArgs { .iter() .enumerate() .map(|(item_id, item)| (item.loc.source_id, item_id)) - .fold(HashMap::new(), |mut map, (source_id, item_id)| { - map.entry(source_id).or_insert_with(Vec::new).push(item_id); + .fold(HashMap::new(), |mut map: HashMap>, (source_id, item_id)| { + map.entry(source_id).or_default().push(item_id); map }); From 4e85dee75dc5ee8d6106c217a7d3cc58c0ffb825 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Thu, 28 Mar 2024 07:05:00 +0400 Subject: [PATCH 07/10] clippy + fmt --- crates/evm/coverage/src/analysis.rs | 13 +++++-------- crates/evm/coverage/src/anchors.rs | 2 +- crates/forge/bin/cmd/coverage.rs | 11 +++++++---- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/crates/evm/coverage/src/analysis.rs b/crates/evm/coverage/src/analysis.rs index fd2ecf1325b3..4d57b91ee3a2 100644 --- a/crates/evm/coverage/src/analysis.rs +++ b/crates/evm/coverage/src/analysis.rs @@ -296,16 +296,13 @@ impl<'a> ContractVisitor<'a> { }); let expr: Option = node.attribute("expression"); - match expr.as_ref().map(|expr| &expr.node_type) { + if let Some(NodeType::Identifier) = expr.as_ref().map(|expr| &expr.node_type) { // Might be a require/assert call - Some(NodeType::Identifier) => { - let name: Option = expr.and_then(|expr| expr.attribute("name")); - if let Some("assert" | "require") = name.as_deref() { - self.push_branches(&node.src, self.branch_id); - self.branch_id += 1; - } + let name: Option = expr.and_then(|expr| expr.attribute("name")); + if let Some("assert" | "require") = name.as_deref() { + self.push_branches(&node.src, self.branch_id); + self.branch_id += 1; } - _ => (), } Ok(()) diff --git a/crates/evm/coverage/src/anchors.rs b/crates/evm/coverage/src/anchors.rs index 5e05f2520c01..d45fdae57dca 100644 --- a/crates/evm/coverage/src/anchors.rs +++ b/crates/evm/coverage/src/anchors.rs @@ -20,7 +20,7 @@ pub fn find_anchors( // Prepare coverage items from all sources referenced in the source map let potential_item_ids = source_map .iter() - .filter_map(|element| Some(items_by_source_id.get(&(element.index? as usize))?)) + .filter_map(|element| items_by_source_id.get(&(element.index? as usize))) .flatten() .collect::>(); diff --git a/crates/forge/bin/cmd/coverage.rs b/crates/forge/bin/cmd/coverage.rs index f55260b712f4..0ec6a727f9c8 100644 --- a/crates/forge/bin/cmd/coverage.rs +++ b/crates/forge/bin/cmd/coverage.rs @@ -264,10 +264,13 @@ impl CoverageArgs { .iter() .enumerate() .map(|(item_id, item)| (item.loc.source_id, item_id)) - .fold(HashMap::new(), |mut map: HashMap>, (source_id, item_id)| { - map.entry(source_id).or_default().push(item_id); - map - }); + .fold( + HashMap::new(), + |mut map: HashMap>, (source_id, item_id)| { + map.entry(source_id).or_default().push(item_id); + map + }, + ); let anchors: HashMap> = source_maps .iter() From 82501c3f03158a6a53bcf463f7f58bd2522d71b8 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Thu, 28 Mar 2024 18:59:11 +0400 Subject: [PATCH 08/10] clean up --- crates/evm/coverage/src/analysis.rs | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/crates/evm/coverage/src/analysis.rs b/crates/evm/coverage/src/analysis.rs index 4d57b91ee3a2..a136f2d79cd2 100644 --- a/crates/evm/coverage/src/analysis.rs +++ b/crates/evm/coverage/src/analysis.rs @@ -408,8 +408,6 @@ pub struct SourceAnalysis { pub struct SourceAnalyzer { /// A map of source IDs to their source code sources: FxHashMap, - /// A map of AST node IDs of contracts to their contract IDs. - contract_ids: FxHashMap, /// A map of contract IDs to their AST nodes. contracts: HashMap, /// A collection of coverage items. @@ -435,8 +433,6 @@ impl SourceAnalyzer { continue } - let node_id = - child.id.ok_or_else(|| eyre::eyre!("The contract's AST node has no ID"))?; let contract_id = ContractId { version: version.clone(), source_id, @@ -444,7 +440,6 @@ impl SourceAnalyzer { .attribute("name") .ok_or_else(|| eyre::eyre!("Contract has no name"))?, }; - analyzer.contract_ids.insert(node_id, contract_id.clone()); analyzer.contracts.insert(contract_id, child); } } @@ -466,22 +461,18 @@ impl SourceAnalyzer { /// two different solc versions will produce overlapping source IDs if the compiler version is /// not taken into account. pub fn analyze(mut self) -> eyre::Result { - for contract_id in self.contracts.keys() { + for (contract_id, ast) in self.contracts { let ContractVisitor { items, .. } = ContractVisitor::new( contract_id.source_id, - self.sources.get(&contract_id.source_id).unwrap_or_else(|| { - panic!("We should have the source code for source ID {}", contract_id.source_id) - }), + self.sources.get(&contract_id.source_id).ok_or_else(|| { + eyre::eyre!( + "We should have the source code for source ID {}", + contract_id.source_id + ) + })?, contract_id.contract_name.clone(), ) - .visit( - self.contracts - .get(contract_id) - .unwrap_or_else(|| { - panic!("We should have the AST of contract: {contract_id:?}") - }) - .clone(), - )?; + .visit(ast)?; let is_test = items.iter().any(|item| { if let CoverageItemKind::Function { name } = &item.kind { From 6cd84bc7d24323104df420262ddffa7aaabe0095 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Thu, 28 Mar 2024 19:01:25 +0400 Subject: [PATCH 09/10] for loop --- crates/forge/bin/cmd/coverage.rs | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/crates/forge/bin/cmd/coverage.rs b/crates/forge/bin/cmd/coverage.rs index 0ec6a727f9c8..c3612d6bfd1d 100644 --- a/crates/forge/bin/cmd/coverage.rs +++ b/crates/forge/bin/cmd/coverage.rs @@ -259,18 +259,11 @@ impl CoverageArgs { .analyze()?; // Build helper mapping used by `find_anchors` - let items_by_source_id = source_analysis - .items - .iter() - .enumerate() - .map(|(item_id, item)| (item.loc.source_id, item_id)) - .fold( - HashMap::new(), - |mut map: HashMap>, (source_id, item_id)| { - map.entry(source_id).or_default().push(item_id); - map - }, - ); + let mut items_by_source_id = HashMap::new(); + + for (item_id, item) in source_analysis.items.iter().enumerate() { + items_by_source_id.entry(item.loc.source_id).or_insert_with(Vec::new).push(item_id); + } let anchors: HashMap> = source_maps .iter() From 1b1ca7f8098929cf36222cb1a877e92d788df607 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Fri, 29 Mar 2024 02:55:47 +0400 Subject: [PATCH 10/10] review fixes --- crates/forge/bin/cmd/coverage.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/forge/bin/cmd/coverage.rs b/crates/forge/bin/cmd/coverage.rs index c3612d6bfd1d..c59a54ade72f 100644 --- a/crates/forge/bin/cmd/coverage.rs +++ b/crates/forge/bin/cmd/coverage.rs @@ -259,10 +259,11 @@ impl CoverageArgs { .analyze()?; // Build helper mapping used by `find_anchors` - let mut items_by_source_id = HashMap::new(); + let mut items_by_source_id: HashMap<_, Vec<_>> = + HashMap::with_capacity(source_analysis.items.len()); for (item_id, item) in source_analysis.items.iter().enumerate() { - items_by_source_id.entry(item.loc.source_id).or_insert_with(Vec::new).push(item_id); + items_by_source_id.entry(item.loc.source_id).or_default().push(item_id); } let anchors: HashMap> = source_maps