Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(coverage): account for build_id in source identification #9418

Merged
merged 5 commits into from
Nov 27, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix(coverage): use SourceIdentifier in analysis and report.
yash-atreya committed Nov 27, 2024
commit 4c29b09a85e7dfc906eec8494cb7c3374e90dfaa
11 changes: 5 additions & 6 deletions crates/evm/coverage/src/analysis.rs
Original file line number Diff line number Diff line change
@@ -10,7 +10,7 @@ use std::{borrow::Cow, path::PathBuf, sync::Arc};
#[derive(Clone, Debug)]
pub struct ContractVisitor<'a> {
/// The source ID of the contract.
source_id: usize,
source_id: SourceIdentifier,
/// The source code that contains the AST being walked.
source: &'a str,

@@ -27,7 +27,7 @@ pub struct ContractVisitor<'a> {
}

impl<'a> ContractVisitor<'a> {
pub fn new(source_id: usize, source: &'a str, contract_name: &'a Arc<str>) -> Self {
pub fn new(source_id: SourceIdentifier, source: &'a str, contract_name: &'a Arc<str>) -> Self {
Self { source_id, source, contract_name, branch_id: 0, last_line: 0, items: Vec::new() }
}

@@ -474,7 +474,7 @@ impl<'a> ContractVisitor<'a> {
let loc_start =
self.source.char_indices().map(|(i, _)| i).nth(loc.start).unwrap_or_default();
SourceLocation {
source_id: self.source_id,
source_id: self.source_id.clone(),
contract_name: self.contract_name.clone(),
start: loc.start as u32,
length: loc.length.map(|x| x as u32),
@@ -539,8 +539,7 @@ impl<'a> SourceAnalyzer<'a> {
.sources
.sources
.par_iter()
.flat_map_iter(|(SourceIdentifier { path, source_id, build_id }, SourceFile { source, ast, .. })| {
tracing::info!(source_id=?source_id, build_id=?build_id, path=?path, "Analyzing source");
.flat_map_iter(|(source_id, SourceFile { source, ast, .. })| {
ast.nodes.iter().map(move |node| {
if !matches!(node.node_type, NodeType::ContractDefinition) {
return Ok(vec![]);
@@ -558,7 +557,7 @@ impl<'a> SourceAnalyzer<'a> {
.attribute("name")
.ok_or_else(|| eyre::eyre!("Contract has no name"))?;

let mut visitor = ContractVisitor::new(*source_id, source, &name);
let mut visitor = ContractVisitor::new(source_id.clone(), source, &name);
visitor.visit_contract(node)?;
let mut items = visitor.items;

10 changes: 7 additions & 3 deletions crates/evm/coverage/src/anchors.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use crate::analysis::SourceIdentifier;

use super::{CoverageItem, CoverageItemKind, ItemAnchor, SourceLocation};
use alloy_primitives::map::{DefaultHashBuilder, HashMap, HashSet};
use eyre::ensure;
@@ -11,12 +13,13 @@ pub fn find_anchors(
source_map: &SourceMap,
ic_pc_map: &IcPcMap,
items: &[CoverageItem],
items_by_source_id: &HashMap<usize, Vec<usize>>,
items_by_source_id: &HashMap<SourceIdentifier, Vec<usize>>,
source_id: &SourceIdentifier,
) -> Vec<ItemAnchor> {
let mut seen = HashSet::with_hasher(DefaultHashBuilder::default());
source_map
.iter()
.filter_map(|element| items_by_source_id.get(&(element.index()? as usize)))
.filter_map(|_element| items_by_source_id.get(source_id))
.flatten()
.filter_map(|&item_id| {
if !seen.insert(item_id) {
@@ -171,7 +174,8 @@ pub fn find_anchor_branch(
/// Calculates whether `element` is within the range of the target `location`.
fn is_in_source_range(element: &SourceElement, location: &SourceLocation) -> bool {
// Source IDs must match.
let source_ids_match = element.index().is_some_and(|a| a as usize == location.source_id);
let source_ids_match =
element.index().is_some_and(|a| a as usize == location.source_id.source_id);
if !source_ids_match {
return false;
}
23 changes: 12 additions & 11 deletions crates/evm/coverage/src/lib.rs
Original file line number Diff line number Diff line change
@@ -12,6 +12,7 @@ extern crate foundry_common;
extern crate tracing;

use alloy_primitives::{map::HashMap, Bytes, B256};
use analysis::SourceIdentifier;
use eyre::{Context, Result};
use foundry_compilers::artifacts::sourcemap::SourceMap;
use semver::Version;
@@ -36,9 +37,9 @@ pub use inspector::CoverageCollector;
#[derive(Clone, Debug, Default)]
pub struct CoverageReport {
/// A map of source IDs to the source path.
pub source_paths: HashMap<(Version, usize), PathBuf>,
pub source_paths: HashMap<(Version, SourceIdentifier), PathBuf>,
/// A map of source paths to source IDs.
pub source_paths_to_ids: HashMap<(Version, PathBuf), usize>,
pub source_paths_to_ids: HashMap<(Version, PathBuf), SourceIdentifier>,
/// All coverage items for the codebase, keyed by the compiler version.
pub items: HashMap<Version, Vec<CoverageItem>>,
/// All item anchors for the codebase, keyed by their contract ID.
@@ -51,14 +52,14 @@ pub struct CoverageReport {

impl CoverageReport {
/// Add a source file path.
pub fn add_source(&mut self, version: Version, source_id: usize, path: PathBuf) {
self.source_paths.insert((version.clone(), source_id), path.clone());
pub fn add_source(&mut self, version: Version, source_id: SourceIdentifier, path: PathBuf) {
self.source_paths.insert((version.clone(), source_id.clone()), path.clone());
self.source_paths_to_ids.insert((version, path), source_id);
}

/// Get the source ID for a specific source file path.
pub fn get_source_id(&self, version: Version, path: PathBuf) -> Option<usize> {
self.source_paths_to_ids.get(&(version, path)).copied()
pub fn get_source_id(&self, version: Version, path: PathBuf) -> Option<SourceIdentifier> {
self.source_paths_to_ids.get(&(version, path)).cloned()
}

/// Add the source maps.
@@ -89,7 +90,7 @@ impl CoverageReport {
for (version, items) in self.items.iter() {
for item in items {
let Some(path) =
self.source_paths.get(&(version.clone(), item.loc.source_id)).cloned()
self.source_paths.get(&(version.clone(), item.loc.source_id.clone())).cloned()
else {
continue;
};
@@ -107,7 +108,7 @@ impl CoverageReport {
for (version, items) in self.items.iter() {
for item in items {
let Some(path) =
self.source_paths.get(&(version.clone(), item.loc.source_id)).cloned()
self.source_paths.get(&(version.clone(), item.loc.source_id.clone())).cloned()
else {
continue;
};
@@ -160,7 +161,7 @@ impl CoverageReport {
self.items.retain(|version, items| {
items.retain(|item| {
self.source_paths
.get(&(version.clone(), item.loc.source_id))
.get(&(version.clone(), item.loc.source_id.clone()))
.map(|path| filter(path))
.unwrap_or(false)
});
@@ -259,7 +260,7 @@ impl HitMap {
#[derive(Clone, Debug, PartialEq, Eq, Hash)]
pub struct ContractId {
pub version: Version,
pub source_id: usize,
pub source_id: SourceIdentifier,
pub contract_name: Arc<str>,
}

@@ -348,7 +349,7 @@ impl Display for CoverageItem {
#[derive(Clone, Debug)]
pub struct SourceLocation {
/// The source ID.
pub source_id: usize,
pub source_id: SourceIdentifier,
/// The contract this source range is in.
pub contract_name: Arc<str>,
/// Start byte in the source code.
37 changes: 22 additions & 15 deletions crates/forge/bin/cmd/coverage.rs
Original file line number Diff line number Diff line change
@@ -178,7 +178,9 @@ impl CoverageArgs {
continue;
};

report.add_source(version.clone(), source_file.id as usize, id.source.clone());
let file = project_paths.root.join(id.source.clone());
let identifier = SourceIdentifier::new(source_file.id as usize, build_id.clone(), file);
report.add_source(version.clone(), identifier, id.source.clone());

if let Some(ast) = source_file.ast {
let file = project_paths.root.join(id.source);
@@ -201,13 +203,6 @@ impl CoverageArgs {
}
}

// Print versioned sources
for (identifier, sources) in &versioned_sources {
for (id, source) in &sources.sources {
tracing::info!(id=?id, "source");
}
}

// Get source maps and bytecodes
let artifacts: Vec<ArtifactData> = output
.artifact_ids()
@@ -229,17 +224,23 @@ impl CoverageArgs {
);

for (item_id, item) in source_analysis.items.iter().enumerate() {
items_by_source_id.entry(item.loc.source_id).or_default().push(item_id);
items_by_source_id.entry(item.loc.source_id.clone()).or_default().push(item_id);
}

let anchors = artifacts
.par_iter()
.filter(|artifact| artifact.contract_id.version == *version)
.map(|artifact| {
let creation_code_anchors =
artifact.creation.find_anchors(&source_analysis, &items_by_source_id);
let deployed_code_anchors =
artifact.deployed.find_anchors(&source_analysis, &items_by_source_id);
let creation_code_anchors = artifact.creation.find_anchors(
&source_analysis,
&items_by_source_id,
&artifact.contract_id.source_id,
);
let deployed_code_anchors = artifact.deployed.find_anchors(
&source_analysis,
&items_by_source_id,
&artifact.contract_id.source_id,
);
(artifact.contract_id.clone(), (creation_code_anchors, deployed_code_anchors))
})
.collect::<Vec<_>>();
@@ -397,7 +398,11 @@ pub struct ArtifactData {
}

impl ArtifactData {
pub fn new(id: &ArtifactId, source_id: usize, artifact: &impl Artifact) -> Option<Self> {
pub fn new(
id: &ArtifactId,
source_id: SourceIdentifier,
artifact: &impl Artifact,
) -> Option<Self> {
Some(Self {
contract_id: ContractId {
version: id.version.clone(),
@@ -442,14 +447,16 @@ impl BytecodeData {
pub fn find_anchors(
&self,
source_analysis: &SourceAnalysis,
items_by_source_id: &HashMap<usize, Vec<usize>>,
items_by_source_id: &HashMap<SourceIdentifier, Vec<usize>>,
source_id: &SourceIdentifier,
) -> Vec<ItemAnchor> {
find_anchors(
&self.bytecode,
&self.source_map,
&self.ic_pc_map,
&source_analysis.items,
items_by_source_id,
source_id,
)
}
}
7 changes: 5 additions & 2 deletions crates/forge/src/coverage.rs
Original file line number Diff line number Diff line change
@@ -222,8 +222,11 @@ impl CoverageReporter for BytecodeReporter {
.map(|h| format!("[{h:03}]"))
.unwrap_or(" ".to_owned());
let source_id = source_element.index();
let source_path = source_id.and_then(|i| {
report.source_paths.get(&(contract_id.version.clone(), i as usize))
let source_path = source_id.and_then(|_i| {
// report.source_paths.get(&(contract_id.version.clone(), i as usize))
yash-atreya marked this conversation as resolved.
Show resolved Hide resolved
report
.source_paths
.get(&(contract_id.version.clone(), contract_id.source_id.clone()))
});

let code = format!("{code:?}");
42 changes: 31 additions & 11 deletions crates/forge/tests/cli/coverage.rs
Original file line number Diff line number Diff line change
@@ -1662,22 +1662,35 @@ forgetest!(test_coverage_multi_solc_versions, |prj, cmd| {
});

// checks that `clean` also works with the "out" value set in Config
forgetest!(coverage_cache, |prj, cmd| {
prj.add_source("A", r#"
// this test verifies that the coverage is preserved across compiler runs that may result in files
// with different source_id's
forgetest!(coverage_cache_across_compiler_runs, |prj, cmd| {
prj.add_source(
"A",
r#"
contract A {
function f() public pure returns (uint) {
return 1;
}
}"#).unwrap();
}"#,
)
.unwrap();

prj.add_source("B", r#"
prj.add_source(
"B",
r#"
contract B {
function f() public pure returns (uint) {
return 1;
}
}"#).unwrap();
}"#,
)
.unwrap();

let a_test = prj.add_test("A.t.sol", r#"
let a_test = prj
.add_test(
"A.t.sol",
r#"
import {A} from "../src/A.sol";

contract ATest {
@@ -1686,9 +1699,13 @@ contract ATest {
a.f();
}
}
"#).unwrap();
"#,
)
.unwrap();

prj.add_test("B.t.sol", r#"
prj.add_test(
"B.t.sol",
r#"
import {B} from "../src/B.sol";

contract BTest {
@@ -1697,8 +1714,10 @@ contract ATest {
a.f();
}
}
"#).unwrap();

"#,
)
.unwrap();

cmd.forge_fuse().arg("coverage").assert_success().stdout_eq(str![[r#"
...
Ran 2 test suites [ELAPSED]: 2 tests passed, 0 failed, 0 skipped (2 total tests)
@@ -1718,7 +1737,8 @@ Ran 2 test suites [ELAPSED]: 2 tests passed, 0 failed, 0 skipped (2 total tests)
| File | % Lines | % Statements | % Branches | % Funcs |
|-----------|---------------|---------------|---------------|---------------|
| src/A.sol | 100.00% (1/1) | 100.00% (1/1) | 100.00% (0/0) | 100.00% (1/1) |
| Total | 100.00% (1/1) | 100.00% (1/1) | 100.00% (0/0) | 100.00% (1/1) |
| src/B.sol | 100.00% (1/1) | 100.00% (1/1) | 100.00% (0/0) | 100.00% (1/1) |
| Total | 100.00% (2/2) | 100.00% (2/2) | 100.00% (0/0) | 100.00% (2/2) |

"#]]);
});