From 62b50c3fa765b6a9d2dfab0918bbd1c103e0e723 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Thu, 28 Apr 2022 22:19:24 +0200 Subject: [PATCH 01/36] feat(coverage): fn and statement detection - Detects most statements (although there might be some expressions that need to be explored deeper) - Detects all functions - Detects if branches, but still needs work on other types of branches (assert, require) - Has a basic coverage inspector, but is not hooked up yet - Includes a basic summary reporter, and a (maybe) broken LCOV reporter --- Cargo.lock | 1 + Cargo.toml | 12 +- cli/src/cmd/forge/coverage.rs | 626 +++++++++++++++++++++++++ cli/src/cmd/forge/mod.rs | 1 + cli/src/forge.rs | 3 + cli/src/opts/forge.rs | 7 +- evm/Cargo.toml | 3 + evm/src/coverage.rs | 257 ++++++++++ evm/src/executor/builder.rs | 7 + evm/src/executor/inspector/coverage.rs | 139 ++++++ evm/src/executor/inspector/mod.rs | 8 + evm/src/executor/inspector/stack.rs | 47 +- evm/src/executor/mod.rs | 18 +- evm/src/lib.rs | 6 +- evm/src/trace/identifier/local.rs | 12 +- forge/src/multi_runner.rs | 15 + forge/src/result.rs | 5 + forge/src/runner.rs | 88 ++-- 18 files changed, 1190 insertions(+), 65 deletions(-) create mode 100644 cli/src/cmd/forge/coverage.rs create mode 100644 evm/src/coverage.rs create mode 100644 evm/src/executor/inspector/coverage.rs diff --git a/Cargo.lock b/Cargo.lock index 1633037fbe72..f352c5cf5f3a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2008,6 +2008,7 @@ dependencies = [ "parking_lot 0.12.0", "proptest", "revm", + "semver", "serde", "serde_json", "tempfile", diff --git a/Cargo.toml b/Cargo.toml index 6b6fab33cb86..3d32400b3a9c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -56,9 +56,9 @@ debug = 0 ## Patch ethers-rs with a local checkout then run `cargo update -p ethers` #[patch."https://github.com/gakonst/ethers-rs"] -#ethers = { path = "../ethers-rs" } -#ethers-core = { path = "../ethers-rs/ethers-core" } -#ethers-providers = { path = "../ethers-rs/ethers-providers" } -#ethers-signers = { path = "../ethers-rs/ethers-signers" } -#ethers-etherscan = { path = "../ethers-rs/ethers-etherscan" } -#ethers-solc = { path = "../ethers-rs/ethers-solc" } +#ethers = { path = "../../gakonst/ethers-rs" } +#ethers-core = { path = "../../gakonst/ethers-rs/ethers-core" } +#ethers-providers = { path = "../../gakonst/ethers-rs/ethers-providers" } +#ethers-signers = { path = "../../gakonst/ethers-rs/ethers-signers" } +#ethers-etherscan = { path = "../../gakonst/ethers-rs/ethers-etherscan" } +#ethers-solc = { path = "../../gakonst/ethers-rs/ethers-solc" } \ No newline at end of file diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs new file mode 100644 index 000000000000..45824ea8d460 --- /dev/null +++ b/cli/src/cmd/forge/coverage.rs @@ -0,0 +1,626 @@ +//! Coverage command +use crate::{ + cmd::{ + forge::{build::CoreBuildArgs, test::Filter}, + Cmd, + }, + compile::ProjectCompiler, + utils, +}; +use cast::coverage::{BranchKind, CoverageItem, CoverageMap, CoverageSummary}; +use clap::{AppSettings, ArgEnum, Parser}; +use comfy_table::{Cell, Color, Row, Table}; +use ethers::{ + prelude::{artifacts::Node, Artifact, Project, ProjectCompileOutput}, + solc::{ + artifacts::{ + ast::{Ast, NodeType}, + contract::CompactContractBytecode, + }, + sourcemap::{self, SourceMap}, + ArtifactId, + }, +}; +use forge::{ + executor::opts::EvmOpts, + result::SuiteResult, + trace::{identifier::LocalTraceIdentifier, CallTraceDecoder, CallTraceDecoderBuilder}, + MultiContractRunnerBuilder, +}; +use foundry_common::evm::EvmArgs; +use foundry_config::{figment::Figment, Config}; +use std::{collections::BTreeMap, io::Write, sync::mpsc::channel, thread}; +use tracing::warn; + +// Loads project's figment and merges the build cli arguments into it +foundry_config::impl_figment_convert!(CoverageArgs, opts, evm_opts); + +/// Generate coverage reports for your tests. +#[derive(Debug, Clone, Parser)] +#[clap(global_setting = AppSettings::DeriveDisplayOrder)] +pub struct CoverageArgs { + #[clap( + long, + arg_enum, + default_value = "summary", + help = "The report type to use for coverage." + )] + report: CoverageReportKind, + + #[clap(flatten, next_help_heading = "TEST FILTERING")] + filter: Filter, + + #[clap(flatten, next_help_heading = "EVM OPTIONS")] + evm_opts: EvmArgs, + + #[clap(flatten, next_help_heading = "BUILD OPTIONS")] + opts: CoreBuildArgs, +} + +impl CoverageArgs { + /// Returns the flattened [`CoreBuildArgs`] + pub fn build_args(&self) -> &CoreBuildArgs { + &self.opts + } + + /// Returns the currently configured [Config] and the extracted [EvmOpts] from that config + pub fn config_and_evm_opts(&self) -> eyre::Result<(Config, EvmOpts)> { + // merge all configs + let figment: Figment = self.into(); + let evm_opts = figment.extract()?; + let config = Config::from_provider(figment).sanitized(); + + Ok((config, evm_opts)) + } +} + +impl Cmd for CoverageArgs { + type Output = (); + + fn run(self) -> eyre::Result { + let (config, evm_opts) = self.configure()?; + let (project, output) = self.build(&config)?; + println!("Analysing contracts..."); + let (map, source_maps) = self.prepare(output.clone())?; + + println!("Running tests..."); + self.collect(project, output, source_maps, map, config, evm_opts) + } +} + +impl CoverageArgs { + /// Collects and adjusts configuration. + fn configure(&self) -> eyre::Result<(Config, EvmOpts)> { + // Merge all configs + let (config, mut evm_opts) = self.config_and_evm_opts()?; + + // We always want traces + evm_opts.verbosity = 3; + + Ok((config, evm_opts)) + } + + /// Builds the project. + fn build(&self, config: &Config) -> eyre::Result<(Project, ProjectCompileOutput)> { + // Set up the project + let project = { + let mut project = config.ephemeral_no_artifacts_project()?; + + // Disable the optimizer for more accurate source maps + project.solc_config.settings.optimizer.disable(); + + project + }; + + // TODO: This does not strip file prefixes for `SourceFiles`... + let output = ProjectCompiler::default() + .compile(&project)? + .with_stripped_file_prefixes(project.root()); + + Ok((project, output)) + } + + /// Builds the coverage map. + fn prepare( + &self, + output: ProjectCompileOutput, + ) -> eyre::Result<(CoverageMap, BTreeMap)> { + // Get sources and source maps + let (artifacts, sources) = output.into_artifacts_with_sources(); + + let source_maps: BTreeMap = artifacts + .into_iter() + .filter(|(_, artifact)| { + // TODO: Filter out dependencies + // Filter out tests + !artifact + .get_abi() + .map(|abi| abi.functions().any(|f| f.name.starts_with("test"))) + .unwrap_or_default() + }) + .map(|(id, artifact)| (id, CompactContractBytecode::from(artifact))) + .filter_map(|(id, artifact): (ArtifactId, CompactContractBytecode)| { + let source_map = artifact + .deployed_bytecode + .as_ref() + .and_then(|bytecode| bytecode.bytecode.as_ref()) + .and_then(|bytecode| bytecode.source_map.as_ref()) + .and_then(|source_map| sourcemap::parse(source_map).ok())?; + + Some((id, source_map)) + }) + .collect(); + + let mut map = CoverageMap::new(); + for (path, versioned_sources) in sources.0.into_iter() { + for mut versioned_source in versioned_sources { + let source = &mut versioned_source.source_file; + if let Some(ast) = source.ast.take() { + let mut visitor = Visitor::new(); + visitor.visit_ast(ast)?; + + if visitor.items.is_empty() { + continue + } + + map.add_source(path.clone(), versioned_source, visitor.items); + } + } + } + + Ok((map, source_maps)) + } + + /// Runs tests, collects coverage data and generates the final report. + fn collect( + self, + project: Project, + output: ProjectCompileOutput, + source_maps: BTreeMap, + map: CoverageMap, + config: Config, + evm_opts: EvmOpts, + ) -> eyre::Result<()> { + // Setup the fuzzer + // TODO: Add CLI Options to modify the persistence + let cfg = proptest::test_runner::Config { + failure_persistence: None, + cases: config.fuzz_runs, + max_local_rejects: config.fuzz_max_local_rejects, + max_global_rejects: config.fuzz_max_global_rejects, + ..Default::default() + }; + let fuzzer = proptest::test_runner::TestRunner::new(cfg); + let root = project.paths.root; + + // Build the contract runner + let evm_spec = crate::utils::evm_spec(&config.evm_version); + let mut runner = MultiContractRunnerBuilder::default() + .fuzzer(fuzzer) + .initial_balance(evm_opts.initial_balance) + .evm_spec(evm_spec) + .sender(evm_opts.sender) + .with_fork(utils::get_fork(&evm_opts, &config.rpc_storage_caching)) + .with_coverage() + .build(root.clone(), output, evm_opts)?; + let (tx, rx) = channel::<(String, SuiteResult)>(); + + // Set up identifier + let local_identifier = LocalTraceIdentifier::new(&runner.known_contracts); + + // TODO: Coverage for fuzz tests + let handle = thread::spawn(move || runner.test(&self.filter, Some(tx), false).unwrap()); + for mut result in rx.into_iter().flat_map(|(_, suite)| suite.test_results.into_values()) { + if let Some(hit_data) = result.coverage.take() { + let mut decoder = + CallTraceDecoderBuilder::new().with_events(local_identifier.events()).build(); + for (_, trace) in &mut result.traces { + decoder.identify(trace, &local_identifier); + } + // TODO: We need an ArtifactId here for the addresses + let CallTraceDecoder { contracts, .. } = decoder; + + // .. + } + } + + // Reattach the thread + let _ = handle.join(); + + match self.report { + CoverageReportKind::Summary => { + let mut reporter = SummaryReporter::new(); + reporter.build(map); + reporter.finalize() + } + // TODO: Sensible place to put the LCOV file + CoverageReportKind::Lcov => { + let mut reporter = + LcovReporter::new(std::fs::File::create(root.join("lcov.info"))?); + reporter.build(map); + reporter.finalize() + } + } + } +} + +// TODO: HTML +#[derive(Debug, Clone, ArgEnum)] +pub enum CoverageReportKind { + Summary, + Lcov, +} + +// TODO: Move to other module +#[derive(Debug, Default, Clone)] +struct Visitor { + /// Coverage items + pub items: Vec, + /// The current branch ID + // TODO: Does this need to be unique across files? + pub branch_id: usize, +} + +impl Visitor { + pub fn new() -> Self { + Self::default() + } + + pub fn visit_ast(&mut self, ast: Ast) -> eyre::Result<()> { + for node in ast.nodes.into_iter() { + if !matches!(node.node_type, NodeType::ContractDefinition) { + continue + } + + self.visit_contract(node)?; + } + + Ok(()) + } + + pub fn visit_contract(&mut self, node: Node) -> eyre::Result<()> { + let is_contract = + node.attribute("contractKind").map_or(false, |kind: String| kind == "contract"); + let is_abstract: bool = node.attribute("abstract").unwrap_or_default(); + + // Skip interfaces, libraries and abstract contracts + if !is_contract || is_abstract { + return Ok(()) + } + + for node in node.nodes { + if node.node_type == NodeType::FunctionDefinition { + self.visit_function_definition(node)?; + } + } + + Ok(()) + } + + pub fn visit_function_definition(&mut self, mut node: Node) -> eyre::Result<()> { + let name: String = + node.attribute("name").ok_or_else(|| eyre::eyre!("function has no name"))?; + let is_virtual: bool = node.attribute("virtual").unwrap_or_default(); + + // Skip virtual functions + if is_virtual { + return Ok(()) + } + + match node.body.take() { + // Skip virtual functions + Some(body) if !is_virtual => { + self.items.push(CoverageItem::Function { name, offset: node.src.start, hits: 0 }); + self.visit_block(*body) + } + _ => Ok(()), + } + } + + pub fn visit_block(&mut self, node: Node) -> eyre::Result<()> { + let statements: Vec = node.attribute("statements").unwrap_or_default(); + + for statement in statements { + self.visit_statement(statement)?; + } + + Ok(()) + } + pub fn visit_statement(&mut self, node: Node) -> eyre::Result<()> { + // TODO: inlineassembly + match node.node_type { + // Blocks + NodeType::Block | NodeType::UncheckedBlock => self.visit_block(node), + // Simple statements + NodeType::Break | + NodeType::Continue | + NodeType::EmitStatement | + NodeType::PlaceholderStatement | + NodeType::Return | + NodeType::RevertStatement => { + self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); + Ok(()) + } + // Variable declaration + NodeType::VariableDeclarationStatement => { + self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); + if let Some(expr) = node.attribute("initialValue") { + self.visit_expression(expr)?; + } + Ok(()) + } + // While loops + NodeType::DoWhileStatement | NodeType::WhileStatement => { + self.visit_expression( + node.attribute("condition") + .ok_or_else(|| eyre::eyre!("while statement had no condition"))?, + )?; + + let body = + node.body.ok_or_else(|| eyre::eyre!("while statement had no body node"))?; + self.visit_block_or_statement(*body) + } + // For loops + NodeType::ForStatement => { + if let Some(stmt) = node.attribute("initializationExpression") { + self.visit_statement(stmt)?; + } + if let Some(expr) = node.attribute("condition") { + self.visit_expression(expr)?; + } + if let Some(stmt) = node.attribute("loopExpression") { + self.visit_statement(stmt)?; + } + + let body = + node.body.ok_or_else(|| eyre::eyre!("for statement had no body node"))?; + self.visit_block_or_statement(*body) + } + // Expression statement + NodeType::ExpressionStatement => self.visit_expression( + node.attribute("expression") + .ok_or_else(|| eyre::eyre!("expression statement had no expression"))?, + ), + // If statement + NodeType::IfStatement => { + // TODO: create branch + self.visit_expression( + node.attribute("condition") + .ok_or_else(|| eyre::eyre!("while statement had no condition"))?, + )?; + + let true_body: Node = node + .attribute("trueBody") + .ok_or_else(|| eyre::eyre!("if statement had no true body"))?; + self.items.push(CoverageItem::Branch { + id: self.branch_id, + kind: BranchKind::True, + offset: true_body.src.start, + hits: 0, + }); + self.visit_block_or_statement(true_body)?; + + let false_body: Option = node.attribute("falseBody"); + if let Some(false_body) = false_body { + self.items.push(CoverageItem::Branch { + id: self.branch_id, + kind: BranchKind::False, + offset: false_body.src.start, + hits: 0, + }); + self.visit_block_or_statement(false_body)?; + } + + self.branch_id += 1; + Ok(()) + } + // Try-catch statement + NodeType::TryStatement => { + // TODO: Clauses + // TODO: This is branching, right? + self.visit_expression( + node.attribute("externalCall") + .ok_or_else(|| eyre::eyre!("try statement had no call"))?, + ) + } + _ => { + warn!("unexpected node type, expected a statement: {:?}", node.node_type); + Ok(()) + } + } + } + + pub fn visit_expression(&mut self, node: Node) -> eyre::Result<()> { + // TODO + // elementarytypenameexpression + // memberaccess + // newexpression + // tupleexpression + match node.node_type { + NodeType::Assignment | NodeType::UnaryOperation | NodeType::BinaryOperation => { + // TODO: Should we explore the subexpressions? + self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); + Ok(()) + } + NodeType::FunctionCall => { + // TODO: Handle assert and require + self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); + Ok(()) + } + NodeType::Conditional => { + // TODO: Do we count these as branches? + self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); + Ok(()) + } + // Does not count towards coverage + NodeType::FunctionCallOptions | + NodeType::Identifier | + NodeType::IndexAccess | + NodeType::IndexRangeAccess | + NodeType::Literal => Ok(()), + _ => { + warn!("unexpected node type, expected an expression: {:?}", node.node_type); + Ok(()) + } + } + } + + pub fn visit_block_or_statement(&mut self, node: Node) -> eyre::Result<()> { + match node.node_type { + NodeType::Block => self.visit_block(node), + NodeType::Break | + NodeType::Continue | + NodeType::DoWhileStatement | + NodeType::EmitStatement | + NodeType::ExpressionStatement | + NodeType::ForStatement | + NodeType::IfStatement | + NodeType::InlineAssembly | + NodeType::PlaceholderStatement | + NodeType::Return | + NodeType::RevertStatement | + NodeType::TryStatement | + NodeType::VariableDeclarationStatement | + NodeType::WhileStatement => self.visit_statement(node), + _ => { + warn!("unexpected node type, expected block or statement: {:?}", node.node_type); + Ok(()) + } + } + } +} + +// TODO: Move reporters to own module +/// A coverage reporter. +pub trait CoverageReporter { + fn build(&mut self, map: CoverageMap); + fn finalize(self) -> eyre::Result<()>; +} + +/// A simple summary reporter that prints the coverage results in a table. +struct SummaryReporter { + /// The summary table. + table: Table, + /// The total coverage of the entire project. + total: CoverageSummary, +} + +impl SummaryReporter { + pub fn new() -> Self { + let mut table = Table::new(); + table.set_header(&["File", "% Lines", "% Statements", "% Branches", "% Funcs"]); + + Self { table, total: CoverageSummary::default() } + } + + fn add_row(&mut self, name: impl Into, summary: CoverageSummary) { + let mut row = Row::new(); + row.add_cell(name.into()) + .add_cell(format_cell(summary.line_hits, summary.line_count)) + .add_cell(format_cell(summary.statement_hits, summary.statement_count)) + .add_cell(format_cell(summary.branch_hits, summary.branch_count)) + .add_cell(format_cell(summary.function_hits, summary.function_count)); + self.table.add_row(row); + } +} + +impl CoverageReporter for SummaryReporter { + fn build(&mut self, map: CoverageMap) { + for file in map { + let summary = file.summary(); + + self.total.add(&summary); + self.add_row(file.path.to_string_lossy(), summary); + } + } + + fn finalize(mut self) -> eyre::Result<()> { + self.add_row("Total", self.total.clone()); + println!("{}", self.table); + Ok(()) + } +} + +fn format_cell(hits: usize, total: usize) -> Cell { + let percentage = if total == 0 { 1. } else { hits as f64 / total as f64 }; + + Cell::new(format!("{}% ({hits}/{total})", percentage * 100.)).fg(match percentage { + _ if percentage < 0.5 => Color::Red, + _ if percentage < 0.75 => Color::Yellow, + _ => Color::Green, + }) +} + +struct LcovReporter { + /// Destination buffer + destination: W, + /// The coverage map to write + map: Option, +} + +impl LcovReporter { + pub fn new(destination: W) -> Self { + Self { destination, map: None } + } +} + +impl CoverageReporter for LcovReporter +where + W: Write, +{ + fn build(&mut self, map: CoverageMap) { + self.map = Some(map); + } + + fn finalize(mut self) -> eyre::Result<()> { + let map = self.map.ok_or_else(|| eyre::eyre!("no coverage map given to reporter"))?; + + for file in map { + let summary = file.summary(); + + writeln!(self.destination, "TN:")?; + writeln!(self.destination, "SF:{}", file.path.to_string_lossy())?; + + // TODO: Line numbers instead of byte offsets + for item in file.items { + match item { + CoverageItem::Function { name, offset, hits } => { + writeln!(self.destination, "FN:{offset},{name}")?; + writeln!(self.destination, "FNDA:{hits},{name}")?; + } + CoverageItem::Line { offset, hits } => { + writeln!(self.destination, "DA:{offset},{hits}")?; + } + CoverageItem::Branch { id, offset, hits, .. } => { + // TODO: Block ID + writeln!( + self.destination, + "BRDA:{offset},{id},{id},{}", + if hits == 0 { "-".to_string() } else { hits.to_string() } + )?; + } + // Statements are not in the LCOV format + CoverageItem::Statement { .. } => (), + } + } + + // Function summary + writeln!(self.destination, "FNF:{}", summary.function_count)?; + writeln!(self.destination, "FNH:{}", summary.function_hits)?; + + // Line summary + writeln!(self.destination, "LF:{}", summary.line_count)?; + writeln!(self.destination, "LH:{}", summary.line_hits)?; + + // Branch summary + writeln!(self.destination, "BRF:{}", summary.branch_count)?; + writeln!(self.destination, "BRH:{}", summary.branch_hits)?; + + writeln!(self.destination, "end_of_record")?; + } + + println!("Wrote LCOV report."); + + Ok(()) + } +} diff --git a/cli/src/cmd/forge/mod.rs b/cli/src/cmd/forge/mod.rs index e14dfc198f68..b01a3057cf41 100644 --- a/cli/src/cmd/forge/mod.rs +++ b/cli/src/cmd/forge/mod.rs @@ -41,6 +41,7 @@ pub mod bind; pub mod build; pub mod cache; pub mod config; +pub mod coverage; pub mod create; pub mod debug; pub mod flatten; diff --git a/cli/src/forge.rs b/cli/src/forge.rs index 592e4fe24614..5b1d95d540a6 100644 --- a/cli/src/forge.rs +++ b/cli/src/forge.rs @@ -35,6 +35,9 @@ fn main() -> eyre::Result<()> { Subcommands::Script(cmd) => { utils::block_on(cmd.run_script())?; } + Subcommands::Coverage(cmd) => { + cmd.run()?; + } Subcommands::Bind(cmd) => { cmd.run()?; } diff --git a/cli/src/opts/forge.rs b/cli/src/opts/forge.rs index 88a4960bdad6..bbef0b2ba9e9 100644 --- a/cli/src/opts/forge.rs +++ b/cli/src/opts/forge.rs @@ -7,7 +7,7 @@ use crate::cmd::forge::{ bind::BindArgs, build::BuildArgs, cache::CacheArgs, - config, + config, coverage, create::CreateArgs, debug::DebugArgs, flatten, @@ -59,7 +59,10 @@ pub enum Subcommands { #[clap(visible_alias = "s")] Script(ScriptArgs), - #[clap(visible_alias = "bi")] + #[clap(about = "Generate coverage reports.")] + Coverage(coverage::CoverageArgs), + + #[clap(alias = "bi")] #[clap(about = "Generate Rust bindings for smart contracts.")] Bind(BindArgs), diff --git a/evm/Cargo.toml b/evm/Cargo.toml index e927523c8d0a..00f9dcb0965b 100644 --- a/evm/Cargo.toml +++ b/evm/Cargo.toml @@ -46,5 +46,8 @@ proptest = "1.0.0" yansi = "0.5.1" url = "2.2.2" +# Coverage +semver = "1.0.5" + [dev-dependencies] tempfile = "3.3.0" diff --git a/evm/src/coverage.rs b/evm/src/coverage.rs new file mode 100644 index 000000000000..8d121d4d0eaa --- /dev/null +++ b/evm/src/coverage.rs @@ -0,0 +1,257 @@ +use ethers::{ + prelude::{sourcemap::SourceMap, sources::VersionedSourceFile}, + types::Address, +}; +use semver::Version; +use std::{ + collections::{BTreeMap, HashMap}, + path::PathBuf, + usize, +}; + +/// A coverage map. +/// +/// The coverage map collects coverage items for sources. It also converts hit data from +/// [HitMap]s into their appropriate coverage items. +/// +/// You **MUST** add all the sources before you start adding hit data. +#[derive(Default, Debug, Clone)] +pub struct CoverageMap { + /// A map of `(version, source id)` -> `source file` + sources: BTreeMap<(Version, u32), SourceFile>, +} + +impl CoverageMap { + pub fn new() -> Self { + Default::default() + } + + /// Adds coverage items and a source map for the given source. + /// + /// Sources are identified by path, and then by source ID and version. + /// + /// We need both the version and the source ID in case the project has distinct file + /// hierarchies that use different compiler versions, as that will result in multiple compile + /// jobs, and source IDs are only guaranteed to be stable across a single compile job. + pub fn add_source( + &mut self, + path: String, + source: VersionedSourceFile, + items: Vec, + ) { + let VersionedSourceFile { version, source_file: source } = source; + + self.sources.insert((version, source.id), SourceFile { path: PathBuf::from(path), items }); + } + + pub fn add_hit_map( + &mut self, + source_version: Version, + source_map: &SourceMap, + hit_map: HitMap, + ) { + // NOTE(onbjerg): I've made an assumption here that the coverage items are laid out in + // sorted order, ordered by their offset in the source code. + // + // This assumption is based on the way we process the AST - we start at the root node, + // and work our way down. If we change up how we process the AST, we *have* to either + // change this logic to work with unsorted data, or sort the data prior to calling + // this function. + for (ic, instruction_hits) in hit_map.hits.into_iter() { + if instruction_hits == 0 { + continue + } + + // Get the instruction offset and the source ID in the source map. + let (instruction_offset, source_id) = if let Some((instruction_offset, source_id)) = + source_map + .get(ic) + .and_then(|source_element| Some((source_element.offset, source_element.index?))) + { + (instruction_offset, source_id) + } else { + continue + }; + + // Get the coverage items corresponding to the source ID in the source map. + if let Some(source) = self.sources.get_mut(&(source_version.clone(), source_id)) { + for item in source.items.iter_mut() { + match item { + CoverageItem::Line { offset, hits } | + CoverageItem::Statement { offset, hits } | + CoverageItem::Branch { offset, hits, .. } | + CoverageItem::Function { offset, hits, .. } => { + // We've reached a point where we will no longer be able to map this + // instruction to coverage items + if instruction_offset < *offset { + break + } + + // We found a matching coverage item, but there may be more + if instruction_offset == *offset { + *hits += instruction_hits; + } + } + } + } + } + } + } +} + +impl IntoIterator for CoverageMap { + type Item = SourceFile; + type IntoIter = std::collections::btree_map::IntoValues<(Version, u32), Self::Item>; + + fn into_iter(self) -> Self::IntoIter { + self.sources.into_values() + } +} + +/// A collection of [HitMap]s +pub type HitMaps = HashMap; + +/// Hit data for an address. +/// +/// Contains low-level data about hit counters for the instructions in the bytecode of a contract. +#[derive(Debug, Clone, Default)] +pub struct HitMap { + hits: BTreeMap, +} + +impl HitMap { + /// Increase the hit counter for the given instruction counter. + pub fn hit(&mut self, ic: usize) { + *self.hits.entry(ic).or_default() += 1; + } +} + +/// A source file. +#[derive(Default, Debug, Clone)] +pub struct SourceFile { + pub path: PathBuf, + pub items: Vec, +} + +impl SourceFile { + /// Get a simple summary of the coverage for the file. + pub fn summary(&self) -> CoverageSummary { + self.items.iter().fold(CoverageSummary::default(), |mut summary, item| match item { + CoverageItem::Line { hits, .. } => { + summary.line_count += 1; + if *hits > 0 { + summary.line_hits += 1; + } + summary + } + CoverageItem::Statement { hits, .. } => { + summary.statement_count += 1; + if *hits > 0 { + summary.statement_hits += 1; + } + summary + } + CoverageItem::Branch { hits, .. } => { + summary.branch_count += 1; + if *hits > 0 { + summary.branch_hits += 1; + } + summary + } + CoverageItem::Function { hits, .. } => { + summary.function_count += 1; + if *hits > 0 { + summary.function_hits += 1; + } + summary + } + }) + } +} + +#[derive(Clone, Debug)] +pub enum CoverageItem { + /// An executable line in the code. + Line { + /// The byte offset in the source file for the start of the line. + offset: usize, + /// The number of times this item was hit. + hits: u64, + }, + + /// A statement in the code. + Statement { + /// The byte offset in the source file for the start of the statement. + offset: usize, + /// The number of times this statement was hit. + hits: u64, + }, + + /// A branch in the code. + Branch { + /// The ID that identifies the branch. + /// + /// There are two branches with the same ID, + /// one for each outcome (true and false). + id: usize, + /// The branch kind. + kind: BranchKind, + /// The byte offset which the branch is on in the source file. + offset: usize, + /// The number of times this item was hit. + hits: u64, + }, + + /// A function in the code. + Function { + /// The name of the function. + name: String, + /// The byte offset which the function is on in the source file. + offset: usize, + /// The number of times this item was hit. + hits: u64, + }, +} + +#[derive(Debug, Clone)] +pub enum BranchKind { + /// A false branch + True, + /// A true branch + False, +} + +/// Coverage summary for a source file. +#[derive(Default, Debug, Clone)] +pub struct CoverageSummary { + /// The number of executable lines in the source file. + pub line_count: usize, + /// The number of lines that were hit. + pub line_hits: usize, + /// The number of statements in the source file. + pub statement_count: usize, + /// The number of statements that were hit. + pub statement_hits: usize, + /// The number of branches in the source file. + pub branch_count: usize, + /// The number of branches that were hit. + pub branch_hits: usize, + /// The number of functions in the source file. + pub function_count: usize, + /// The number of functions hit. + pub function_hits: usize, +} + +impl CoverageSummary { + /// Add the data of another [CoverageSummary] to this one. + pub fn add(&mut self, other: &CoverageSummary) { + self.line_count += other.line_count; + self.line_hits += other.line_hits; + self.statement_count += other.statement_count; + self.statement_hits += other.statement_hits; + self.branch_count += other.branch_count; + self.branch_hits += other.branch_hits; + self.function_count += other.function_count; + self.function_hits += other.function_hits; + } +} diff --git a/evm/src/executor/builder.rs b/evm/src/executor/builder.rs index 030e5665d179..cde771408b42 100644 --- a/evm/src/executor/builder.rs +++ b/evm/src/executor/builder.rs @@ -152,6 +152,13 @@ impl ExecutorBuilder { self } + /// Enables coverage collection + #[must_use] + pub fn with_coverage(mut self) -> Self { + self.inspector_config.coverage = true; + self + } + /// Sets the EVM spec to use #[must_use] pub fn with_spec(mut self, spec: SpecId) -> Self { diff --git a/evm/src/executor/inspector/coverage.rs b/evm/src/executor/inspector/coverage.rs new file mode 100644 index 000000000000..0178df2eb4cc --- /dev/null +++ b/evm/src/executor/inspector/coverage.rs @@ -0,0 +1,139 @@ +use crate::{coverage::HitMaps, executor::inspector::utils::get_create_address}; +use bytes::Bytes; +use ethers::types::Address; +use revm::{ + opcode, spec_opcode_gas, CallInputs, CreateInputs, Database, EVMData, Gas, Inspector, + Interpreter, Return, SpecId, +}; +use std::collections::BTreeMap; + +#[derive(Default, Debug)] +pub struct CoverageCollector { + /// Maps that track instruction hit data. + pub maps: HitMaps, + + /// The execution addresses, with the topmost one being the current address. + context: Vec
, + + /// A mapping of program counters to instruction counters. + /// + /// The program counter keeps track of where we are in the contract bytecode as a whole, + /// including push bytes, while the instruction counter ignores push bytes. + /// + /// The instruction counter is used in Solidity source maps. + pub ic_map: BTreeMap>, +} + +impl CoverageCollector { + /// Builds the instruction counter map for the given bytecode. + // TODO: Some of the same logic is performed in REVM, but then later discarded. We should + // investigate if we can reuse it + // TODO: Duplicate code of the debugger inspector + pub fn build_ic_map(&mut self, spec: SpecId, code: &Bytes) { + if let Some(context) = self.context.last() { + let opcode_infos = spec_opcode_gas(spec); + let mut ic_map: BTreeMap = BTreeMap::new(); + + let mut i = 0; + let mut cumulative_push_size = 0; + while i < code.len() { + let op = code[i]; + ic_map.insert(i, i - cumulative_push_size); + if opcode_infos[op as usize].is_push { + // Skip the push bytes. + // + // For more context on the math, see: https://github.com/bluealloy/revm/blob/007b8807b5ad7705d3cacce4d92b89d880a83301/crates/revm/src/interpreter/contract.rs#L114-L115 + i += (op - opcode::PUSH1 + 1) as usize; + cumulative_push_size += (op - opcode::PUSH1 + 1) as usize; + } + i += 1; + } + + self.ic_map.insert(*context, ic_map); + } + } + + pub fn enter(&mut self, address: Address) { + self.context.push(address); + } + + pub fn exit(&mut self) { + self.context.pop(); + } +} + +impl Inspector for CoverageCollector +where + DB: Database, +{ + fn call( + &mut self, + _: &mut EVMData<'_, DB>, + call: &mut CallInputs, + _: bool, + ) -> (Return, Gas, Bytes) { + self.enter(call.context.code_address); + + (Return::Continue, Gas::new(call.gas_limit), Bytes::new()) + } + + // TODO: Don't collect coverage for test contract if possible + fn step( + &mut self, + interpreter: &mut Interpreter, + _: &mut EVMData<'_, DB>, + _is_static: bool, + ) -> Return { + let pc = interpreter.program_counter(); + if let Some(context) = self.context.last() { + if let Some(ic) = self.ic_map.get(context).and_then(|ic_map| ic_map.get(&pc)) { + let map = self.maps.entry(*context).or_default(); + + map.hit(*ic); + } + } + + Return::Continue + } + + fn call_end( + &mut self, + _: &mut EVMData<'_, DB>, + _: &CallInputs, + gas: Gas, + status: Return, + retdata: Bytes, + _: bool, + ) -> (Return, Gas, Bytes) { + self.exit(); + + (status, gas, retdata) + } + + fn create( + &mut self, + data: &mut EVMData<'_, DB>, + call: &mut CreateInputs, + ) -> (Return, Option
, Gas, Bytes) { + // TODO: Does this increase gas cost? + data.subroutine.load_account(call.caller, data.db); + let nonce = data.subroutine.account(call.caller).info.nonce; + self.enter(get_create_address(call, nonce)); + + (Return::Continue, None, Gas::new(call.gas_limit), Bytes::new()) + } + + fn create_end( + &mut self, + _: &mut EVMData<'_, DB>, + _: &CreateInputs, + status: Return, + address: Option
, + gas: Gas, + retdata: Bytes, + ) -> (Return, Option
, Gas, Bytes) { + self.exit(); + + (status, address, gas, retdata) + } +} diff --git a/evm/src/executor/inspector/mod.rs b/evm/src/executor/inspector/mod.rs index ee3931afadb4..99f84f9a264c 100644 --- a/evm/src/executor/inspector/mod.rs +++ b/evm/src/executor/inspector/mod.rs @@ -10,6 +10,9 @@ pub use tracer::Tracer; mod debugger; pub use debugger::Debugger; +mod coverage; +pub use coverage::CoverageCollector; + mod stack; pub use stack::{InspectorData, InspectorStack}; @@ -38,6 +41,8 @@ pub struct InspectorStackConfig { pub tracing: bool, /// Whether or not the debugger is enabled pub debugger: bool, + /// Whether or not coverage info should be collected + pub coverage: bool, } impl InspectorStackConfig { @@ -57,6 +62,9 @@ impl InspectorStackConfig { if self.debugger { stack.debugger = Some(Debugger::default()); } + if self.coverage { + stack.coverage = Some(CoverageCollector::default()); + } stack } } diff --git a/evm/src/executor/inspector/stack.rs b/evm/src/executor/inspector/stack.rs index 2ee44a0f579c..b50b7cb103d8 100644 --- a/evm/src/executor/inspector/stack.rs +++ b/evm/src/executor/inspector/stack.rs @@ -1,5 +1,5 @@ -use super::{Cheatcodes, Debugger, LogCollector, Tracer}; -use crate::{debug::DebugArena, trace::CallTraceArena}; +use super::{Cheatcodes, CoverageCollector, Debugger, LogCollector, Tracer}; +use crate::{coverage::HitMaps, debug::DebugArena, trace::CallTraceArena}; use bytes::Bytes; use ethers::types::{Address, Log, H256}; use revm::{db::Database, CallInputs, CreateInputs, EVMData, Gas, Inspector, Interpreter, Return}; @@ -22,6 +22,7 @@ pub struct InspectorData { pub labels: BTreeMap, pub traces: Option, pub debug: Option, + pub coverage: Option, pub cheatcodes: Option, } @@ -35,6 +36,7 @@ pub struct InspectorStack { pub logs: Option, pub cheatcodes: Option, pub debugger: Option, + pub coverage: Option, } impl InspectorStack { @@ -48,6 +50,7 @@ impl InspectorStack { .unwrap_or_default(), traces: self.tracer.map(|tracer| tracer.traces), debug: self.debugger.map(|debugger| debugger.arena), + coverage: self.coverage.map(|coverage| coverage.maps), cheatcodes: self.cheatcodes, } } @@ -87,7 +90,13 @@ where ) -> Return { call_inspectors!( inspector, - [&mut self.debugger, &mut self.tracer, &mut self.logs, &mut self.cheatcodes], + [ + &mut self.debugger, + &mut self.tracer, + &mut self.coverage, + &mut self.logs, + &mut self.cheatcodes + ], { let status = inspector.step(interpreter, data, is_static); @@ -144,7 +153,13 @@ where ) -> (Return, Gas, Bytes) { call_inspectors!( inspector, - [&mut self.debugger, &mut self.tracer, &mut self.logs, &mut self.cheatcodes], + [ + &mut self.debugger, + &mut self.tracer, + &mut self.coverage, + &mut self.logs, + &mut self.cheatcodes + ], { let (status, gas, retdata) = inspector.call(data, call, is_static); @@ -169,7 +184,13 @@ where ) -> (Return, Gas, Bytes) { call_inspectors!( inspector, - [&mut self.debugger, &mut self.tracer, &mut self.logs, &mut self.cheatcodes], + [ + &mut self.debugger, + &mut self.tracer, + &mut self.coverage, + &mut self.logs, + &mut self.cheatcodes + ], { let (new_status, new_gas, new_retdata) = inspector.call_end( data, @@ -198,7 +219,13 @@ where ) -> (Return, Option
, Gas, Bytes) { call_inspectors!( inspector, - [&mut self.debugger, &mut self.tracer, &mut self.logs, &mut self.cheatcodes], + [ + &mut self.debugger, + &mut self.tracer, + &mut self.coverage, + &mut self.logs, + &mut self.cheatcodes + ], { let (status, addr, gas, retdata) = inspector.create(data, call); @@ -223,7 +250,13 @@ where ) -> (Return, Option
, Gas, Bytes) { call_inspectors!( inspector, - [&mut self.debugger, &mut self.tracer, &mut self.logs, &mut self.cheatcodes], + [ + &mut self.debugger, + &mut self.tracer, + &mut self.coverage, + &mut self.logs, + &mut self.cheatcodes + ], { let (new_status, new_address, new_gas, new_retdata) = inspector.create_end( data, diff --git a/evm/src/executor/mod.rs b/evm/src/executor/mod.rs index 8fb61b0d9f59..b7398aebde0f 100644 --- a/evm/src/executor/mod.rs +++ b/evm/src/executor/mod.rs @@ -28,7 +28,8 @@ pub use revm::Env; use self::inspector::{InspectorData, InspectorStackConfig}; use crate::{ - debug::DebugArena, executor::inspector::DEFAULT_CREATE2_DEPLOYER, trace::CallTraceArena, CALLER, + coverage::HitMaps, debug::DebugArena, executor::inspector::DEFAULT_CREATE2_DEPLOYER, + trace::CallTraceArena, CALLER, }; use bytes::Bytes; use ethers::{ @@ -108,6 +109,8 @@ pub struct CallResult { pub labels: BTreeMap, /// The traces of the call pub traces: Option, + /// The coverage info collected during the call + pub coverage: Option, /// The debug nodes of the call pub debug: Option, /// Scripted transactions generated from this call @@ -138,6 +141,8 @@ pub struct RawCallResult { pub labels: BTreeMap, /// The traces of the call pub traces: Option, + /// The coverage info collected during the call + pub coverage: Option, /// The debug nodes of the call pub debug: Option, /// Scripted transactions generated from this call @@ -160,6 +165,7 @@ impl Default for RawCallResult { logs: Vec::new(), labels: BTreeMap::new(), traces: None, + coverage: None, debug: None, transactions: None, state_changeset: None, @@ -297,6 +303,7 @@ where logs, labels, traces, + coverage, debug, transactions, state_changeset, @@ -312,6 +319,7 @@ where logs, labels, traces, + coverage, debug, transactions, state_changeset, @@ -361,7 +369,7 @@ where _ => Bytes::default(), }; - let InspectorData { logs, labels, traces, debug, mut cheatcodes } = + let InspectorData { logs, labels, traces, coverage, debug, mut cheatcodes } = inspector.collect_inspector_states(); // Persist the changed block environment @@ -394,6 +402,7 @@ where stipend, logs, labels, + coverage, traces, debug, transactions, @@ -424,6 +433,7 @@ where logs, labels, traces, + coverage, debug, transactions, state_changeset, @@ -439,6 +449,7 @@ where logs, labels, traces, + coverage, debug, transactions, state_changeset, @@ -488,7 +499,7 @@ where _ => Bytes::default(), }; - let InspectorData { logs, labels, traces, debug, cheatcodes, .. } = + let InspectorData { logs, labels, traces, debug, coverage, cheatcodes, .. } = inspector.collect_inspector_states(); let transactions = if let Some(cheats) = cheatcodes { @@ -510,6 +521,7 @@ where logs: logs.to_vec(), labels, traces, + coverage, debug, transactions, state_changeset: Some(state_changeset), diff --git a/evm/src/lib.rs b/evm/src/lib.rs index 1de1c3a42166..224935654975 100644 --- a/evm/src/lib.rs +++ b/evm/src/lib.rs @@ -1,12 +1,16 @@ /// Decoding helpers pub mod decode; -/// Call trace arena, decoding and formatting +/// Call tracing +/// Contains a call trace arena, decoding and formatting utilities pub mod trace; /// Debugger data structures pub mod debug; +/// Coverage data structures +pub mod coverage; + /// Forge test execution backends pub mod executor; diff --git a/evm/src/trace/identifier/local.rs b/evm/src/trace/identifier/local.rs index 5539eb70ff66..50e73d71c216 100644 --- a/evm/src/trace/identifier/local.rs +++ b/evm/src/trace/identifier/local.rs @@ -8,7 +8,7 @@ use std::{borrow::Cow, collections::BTreeMap}; /// A trace identifier that tries to identify addresses using local contracts. pub struct LocalTraceIdentifier { - local_contracts: BTreeMap, (String, Abi)>, + local_contracts: BTreeMap, (ArtifactId, Abi)>, } impl LocalTraceIdentifier { @@ -16,9 +16,7 @@ impl LocalTraceIdentifier { Self { local_contracts: known_contracts .iter() - .map(|(id, (abi, runtime_code))| { - (runtime_code.clone(), (id.name.clone(), abi.clone())) - }) + .map(|(id, (abi, runtime_code))| (runtime_code.clone(), (id.clone(), abi.clone()))) .collect(), } } @@ -38,15 +36,15 @@ impl TraceIdentifier for LocalTraceIdentifier { .into_iter() .filter_map(|(address, code)| { let code = code?; - let (_, (name, abi)) = self + let (_, (id, abi)) = self .local_contracts .iter() .find(|(known_code, _)| diff_score(known_code, code) < 0.1)?; Some(AddressIdentity { address: *address, - contract: Some(name.clone()), - label: Some(name.clone()), + contract: Some(id.identifier()), + label: Some(id.name.clone()), abi: Some(Cow::Borrowed(abi)), }) }) diff --git a/forge/src/multi_runner.rs b/forge/src/multi_runner.rs index fc9c778fb303..9622e7ee9b0c 100644 --- a/forge/src/multi_runner.rs +++ b/forge/src/multi_runner.rs @@ -31,6 +31,8 @@ pub struct MultiContractRunnerBuilder { pub fork: Option, /// Additional cheatcode inspector related settings derived from the `Config` pub cheats_config: Option, + /// Whether or not to collect coverage info + pub coverage: bool, } pub type DeployableContracts = BTreeMap)>; @@ -128,6 +130,7 @@ impl MultiContractRunnerBuilder { source_paths, fork: self.fork, cheats_config: self.cheats_config.unwrap_or_default(), + coverage: self.coverage, }) } @@ -166,6 +169,12 @@ impl MultiContractRunnerBuilder { self.cheats_config = Some(cheats_config); self } + + #[must_use] + pub fn with_coverage(mut self) -> Self { + self.coverage = true; + self + } } /// A multi contract runner receives a set of contracts deployed in an EVM instance and proceeds @@ -192,6 +201,8 @@ pub struct MultiContractRunner { pub fork: Option, /// Additional cheatcode inspector related settings derived from the `Config` pub cheats_config: CheatsConfig, + /// Whether or not to collect coverage info + pub coverage: bool, } impl MultiContractRunner { @@ -281,6 +292,10 @@ impl MultiContractRunner { builder = builder.with_tracing(); } + if self.coverage { + builder = builder.with_coverage(); + } + let executor = builder.build(db.clone()); let result = self.run_tests( &id.identifier(), diff --git a/forge/src/result.rs b/forge/src/result.rs index 22ac386fff8d..d22aee936fea 100644 --- a/forge/src/result.rs +++ b/forge/src/result.rs @@ -3,6 +3,7 @@ use crate::Address; use ethers::prelude::Log; use foundry_evm::{ + coverage::HitMaps, fuzz::{CounterExample, FuzzedCases}, trace::{CallTraceArena, TraceKind}, }; @@ -63,6 +64,10 @@ pub struct TestResult { /// Traces pub traces: Vec<(TraceKind, CallTraceArena)>, + /// Raw coverage info + #[serde(skip)] + pub coverage: Option, + /// Labeled addresses pub labeled_addresses: BTreeMap, } diff --git a/forge/src/runner.rs b/forge/src/runner.rs index 84e3a8455e03..fa13c5348049 100644 --- a/forge/src/runner.rs +++ b/forge/src/runner.rs @@ -15,7 +15,6 @@ use foundry_evm::{ }; use proptest::test_runner::TestRunner; use rayon::iter::{IntoParallelRefIterator, ParallelIterator}; - use std::{collections::BTreeMap, time::Instant}; pub struct ContractRunner<'a, DB: DatabaseRef> { @@ -203,6 +202,7 @@ impl<'a, DB: DatabaseRef + Send + Sync> ContractRunner<'a, DB> { logs: vec![], kind: TestKind::Standard(0), traces: vec![], + coverage: None, labeled_addresses: BTreeMap::new(), }, )] @@ -225,6 +225,7 @@ impl<'a, DB: DatabaseRef + Send + Sync> ContractRunner<'a, DB> { logs: setup.logs, kind: TestKind::Standard(0), traces: setup.traces, + coverage: None, labeled_addresses: setup.labeled_addresses, }, )] @@ -285,44 +286,50 @@ impl<'a, DB: DatabaseRef + Send + Sync> ContractRunner<'a, DB> { // Run unit test let start = Instant::now(); - let (reverted, reason, gas, stipend, execution_traces, state_changeset) = match self - .executor - .call::<(), _, _>(self.sender, address, func.clone(), (), 0.into(), self.errors) - { - Ok(CallResult { - reverted, - gas, - stipend, - logs: execution_logs, - traces: execution_trace, - labels: new_labels, - state_changeset, - .. - }) => { - labeled_addresses.extend(new_labels); - logs.extend(execution_logs); - (reverted, None, gas, stipend, execution_trace, state_changeset) - } - Err(EvmError::Execution { - reverted, - reason, - gas, - stipend, - logs: execution_logs, - traces: execution_trace, - labels: new_labels, - state_changeset, - .. - }) => { - labeled_addresses.extend(new_labels); - logs.extend(execution_logs); - (reverted, Some(reason), gas, stipend, execution_trace, state_changeset) - } - Err(err) => { - tracing::error!(?err); - return Err(err.into()) - } - }; + let (reverted, reason, gas, stipend, execution_traces, coverage, state_changeset) = + match self.executor.call::<(), _, _>( + self.sender, + address, + func.clone(), + (), + 0.into(), + self.errors, + ) { + Ok(CallResult { + reverted, + gas, + stipend, + logs: execution_logs, + traces: execution_trace, + coverage, + labels: new_labels, + state_changeset, + .. + }) => { + labeled_addresses.extend(new_labels); + logs.extend(execution_logs); + (reverted, None, gas, stipend, execution_trace, coverage, state_changeset) + } + Err(EvmError::Execution { + reverted, + reason, + gas, + stipend, + logs: execution_logs, + traces: execution_trace, + labels: new_labels, + state_changeset, + .. + }) => { + labeled_addresses.extend(new_labels); + logs.extend(execution_logs); + (reverted, Some(reason), gas, stipend, execution_trace, None, state_changeset) + } + Err(err) => { + tracing::error!(?err); + return Err(err.into()) + } + }; traces.extend(execution_traces.map(|traces| (TraceKind::Execution, traces)).into_iter()); let success = self.executor.is_success( @@ -346,6 +353,7 @@ impl<'a, DB: DatabaseRef + Send + Sync> ContractRunner<'a, DB> { logs, kind: TestKind::Standard(gas.overflowing_sub(stipend).0), traces, + coverage, labeled_addresses, }) } @@ -387,6 +395,8 @@ impl<'a, DB: DatabaseRef + Send + Sync> ContractRunner<'a, DB> { logs, kind: TestKind::Fuzz(result.cases), traces, + // TODO: Maybe support coverage for fuzz tests + coverage: None, labeled_addresses, }) } From d0f63893bc213395f60670382294168ca068998d Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Mon, 9 May 2022 00:46:15 +0200 Subject: [PATCH 02/36] fix: account for nested branches --- cli/src/cmd/forge/coverage.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index 45824ea8d460..b789b5b94e73 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -392,8 +392,17 @@ impl Visitor { let true_body: Node = node .attribute("trueBody") .ok_or_else(|| eyre::eyre!("if statement had no true body"))?; + + // We need to store the current branch ID here since visiting the body of either of + // the if blocks may increase `self.branch_id` in the case of nested if statements. + let branch_id = self.branch_id; + + // We increase the branch ID here such that nested branches do not use the same + // branch ID as we do + self.branch_id += 1; + self.items.push(CoverageItem::Branch { - id: self.branch_id, + id: branch_id, kind: BranchKind::True, offset: true_body.src.start, hits: 0, @@ -403,7 +412,7 @@ impl Visitor { let false_body: Option = node.attribute("falseBody"); if let Some(false_body) = false_body { self.items.push(CoverageItem::Branch { - id: self.branch_id, + id: branch_id, kind: BranchKind::False, offset: false_body.src.start, hits: 0, @@ -411,7 +420,6 @@ impl Visitor { self.visit_block_or_statement(false_body)?; } - self.branch_id += 1; Ok(()) } // Try-catch statement From d56baa9d1af2ea567485b7498071215ebc0b370e Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Thu, 12 May 2022 15:34:24 +0200 Subject: [PATCH 03/36] refactor: move stuff to new modules --- cli/src/cmd/forge/coverage.rs | 399 +---------------------- evm/src/{coverage.rs => coverage/mod.rs} | 3 + evm/src/coverage/visitor.rs | 249 ++++++++++++++ forge/src/coverage.rs | 143 ++++++++ forge/src/lib.rs | 3 + 5 files changed, 405 insertions(+), 392 deletions(-) rename evm/src/{coverage.rs => coverage/mod.rs} (99%) create mode 100644 evm/src/coverage/visitor.rs create mode 100644 forge/src/coverage.rs diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index b789b5b94e73..72c3f892b03d 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -7,21 +7,17 @@ use crate::{ compile::ProjectCompiler, utils, }; -use cast::coverage::{BranchKind, CoverageItem, CoverageMap, CoverageSummary}; use clap::{AppSettings, ArgEnum, Parser}; -use comfy_table::{Cell, Color, Row, Table}; use ethers::{ - prelude::{artifacts::Node, Artifact, Project, ProjectCompileOutput}, + prelude::{Artifact, Project, ProjectCompileOutput}, solc::{ - artifacts::{ - ast::{Ast, NodeType}, - contract::CompactContractBytecode, - }, + artifacts::contract::CompactContractBytecode, sourcemap::{self, SourceMap}, ArtifactId, }, }; use forge::{ + coverage::{CoverageMap, CoverageReporter, LcovReporter, SummaryReporter, Visitor}, executor::opts::EvmOpts, result::SuiteResult, trace::{identifier::LocalTraceIdentifier, CallTraceDecoder, CallTraceDecoderBuilder}, @@ -29,8 +25,7 @@ use forge::{ }; use foundry_common::evm::EvmArgs; use foundry_config::{figment::Figment, Config}; -use std::{collections::BTreeMap, io::Write, sync::mpsc::channel, thread}; -use tracing::warn; +use std::{collections::BTreeMap, sync::mpsc::channel, thread}; // Loads project's figment and merges the build cli arguments into it foundry_config::impl_figment_convert!(CoverageArgs, opts, evm_opts); @@ -176,7 +171,7 @@ impl CoverageArgs { self, project: Project, output: ProjectCompileOutput, - source_maps: BTreeMap, + _source_maps: BTreeMap, map: CoverageMap, config: Config, evm_opts: EvmOpts, @@ -211,14 +206,14 @@ impl CoverageArgs { // TODO: Coverage for fuzz tests let handle = thread::spawn(move || runner.test(&self.filter, Some(tx), false).unwrap()); for mut result in rx.into_iter().flat_map(|(_, suite)| suite.test_results.into_values()) { - if let Some(hit_data) = result.coverage.take() { + if let Some(_hit_data) = result.coverage.take() { let mut decoder = CallTraceDecoderBuilder::new().with_events(local_identifier.events()).build(); for (_, trace) in &mut result.traces { decoder.identify(trace, &local_identifier); } // TODO: We need an ArtifactId here for the addresses - let CallTraceDecoder { contracts, .. } = decoder; + let CallTraceDecoder { contracts: _, .. } = decoder; // .. } @@ -251,384 +246,4 @@ pub enum CoverageReportKind { Lcov, } -// TODO: Move to other module -#[derive(Debug, Default, Clone)] -struct Visitor { - /// Coverage items - pub items: Vec, - /// The current branch ID - // TODO: Does this need to be unique across files? - pub branch_id: usize, -} - -impl Visitor { - pub fn new() -> Self { - Self::default() - } - - pub fn visit_ast(&mut self, ast: Ast) -> eyre::Result<()> { - for node in ast.nodes.into_iter() { - if !matches!(node.node_type, NodeType::ContractDefinition) { - continue - } - - self.visit_contract(node)?; - } - - Ok(()) - } - - pub fn visit_contract(&mut self, node: Node) -> eyre::Result<()> { - let is_contract = - node.attribute("contractKind").map_or(false, |kind: String| kind == "contract"); - let is_abstract: bool = node.attribute("abstract").unwrap_or_default(); - - // Skip interfaces, libraries and abstract contracts - if !is_contract || is_abstract { - return Ok(()) - } - - for node in node.nodes { - if node.node_type == NodeType::FunctionDefinition { - self.visit_function_definition(node)?; - } - } - - Ok(()) - } - - pub fn visit_function_definition(&mut self, mut node: Node) -> eyre::Result<()> { - let name: String = - node.attribute("name").ok_or_else(|| eyre::eyre!("function has no name"))?; - let is_virtual: bool = node.attribute("virtual").unwrap_or_default(); - - // Skip virtual functions - if is_virtual { - return Ok(()) - } - - match node.body.take() { - // Skip virtual functions - Some(body) if !is_virtual => { - self.items.push(CoverageItem::Function { name, offset: node.src.start, hits: 0 }); - self.visit_block(*body) - } - _ => Ok(()), - } - } - - pub fn visit_block(&mut self, node: Node) -> eyre::Result<()> { - let statements: Vec = node.attribute("statements").unwrap_or_default(); - - for statement in statements { - self.visit_statement(statement)?; - } - - Ok(()) - } - pub fn visit_statement(&mut self, node: Node) -> eyre::Result<()> { - // TODO: inlineassembly - match node.node_type { - // Blocks - NodeType::Block | NodeType::UncheckedBlock => self.visit_block(node), - // Simple statements - NodeType::Break | - NodeType::Continue | - NodeType::EmitStatement | - NodeType::PlaceholderStatement | - NodeType::Return | - NodeType::RevertStatement => { - self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); - Ok(()) - } - // Variable declaration - NodeType::VariableDeclarationStatement => { - self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); - if let Some(expr) = node.attribute("initialValue") { - self.visit_expression(expr)?; - } - Ok(()) - } - // While loops - NodeType::DoWhileStatement | NodeType::WhileStatement => { - self.visit_expression( - node.attribute("condition") - .ok_or_else(|| eyre::eyre!("while statement had no condition"))?, - )?; - - let body = - node.body.ok_or_else(|| eyre::eyre!("while statement had no body node"))?; - self.visit_block_or_statement(*body) - } - // For loops - NodeType::ForStatement => { - if let Some(stmt) = node.attribute("initializationExpression") { - self.visit_statement(stmt)?; - } - if let Some(expr) = node.attribute("condition") { - self.visit_expression(expr)?; - } - if let Some(stmt) = node.attribute("loopExpression") { - self.visit_statement(stmt)?; - } - - let body = - node.body.ok_or_else(|| eyre::eyre!("for statement had no body node"))?; - self.visit_block_or_statement(*body) - } - // Expression statement - NodeType::ExpressionStatement => self.visit_expression( - node.attribute("expression") - .ok_or_else(|| eyre::eyre!("expression statement had no expression"))?, - ), - // If statement - NodeType::IfStatement => { - // TODO: create branch - self.visit_expression( - node.attribute("condition") - .ok_or_else(|| eyre::eyre!("while statement had no condition"))?, - )?; - - let true_body: Node = node - .attribute("trueBody") - .ok_or_else(|| eyre::eyre!("if statement had no true body"))?; - - // We need to store the current branch ID here since visiting the body of either of - // the if blocks may increase `self.branch_id` in the case of nested if statements. - let branch_id = self.branch_id; - - // We increase the branch ID here such that nested branches do not use the same - // branch ID as we do - self.branch_id += 1; - - self.items.push(CoverageItem::Branch { - id: branch_id, - kind: BranchKind::True, - offset: true_body.src.start, - hits: 0, - }); - self.visit_block_or_statement(true_body)?; - - let false_body: Option = node.attribute("falseBody"); - if let Some(false_body) = false_body { - self.items.push(CoverageItem::Branch { - id: branch_id, - kind: BranchKind::False, - offset: false_body.src.start, - hits: 0, - }); - self.visit_block_or_statement(false_body)?; - } - - Ok(()) - } - // Try-catch statement - NodeType::TryStatement => { - // TODO: Clauses - // TODO: This is branching, right? - self.visit_expression( - node.attribute("externalCall") - .ok_or_else(|| eyre::eyre!("try statement had no call"))?, - ) - } - _ => { - warn!("unexpected node type, expected a statement: {:?}", node.node_type); - Ok(()) - } - } - } - - pub fn visit_expression(&mut self, node: Node) -> eyre::Result<()> { - // TODO - // elementarytypenameexpression - // memberaccess - // newexpression - // tupleexpression - match node.node_type { - NodeType::Assignment | NodeType::UnaryOperation | NodeType::BinaryOperation => { - // TODO: Should we explore the subexpressions? - self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); - Ok(()) - } - NodeType::FunctionCall => { - // TODO: Handle assert and require - self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); - Ok(()) - } - NodeType::Conditional => { - // TODO: Do we count these as branches? - self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); - Ok(()) - } - // Does not count towards coverage - NodeType::FunctionCallOptions | - NodeType::Identifier | - NodeType::IndexAccess | - NodeType::IndexRangeAccess | - NodeType::Literal => Ok(()), - _ => { - warn!("unexpected node type, expected an expression: {:?}", node.node_type); - Ok(()) - } - } - } - - pub fn visit_block_or_statement(&mut self, node: Node) -> eyre::Result<()> { - match node.node_type { - NodeType::Block => self.visit_block(node), - NodeType::Break | - NodeType::Continue | - NodeType::DoWhileStatement | - NodeType::EmitStatement | - NodeType::ExpressionStatement | - NodeType::ForStatement | - NodeType::IfStatement | - NodeType::InlineAssembly | - NodeType::PlaceholderStatement | - NodeType::Return | - NodeType::RevertStatement | - NodeType::TryStatement | - NodeType::VariableDeclarationStatement | - NodeType::WhileStatement => self.visit_statement(node), - _ => { - warn!("unexpected node type, expected block or statement: {:?}", node.node_type); - Ok(()) - } - } - } -} - // TODO: Move reporters to own module -/// A coverage reporter. -pub trait CoverageReporter { - fn build(&mut self, map: CoverageMap); - fn finalize(self) -> eyre::Result<()>; -} - -/// A simple summary reporter that prints the coverage results in a table. -struct SummaryReporter { - /// The summary table. - table: Table, - /// The total coverage of the entire project. - total: CoverageSummary, -} - -impl SummaryReporter { - pub fn new() -> Self { - let mut table = Table::new(); - table.set_header(&["File", "% Lines", "% Statements", "% Branches", "% Funcs"]); - - Self { table, total: CoverageSummary::default() } - } - - fn add_row(&mut self, name: impl Into, summary: CoverageSummary) { - let mut row = Row::new(); - row.add_cell(name.into()) - .add_cell(format_cell(summary.line_hits, summary.line_count)) - .add_cell(format_cell(summary.statement_hits, summary.statement_count)) - .add_cell(format_cell(summary.branch_hits, summary.branch_count)) - .add_cell(format_cell(summary.function_hits, summary.function_count)); - self.table.add_row(row); - } -} - -impl CoverageReporter for SummaryReporter { - fn build(&mut self, map: CoverageMap) { - for file in map { - let summary = file.summary(); - - self.total.add(&summary); - self.add_row(file.path.to_string_lossy(), summary); - } - } - - fn finalize(mut self) -> eyre::Result<()> { - self.add_row("Total", self.total.clone()); - println!("{}", self.table); - Ok(()) - } -} - -fn format_cell(hits: usize, total: usize) -> Cell { - let percentage = if total == 0 { 1. } else { hits as f64 / total as f64 }; - - Cell::new(format!("{}% ({hits}/{total})", percentage * 100.)).fg(match percentage { - _ if percentage < 0.5 => Color::Red, - _ if percentage < 0.75 => Color::Yellow, - _ => Color::Green, - }) -} - -struct LcovReporter { - /// Destination buffer - destination: W, - /// The coverage map to write - map: Option, -} - -impl LcovReporter { - pub fn new(destination: W) -> Self { - Self { destination, map: None } - } -} - -impl CoverageReporter for LcovReporter -where - W: Write, -{ - fn build(&mut self, map: CoverageMap) { - self.map = Some(map); - } - - fn finalize(mut self) -> eyre::Result<()> { - let map = self.map.ok_or_else(|| eyre::eyre!("no coverage map given to reporter"))?; - - for file in map { - let summary = file.summary(); - - writeln!(self.destination, "TN:")?; - writeln!(self.destination, "SF:{}", file.path.to_string_lossy())?; - - // TODO: Line numbers instead of byte offsets - for item in file.items { - match item { - CoverageItem::Function { name, offset, hits } => { - writeln!(self.destination, "FN:{offset},{name}")?; - writeln!(self.destination, "FNDA:{hits},{name}")?; - } - CoverageItem::Line { offset, hits } => { - writeln!(self.destination, "DA:{offset},{hits}")?; - } - CoverageItem::Branch { id, offset, hits, .. } => { - // TODO: Block ID - writeln!( - self.destination, - "BRDA:{offset},{id},{id},{}", - if hits == 0 { "-".to_string() } else { hits.to_string() } - )?; - } - // Statements are not in the LCOV format - CoverageItem::Statement { .. } => (), - } - } - - // Function summary - writeln!(self.destination, "FNF:{}", summary.function_count)?; - writeln!(self.destination, "FNH:{}", summary.function_hits)?; - - // Line summary - writeln!(self.destination, "LF:{}", summary.line_count)?; - writeln!(self.destination, "LH:{}", summary.line_hits)?; - - // Branch summary - writeln!(self.destination, "BRF:{}", summary.branch_count)?; - writeln!(self.destination, "BRH:{}", summary.branch_hits)?; - - writeln!(self.destination, "end_of_record")?; - } - - println!("Wrote LCOV report."); - - Ok(()) - } -} diff --git a/evm/src/coverage.rs b/evm/src/coverage/mod.rs similarity index 99% rename from evm/src/coverage.rs rename to evm/src/coverage/mod.rs index 8d121d4d0eaa..7551b4efd20a 100644 --- a/evm/src/coverage.rs +++ b/evm/src/coverage/mod.rs @@ -1,3 +1,6 @@ +mod visitor; +pub use visitor::Visitor; + use ethers::{ prelude::{sourcemap::SourceMap, sources::VersionedSourceFile}, types::Address, diff --git a/evm/src/coverage/visitor.rs b/evm/src/coverage/visitor.rs new file mode 100644 index 000000000000..37658f96c9b5 --- /dev/null +++ b/evm/src/coverage/visitor.rs @@ -0,0 +1,249 @@ +use super::{BranchKind, CoverageItem}; +use ethers::solc::artifacts::ast::{Ast, Node, NodeType}; +use tracing::warn; + +#[derive(Debug, Default, Clone)] +pub struct Visitor { + /// Coverage items + pub items: Vec, + /// The current branch ID + // TODO: Does this need to be unique across files? + pub branch_id: usize, +} + +impl Visitor { + pub fn new() -> Self { + Self::default() + } + + pub fn visit_ast(&mut self, ast: Ast) -> eyre::Result<()> { + for node in ast.nodes.into_iter() { + if !matches!(node.node_type, NodeType::ContractDefinition) { + continue + } + + self.visit_contract(node)?; + } + + Ok(()) + } + + pub fn visit_contract(&mut self, node: Node) -> eyre::Result<()> { + let is_contract = + node.attribute("contractKind").map_or(false, |kind: String| kind == "contract"); + let is_abstract: bool = node.attribute("abstract").unwrap_or_default(); + + // Skip interfaces, libraries and abstract contracts + if !is_contract || is_abstract { + return Ok(()) + } + + for node in node.nodes { + if node.node_type == NodeType::FunctionDefinition { + self.visit_function_definition(node)?; + } + } + + Ok(()) + } + + pub fn visit_function_definition(&mut self, mut node: Node) -> eyre::Result<()> { + let name: String = + node.attribute("name").ok_or_else(|| eyre::eyre!("function has no name"))?; + let is_virtual: bool = node.attribute("virtual").unwrap_or_default(); + + // Skip virtual functions + if is_virtual { + return Ok(()) + } + + match node.body.take() { + // Skip virtual functions + Some(body) if !is_virtual => { + self.items.push(CoverageItem::Function { name, offset: node.src.start, hits: 0 }); + self.visit_block(*body) + } + _ => Ok(()), + } + } + + pub fn visit_block(&mut self, node: Node) -> eyre::Result<()> { + let statements: Vec = node.attribute("statements").unwrap_or_default(); + + for statement in statements { + self.visit_statement(statement)?; + } + + Ok(()) + } + pub fn visit_statement(&mut self, node: Node) -> eyre::Result<()> { + // TODO: inlineassembly + match node.node_type { + // Blocks + NodeType::Block | NodeType::UncheckedBlock => self.visit_block(node), + // Simple statements + NodeType::Break | + NodeType::Continue | + NodeType::EmitStatement | + NodeType::PlaceholderStatement | + NodeType::Return | + NodeType::RevertStatement => { + self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); + Ok(()) + } + // Variable declaration + NodeType::VariableDeclarationStatement => { + self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); + if let Some(expr) = node.attribute("initialValue") { + self.visit_expression(expr)?; + } + Ok(()) + } + // While loops + NodeType::DoWhileStatement | NodeType::WhileStatement => { + self.visit_expression( + node.attribute("condition") + .ok_or_else(|| eyre::eyre!("while statement had no condition"))?, + )?; + + let body = + node.body.ok_or_else(|| eyre::eyre!("while statement had no body node"))?; + self.visit_block_or_statement(*body) + } + // For loops + NodeType::ForStatement => { + if let Some(stmt) = node.attribute("initializationExpression") { + self.visit_statement(stmt)?; + } + if let Some(expr) = node.attribute("condition") { + self.visit_expression(expr)?; + } + if let Some(stmt) = node.attribute("loopExpression") { + self.visit_statement(stmt)?; + } + + let body = + node.body.ok_or_else(|| eyre::eyre!("for statement had no body node"))?; + self.visit_block_or_statement(*body) + } + // Expression statement + NodeType::ExpressionStatement => self.visit_expression( + node.attribute("expression") + .ok_or_else(|| eyre::eyre!("expression statement had no expression"))?, + ), + // If statement + NodeType::IfStatement => { + // TODO: create branch + self.visit_expression( + node.attribute("condition") + .ok_or_else(|| eyre::eyre!("while statement had no condition"))?, + )?; + + let true_body: Node = node + .attribute("trueBody") + .ok_or_else(|| eyre::eyre!("if statement had no true body"))?; + + // We need to store the current branch ID here since visiting the body of either of + // the if blocks may increase `self.branch_id` in the case of nested if statements. + let branch_id = self.branch_id; + + // We increase the branch ID here such that nested branches do not use the same + // branch ID as we do + self.branch_id += 1; + + self.items.push(CoverageItem::Branch { + id: branch_id, + kind: BranchKind::True, + offset: true_body.src.start, + hits: 0, + }); + self.visit_block_or_statement(true_body)?; + + let false_body: Option = node.attribute("falseBody"); + if let Some(false_body) = false_body { + self.items.push(CoverageItem::Branch { + id: branch_id, + kind: BranchKind::False, + offset: false_body.src.start, + hits: 0, + }); + self.visit_block_or_statement(false_body)?; + } + + Ok(()) + } + // Try-catch statement + NodeType::TryStatement => { + // TODO: Clauses + // TODO: This is branching, right? + self.visit_expression( + node.attribute("externalCall") + .ok_or_else(|| eyre::eyre!("try statement had no call"))?, + ) + } + _ => { + warn!("unexpected node type, expected a statement: {:?}", node.node_type); + Ok(()) + } + } + } + + pub fn visit_expression(&mut self, node: Node) -> eyre::Result<()> { + // TODO + // elementarytypenameexpression + // memberaccess + // newexpression + // tupleexpression + match node.node_type { + NodeType::Assignment | NodeType::UnaryOperation | NodeType::BinaryOperation => { + // TODO: Should we explore the subexpressions? + self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); + Ok(()) + } + NodeType::FunctionCall => { + // TODO: Handle assert and require + self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); + Ok(()) + } + NodeType::Conditional => { + // TODO: Do we count these as branches? + self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); + Ok(()) + } + // Does not count towards coverage + NodeType::FunctionCallOptions | + NodeType::Identifier | + NodeType::IndexAccess | + NodeType::IndexRangeAccess | + NodeType::Literal => Ok(()), + _ => { + warn!("unexpected node type, expected an expression: {:?}", node.node_type); + Ok(()) + } + } + } + + pub fn visit_block_or_statement(&mut self, node: Node) -> eyre::Result<()> { + match node.node_type { + NodeType::Block => self.visit_block(node), + NodeType::Break | + NodeType::Continue | + NodeType::DoWhileStatement | + NodeType::EmitStatement | + NodeType::ExpressionStatement | + NodeType::ForStatement | + NodeType::IfStatement | + NodeType::InlineAssembly | + NodeType::PlaceholderStatement | + NodeType::Return | + NodeType::RevertStatement | + NodeType::TryStatement | + NodeType::VariableDeclarationStatement | + NodeType::WhileStatement => self.visit_statement(node), + _ => { + warn!("unexpected node type, expected block or statement: {:?}", node.node_type); + Ok(()) + } + } + } +} diff --git a/forge/src/coverage.rs b/forge/src/coverage.rs new file mode 100644 index 000000000000..3b78a620b168 --- /dev/null +++ b/forge/src/coverage.rs @@ -0,0 +1,143 @@ +use comfy_table::{Cell, Color, Row, Table}; +pub use foundry_evm::coverage::*; +use std::io::Write; + +/// A coverage reporter. +pub trait CoverageReporter { + fn build(&mut self, map: CoverageMap); + fn finalize(self) -> eyre::Result<()>; +} + +/// A simple summary reporter that prints the coverage results in a table. +pub struct SummaryReporter { + /// The summary table. + table: Table, + /// The total coverage of the entire project. + total: CoverageSummary, +} + +impl Default for SummaryReporter { + fn default() -> Self { + let mut table = Table::new(); + table.set_header(&["File", "% Lines", "% Statements", "% Branches", "% Funcs"]); + + Self { table, total: CoverageSummary::default() } + } +} + +impl SummaryReporter { + pub fn new() -> Self { + Default::default() + } + + fn add_row(&mut self, name: impl Into, summary: CoverageSummary) { + let mut row = Row::new(); + row.add_cell(name.into()) + .add_cell(format_cell(summary.line_hits, summary.line_count)) + .add_cell(format_cell(summary.statement_hits, summary.statement_count)) + .add_cell(format_cell(summary.branch_hits, summary.branch_count)) + .add_cell(format_cell(summary.function_hits, summary.function_count)); + self.table.add_row(row); + } +} + +impl CoverageReporter for SummaryReporter { + fn build(&mut self, map: CoverageMap) { + for file in map { + let summary = file.summary(); + + self.total.add(&summary); + self.add_row(file.path.to_string_lossy(), summary); + } + } + + fn finalize(mut self) -> eyre::Result<()> { + self.add_row("Total", self.total.clone()); + println!("{}", self.table); + Ok(()) + } +} + +fn format_cell(hits: usize, total: usize) -> Cell { + let percentage = if total == 0 { 1. } else { hits as f64 / total as f64 }; + + Cell::new(format!("{}% ({hits}/{total})", percentage * 100.)).fg(match percentage { + _ if percentage < 0.5 => Color::Red, + _ if percentage < 0.75 => Color::Yellow, + _ => Color::Green, + }) +} + +pub struct LcovReporter { + /// Destination buffer + destination: W, + /// The coverage map to write + map: Option, +} + +impl LcovReporter { + pub fn new(destination: W) -> Self { + Self { destination, map: None } + } +} + +impl CoverageReporter for LcovReporter +where + W: Write, +{ + fn build(&mut self, map: CoverageMap) { + self.map = Some(map); + } + + fn finalize(mut self) -> eyre::Result<()> { + let map = self.map.ok_or_else(|| eyre::eyre!("no coverage map given to reporter"))?; + + for file in map { + let summary = file.summary(); + + writeln!(self.destination, "TN:")?; + writeln!(self.destination, "SF:{}", file.path.to_string_lossy())?; + + // TODO: Line numbers instead of byte offsets + for item in file.items { + match item { + CoverageItem::Function { name, offset, hits } => { + writeln!(self.destination, "FN:{offset},{name}")?; + writeln!(self.destination, "FNDA:{hits},{name}")?; + } + CoverageItem::Line { offset, hits } => { + writeln!(self.destination, "DA:{offset},{hits}")?; + } + CoverageItem::Branch { id, offset, hits, .. } => { + // TODO: Block ID + writeln!( + self.destination, + "BRDA:{offset},{id},{id},{}", + if hits == 0 { "-".to_string() } else { hits.to_string() } + )?; + } + // Statements are not in the LCOV format + CoverageItem::Statement { .. } => (), + } + } + + // Function summary + writeln!(self.destination, "FNF:{}", summary.function_count)?; + writeln!(self.destination, "FNH:{}", summary.function_hits)?; + + // Line summary + writeln!(self.destination, "LF:{}", summary.line_count)?; + writeln!(self.destination, "LH:{}", summary.line_hits)?; + + // Branch summary + writeln!(self.destination, "BRF:{}", summary.branch_count)?; + writeln!(self.destination, "BRH:{}", summary.branch_hits)?; + + writeln!(self.destination, "end_of_record")?; + } + + println!("Wrote LCOV report."); + + Ok(()) + } +} diff --git a/forge/src/lib.rs b/forge/src/lib.rs index 29b18554ed85..a8f203fa6887 100644 --- a/forge/src/lib.rs +++ b/forge/src/lib.rs @@ -1,6 +1,9 @@ /// Gas reports pub mod gas_report; +/// Coverage reports +pub mod coverage; + /// The Forge test runner mod runner; pub use runner::ContractRunner; From 68622584f6e4a52c81cdd8dcfab0ca7308ddf3b0 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Thu, 12 May 2022 15:55:47 +0200 Subject: [PATCH 04/36] feat: support some yul nodes --- evm/src/coverage/visitor.rs | 60 +++++++++++++++++++++++++++++++------ 1 file changed, 51 insertions(+), 9 deletions(-) diff --git a/evm/src/coverage/visitor.rs b/evm/src/coverage/visitor.rs index 37658f96c9b5..e5234a031435 100644 --- a/evm/src/coverage/visitor.rs +++ b/evm/src/coverage/visitor.rs @@ -77,17 +77,28 @@ impl Visitor { Ok(()) } pub fn visit_statement(&mut self, node: Node) -> eyre::Result<()> { - // TODO: inlineassembly + // TODO: YulSwitch, YulForLoop, YulFunctionDefinition, YulVariableDeclaration match node.node_type { // Blocks - NodeType::Block | NodeType::UncheckedBlock => self.visit_block(node), + NodeType::Block | NodeType::UncheckedBlock | NodeType::YulBlock => { + self.visit_block(node) + } + // Inline assembly block + NodeType::InlineAssembly => self.visit_block( + node.attribute("AST") + .ok_or_else(|| eyre::eyre!("inline assembly block with no AST attribute"))?, + ), // Simple statements NodeType::Break | NodeType::Continue | NodeType::EmitStatement | NodeType::PlaceholderStatement | NodeType::Return | - NodeType::RevertStatement => { + NodeType::RevertStatement | + NodeType::YulAssignment | + NodeType::YulBreak | + NodeType::YulContinue | + NodeType::YulLeave => { self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); Ok(()) } @@ -127,13 +138,13 @@ impl Visitor { self.visit_block_or_statement(*body) } // Expression statement - NodeType::ExpressionStatement => self.visit_expression( - node.attribute("expression") - .ok_or_else(|| eyre::eyre!("expression statement had no expression"))?, - ), + NodeType::ExpressionStatement | NodeType::YulExpressionStatement => self + .visit_expression( + node.attribute("expression") + .ok_or_else(|| eyre::eyre!("expression statement had no expression"))?, + ), // If statement NodeType::IfStatement => { - // TODO: create branch self.visit_expression( node.attribute("condition") .ok_or_else(|| eyre::eyre!("while statement had no condition"))?, @@ -172,6 +183,34 @@ impl Visitor { Ok(()) } + NodeType::YulIf => { + self.visit_expression( + node.attribute("condition") + .ok_or_else(|| eyre::eyre!("while statement had no condition"))?, + )?; + + let body: Node = node + .attribute("body") + .ok_or_else(|| eyre::eyre!("yul if statement had no body"))?; + + // We need to store the current branch ID here since visiting the body of either of + // the if blocks may increase `self.branch_id` in the case of nested if statements. + let branch_id = self.branch_id; + + // We increase the branch ID here such that nested branches do not use the same + // branch ID as we do + self.branch_id += 1; + + self.items.push(CoverageItem::Branch { + id: branch_id, + kind: BranchKind::True, + offset: body.src.start, + hits: 0, + }); + self.visit_block(body)?; + + Ok(()) + } // Try-catch statement NodeType::TryStatement => { // TODO: Clauses @@ -194,6 +233,7 @@ impl Visitor { // memberaccess // newexpression // tupleexpression + // yulfunctioncall match node.node_type { NodeType::Assignment | NodeType::UnaryOperation | NodeType::BinaryOperation => { // TODO: Should we explore the subexpressions? @@ -215,7 +255,9 @@ impl Visitor { NodeType::Identifier | NodeType::IndexAccess | NodeType::IndexRangeAccess | - NodeType::Literal => Ok(()), + NodeType::Literal | + NodeType::YulLiteralValue | + NodeType::YulIdentifier => Ok(()), _ => { warn!("unexpected node type, expected an expression: {:?}", node.node_type); Ok(()) From f27f3de605556ce46024c5c4ed0b0769cfe1b3a9 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Thu, 12 May 2022 16:53:41 +0200 Subject: [PATCH 05/36] feat: add branch path id --- evm/src/coverage/mod.rs | 10 +++++++--- evm/src/coverage/visitor.rs | 9 ++++++--- forge/src/coverage.rs | 5 ++--- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/evm/src/coverage/mod.rs b/evm/src/coverage/mod.rs index 7551b4efd20a..8e2d0f5d395a 100644 --- a/evm/src/coverage/mod.rs +++ b/evm/src/coverage/mod.rs @@ -194,9 +194,13 @@ pub enum CoverageItem { Branch { /// The ID that identifies the branch. /// - /// There are two branches with the same ID, - /// one for each outcome (true and false). - id: usize, + /// There may be multiple items with the same branch ID - they belong to the same branch, + /// but represent different paths. + branch_id: usize, + /// The path ID for this branch. + /// + /// The first path has ID 0, the next ID 1, and so on. + path_id: usize, /// The branch kind. kind: BranchKind, /// The byte offset which the branch is on in the source file. diff --git a/evm/src/coverage/visitor.rs b/evm/src/coverage/visitor.rs index e5234a031435..20382c8a40db 100644 --- a/evm/src/coverage/visitor.rs +++ b/evm/src/coverage/visitor.rs @@ -163,7 +163,8 @@ impl Visitor { self.branch_id += 1; self.items.push(CoverageItem::Branch { - id: branch_id, + branch_id, + path_id: 0, kind: BranchKind::True, offset: true_body.src.start, hits: 0, @@ -173,7 +174,8 @@ impl Visitor { let false_body: Option = node.attribute("falseBody"); if let Some(false_body) = false_body { self.items.push(CoverageItem::Branch { - id: branch_id, + branch_id, + path_id: 1, kind: BranchKind::False, offset: false_body.src.start, hits: 0, @@ -202,7 +204,8 @@ impl Visitor { self.branch_id += 1; self.items.push(CoverageItem::Branch { - id: branch_id, + branch_id, + path_id: 0, kind: BranchKind::True, offset: body.src.start, hits: 0, diff --git a/forge/src/coverage.rs b/forge/src/coverage.rs index 3b78a620b168..c42205810ea5 100644 --- a/forge/src/coverage.rs +++ b/forge/src/coverage.rs @@ -108,11 +108,10 @@ where CoverageItem::Line { offset, hits } => { writeln!(self.destination, "DA:{offset},{hits}")?; } - CoverageItem::Branch { id, offset, hits, .. } => { - // TODO: Block ID + CoverageItem::Branch { branch_id, path_id, offset, hits, .. } => { writeln!( self.destination, - "BRDA:{offset},{id},{id},{}", + "BRDA:{offset},{branch_id},{path_id},{}", if hits == 0 { "-".to_string() } else { hits.to_string() } )?; } From fe48e2e7c3c736d9dc465a2d994247390f96002b Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Thu, 12 May 2022 17:10:53 +0200 Subject: [PATCH 06/36] feat: more detailed source locations for items --- evm/src/coverage/mod.rs | 71 +++++++++++++++++++++++++------------ evm/src/coverage/visitor.rs | 46 ++++++++++++++++++------ forge/src/coverage.rs | 21 +++++++---- 3 files changed, 97 insertions(+), 41 deletions(-) diff --git a/evm/src/coverage/mod.rs b/evm/src/coverage/mod.rs index 8e2d0f5d395a..8a575706cd09 100644 --- a/evm/src/coverage/mod.rs +++ b/evm/src/coverage/mod.rs @@ -79,22 +79,17 @@ impl CoverageMap { // Get the coverage items corresponding to the source ID in the source map. if let Some(source) = self.sources.get_mut(&(source_version.clone(), source_id)) { for item in source.items.iter_mut() { - match item { - CoverageItem::Line { offset, hits } | - CoverageItem::Statement { offset, hits } | - CoverageItem::Branch { offset, hits, .. } | - CoverageItem::Function { offset, hits, .. } => { - // We've reached a point where we will no longer be able to map this - // instruction to coverage items - if instruction_offset < *offset { - break - } + let loc = item.source_location(); - // We found a matching coverage item, but there may be more - if instruction_offset == *offset { - *hits += instruction_hits; - } - } + // We've reached a point where we will no longer be able to map this + // instruction to coverage items + if instruction_offset < loc.start { + break + } + + // We found a matching coverage item, but there may be more + if instruction_offset == loc.start { + item.increment_hits(instruction_hits); } } } @@ -176,22 +171,24 @@ impl SourceFile { pub enum CoverageItem { /// An executable line in the code. Line { - /// The byte offset in the source file for the start of the line. - offset: usize, + /// The location of the line in the source code. + loc: SourceLocation, /// The number of times this item was hit. hits: u64, }, /// A statement in the code. Statement { - /// The byte offset in the source file for the start of the statement. - offset: usize, + /// The location of the statement in the source code. + loc: SourceLocation, /// The number of times this statement was hit. hits: u64, }, /// A branch in the code. Branch { + /// The location of the branch in the source code. + loc: SourceLocation, /// The ID that identifies the branch. /// /// There may be multiple items with the same branch ID - they belong to the same branch, @@ -203,23 +200,51 @@ pub enum CoverageItem { path_id: usize, /// The branch kind. kind: BranchKind, - /// The byte offset which the branch is on in the source file. - offset: usize, /// The number of times this item was hit. hits: u64, }, /// A function in the code. Function { + /// The location of the function in the source code. + loc: SourceLocation, /// The name of the function. name: String, - /// The byte offset which the function is on in the source file. - offset: usize, /// The number of times this item was hit. hits: u64, }, } +impl CoverageItem { + pub fn source_location(&self) -> &SourceLocation { + match self { + Self::Line { loc, .. } | + Self::Statement { loc, .. } | + Self::Branch { loc, .. } | + Self::Function { loc, .. } => loc, + } + } + + pub fn increment_hits(&mut self, delta: u64) { + match self { + Self::Line { hits, .. } | + Self::Statement { hits, .. } | + Self::Branch { hits, .. } | + Self::Function { hits, .. } => *hits += delta, + } + } +} + +#[derive(Debug, Clone)] +pub struct SourceLocation { + /// Start byte in the source code. + pub start: usize, + /// Number of bytes in the source code. + pub length: Option, + /// The line in the source code. + pub line: usize, +} + #[derive(Debug, Clone)] pub enum BranchKind { /// A false branch diff --git a/evm/src/coverage/visitor.rs b/evm/src/coverage/visitor.rs index 20382c8a40db..3fa79bdba734 100644 --- a/evm/src/coverage/visitor.rs +++ b/evm/src/coverage/visitor.rs @@ -1,5 +1,5 @@ -use super::{BranchKind, CoverageItem}; -use ethers::solc::artifacts::ast::{Ast, Node, NodeType}; +use super::{BranchKind, CoverageItem, SourceLocation}; +use ethers::solc::artifacts::ast::{self, Ast, Node, NodeType}; use tracing::warn; #[derive(Debug, Default, Clone)] @@ -60,7 +60,11 @@ impl Visitor { match node.body.take() { // Skip virtual functions Some(body) if !is_virtual => { - self.items.push(CoverageItem::Function { name, offset: node.src.start, hits: 0 }); + self.items.push(CoverageItem::Function { + name, + loc: self.source_location_for(&node.src), + hits: 0, + }); self.visit_block(*body) } _ => Ok(()), @@ -99,12 +103,18 @@ impl Visitor { NodeType::YulBreak | NodeType::YulContinue | NodeType::YulLeave => { - self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); + self.items.push(CoverageItem::Statement { + loc: self.source_location_for(&node.src), + hits: 0, + }); Ok(()) } // Variable declaration NodeType::VariableDeclarationStatement => { - self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); + self.items.push(CoverageItem::Statement { + loc: self.source_location_for(&node.src), + hits: 0, + }); if let Some(expr) = node.attribute("initialValue") { self.visit_expression(expr)?; } @@ -166,7 +176,7 @@ impl Visitor { branch_id, path_id: 0, kind: BranchKind::True, - offset: true_body.src.start, + loc: self.source_location_for(&node.src), hits: 0, }); self.visit_block_or_statement(true_body)?; @@ -177,7 +187,7 @@ impl Visitor { branch_id, path_id: 1, kind: BranchKind::False, - offset: false_body.src.start, + loc: self.source_location_for(&node.src), hits: 0, }); self.visit_block_or_statement(false_body)?; @@ -207,7 +217,7 @@ impl Visitor { branch_id, path_id: 0, kind: BranchKind::True, - offset: body.src.start, + loc: self.source_location_for(&node.src), hits: 0, }); self.visit_block(body)?; @@ -240,17 +250,26 @@ impl Visitor { match node.node_type { NodeType::Assignment | NodeType::UnaryOperation | NodeType::BinaryOperation => { // TODO: Should we explore the subexpressions? - self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); + self.items.push(CoverageItem::Statement { + loc: self.source_location_for(&node.src), + hits: 0, + }); Ok(()) } NodeType::FunctionCall => { // TODO: Handle assert and require - self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); + self.items.push(CoverageItem::Statement { + loc: self.source_location_for(&node.src), + hits: 0, + }); Ok(()) } NodeType::Conditional => { // TODO: Do we count these as branches? - self.items.push(CoverageItem::Statement { offset: node.src.start, hits: 0 }); + self.items.push(CoverageItem::Statement { + loc: self.source_location_for(&node.src), + hits: 0, + }); Ok(()) } // Does not count towards coverage @@ -291,4 +310,9 @@ impl Visitor { } } } + + fn source_location_for(&self, loc: &ast::SourceLocation) -> SourceLocation { + // TODO: Map to line + SourceLocation { start: loc.start, length: loc.length, line: 0 } + } } diff --git a/forge/src/coverage.rs b/forge/src/coverage.rs index c42205810ea5..a2cfd34f5675 100644 --- a/forge/src/coverage.rs +++ b/forge/src/coverage.rs @@ -98,20 +98,27 @@ where writeln!(self.destination, "TN:")?; writeln!(self.destination, "SF:{}", file.path.to_string_lossy())?; - // TODO: Line numbers instead of byte offsets for item in file.items { match item { - CoverageItem::Function { name, offset, hits } => { - writeln!(self.destination, "FN:{offset},{name}")?; + CoverageItem::Function { + loc: SourceLocation { line, .. }, name, hits, .. + } => { + writeln!(self.destination, "FN:{line},{name}")?; writeln!(self.destination, "FNDA:{hits},{name}")?; } - CoverageItem::Line { offset, hits } => { - writeln!(self.destination, "DA:{offset},{hits}")?; + CoverageItem::Line { loc: SourceLocation { line, .. }, hits, .. } => { + writeln!(self.destination, "DA:{line},{hits}")?; } - CoverageItem::Branch { branch_id, path_id, offset, hits, .. } => { + CoverageItem::Branch { + loc: SourceLocation { line, .. }, + branch_id, + path_id, + hits, + .. + } => { writeln!( self.destination, - "BRDA:{offset},{branch_id},{path_id},{}", + "BRDA:{line},{branch_id},{path_id},{}", if hits == 0 { "-".to_string() } else { hits.to_string() } )?; } From aa233601b64c33099622ad0e1771cebfea6e679f Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Fri, 3 Jun 2022 06:15:44 +0200 Subject: [PATCH 07/36] feat: map addresses to artifact ids --- cli/src/cmd/forge/coverage.rs | 26 ++++++++++++++++---------- evm/src/trace/identifier/etherscan.rs | 1 + evm/src/trace/identifier/local.rs | 1 + evm/src/trace/identifier/mod.rs | 7 ++++++- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index 72c3f892b03d..726ce4e447fc 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -7,8 +7,10 @@ use crate::{ compile::ProjectCompiler, utils, }; +use cast::trace::identifier::TraceIdentifier; use clap::{AppSettings, ArgEnum, Parser}; use ethers::{ + abi::Address, prelude::{Artifact, Project, ProjectCompileOutput}, solc::{ artifacts::contract::CompactContractBytecode, @@ -20,7 +22,7 @@ use forge::{ coverage::{CoverageMap, CoverageReporter, LcovReporter, SummaryReporter, Visitor}, executor::opts::EvmOpts, result::SuiteResult, - trace::{identifier::LocalTraceIdentifier, CallTraceDecoder, CallTraceDecoderBuilder}, + trace::identifier::LocalTraceIdentifier, MultiContractRunnerBuilder, }; use foundry_common::evm::EvmArgs; @@ -60,7 +62,7 @@ impl CoverageArgs { /// Returns the currently configured [Config] and the extracted [EvmOpts] from that config pub fn config_and_evm_opts(&self) -> eyre::Result<(Config, EvmOpts)> { - // merge all configs + // Merge all configs let figment: Figment = self.into(); let evm_opts = figment.extract()?; let config = Config::from_provider(figment).sanitized(); @@ -83,6 +85,7 @@ impl Cmd for CoverageArgs { } } +// The main flow of the command itself impl CoverageArgs { /// Collects and adjusts configuration. fn configure(&self) -> eyre::Result<(Config, EvmOpts)> { @@ -107,7 +110,6 @@ impl CoverageArgs { project }; - // TODO: This does not strip file prefixes for `SourceFiles`... let output = ProjectCompiler::default() .compile(&project)? .with_stripped_file_prefixes(project.root()); @@ -207,13 +209,19 @@ impl CoverageArgs { let handle = thread::spawn(move || runner.test(&self.filter, Some(tx), false).unwrap()); for mut result in rx.into_iter().flat_map(|(_, suite)| suite.test_results.into_values()) { if let Some(_hit_data) = result.coverage.take() { - let mut decoder = - CallTraceDecoderBuilder::new().with_events(local_identifier.events()).build(); + let mut identities: BTreeMap = BTreeMap::new(); for (_, trace) in &mut result.traces { - decoder.identify(trace, &local_identifier); + local_identifier + .identify_addresses(trace.addresses().into_iter().collect()) + .into_iter() + .for_each(|identity| { + if let Some(artifact_id) = identity.artifact_id { + identities.insert(identity.address, artifact_id); + } + }) } - // TODO: We need an ArtifactId here for the addresses - let CallTraceDecoder { contracts: _, .. } = decoder; + + println!("{identities:?}"); // .. } @@ -245,5 +253,3 @@ pub enum CoverageReportKind { Summary, Lcov, } - -// TODO: Move reporters to own module diff --git a/evm/src/trace/identifier/etherscan.rs b/evm/src/trace/identifier/etherscan.rs index 8854a7f5a953..0626c7bf8ef6 100644 --- a/evm/src/trace/identifier/etherscan.rs +++ b/evm/src/trace/identifier/etherscan.rs @@ -65,6 +65,7 @@ impl TraceIdentifier for EtherscanIdentifier { label: Some(label.clone()), contract: Some(label), abi: Some(Cow::Owned(abi)), + artifact_id: None, }) .collect(); diff --git a/evm/src/trace/identifier/local.rs b/evm/src/trace/identifier/local.rs index 50e73d71c216..74597ad877e3 100644 --- a/evm/src/trace/identifier/local.rs +++ b/evm/src/trace/identifier/local.rs @@ -46,6 +46,7 @@ impl TraceIdentifier for LocalTraceIdentifier { contract: Some(id.identifier()), label: Some(id.name.clone()), abi: Some(Cow::Borrowed(abi)), + artifact_id: Some(id.clone()), }) }) .collect() diff --git a/evm/src/trace/identifier/mod.rs b/evm/src/trace/identifier/mod.rs index 760327e42f93..67617bf15d8b 100644 --- a/evm/src/trace/identifier/mod.rs +++ b/evm/src/trace/identifier/mod.rs @@ -7,7 +7,10 @@ pub use etherscan::EtherscanIdentifier; mod signatures; pub use signatures::SignaturesIdentifier; -use ethers::abi::{Abi, Address}; +use ethers::{ + abi::{Abi, Address}, + prelude::ArtifactId, +}; use std::borrow::Cow; /// An address identity @@ -22,6 +25,8 @@ pub struct AddressIdentity<'a> { pub contract: Option, /// The ABI of the contract at this address pub abi: Option>, + /// The artifact ID of the contract, if any. + pub artifact_id: Option, } /// Trace identifiers figure out what ABIs and labels belong to all the addresses of the trace. From 6235a0c74fa0ef7de912b251a4573d4d68a79bdf Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Fri, 3 Jun 2022 06:44:04 +0200 Subject: [PATCH 08/36] feat: sort of hook up hit map Currently no hit maps are returned on L228, so something is wrong with the hit map collection step, or the identification step. --- cli/src/cmd/forge/coverage.rs | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index 726ce4e447fc..1e2f4a5bb6b1 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -11,7 +11,7 @@ use cast::trace::identifier::TraceIdentifier; use clap::{AppSettings, ArgEnum, Parser}; use ethers::{ abi::Address, - prelude::{Artifact, Project, ProjectCompileOutput}, + prelude::{artifacts, Artifact, Project, ProjectCompileOutput}, solc::{ artifacts::contract::CompactContractBytecode, sourcemap::{self, SourceMap}, @@ -173,8 +173,8 @@ impl CoverageArgs { self, project: Project, output: ProjectCompileOutput, - _source_maps: BTreeMap, - map: CoverageMap, + source_maps: BTreeMap, + mut map: CoverageMap, config: Config, evm_opts: EvmOpts, ) -> eyre::Result<()> { @@ -208,7 +208,7 @@ impl CoverageArgs { // TODO: Coverage for fuzz tests let handle = thread::spawn(move || runner.test(&self.filter, Some(tx), false).unwrap()); for mut result in rx.into_iter().flat_map(|(_, suite)| suite.test_results.into_values()) { - if let Some(_hit_data) = result.coverage.take() { + if let Some(hit_map) = result.coverage.take() { let mut identities: BTreeMap = BTreeMap::new(); for (_, trace) in &mut result.traces { local_identifier @@ -221,9 +221,24 @@ impl CoverageArgs { }) } - println!("{identities:?}"); - - // .. + // TODO: Do we actually only need to identify the test contract that was run in + // order to get the source map and source version for that contract? + for (addr, artifact_id) in identities.into_iter() { + // TODO: Make this more sane + if let Some(source_map) = source_maps.get(&artifact_id) { + if let Some(hit_map) = hit_map.get(&addr) { + map.add_hit_map( + artifact_id.version.clone(), + source_map, + hit_map.clone(), + ); + } else { + println!("Found no hit map: {:?}", artifact_id); + } + } else { + println!("Found no source map: {:?}", artifact_id); + } + } } } From 4791b2ef403d083200774f2346960beac53f3f2f Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Fri, 3 Jun 2022 07:15:32 +0200 Subject: [PATCH 09/36] fix: hit map collection This marks some of the statements as covered, however: - Branches are not currently marked - It does not seem to be entirely accurate currently --- cli/src/cmd/forge/coverage.rs | 6 +----- evm/src/executor/inspector/coverage.rs | 14 ++++++++++++++ evm/src/executor/inspector/stack.rs | 8 +++++++- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index 1e2f4a5bb6b1..368158573b00 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -11,7 +11,7 @@ use cast::trace::identifier::TraceIdentifier; use clap::{AppSettings, ArgEnum, Parser}; use ethers::{ abi::Address, - prelude::{artifacts, Artifact, Project, ProjectCompileOutput}, + prelude::{Artifact, Project, ProjectCompileOutput}, solc::{ artifacts::contract::CompactContractBytecode, sourcemap::{self, SourceMap}, @@ -232,11 +232,7 @@ impl CoverageArgs { source_map, hit_map.clone(), ); - } else { - println!("Found no hit map: {:?}", artifact_id); } - } else { - println!("Found no source map: {:?}", artifact_id); } } } diff --git a/evm/src/executor/inspector/coverage.rs b/evm/src/executor/inspector/coverage.rs index 0178df2eb4cc..5c4dbeb0af9b 100644 --- a/evm/src/executor/inspector/coverage.rs +++ b/evm/src/executor/inspector/coverage.rs @@ -77,6 +77,20 @@ where (Return::Continue, Gas::new(call.gas_limit), Bytes::new()) } + // TODO: Duplicate code of the debugger inspector + fn initialize_interp( + &mut self, + interp: &mut Interpreter, + data: &mut EVMData<'_, DB>, + _: bool, + ) -> Return { + // TODO: This is rebuilt for all contracts every time. We should only run this if the IC + // map for a given address does not exist, *but* we need to account for the fact that the + // code given by the interpreter may either be the contract init code, or the runtime code. + self.build_ic_map(data.env.cfg.spec_id, &interp.contract().code); + Return::Continue + } + // TODO: Don't collect coverage for test contract if possible fn step( &mut self, diff --git a/evm/src/executor/inspector/stack.rs b/evm/src/executor/inspector/stack.rs index b50b7cb103d8..a16d437a610a 100644 --- a/evm/src/executor/inspector/stack.rs +++ b/evm/src/executor/inspector/stack.rs @@ -68,7 +68,13 @@ where ) -> Return { call_inspectors!( inspector, - [&mut self.debugger, &mut self.tracer, &mut self.logs, &mut self.cheatcodes], + [ + &mut self.debugger, + &mut self.coverage, + &mut self.tracer, + &mut self.logs, + &mut self.cheatcodes + ], { let status = inspector.initialize_interp(interpreter, data, is_static); From caa4d2cb4923fbb4c605b2d61fde1f0484190b16 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Fri, 3 Jun 2022 07:20:32 +0200 Subject: [PATCH 10/36] fix: cap summary reporter percentage decimals --- forge/src/coverage.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/forge/src/coverage.rs b/forge/src/coverage.rs index a2cfd34f5675..b449d5674201 100644 --- a/forge/src/coverage.rs +++ b/forge/src/coverage.rs @@ -61,7 +61,7 @@ impl CoverageReporter for SummaryReporter { fn format_cell(hits: usize, total: usize) -> Cell { let percentage = if total == 0 { 1. } else { hits as f64 / total as f64 }; - Cell::new(format!("{}% ({hits}/{total})", percentage * 100.)).fg(match percentage { + Cell::new(format!("{:.2}% ({hits}/{total})", percentage * 100.)).fg(match percentage { _ if percentage < 0.5 => Color::Red, _ if percentage < 0.75 => Color::Yellow, _ => Color::Green, From 52641e4b99e2e6f4c8ca2c6310db3851961ae5f3 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Sun, 5 Jun 2022 11:57:22 +0200 Subject: [PATCH 11/36] refactor: use new source map getter --- cli/src/cmd/forge/coverage.rs | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index 368158573b00..b96c32ad1bc3 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -12,11 +12,7 @@ use clap::{AppSettings, ArgEnum, Parser}; use ethers::{ abi::Address, prelude::{Artifact, Project, ProjectCompileOutput}, - solc::{ - artifacts::contract::CompactContractBytecode, - sourcemap::{self, SourceMap}, - ArtifactId, - }, + solc::{artifacts::contract::CompactContractBytecode, sourcemap::SourceMap, ArtifactId}, }; use forge::{ coverage::{CoverageMap, CoverageReporter, LcovReporter, SummaryReporter, Visitor}, @@ -137,14 +133,7 @@ impl CoverageArgs { }) .map(|(id, artifact)| (id, CompactContractBytecode::from(artifact))) .filter_map(|(id, artifact): (ArtifactId, CompactContractBytecode)| { - let source_map = artifact - .deployed_bytecode - .as_ref() - .and_then(|bytecode| bytecode.bytecode.as_ref()) - .and_then(|bytecode| bytecode.source_map.as_ref()) - .and_then(|source_map| sourcemap::parse(source_map).ok())?; - - Some((id, source_map)) + artifact.get_source_map().and_then(|source_map| Some((id, source_map.ok()?))) }) .collect(); From b62ba81fe9794f10d71a60839576b985419a0339 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Sun, 5 Jun 2022 12:02:23 +0200 Subject: [PATCH 12/36] refactor: clean up a bit --- cli/src/cmd/forge/coverage.rs | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index b96c32ad1bc3..9b27d6702aec 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -23,7 +23,7 @@ use forge::{ }; use foundry_common::evm::EvmArgs; use foundry_config::{figment::Figment, Config}; -use std::{collections::BTreeMap, sync::mpsc::channel, thread}; +use std::{collections::HashMap, sync::mpsc::channel, thread}; // Loads project's figment and merges the build cli arguments into it foundry_config::impl_figment_convert!(CoverageArgs, opts, evm_opts); @@ -117,11 +117,11 @@ impl CoverageArgs { fn prepare( &self, output: ProjectCompileOutput, - ) -> eyre::Result<(CoverageMap, BTreeMap)> { + ) -> eyre::Result<(CoverageMap, HashMap)> { // Get sources and source maps let (artifacts, sources) = output.into_artifacts_with_sources(); - let source_maps: BTreeMap = artifacts + let source_maps: HashMap = artifacts .into_iter() .filter(|(_, artifact)| { // TODO: Filter out dependencies @@ -162,7 +162,7 @@ impl CoverageArgs { self, project: Project, output: ProjectCompileOutput, - source_maps: BTreeMap, + source_maps: HashMap, mut map: CoverageMap, config: Config, evm_opts: EvmOpts, @@ -198,7 +198,7 @@ impl CoverageArgs { let handle = thread::spawn(move || runner.test(&self.filter, Some(tx), false).unwrap()); for mut result in rx.into_iter().flat_map(|(_, suite)| suite.test_results.into_values()) { if let Some(hit_map) = result.coverage.take() { - let mut identities: BTreeMap = BTreeMap::new(); + let mut identities: HashMap = HashMap::new(); for (_, trace) in &mut result.traces { local_identifier .identify_addresses(trace.addresses().into_iter().collect()) @@ -210,19 +210,17 @@ impl CoverageArgs { }) } - // TODO: Do we actually only need to identify the test contract that was run in - // order to get the source map and source version for that contract? - for (addr, artifact_id) in identities.into_iter() { - // TODO: Make this more sane - if let Some(source_map) = source_maps.get(&artifact_id) { - if let Some(hit_map) = hit_map.get(&addr) { - map.add_hit_map( - artifact_id.version.clone(), - source_map, - hit_map.clone(), - ); - } - } + // Record hits for contracts we have source maps for + for (version, hits, source_map) in + identities.into_iter().filter_map(|(addr, artifact_id)| { + Some(( + artifact_id.version.clone(), + hit_map.get(&addr)?, + source_maps.get(&artifact_id)?, + )) + }) + { + map.add_hit_map(version, source_map, hits.clone()); } } } From ccf5b0238d2aacbfe301cf01a7ab7232901f81b4 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Sun, 5 Jun 2022 13:39:11 +0200 Subject: [PATCH 13/36] feat: coverage for both runtime and creation --- cli/src/cmd/forge/coverage.rs | 62 ++++++++++++++++------------------- evm/src/coverage/mod.rs | 16 +++++++++ 2 files changed, 45 insertions(+), 33 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index 9b27d6702aec..99529b880d1f 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -10,7 +10,6 @@ use crate::{ use cast::trace::identifier::TraceIdentifier; use clap::{AppSettings, ArgEnum, Parser}; use ethers::{ - abi::Address, prelude::{Artifact, Project, ProjectCompileOutput}, solc::{artifacts::contract::CompactContractBytecode, sourcemap::SourceMap, ArtifactId}, }; @@ -117,23 +116,28 @@ impl CoverageArgs { fn prepare( &self, output: ProjectCompileOutput, - ) -> eyre::Result<(CoverageMap, HashMap)> { + ) -> eyre::Result<(CoverageMap, HashMap)> { // Get sources and source maps let (artifacts, sources) = output.into_artifacts_with_sources(); - let source_maps: HashMap = artifacts - .into_iter() - .filter(|(_, artifact)| { - // TODO: Filter out dependencies - // Filter out tests - !artifact - .get_abi() - .map(|abi| abi.functions().any(|f| f.name.starts_with("test"))) - .unwrap_or_default() - }) - .map(|(id, artifact)| (id, CompactContractBytecode::from(artifact))) + let source_maps: HashMap = artifacts + .iter() + // TODO: Filter out dependencies + .map(|(id, artifact)| (id.clone(), CompactContractBytecode::from(artifact.clone()))) .filter_map(|(id, artifact): (ArtifactId, CompactContractBytecode)| { - artifact.get_source_map().and_then(|source_map| Some((id, source_map.ok()?))) + Some(( + id, + ( + artifact.get_source_map()?.ok()?, + artifact + .get_deployed_bytecode() + .as_ref()? + .bytecode + .as_ref()? + .source_map()? + .ok()?, + ), + )) }) .collect(); @@ -162,7 +166,7 @@ impl CoverageArgs { self, project: Project, output: ProjectCompileOutput, - source_maps: HashMap, + source_maps: HashMap, mut map: CoverageMap, config: Config, evm_opts: EvmOpts, @@ -198,29 +202,21 @@ impl CoverageArgs { let handle = thread::spawn(move || runner.test(&self.filter, Some(tx), false).unwrap()); for mut result in rx.into_iter().flat_map(|(_, suite)| suite.test_results.into_values()) { if let Some(hit_map) = result.coverage.take() { - let mut identities: HashMap = HashMap::new(); for (_, trace) in &mut result.traces { local_identifier .identify_addresses(trace.addresses().into_iter().collect()) .into_iter() - .for_each(|identity| { - if let Some(artifact_id) = identity.artifact_id { - identities.insert(identity.address, artifact_id); - } - }) - } + .filter_map(|identity| { + let artifact_id = identity.artifact_id?; + let source_map = source_maps.get(&artifact_id)?; - // Record hits for contracts we have source maps for - for (version, hits, source_map) in - identities.into_iter().filter_map(|(addr, artifact_id)| { - Some(( - artifact_id.version.clone(), - hit_map.get(&addr)?, - source_maps.get(&artifact_id)?, - )) - }) - { - map.add_hit_map(version, source_map, hits.clone()); + Some((artifact_id, source_map, hit_map.get(&identity.address)?)) + }) + .for_each(|(id, source_map, hits)| { + // TODO: Distinguish between creation/runtime in a smart way + map.add_hit_map(id.version.clone(), &source_map.0, hits.clone()); + map.add_hit_map(id.version, &source_map.1, hits.clone()) + }); } } } diff --git a/evm/src/coverage/mod.rs b/evm/src/coverage/mod.rs index 8a575706cd09..7fd4b789ce4c 100644 --- a/evm/src/coverage/mod.rs +++ b/evm/src/coverage/mod.rs @@ -134,6 +134,13 @@ pub struct SourceFile { impl SourceFile { /// Get a simple summary of the coverage for the file. pub fn summary(&self) -> CoverageSummary { + println!("Uncovered for {:?}:", self.path); + self.items.iter().for_each(|item| { + if item.hits() == 0 { + println!("- {:?}", item); + } + }); + self.items.iter().fold(CoverageSummary::default(), |mut summary, item| match item { CoverageItem::Line { hits, .. } => { summary.line_count += 1; @@ -233,6 +240,15 @@ impl CoverageItem { Self::Function { hits, .. } => *hits += delta, } } + + pub fn hits(&self) -> u64 { + match self { + Self::Line { hits, .. } | + Self::Statement { hits, .. } | + Self::Branch { hits, .. } | + Self::Function { hits, .. } => *hits, + } + } } #[derive(Debug, Clone)] From 39f6aed650e1296592822d9be292c9be43d88b49 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Sun, 5 Jun 2022 13:51:55 +0200 Subject: [PATCH 14/36] feat: debug reporter --- cli/src/cmd/forge/coverage.rs | 10 ++++- evm/src/coverage/mod.rs | 7 ---- forge/src/coverage.rs | 69 ++++++++++++++++++++++++++++++++++- 3 files changed, 77 insertions(+), 9 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index 99529b880d1f..a2bd836f2bbb 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -14,7 +14,9 @@ use ethers::{ solc::{artifacts::contract::CompactContractBytecode, sourcemap::SourceMap, ArtifactId}, }; use forge::{ - coverage::{CoverageMap, CoverageReporter, LcovReporter, SummaryReporter, Visitor}, + coverage::{ + CoverageMap, CoverageReporter, DebugReporter, LcovReporter, SummaryReporter, Visitor, + }, executor::opts::EvmOpts, result::SuiteResult, trace::identifier::LocalTraceIdentifier, @@ -237,6 +239,11 @@ impl CoverageArgs { reporter.build(map); reporter.finalize() } + CoverageReportKind::Debug => { + let mut reporter = DebugReporter::new(); + reporter.build(map); + reporter.finalize() + } } } } @@ -246,4 +253,5 @@ impl CoverageArgs { pub enum CoverageReportKind { Summary, Lcov, + Debug, } diff --git a/evm/src/coverage/mod.rs b/evm/src/coverage/mod.rs index 7fd4b789ce4c..9914f6ec9c37 100644 --- a/evm/src/coverage/mod.rs +++ b/evm/src/coverage/mod.rs @@ -134,13 +134,6 @@ pub struct SourceFile { impl SourceFile { /// Get a simple summary of the coverage for the file. pub fn summary(&self) -> CoverageSummary { - println!("Uncovered for {:?}:", self.path); - self.items.iter().for_each(|item| { - if item.hits() == 0 { - println!("- {:?}", item); - } - }); - self.items.iter().fold(CoverageSummary::default(), |mut summary, item| match item { CoverageItem::Line { hits, .. } => { summary.line_count += 1; diff --git a/forge/src/coverage.rs b/forge/src/coverage.rs index b449d5674201..b913983168cb 100644 --- a/forge/src/coverage.rs +++ b/forge/src/coverage.rs @@ -1,6 +1,6 @@ use comfy_table::{Cell, Color, Row, Table}; pub use foundry_evm::coverage::*; -use std::io::Write; +use std::{collections::HashMap, io::Write, path::PathBuf}; /// A coverage reporter. pub trait CoverageReporter { @@ -147,3 +147,70 @@ where Ok(()) } } + +/// A super verbose reporter for debugging coverage while it is still unstable. +pub struct DebugReporter { + /// The summary table. + table: Table, + /// The total coverage of the entire project. + total: CoverageSummary, + /// Uncovered items + uncovered: HashMap>, +} + +impl Default for DebugReporter { + fn default() -> Self { + let mut table = Table::new(); + table.set_header(&["File", "% Lines", "% Statements", "% Branches", "% Funcs"]); + + Self { table, total: CoverageSummary::default(), uncovered: HashMap::default() } + } +} + +impl DebugReporter { + pub fn new() -> Self { + Default::default() + } + + fn add_row(&mut self, name: impl Into, summary: CoverageSummary) { + let mut row = Row::new(); + row.add_cell(name.into()) + .add_cell(format_cell(summary.line_hits, summary.line_count)) + .add_cell(format_cell(summary.statement_hits, summary.statement_count)) + .add_cell(format_cell(summary.branch_hits, summary.branch_count)) + .add_cell(format_cell(summary.function_hits, summary.function_count)); + self.table.add_row(row); + } +} + +impl CoverageReporter for DebugReporter { + fn build(&mut self, map: CoverageMap) { + for file in map { + let summary = file.summary(); + + self.total.add(&summary); + self.add_row(file.path.to_string_lossy(), summary); + + file.items.iter().for_each(|item| { + if item.hits() == 0 { + self.uncovered.entry(file.path.clone()).or_default().push(item.clone()); + } + }) + } + } + + fn finalize(mut self) -> eyre::Result<()> { + self.add_row("Total", self.total.clone()); + println!("{}", self.table); + + for (path, items) in self.uncovered { + println!("Uncovered for {:?}:", path); + items.iter().for_each(|item| { + if item.hits() == 0 { + println!("- {:?}", item); + } + }); + } + Ok(()) + } +} From a17b67336be1082e15b9422115dd0f921fba1b86 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Sun, 5 Jun 2022 14:05:42 +0200 Subject: [PATCH 15/36] feat: naively filter out tests and deps --- cli/src/cmd/forge/coverage.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index a2bd836f2bbb..5cc2eddb509b 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -123,9 +123,8 @@ impl CoverageArgs { let (artifacts, sources) = output.into_artifacts_with_sources(); let source_maps: HashMap = artifacts - .iter() - // TODO: Filter out dependencies - .map(|(id, artifact)| (id.clone(), CompactContractBytecode::from(artifact.clone()))) + .into_iter() + .map(|(id, artifact)| (id, CompactContractBytecode::from(artifact))) .filter_map(|(id, artifact): (ArtifactId, CompactContractBytecode)| { Some(( id, @@ -145,6 +144,13 @@ impl CoverageArgs { let mut map = CoverageMap::new(); for (path, versioned_sources) in sources.0.into_iter() { + // TODO: Make these checks robust + let is_test = path.ends_with(".t.sol"); + let is_dependency = path.starts_with("lib"); + if is_test || is_dependency { + continue + } + for mut versioned_source in versioned_sources { let source = &mut versioned_source.source_file; if let Some(ast) = source.ast.take() { From 6e32d3621077776d50d8dc0c24cdcb2878136914 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Sun, 5 Jun 2022 14:31:38 +0200 Subject: [PATCH 16/36] chore: note to self --- cli/src/cmd/forge/coverage.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index 5cc2eddb509b..d3e5e13a9575 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -145,6 +145,10 @@ impl CoverageArgs { let mut map = CoverageMap::new(); for (path, versioned_sources) in sources.0.into_iter() { // TODO: Make these checks robust + // NOTE: We should actually filter out test contracts in the AST + // instead of on a source file level. Repositories like Solmate + // have a lot of abstract contracts that are being tested, and these + // are usually defined in the test files themselves. let is_test = path.ends_with(".t.sol"); let is_dependency = path.starts_with("lib"); if is_test || is_dependency { From d20e0507d0315e8d790a3eb44a18e66fceb70dda Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Tue, 14 Jun 2022 20:23:32 +0200 Subject: [PATCH 17/36] feat: detect executable lines --- cli/src/cmd/forge/coverage.rs | 10 +++--- evm/src/coverage/mod.rs | 23 ++++++++++--- evm/src/coverage/visitor.rs | 62 ++++++++++++++++++++++++++--------- 3 files changed, 71 insertions(+), 24 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index d3e5e13a9575..04100c65cc6e 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -24,7 +24,7 @@ use forge::{ }; use foundry_common::evm::EvmArgs; use foundry_config::{figment::Figment, Config}; -use std::{collections::HashMap, sync::mpsc::channel, thread}; +use std::{collections::HashMap, path::PathBuf, sync::mpsc::channel, thread}; // Loads project's figment and merges the build cli arguments into it foundry_config::impl_figment_convert!(CoverageArgs, opts, evm_opts); @@ -158,14 +158,14 @@ impl CoverageArgs { for mut versioned_source in versioned_sources { let source = &mut versioned_source.source_file; if let Some(ast) = source.ast.take() { - let mut visitor = Visitor::new(); - visitor.visit_ast(ast)?; + let items = Visitor::new(std::fs::read_to_string(PathBuf::from(&path))?) + .visit_ast(ast)?; - if visitor.items.is_empty() { + if items.is_empty() { continue } - map.add_source(path.clone(), versioned_source, visitor.items); + map.add_source(path.clone(), versioned_source, items); } } } diff --git a/evm/src/coverage/mod.rs b/evm/src/coverage/mod.rs index 9914f6ec9c37..38077b0afa10 100644 --- a/evm/src/coverage/mod.rs +++ b/evm/src/coverage/mod.rs @@ -79,16 +79,14 @@ impl CoverageMap { // Get the coverage items corresponding to the source ID in the source map. if let Some(source) = self.sources.get_mut(&(source_version.clone(), source_id)) { for item in source.items.iter_mut() { - let loc = item.source_location(); - // We've reached a point where we will no longer be able to map this // instruction to coverage items - if instruction_offset < loc.start { + if instruction_offset < item.anchor() { break } // We found a matching coverage item, but there may be more - if instruction_offset == loc.start { + if instruction_offset == item.anchor() { item.increment_hits(instruction_hits); } } @@ -173,6 +171,8 @@ pub enum CoverageItem { Line { /// The location of the line in the source code. loc: SourceLocation, + /// The instruction counter that covers this line. + anchor: usize, /// The number of times this item was hit. hits: u64, }, @@ -181,6 +181,8 @@ pub enum CoverageItem { Statement { /// The location of the statement in the source code. loc: SourceLocation, + /// The instruction counter that covers this statement. + anchor: usize, /// The number of times this statement was hit. hits: u64, }, @@ -189,6 +191,8 @@ pub enum CoverageItem { Branch { /// The location of the branch in the source code. loc: SourceLocation, + /// The instruction counter that covers this branch. + anchor: usize, /// The ID that identifies the branch. /// /// There may be multiple items with the same branch ID - they belong to the same branch, @@ -208,6 +212,8 @@ pub enum CoverageItem { Function { /// The location of the function in the source code. loc: SourceLocation, + /// The instruction counter that covers this function. + anchor: usize, /// The name of the function. name: String, /// The number of times this item was hit. @@ -225,6 +231,15 @@ impl CoverageItem { } } + pub fn anchor(&self) -> usize { + match self { + Self::Line { anchor, .. } | + Self::Statement { anchor, .. } | + Self::Branch { anchor, .. } | + Self::Function { anchor, .. } => *anchor, + } + } + pub fn increment_hits(&mut self, delta: u64) { match self { Self::Line { hits, .. } | diff --git a/evm/src/coverage/visitor.rs b/evm/src/coverage/visitor.rs index 3fa79bdba734..9bb1ed3ccec7 100644 --- a/evm/src/coverage/visitor.rs +++ b/evm/src/coverage/visitor.rs @@ -9,14 +9,19 @@ pub struct Visitor { /// The current branch ID // TODO: Does this need to be unique across files? pub branch_id: usize, + /// The source code that contains the AST being walked. + pub source: String, + /// Stores the last line we put in the items collection to ensure we don't push duplicate lines + last_line: usize, } impl Visitor { - pub fn new() -> Self { - Self::default() + pub fn new(source: String) -> Self { + Self { source, ..Default::default() } } - pub fn visit_ast(&mut self, ast: Ast) -> eyre::Result<()> { + pub fn visit_ast(mut self, ast: Ast) -> eyre::Result> { + // Walk AST for node in ast.nodes.into_iter() { if !matches!(node.node_type, NodeType::ContractDefinition) { continue @@ -25,7 +30,7 @@ impl Visitor { self.visit_contract(node)?; } - Ok(()) + Ok(self.items) } pub fn visit_contract(&mut self, node: Node) -> eyre::Result<()> { @@ -60,9 +65,10 @@ impl Visitor { match node.body.take() { // Skip virtual functions Some(body) if !is_virtual => { - self.items.push(CoverageItem::Function { + self.push_item(CoverageItem::Function { name, loc: self.source_location_for(&node.src), + anchor: node.src.start, hits: 0, }); self.visit_block(*body) @@ -103,16 +109,18 @@ impl Visitor { NodeType::YulBreak | NodeType::YulContinue | NodeType::YulLeave => { - self.items.push(CoverageItem::Statement { + self.push_item(CoverageItem::Statement { loc: self.source_location_for(&node.src), + anchor: node.src.start, hits: 0, }); Ok(()) } // Variable declaration NodeType::VariableDeclarationStatement => { - self.items.push(CoverageItem::Statement { + self.push_item(CoverageItem::Statement { loc: self.source_location_for(&node.src), + anchor: node.src.start, hits: 0, }); if let Some(expr) = node.attribute("initialValue") { @@ -172,22 +180,24 @@ impl Visitor { // branch ID as we do self.branch_id += 1; - self.items.push(CoverageItem::Branch { + self.push_item(CoverageItem::Branch { branch_id, path_id: 0, kind: BranchKind::True, loc: self.source_location_for(&node.src), + anchor: true_body.src.start, hits: 0, }); self.visit_block_or_statement(true_body)?; let false_body: Option = node.attribute("falseBody"); if let Some(false_body) = false_body { - self.items.push(CoverageItem::Branch { + self.push_item(CoverageItem::Branch { branch_id, path_id: 1, kind: BranchKind::False, loc: self.source_location_for(&node.src), + anchor: false_body.src.start, hits: 0, }); self.visit_block_or_statement(false_body)?; @@ -213,11 +223,12 @@ impl Visitor { // branch ID as we do self.branch_id += 1; - self.items.push(CoverageItem::Branch { + self.push_item(CoverageItem::Branch { branch_id, path_id: 0, kind: BranchKind::True, loc: self.source_location_for(&node.src), + anchor: node.src.start, hits: 0, }); self.visit_block(body)?; @@ -250,24 +261,27 @@ impl Visitor { match node.node_type { NodeType::Assignment | NodeType::UnaryOperation | NodeType::BinaryOperation => { // TODO: Should we explore the subexpressions? - self.items.push(CoverageItem::Statement { + self.push_item(CoverageItem::Statement { loc: self.source_location_for(&node.src), + anchor: node.src.start, hits: 0, }); Ok(()) } NodeType::FunctionCall => { // TODO: Handle assert and require - self.items.push(CoverageItem::Statement { + self.push_item(CoverageItem::Statement { loc: self.source_location_for(&node.src), + anchor: node.src.start, hits: 0, }); Ok(()) } NodeType::Conditional => { // TODO: Do we count these as branches? - self.items.push(CoverageItem::Statement { + self.push_item(CoverageItem::Statement { loc: self.source_location_for(&node.src), + anchor: node.src.start, hits: 0, }); Ok(()) @@ -311,8 +325,26 @@ impl Visitor { } } + /// Pushes a coverage item to the internal collection, and might push a line item as well. + fn push_item(&mut self, item: CoverageItem) { + let source_location = item.source_location(); + if self.last_line < source_location.line { + self.items.push(CoverageItem::Line { + loc: source_location.clone(), + anchor: item.anchor(), + hits: 0, + }); + self.last_line = source_location.line; + } + + self.items.push(item); + } + fn source_location_for(&self, loc: &ast::SourceLocation) -> SourceLocation { - // TODO: Map to line - SourceLocation { start: loc.start, length: loc.length, line: 0 } + SourceLocation { + start: loc.start, + length: loc.length, + line: self.source[..loc.start].as_bytes().iter().filter(|&&c| c == b'\n').count() + 1, + } } } From 70bf2c4e0545b75f3e2dd8d3ba6b4dcc8f38a96e Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Tue, 14 Jun 2022 20:23:56 +0200 Subject: [PATCH 18/36] feat: improve reporters a bit --- evm/src/coverage/mod.rs | 35 +++++++++++++++++++++++++++++++++++ forge/src/coverage.rs | 24 ++++++++++++++++-------- 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/evm/src/coverage/mod.rs b/evm/src/coverage/mod.rs index 38077b0afa10..307e08e395be 100644 --- a/evm/src/coverage/mod.rs +++ b/evm/src/coverage/mod.rs @@ -8,6 +8,7 @@ use ethers::{ use semver::Version; use std::{ collections::{BTreeMap, HashMap}, + fmt::Display, path::PathBuf, usize, }; @@ -259,6 +260,28 @@ impl CoverageItem { } } +impl Display for CoverageItem { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match &self { + CoverageItem::Line { loc, anchor, hits } => { + write!(f, "Line (location: {loc}, anchor: {anchor}, hits: {hits})") + } + CoverageItem::Statement { loc, anchor, hits } => { + write!(f, "Statement (location: {loc}, anchor: {anchor}, hits: {hits})") + } + CoverageItem::Branch { loc, anchor, hits, branch_id, path_id, kind } => { + write!(f, "{} Branch (branch: {branch_id}, path: {path_id}) (location: {loc}, anchor: {anchor}, hits: {hits})", match kind { + BranchKind::True => "True", + BranchKind::False => "False", + }) + } + CoverageItem::Function { loc, anchor, hits, name } => { + write!(f, r#"Function "{name}" (location: {loc}, anchor: {anchor}, hits: {hits})"#) + } + } + } +} + #[derive(Debug, Clone)] pub struct SourceLocation { /// Start byte in the source code. @@ -269,6 +292,18 @@ pub struct SourceLocation { pub line: usize, } +impl Display for SourceLocation { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "L{}, C{}-{}", + self.line, + self.start, + self.length.map_or(self.start, |length| self.start + length) + ) + } +} + #[derive(Debug, Clone)] pub enum BranchKind { /// A false branch diff --git a/forge/src/coverage.rs b/forge/src/coverage.rs index b913983168cb..8ec30a45c73e 100644 --- a/forge/src/coverage.rs +++ b/forge/src/coverage.rs @@ -1,4 +1,4 @@ -use comfy_table::{Cell, Color, Row, Table}; +use comfy_table::{Attribute, Cell, Color, Row, Table}; pub use foundry_evm::coverage::*; use std::{collections::HashMap, io::Write, path::PathBuf}; @@ -61,11 +61,18 @@ impl CoverageReporter for SummaryReporter { fn format_cell(hits: usize, total: usize) -> Cell { let percentage = if total == 0 { 1. } else { hits as f64 / total as f64 }; - Cell::new(format!("{:.2}% ({hits}/{total})", percentage * 100.)).fg(match percentage { - _ if percentage < 0.5 => Color::Red, - _ if percentage < 0.75 => Color::Yellow, - _ => Color::Green, - }) + let mut cell = + Cell::new(format!("{:.2}% ({hits}/{total})", percentage * 100.)).fg(match percentage { + _ if total == 0 => Color::Grey, + _ if percentage < 0.5 => Color::Red, + _ if percentage < 0.75 => Color::Yellow, + _ => Color::Green, + }); + + if total == 0 { + cell = cell.add_attribute(Attribute::Dim); + } + cell } pub struct LcovReporter { @@ -204,12 +211,13 @@ impl CoverageReporter for DebugReporter { println!("{}", self.table); for (path, items) in self.uncovered { - println!("Uncovered for {:?}:", path); + println!("Uncovered for {}:", path.to_string_lossy()); items.iter().for_each(|item| { if item.hits() == 0 { - println!("- {:?}", item); + println!("- {}", item); } }); + println!(); } Ok(()) } From a6faf221c73a3adaeb0b8f9fea7f2fb030991df4 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Tue, 14 Jun 2022 20:42:34 +0200 Subject: [PATCH 19/36] fix: don't add exec lines for function defs --- evm/src/coverage/visitor.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/evm/src/coverage/visitor.rs b/evm/src/coverage/visitor.rs index 9bb1ed3ccec7..80e6bbff26d4 100644 --- a/evm/src/coverage/visitor.rs +++ b/evm/src/coverage/visitor.rs @@ -328,7 +328,11 @@ impl Visitor { /// Pushes a coverage item to the internal collection, and might push a line item as well. fn push_item(&mut self, item: CoverageItem) { let source_location = item.source_location(); - if self.last_line < source_location.line { + + // Push a line item if we haven't already + if matches!(item, CoverageItem::Statement { .. } | CoverageItem::Branch { .. }) && + self.last_line < source_location.line + { self.items.push(CoverageItem::Line { loc: source_location.clone(), anchor: item.anchor(), From 6cacea9c9a7d250ef9af00d2e958da3a6e257bc5 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Tue, 14 Jun 2022 21:39:19 +0200 Subject: [PATCH 20/36] fix: better coverage detection using anchors Previously coverage was based on the start of AST nodes in the source code. With the anchoring system, opcodes in relevant source maps are searched, and the first one within the source range of the AST node is selected as an anchor. If the anchor is executed once, then the coverage item is considered to be covered. This works very well, but false positives and edge cases still need to be found. --- cli/src/cmd/forge/coverage.rs | 20 ++++++++- evm/src/coverage/mod.rs | 25 +++++------ evm/src/coverage/visitor.rs | 81 +++++++++++++++++++++++++---------- 3 files changed, 87 insertions(+), 39 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index 04100c65cc6e..e4f7f1b8b2fd 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -158,8 +158,24 @@ impl CoverageArgs { for mut versioned_source in versioned_sources { let source = &mut versioned_source.source_file; if let Some(ast) = source.ast.take() { - let items = Visitor::new(std::fs::read_to_string(PathBuf::from(&path))?) - .visit_ast(ast)?; + let source_maps: HashMap = source_maps + .iter() + .filter(|(id, _)| { + id.version == versioned_source.version && + id.source == PathBuf::from(&path) + }) + .map(|(id, (_, source_map))| { + // TODO: Deploy source map too? + (id.name.clone(), source_map.clone()) + }) + .collect(); + + let items = Visitor::new( + source.id, + std::fs::read_to_string(PathBuf::from(&path))?, + source_maps, + ) + .visit_ast(ast)?; if items.is_empty() { continue diff --git a/evm/src/coverage/mod.rs b/evm/src/coverage/mod.rs index 307e08e395be..f7c3a1ab0ce1 100644 --- a/evm/src/coverage/mod.rs +++ b/evm/src/coverage/mod.rs @@ -66,28 +66,25 @@ impl CoverageMap { continue } - // Get the instruction offset and the source ID in the source map. - let (instruction_offset, source_id) = if let Some((instruction_offset, source_id)) = - source_map - .get(ic) - .and_then(|source_element| Some((source_element.offset, source_element.index?))) - { - (instruction_offset, source_id) - } else { - continue - }; + // Get the source ID in the source map. + let source_id = + if let Some(source_id) = source_map.get(ic).and_then(|element| element.index) { + source_id + } else { + continue + }; // Get the coverage items corresponding to the source ID in the source map. if let Some(source) = self.sources.get_mut(&(source_version.clone(), source_id)) { for item in source.items.iter_mut() { // We've reached a point where we will no longer be able to map this // instruction to coverage items - if instruction_offset < item.anchor() { - break - } + /*if ic > item.anchor() { + break; + }*/ // We found a matching coverage item, but there may be more - if instruction_offset == item.anchor() { + if ic == item.anchor() { item.increment_hits(instruction_hits); } } diff --git a/evm/src/coverage/visitor.rs b/evm/src/coverage/visitor.rs index 80e6bbff26d4..1c10ff9c3446 100644 --- a/evm/src/coverage/visitor.rs +++ b/evm/src/coverage/visitor.rs @@ -1,23 +1,35 @@ use super::{BranchKind, CoverageItem, SourceLocation}; -use ethers::solc::artifacts::ast::{self, Ast, Node, NodeType}; +use ethers::{ + prelude::sourcemap::SourceMap, + solc::artifacts::ast::{self, Ast, Node, NodeType}, +}; +use std::collections::HashMap; use tracing::warn; #[derive(Debug, Default, Clone)] pub struct Visitor { - /// Coverage items - pub items: Vec, + /// The source ID containing the AST being walked. + source_id: u32, + /// The source code that contains the AST being walked. + source: String, + /// Source maps for this specific source file, keyed by the contract name. + source_maps: HashMap, + + /// The contract whose AST we are currently walking + context: String, /// The current branch ID // TODO: Does this need to be unique across files? - pub branch_id: usize, - /// The source code that contains the AST being walked. - pub source: String, + branch_id: usize, /// Stores the last line we put in the items collection to ensure we don't push duplicate lines last_line: usize, + + /// Coverage items + items: Vec, } impl Visitor { - pub fn new(source: String) -> Self { - Self { source, ..Default::default() } + pub fn new(source_id: u32, source: String, source_maps: HashMap) -> Self { + Self { source_id, source, source_maps, ..Default::default() } } pub fn visit_ast(mut self, ast: Ast) -> eyre::Result> { @@ -33,7 +45,7 @@ impl Visitor { Ok(self.items) } - pub fn visit_contract(&mut self, node: Node) -> eyre::Result<()> { + fn visit_contract(&mut self, node: Node) -> eyre::Result<()> { let is_contract = node.attribute("contractKind").map_or(false, |kind: String| kind == "contract"); let is_abstract: bool = node.attribute("abstract").unwrap_or_default(); @@ -43,6 +55,12 @@ impl Visitor { return Ok(()) } + // Set the current context + let contract_name: String = + node.attribute("name").ok_or_else(|| eyre::eyre!("contract has no name"))?; + self.context = contract_name; + + // Find all functions and walk their AST for node in node.nodes { if node.node_type == NodeType::FunctionDefinition { self.visit_function_definition(node)?; @@ -52,7 +70,7 @@ impl Visitor { Ok(()) } - pub fn visit_function_definition(&mut self, mut node: Node) -> eyre::Result<()> { + fn visit_function_definition(&mut self, mut node: Node) -> eyre::Result<()> { let name: String = node.attribute("name").ok_or_else(|| eyre::eyre!("function has no name"))?; let is_virtual: bool = node.attribute("virtual").unwrap_or_default(); @@ -68,7 +86,7 @@ impl Visitor { self.push_item(CoverageItem::Function { name, loc: self.source_location_for(&node.src), - anchor: node.src.start, + anchor: self.anchor_for(&body.src), hits: 0, }); self.visit_block(*body) @@ -77,7 +95,7 @@ impl Visitor { } } - pub fn visit_block(&mut self, node: Node) -> eyre::Result<()> { + fn visit_block(&mut self, node: Node) -> eyre::Result<()> { let statements: Vec = node.attribute("statements").unwrap_or_default(); for statement in statements { @@ -86,7 +104,7 @@ impl Visitor { Ok(()) } - pub fn visit_statement(&mut self, node: Node) -> eyre::Result<()> { + fn visit_statement(&mut self, node: Node) -> eyre::Result<()> { // TODO: YulSwitch, YulForLoop, YulFunctionDefinition, YulVariableDeclaration match node.node_type { // Blocks @@ -111,7 +129,7 @@ impl Visitor { NodeType::YulLeave => { self.push_item(CoverageItem::Statement { loc: self.source_location_for(&node.src), - anchor: node.src.start, + anchor: self.anchor_for(&node.src), hits: 0, }); Ok(()) @@ -120,7 +138,7 @@ impl Visitor { NodeType::VariableDeclarationStatement => { self.push_item(CoverageItem::Statement { loc: self.source_location_for(&node.src), - anchor: node.src.start, + anchor: self.anchor_for(&node.src), hits: 0, }); if let Some(expr) = node.attribute("initialValue") { @@ -185,7 +203,7 @@ impl Visitor { path_id: 0, kind: BranchKind::True, loc: self.source_location_for(&node.src), - anchor: true_body.src.start, + anchor: self.anchor_for(&true_body.src), hits: 0, }); self.visit_block_or_statement(true_body)?; @@ -197,7 +215,7 @@ impl Visitor { path_id: 1, kind: BranchKind::False, loc: self.source_location_for(&node.src), - anchor: false_body.src.start, + anchor: self.anchor_for(&false_body.src), hits: 0, }); self.visit_block_or_statement(false_body)?; @@ -228,7 +246,7 @@ impl Visitor { path_id: 0, kind: BranchKind::True, loc: self.source_location_for(&node.src), - anchor: node.src.start, + anchor: self.anchor_for(&node.src), hits: 0, }); self.visit_block(body)?; @@ -251,7 +269,7 @@ impl Visitor { } } - pub fn visit_expression(&mut self, node: Node) -> eyre::Result<()> { + fn visit_expression(&mut self, node: Node) -> eyre::Result<()> { // TODO // elementarytypenameexpression // memberaccess @@ -263,7 +281,7 @@ impl Visitor { // TODO: Should we explore the subexpressions? self.push_item(CoverageItem::Statement { loc: self.source_location_for(&node.src), - anchor: node.src.start, + anchor: self.anchor_for(&node.src), hits: 0, }); Ok(()) @@ -272,7 +290,7 @@ impl Visitor { // TODO: Handle assert and require self.push_item(CoverageItem::Statement { loc: self.source_location_for(&node.src), - anchor: node.src.start, + anchor: self.anchor_for(&node.src), hits: 0, }); Ok(()) @@ -281,7 +299,7 @@ impl Visitor { // TODO: Do we count these as branches? self.push_item(CoverageItem::Statement { loc: self.source_location_for(&node.src), - anchor: node.src.start, + anchor: self.anchor_for(&node.src), hits: 0, }); Ok(()) @@ -301,7 +319,7 @@ impl Visitor { } } - pub fn visit_block_or_statement(&mut self, node: Node) -> eyre::Result<()> { + fn visit_block_or_statement(&mut self, node: Node) -> eyre::Result<()> { match node.node_type { NodeType::Block => self.visit_block(node), NodeType::Break | @@ -351,4 +369,21 @@ impl Visitor { line: self.source[..loc.start].as_bytes().iter().filter(|&&c| c == b'\n').count() + 1, } } + + fn anchor_for(&self, loc: &ast::SourceLocation) -> usize { + self.source_maps + .get(&self.context) + .and_then(|source_map| { + source_map + .iter() + .enumerate() + .find(|(_, element)| { + element.index.map_or(false, |source_id| { + source_id == self.source_id && element.offset >= loc.start + }) + }) + .map(|(ic, _)| ic) + }) + .unwrap_or(loc.start) + } } From c654db6db7177a758e28d884fad0db53a478a99f Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Tue, 14 Jun 2022 21:46:21 +0200 Subject: [PATCH 21/36] fix: more robust anchor selection Consider the full range instead of just the offset. Unfortunately breaks detection of some items; suspected to be related to not using constructor source maps. --- evm/src/coverage/visitor.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/evm/src/coverage/visitor.rs b/evm/src/coverage/visitor.rs index 1c10ff9c3446..13ea2012fedf 100644 --- a/evm/src/coverage/visitor.rs +++ b/evm/src/coverage/visitor.rs @@ -378,9 +378,17 @@ impl Visitor { .iter() .enumerate() .find(|(_, element)| { - element.index.map_or(false, |source_id| { - source_id == self.source_id && element.offset >= loc.start - }) + element + .index + .and_then(|source_id| { + Some( + source_id == self.source_id && + element.offset >= loc.start && + element.offset + element.length <= + loc.start + loc.length?, + ) + }) + .unwrap_or_default() }) .map(|(ic, _)| ic) }) From 6c1182e1b7da4e55b3b4c49ad8e3848d782375c4 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Tue, 14 Jun 2022 22:26:36 +0200 Subject: [PATCH 22/36] chore: clippy --- cli/src/cmd/forge/coverage.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index e4f7f1b8b2fd..29c18b9f28bc 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -82,6 +82,9 @@ impl Cmd for CoverageArgs { } } +/// A map, keyed by artifact ID, to a tuple of the deployment source map and the runtime source map. +type SourceMaps = HashMap; + // The main flow of the command itself impl CoverageArgs { /// Collects and adjusts configuration. @@ -115,14 +118,11 @@ impl CoverageArgs { } /// Builds the coverage map. - fn prepare( - &self, - output: ProjectCompileOutput, - ) -> eyre::Result<(CoverageMap, HashMap)> { + fn prepare(&self, output: ProjectCompileOutput) -> eyre::Result<(CoverageMap, SourceMaps)> { // Get sources and source maps let (artifacts, sources) = output.into_artifacts_with_sources(); - let source_maps: HashMap = artifacts + let source_maps: SourceMaps = artifacts .into_iter() .map(|(id, artifact)| (id, CompactContractBytecode::from(artifact))) .filter_map(|(id, artifact): (ArtifactId, CompactContractBytecode)| { @@ -194,7 +194,7 @@ impl CoverageArgs { self, project: Project, output: ProjectCompileOutput, - source_maps: HashMap, + source_maps: SourceMaps, mut map: CoverageMap, config: Config, evm_opts: EvmOpts, From b807c964d155ea31452c0dbf8d7a8942f3b10741 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Tue, 14 Jun 2022 22:53:20 +0200 Subject: [PATCH 23/36] chore: revert Cargo.toml --- Cargo.toml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 3d32400b3a9c..83cc47c88cd7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -56,9 +56,9 @@ debug = 0 ## Patch ethers-rs with a local checkout then run `cargo update -p ethers` #[patch."https://github.com/gakonst/ethers-rs"] -#ethers = { path = "../../gakonst/ethers-rs" } -#ethers-core = { path = "../../gakonst/ethers-rs/ethers-core" } -#ethers-providers = { path = "../../gakonst/ethers-rs/ethers-providers" } -#ethers-signers = { path = "../../gakonst/ethers-rs/ethers-signers" } -#ethers-etherscan = { path = "../../gakonst/ethers-rs/ethers-etherscan" } -#ethers-solc = { path = "../../gakonst/ethers-rs/ethers-solc" } \ No newline at end of file +#ethers = { path = "../ethers-rs" } +#ethers-core = { path = "../ethers-rs/ethers-core" } +#ethers-providers = { path = "../ethers-rs/ethers-providers" } +#ethers-signers = { path = "../ethers-rs/ethers-signers" } +#ethers-etherscan = { path = "../ethers-rs/ethers-etherscan" } +#ethers-solc = { path = "../ethers-rs/ethers-solc" } \ No newline at end of file From 5b4e10b9f99e520e6908e1585874d8b7bbf0911b Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Mon, 20 Jun 2022 20:55:09 +0200 Subject: [PATCH 24/36] chore: remove unnecessary fn --- cli/src/cmd/forge/coverage.rs | 2 +- evm/src/coverage/mod.rs | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index 29c18b9f28bc..c202dda40e4c 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -142,7 +142,7 @@ impl CoverageArgs { }) .collect(); - let mut map = CoverageMap::new(); + let mut map = CoverageMap::default(); for (path, versioned_sources) in sources.0.into_iter() { // TODO: Make these checks robust // NOTE: We should actually filter out test contracts in the AST diff --git a/evm/src/coverage/mod.rs b/evm/src/coverage/mod.rs index f7c3a1ab0ce1..f553d36a461d 100644 --- a/evm/src/coverage/mod.rs +++ b/evm/src/coverage/mod.rs @@ -26,10 +26,6 @@ pub struct CoverageMap { } impl CoverageMap { - pub fn new() -> Self { - Default::default() - } - /// Adds coverage items and a source map for the given source. /// /// Sources are identified by path, and then by source ID and version. From bfcf841e932e87340890ec6cf8978e7152c89d3d Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Wed, 22 Jun 2022 16:27:40 +0200 Subject: [PATCH 25/36] feat: support `--silent` --- cli/src/cmd/forge/coverage.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index c202dda40e4c..f04b2f3a9133 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -5,7 +5,7 @@ use crate::{ Cmd, }, compile::ProjectCompiler, - utils, + utils::{self, p_println}, }; use cast::trace::identifier::TraceIdentifier; use clap::{AppSettings, ArgEnum, Parser}; @@ -74,10 +74,10 @@ impl Cmd for CoverageArgs { fn run(self) -> eyre::Result { let (config, evm_opts) = self.configure()?; let (project, output) = self.build(&config)?; - println!("Analysing contracts..."); + p_println!(!self.opts.silent => "Analysing contracts..."); let (map, source_maps) = self.prepare(output.clone())?; - println!("Running tests..."); + p_println!(!self.opts.silent => "Running tests..."); self.collect(project, output, source_maps, map, config, evm_opts) } } @@ -212,7 +212,7 @@ impl CoverageArgs { let root = project.paths.root; // Build the contract runner - let evm_spec = crate::utils::evm_spec(&config.evm_version); + let evm_spec = utils::evm_spec(&config.evm_version); let mut runner = MultiContractRunnerBuilder::default() .fuzzer(fuzzer) .initial_balance(evm_opts.initial_balance) From 0875d882416e6e4e3d7b817d833b24aadb452298 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Wed, 22 Jun 2022 16:29:44 +0200 Subject: [PATCH 26/36] refactor: use `FoundryPathExt::is_sol_test` --- cli/src/cmd/forge/coverage.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index f04b2f3a9133..4937e6b835c6 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -5,7 +5,7 @@ use crate::{ Cmd, }, compile::ProjectCompiler, - utils::{self, p_println}, + utils::{self, p_println, FoundryPathExt}, }; use cast::trace::identifier::TraceIdentifier; use clap::{AppSettings, ArgEnum, Parser}; @@ -149,7 +149,7 @@ impl CoverageArgs { // instead of on a source file level. Repositories like Solmate // have a lot of abstract contracts that are being tested, and these // are usually defined in the test files themselves. - let is_test = path.ends_with(".t.sol"); + let is_test = path.is_sol_test(); let is_dependency = path.starts_with("lib"); if is_test || is_dependency { continue From f559f1075e8625561bd419e219c245af29a0cc06 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Wed, 22 Jun 2022 16:30:19 +0200 Subject: [PATCH 27/36] refactor: unnecessary casting --- cli/src/cmd/forge/coverage.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index 4937e6b835c6..3010f7e7edcf 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -170,12 +170,9 @@ impl CoverageArgs { }) .collect(); - let items = Visitor::new( - source.id, - std::fs::read_to_string(PathBuf::from(&path))?, - source_maps, - ) - .visit_ast(ast)?; + let items = + Visitor::new(source.id, std::fs::read_to_string(&path)?, source_maps) + .visit_ast(ast)?; if items.is_empty() { continue From 5e9840574656325b0199aab999a3198d6a1c6d95 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Wed, 22 Jun 2022 16:32:45 +0200 Subject: [PATCH 28/36] refactor: use `impl Into` --- evm/src/coverage/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/evm/src/coverage/mod.rs b/evm/src/coverage/mod.rs index f553d36a461d..5d2c023fd817 100644 --- a/evm/src/coverage/mod.rs +++ b/evm/src/coverage/mod.rs @@ -35,13 +35,13 @@ impl CoverageMap { /// jobs, and source IDs are only guaranteed to be stable across a single compile job. pub fn add_source( &mut self, - path: String, + path: impl Into, source: VersionedSourceFile, items: Vec, ) { let VersionedSourceFile { version, source_file: source } = source; - self.sources.insert((version, source.id), SourceFile { path: PathBuf::from(path), items }); + self.sources.insert((version, source.id), SourceFile { path: path.into(), items }); } pub fn add_hit_map( From 137bcc521e0180feeee43c4c40cfaefb067637b8 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Wed, 22 Jun 2022 16:35:30 +0200 Subject: [PATCH 29/36] refactor: move assumptions to fn docs --- evm/src/coverage/mod.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/evm/src/coverage/mod.rs b/evm/src/coverage/mod.rs index 5d2c023fd817..4cd25775426c 100644 --- a/evm/src/coverage/mod.rs +++ b/evm/src/coverage/mod.rs @@ -44,19 +44,24 @@ impl CoverageMap { self.sources.insert((version, source.id), SourceFile { path: path.into(), items }); } + /// Processes data from a [HitMap] and sets hit counts for coverage items in this coverage map. + /// + /// This function should only be called *after* all the relevant sources have been processed and + /// added to the map (see [add_source]). + /// + /// NOTE(onbjerg): I've made an assumption here that the coverage items are laid out in + /// sorted order, ordered by their anchors. + /// + /// This assumption is based on the way we process the AST - we start at the root node, + /// and work our way down. If we change up how we process the AST, we *have* to either + /// change this logic to work with unsorted data, or sort the data prior to calling + /// this function. pub fn add_hit_map( &mut self, source_version: Version, source_map: &SourceMap, hit_map: HitMap, ) { - // NOTE(onbjerg): I've made an assumption here that the coverage items are laid out in - // sorted order, ordered by their offset in the source code. - // - // This assumption is based on the way we process the AST - we start at the root node, - // and work our way down. If we change up how we process the AST, we *have* to either - // change this logic to work with unsorted data, or sort the data prior to calling - // this function. for (ic, instruction_hits) in hit_map.hits.into_iter() { if instruction_hits == 0 { continue From 782ce82af5515198ff91aae9e7e56f2f27d402a8 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Wed, 22 Jun 2022 16:44:40 +0200 Subject: [PATCH 30/36] refactor: more ergonomic `ExecutorBuilder` --- cli/src/cmd/forge/script/executor.rs | 17 ++++++----------- evm/src/executor/builder.rs | 18 +++++++++--------- forge/src/multi_runner.rs | 16 +++++----------- 3 files changed, 20 insertions(+), 31 deletions(-) diff --git a/cli/src/cmd/forge/script/executor.rs b/cli/src/cmd/forge/script/executor.rs index 6b3cc901d8f4..974774d68a39 100644 --- a/cli/src/cmd/forge/script/executor.rs +++ b/cli/src/cmd/forge/script/executor.rs @@ -159,20 +159,15 @@ impl ScriptArgs { ) .await; - let mut builder = ExecutorBuilder::default() + let executor = ExecutorBuilder::default() .with_cheatcodes(CheatsConfig::new(&script_config.config, &script_config.evm_opts)) .with_config(env) .with_spec(crate::utils::evm_spec(&script_config.config.evm_version)) - .with_gas_limit(script_config.evm_opts.gas_limit()); + .with_gas_limit(script_config.evm_opts.gas_limit()) + .set_tracing(script_config.evm_opts.verbosity >= 3 || self.debug) + .set_debugger(self.debug) + .build(db); - if script_config.evm_opts.verbosity >= 3 { - builder = builder.with_tracing(); - } - - if self.debug { - builder = builder.with_tracing().with_debugger(); - } - - ScriptRunner::new(builder.build(db), script_config.evm_opts.initial_balance, sender) + ScriptRunner::new(executor, script_config.evm_opts.initial_balance, sender) } } diff --git a/evm/src/executor/builder.rs b/evm/src/executor/builder.rs index cde771408b42..0ab122d14419 100644 --- a/evm/src/executor/builder.rs +++ b/evm/src/executor/builder.rs @@ -138,24 +138,24 @@ impl ExecutorBuilder { self } - /// Enables tracing + /// Enables or disables tracing #[must_use] - pub fn with_tracing(mut self) -> Self { - self.inspector_config.tracing = true; + pub fn set_tracing(mut self, enable: bool) -> Self { + self.inspector_config.tracing = enable; self } - /// Enables the debugger + /// Enables or disables the debugger #[must_use] - pub fn with_debugger(mut self) -> Self { - self.inspector_config.debugger = true; + pub fn set_debugger(mut self, enable: bool) -> Self { + self.inspector_config.debugger = enable; self } - /// Enables coverage collection + /// Enables or disables coverage collection #[must_use] - pub fn with_coverage(mut self) -> Self { - self.inspector_config.coverage = true; + pub fn set_coverage(mut self, enable: bool) -> Self { + self.inspector_config.coverage = enable; self } diff --git a/forge/src/multi_runner.rs b/forge/src/multi_runner.rs index 9622e7ee9b0c..726da8fbb384 100644 --- a/forge/src/multi_runner.rs +++ b/forge/src/multi_runner.rs @@ -282,21 +282,15 @@ impl MultiContractRunner { }) .filter(|(_, (abi, _, _))| abi.functions().any(|func| filter.matches_test(&func.name))) .map(|(id, (abi, deploy_code, libs))| { - let mut builder = ExecutorBuilder::default() + let executor = ExecutorBuilder::default() .with_cheatcodes(self.cheats_config.clone()) .with_config(env.clone()) .with_spec(self.evm_spec) - .with_gas_limit(self.evm_opts.gas_limit()); + .with_gas_limit(self.evm_opts.gas_limit()) + .set_tracing(self.evm_opts.verbosity >= 3) + .set_coverage(self.coverage) + .build(db.clone()); - if self.evm_opts.verbosity >= 3 { - builder = builder.with_tracing(); - } - - if self.coverage { - builder = builder.with_coverage(); - } - - let executor = builder.build(db.clone()); let result = self.run_tests( &id.identifier(), abi, From ba42e725505d51a3c3762be6c09685b4a38d2a9e Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Wed, 22 Jun 2022 16:45:54 +0200 Subject: [PATCH 31/36] refactor: `with_coverage` -> `set_coverage` --- cli/src/cmd/forge/coverage.rs | 2 +- forge/src/multi_runner.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index 3010f7e7edcf..41f06f4ccd1b 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -216,7 +216,7 @@ impl CoverageArgs { .evm_spec(evm_spec) .sender(evm_opts.sender) .with_fork(utils::get_fork(&evm_opts, &config.rpc_storage_caching)) - .with_coverage() + .set_coverage(true) .build(root.clone(), output, evm_opts)?; let (tx, rx) = channel::<(String, SuiteResult)>(); diff --git a/forge/src/multi_runner.rs b/forge/src/multi_runner.rs index 726da8fbb384..af5ec3e9e8ba 100644 --- a/forge/src/multi_runner.rs +++ b/forge/src/multi_runner.rs @@ -171,8 +171,8 @@ impl MultiContractRunnerBuilder { } #[must_use] - pub fn with_coverage(mut self) -> Self { - self.coverage = true; + pub fn set_coverage(mut self, enable: bool) -> Self { + self.coverage = enable; self } } From 3cc28d1f8cca7a3f63eec965922bb85d65b0f279 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Wed, 22 Jun 2022 16:51:03 +0200 Subject: [PATCH 32/36] refactor: simplify coverage reporters --- cli/src/cmd/forge/coverage.rs | 17 +++-------------- forge/src/coverage.rs | 31 +++++-------------------------- 2 files changed, 8 insertions(+), 40 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index 41f06f4ccd1b..aa91d0b255d6 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -250,23 +250,12 @@ impl CoverageArgs { let _ = handle.join(); match self.report { - CoverageReportKind::Summary => { - let mut reporter = SummaryReporter::new(); - reporter.build(map); - reporter.finalize() - } + CoverageReportKind::Summary => SummaryReporter::default().report(map), // TODO: Sensible place to put the LCOV file CoverageReportKind::Lcov => { - let mut reporter = - LcovReporter::new(std::fs::File::create(root.join("lcov.info"))?); - reporter.build(map); - reporter.finalize() - } - CoverageReportKind::Debug => { - let mut reporter = DebugReporter::new(); - reporter.build(map); - reporter.finalize() + LcovReporter::new(std::fs::File::create(root.join("lcov.info"))?).report(map) } + CoverageReportKind::Debug => DebugReporter::default().report(map), } } } diff --git a/forge/src/coverage.rs b/forge/src/coverage.rs index 8ec30a45c73e..b6f62c753ba4 100644 --- a/forge/src/coverage.rs +++ b/forge/src/coverage.rs @@ -4,8 +4,7 @@ use std::{collections::HashMap, io::Write, path::PathBuf}; /// A coverage reporter. pub trait CoverageReporter { - fn build(&mut self, map: CoverageMap); - fn finalize(self) -> eyre::Result<()>; + fn report(self, map: CoverageMap) -> eyre::Result<()>; } /// A simple summary reporter that prints the coverage results in a table. @@ -26,10 +25,6 @@ impl Default for SummaryReporter { } impl SummaryReporter { - pub fn new() -> Self { - Default::default() - } - fn add_row(&mut self, name: impl Into, summary: CoverageSummary) { let mut row = Row::new(); row.add_cell(name.into()) @@ -42,16 +37,14 @@ impl SummaryReporter { } impl CoverageReporter for SummaryReporter { - fn build(&mut self, map: CoverageMap) { + fn report(mut self, map: CoverageMap) -> eyre::Result<()> { for file in map { let summary = file.summary(); self.total.add(&summary); self.add_row(file.path.to_string_lossy(), summary); } - } - fn finalize(mut self) -> eyre::Result<()> { self.add_row("Total", self.total.clone()); println!("{}", self.table); Ok(()) @@ -78,13 +71,11 @@ fn format_cell(hits: usize, total: usize) -> Cell { pub struct LcovReporter { /// Destination buffer destination: W, - /// The coverage map to write - map: Option, } impl LcovReporter { pub fn new(destination: W) -> Self { - Self { destination, map: None } + Self { destination } } } @@ -92,13 +83,7 @@ impl CoverageReporter for LcovReporter where W: Write, { - fn build(&mut self, map: CoverageMap) { - self.map = Some(map); - } - - fn finalize(mut self) -> eyre::Result<()> { - let map = self.map.ok_or_else(|| eyre::eyre!("no coverage map given to reporter"))?; - + fn report(mut self, map: CoverageMap) -> eyre::Result<()> { for file in map { let summary = file.summary(); @@ -175,10 +160,6 @@ impl Default for DebugReporter { } impl DebugReporter { - pub fn new() -> Self { - Default::default() - } - fn add_row(&mut self, name: impl Into, summary: CoverageSummary) { let mut row = Row::new(); row.add_cell(name.into()) @@ -191,7 +172,7 @@ impl DebugReporter { } impl CoverageReporter for DebugReporter { - fn build(&mut self, map: CoverageMap) { + fn report(mut self, map: CoverageMap) -> eyre::Result<()> { for file in map { let summary = file.summary(); @@ -204,9 +185,7 @@ impl CoverageReporter for DebugReporter { } }) } - } - fn finalize(mut self) -> eyre::Result<()> { self.add_row("Total", self.total.clone()); println!("{}", self.table); From 6dc7d7ac18b9c8c42604401d3b2d19eae6a618c9 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Wed, 22 Jun 2022 16:56:55 +0200 Subject: [PATCH 33/36] refactor: impl `AddAssign` for `CoverageSummary` --- evm/src/coverage/mod.rs | 6 +++--- forge/src/coverage.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/evm/src/coverage/mod.rs b/evm/src/coverage/mod.rs index 4cd25775426c..6d5373ff6404 100644 --- a/evm/src/coverage/mod.rs +++ b/evm/src/coverage/mod.rs @@ -9,6 +9,7 @@ use semver::Version; use std::{ collections::{BTreeMap, HashMap}, fmt::Display, + ops::AddAssign, path::PathBuf, usize, }; @@ -331,9 +332,8 @@ pub struct CoverageSummary { pub function_hits: usize, } -impl CoverageSummary { - /// Add the data of another [CoverageSummary] to this one. - pub fn add(&mut self, other: &CoverageSummary) { +impl AddAssign<&Self> for CoverageSummary { + fn add_assign(&mut self, other: &Self) { self.line_count += other.line_count; self.line_hits += other.line_hits; self.statement_count += other.statement_count; diff --git a/forge/src/coverage.rs b/forge/src/coverage.rs index b6f62c753ba4..895882edf647 100644 --- a/forge/src/coverage.rs +++ b/forge/src/coverage.rs @@ -41,7 +41,7 @@ impl CoverageReporter for SummaryReporter { for file in map { let summary = file.summary(); - self.total.add(&summary); + self.total += &summary; self.add_row(file.path.to_string_lossy(), summary); } @@ -176,7 +176,7 @@ impl CoverageReporter for DebugReporter { for file in map { let summary = file.summary(); - self.total.add(&summary); + self.total += &summary; self.add_row(file.path.to_string_lossy(), summary); file.items.iter().for_each(|item| { From 49757dc83fae5cd261e9415065de27621a874914 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Wed, 22 Jun 2022 17:00:13 +0200 Subject: [PATCH 34/36] refactor: lcov reporter dest --- cli/src/cmd/forge/coverage.rs | 2 +- forge/src/coverage.rs | 15 ++++++--------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index aa91d0b255d6..ac593cf3bf82 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -253,7 +253,7 @@ impl CoverageArgs { CoverageReportKind::Summary => SummaryReporter::default().report(map), // TODO: Sensible place to put the LCOV file CoverageReportKind::Lcov => { - LcovReporter::new(std::fs::File::create(root.join("lcov.info"))?).report(map) + LcovReporter::new(&mut std::fs::File::create(root.join("lcov.info"))?).report(map) } CoverageReportKind::Debug => DebugReporter::default().report(map), } diff --git a/forge/src/coverage.rs b/forge/src/coverage.rs index 895882edf647..b9eb1a2684c0 100644 --- a/forge/src/coverage.rs +++ b/forge/src/coverage.rs @@ -68,22 +68,19 @@ fn format_cell(hits: usize, total: usize) -> Cell { cell } -pub struct LcovReporter { +pub struct LcovReporter<'a> { /// Destination buffer - destination: W, + destination: &'a mut (dyn Write + 'a), } -impl LcovReporter { - pub fn new(destination: W) -> Self { +impl<'a> LcovReporter<'a> { + pub fn new(destination: &'a mut (dyn Write + 'a)) -> LcovReporter<'a> { Self { destination } } } -impl CoverageReporter for LcovReporter -where - W: Write, -{ - fn report(mut self, map: CoverageMap) -> eyre::Result<()> { +impl<'a> CoverageReporter for LcovReporter<'a> { + fn report(self, map: CoverageMap) -> eyre::Result<()> { for file in map { let summary = file.summary(); From ea41b29bbd1a8e5586540601e01280dd1068df17 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Wed, 22 Jun 2022 17:03:11 +0200 Subject: [PATCH 35/36] refactor: use wrapped fs --- cli/src/cmd/forge/coverage.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/cli/src/cmd/forge/coverage.rs b/cli/src/cmd/forge/coverage.rs index ac593cf3bf82..e18d8c0b4601 100644 --- a/cli/src/cmd/forge/coverage.rs +++ b/cli/src/cmd/forge/coverage.rs @@ -22,7 +22,7 @@ use forge::{ trace::identifier::LocalTraceIdentifier, MultiContractRunnerBuilder, }; -use foundry_common::evm::EvmArgs; +use foundry_common::{evm::EvmArgs, fs}; use foundry_config::{figment::Figment, Config}; use std::{collections::HashMap, path::PathBuf, sync::mpsc::channel, thread}; @@ -170,9 +170,8 @@ impl CoverageArgs { }) .collect(); - let items = - Visitor::new(source.id, std::fs::read_to_string(&path)?, source_maps) - .visit_ast(ast)?; + let items = Visitor::new(source.id, fs::read_to_string(&path)?, source_maps) + .visit_ast(ast)?; if items.is_empty() { continue @@ -253,7 +252,7 @@ impl CoverageArgs { CoverageReportKind::Summary => SummaryReporter::default().report(map), // TODO: Sensible place to put the LCOV file CoverageReportKind::Lcov => { - LcovReporter::new(&mut std::fs::File::create(root.join("lcov.info"))?).report(map) + LcovReporter::new(&mut fs::create_file(root.join("lcov.info"))?).report(map) } CoverageReportKind::Debug => DebugReporter::default().report(map), } From 5453969c95bfe070f28d0d87f8db1d0623b5d33e Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Wed, 22 Jun 2022 19:18:28 +0200 Subject: [PATCH 36/36] chore: nits --- evm/src/coverage/mod.rs | 4 ++-- evm/src/coverage/visitor.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/evm/src/coverage/mod.rs b/evm/src/coverage/mod.rs index 6d5373ff6404..abf5ab82a6aa 100644 --- a/evm/src/coverage/mod.rs +++ b/evm/src/coverage/mod.rs @@ -23,7 +23,7 @@ use std::{ #[derive(Default, Debug, Clone)] pub struct CoverageMap { /// A map of `(version, source id)` -> `source file` - sources: BTreeMap<(Version, u32), SourceFile>, + sources: HashMap<(Version, u32), SourceFile>, } impl CoverageMap { @@ -97,7 +97,7 @@ impl CoverageMap { impl IntoIterator for CoverageMap { type Item = SourceFile; - type IntoIter = std::collections::btree_map::IntoValues<(Version, u32), Self::Item>; + type IntoIter = std::collections::hash_map::IntoValues<(Version, u32), Self::Item>; fn into_iter(self) -> Self::IntoIter { self.sources.into_values() diff --git a/evm/src/coverage/visitor.rs b/evm/src/coverage/visitor.rs index 13ea2012fedf..cf15722d4221 100644 --- a/evm/src/coverage/visitor.rs +++ b/evm/src/coverage/visitor.rs @@ -366,7 +366,7 @@ impl Visitor { SourceLocation { start: loc.start, length: loc.length, - line: self.source[..loc.start].as_bytes().iter().filter(|&&c| c == b'\n').count() + 1, + line: self.source[..loc.start].lines().count() + 1, } }