fix(coverage): disambiguate duplicate function names (#11188)

Author: DaniPopes

crates/evm/coverage/src/analysis.rs

@@ -10,7 +10,7 @@ use std::sync::Arc;
 
 /// A visitor that walks the AST of a single contract and finds coverage items.
 #[derive(Clone, Debug)]
-pub struct ContractVisitor<'a> {
+struct ContractVisitor<'a> {
     /// The source ID of the contract.
     source_id: u32,
     /// The source code that contains the AST being walked.
@@ -25,11 +25,11 @@ pub struct ContractVisitor<'a> {
     last_line: u32,
 
     /// Coverage items
-    pub items: Vec<CoverageItem>,
+    items: Vec<CoverageItem>,
 }
 
 impl<'a> ContractVisitor<'a> {
-    pub fn new(source_id: usize, source: &'a str, contract_name: &'a Arc<str>) -> Self {
+    fn new(source_id: usize, source: &'a str, contract_name: &'a Arc<str>) -> Self {
         Self {
             source_id: source_id.try_into().expect("too many sources"),
             source,
@@ -40,7 +40,46 @@ impl<'a> ContractVisitor<'a> {
         }
     }
 
-    pub fn visit_contract(&mut self, node: &Node) -> eyre::Result<()> {
+    /// Filter out all items if the contract has any test functions.
+    fn clear_if_test(&mut self) {
+        let has_tests = self.items.iter().any(|item| {
+            if let CoverageItemKind::Function { name } = &item.kind {
+                name.is_any_test()
+            } else {
+                false
+            }
+        });
+        if has_tests {
+            self.items = Vec::new();
+        }
+    }
+
+    /// Disambiguate functions with the same name in the same contract.
+    fn disambiguate_functions(&mut self) {
+        if self.items.is_empty() {
+            return;
+        }
+
+        let mut dups = HashMap::<_, Vec<usize>>::default();
+        for (i, item) in self.items.iter().enumerate() {
+            if let CoverageItemKind::Function { name } = &item.kind {
+                dups.entry(name.clone()).or_default().push(i);
+            }
+        }
+        for dups in dups.values() {
+            if dups.len() > 1 {
+                for (i, &dup) in dups.iter().enumerate() {
+                    let item = &mut self.items[dup];
+                    if let CoverageItemKind::Function { name } = &item.kind {
+                        item.kind =
+                            CoverageItemKind::Function { name: format!("{name}.{i}").into() };
+                    }
+                }
+            }
+        }
+    }
+
+    fn visit_contract(&mut self, node: &Node) -> eyre::Result<()> {
         // Find all functions and walk their AST
         for node in &node.nodes {
             match node.node_type {
@@ -59,14 +98,14 @@ impl<'a> ContractVisitor<'a> {
     fn visit_function_definition(&mut self, node: &Node) -> eyre::Result<()> {
         let Some(body) = &node.body else { return Ok(()) };
 
-        let name: String =
+        let name: Box<str> =
             node.attribute("name").ok_or_else(|| eyre::eyre!("Function has no name"))?;
-        let kind: String =
+        let kind: Box<str> =
             node.attribute("kind").ok_or_else(|| eyre::eyre!("Function has no kind"))?;
 
         // TODO: We currently can only detect empty bodies in normal functions, not any of the other
         // kinds: https://github.com/foundry-rs/foundry/issues/9458
-        if kind != "function" && !has_statements(body) {
+        if &*kind != "function" && !has_statements(body) {
             return Ok(());
         }
 
@@ -79,16 +118,12 @@ impl<'a> ContractVisitor<'a> {
     }
 
     fn visit_modifier_or_yul_fn_definition(&mut self, node: &Node) -> eyre::Result<()> {
-        let name: String =
-            node.attribute("name").ok_or_else(|| eyre::eyre!("Modifier has no name"))?;
+        let Some(body) = &node.body else { return Ok(()) };
 
-        match &node.body {
-            Some(body) => {
-                self.push_item_kind(CoverageItemKind::Function { name }, &node.src);
-                self.visit_block(body)
-            }
-            _ => Ok(()),
-        }
+        let name: Box<str> =
+            node.attribute("name").ok_or_else(|| eyre::eyre!("Modifier has no name"))?;
+        self.push_item_kind(CoverageItemKind::Function { name }, &node.src);
+        self.visit_block(body)
     }
 
     fn visit_block(&mut self, node: &Node) -> eyre::Result<()> {
@@ -571,6 +606,7 @@ impl SourceAnalysis {
     /// Note: Source IDs are only unique per compilation job; that is, a code base compiled with
     /// two different solc versions will produce overlapping source IDs if the compiler version is
     /// not taken into account.
+    #[instrument(name = "SourceAnalysis::new", skip_all)]
     pub fn new(data: &SourceFiles<'_>) -> eyre::Result<Self> {
         let mut sourced_items = data
             .sources
@@ -592,23 +628,12 @@ impl SourceAnalysis {
                     let name = node
                         .attribute("name")
                         .ok_or_else(|| eyre::eyre!("Contract has no name"))?;
-
+                    let _guard = debug_span!("visit_contract", %name).entered();
                     let mut visitor = ContractVisitor::new(source_id, &source.content, &name);
                     visitor.visit_contract(node)?;
-                    let mut items = visitor.items;
-
-                    let is_test = items.iter().any(|item| {
-                        if let CoverageItemKind::Function { name } = &item.kind {
-                            name.is_any_test()
-                        } else {
-                            false
-                        }
-                    });
-                    if is_test {
-                        items.clear();
-                    }
-
-                    Ok(items)
+                    visitor.clear_if_test();
+                    visitor.disambiguate_functions();
+                    Ok(visitor.items)
                 });
                 items.map(move |items| items.map(|items| (source_id, items)))
             })

crates/evm/coverage/src/lib.rs

@@ -326,7 +326,7 @@ pub enum CoverageItemKind {
     /// A function in the code.
     Function {
         /// The name of the function.
-        name: String,
+        name: Box<str>,
     },
 }
 

crates/forge/tests/cli/coverage.rs

@@ -5,6 +5,11 @@ use foundry_test_utils::{
 };
 use std::path::Path;
 
+#[track_caller]
+fn assert_lcov(cmd: &mut TestCommand, data: impl IntoData) {
+    cmd.args(["--report=lcov", "--report-file"]).assert_file(data.into_data());
+}
+
 fn basic_base(prj: TestProject, mut cmd: TestCommand) {
     cmd.args(["coverage", "--report=lcov", "--report=summary"]).assert_success().stdout_eq(str![[
         r#"
@@ -1911,7 +1916,68 @@ end_of_record
     );
 });
 
-#[track_caller]
-fn assert_lcov(cmd: &mut TestCommand, data: impl IntoData) {
-    cmd.args(["--report=lcov", "--report-file"]).assert_file(data.into_data());
+// <https://github.com/foundry-rs/foundry/issues/11183>
+// Test that overridden functions are disambiguated in the LCOV report.
+forgetest!(disambiguate_functions, |prj, cmd| {
+    prj.insert_ds_test();
+    prj.add_source(
+        "Counter.sol",
+        r#"
+contract Counter {
+    uint256 public number;
+
+    function increment() public {
+        number++;
+    }
+    function increment(uint256 amount) public {
+        number += amount;
+    }
+}
+    "#,
+    )
+    .unwrap();
+
+    prj.add_source(
+        "Counter.t.sol",
+        r#"
+import "./test.sol";
+import "./Counter.sol";
+
+contract CounterTest is DSTest {
+    function test_overridden() public {
+        Counter counter = new Counter();
+        counter.increment();
+        counter.increment(1);
+        counter.increment(2);
+        counter.increment(3);
+        assertEq(counter.number(), 7);
+    }
 }
+    "#,
+    )
+    .unwrap();
+
+    assert_lcov(
+        cmd.arg("coverage"),
+        str![[r#"
+TN:
+SF:src/Counter.sol
+DA:7,1
+FN:7,Counter.increment.0
+FNDA:1,Counter.increment.0
+DA:8,1
+DA:10,3
+FN:10,Counter.increment.1
+FNDA:3,Counter.increment.1
+DA:11,3
+FNF:2
+FNH:2
+LF:4
+LH:4
+BRF:0
+BRH:0
+end_of_record
+
+"#]],
+    );
+});