fix: coverage for libraries #7510
Conversation
klkvr
changed the title
| // end up here again | ||
| if items.is_empty() || is_test { | ||
| self.contract_items.insert(contract_id.clone(), Vec::new()); | ||
| let ContractVisitor { items, .. } = ContractVisitor::new( |
There was a problem hiding this comment.
w/o the if !self.contract_items.contains_key(contract_id) { check, will this not lead to re-analyzing contracts? and possibly duping coverage items?
There was a problem hiding this comment.
We're walking only ContractDefinition nodes from self.contracts, so it should be fine as long as there is no duplicating ContractId keys
There was a problem hiding this comment.
i think thats a fair assumption
crates/forge/bin/cmd/coverage.rs
Outdated
| .contract_items | ||
|
|
||
| // Build helper mapping used by `find_anchors` | ||
| let mut items_by_source_id = HashMap::new(); |
There was a problem hiding this comment.
| let mut items_by_source_id = HashMap::new(); | |
| let mut items_by_source_id = HashMap::with_capacity(source_analysis.items.len()); |
crates/forge/bin/cmd/coverage.rs
Outdated
| let mut items_by_source_id = HashMap::new(); | ||
|
|
||
| for (item_id, item) in source_analysis.items.iter().enumerate() { | ||
| items_by_source_id.entry(item.loc.source_id).or_insert_with(Vec::new).push(item_id); |
There was a problem hiding this comment.
there should be or_default() I believe
* fix: coverage for internal libraries * optimize * optimize * doc * rm tests * clippy * clippy + fmt * clean up * for loop * review fixes
|
This sounds great, thanks @klkvr It seems to have a regression for different
FWIW, I run via this flow to get a html representation of the coverage and branches: |
|
@frontier159 Thank you for reporting and really sorry for breaking those. Any chance you could share a repository on which this can be reproduced? |
|
@klkvr it seems after a You can try this repo:
After a |
Motivation
Closes #7054
Closes #2567
Closes #4854
Closes #4654
Solution
Currently we're collecting anchors via the following steps:
This approach requires us to be able to reliably find libraries used by a contract which turned out to be non-trivial.
One possible approach could be to reuse some of #6936 code to walk through typed AST and find all contracts/libs used by a given contract. However, this might be breaking due to typed AST not working for some solc versions and this approach is also not perfect.
Instead, we now just collect all coverage items and then create a mapping from
source_idto all coverage items given source contains.After that, when looking for anchors, we only try finding anchors for items from source ids appearing in source map. This allows us to get rid of dependency resolution through AST.
Example
I was mainly testing those on Uniswap/v4-core contracts.
Report via latest nightly: report_nightly.txt
Report via this PR: report_pr.txt
diff:
cc @onbjerg