Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The metric includes disabled sources, excludes valid nodes, and omits its denominator row.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds Unified taint-reach metrics for DCA reporting.
Changes:
- Computes tainted-node counts and per-million reach.
- Exposes reach metrics through summary statistics.
| File | Description |
|---|---|
unified/ql/lib/codeql/unified/internal/AnalysisQuality.qll |
Adds taint-reach analysis and reporting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| module TaintReach { | ||
| private class Candidate extends DataFlow::Node { | ||
| Candidate() { exists(this.asExpr()) } |
There was a problem hiding this comment.
I would have written any() here as well I think. But if there's a reason you restricted candidate nodes to those corresponding with expressions, I think it's fine and we still have a useful metric.
There was a problem hiding this comment.
We avoid counting synthetic nodes, so we don't move the goal posts whenever synthetic nodes are added/removed. E.g. something like the introduction of use-use flow can add a large number of synthetic nodes, but then you can't compare the taint reach numbers across that change.
IMO Rust should should also stop measuring synthetic nodes.
There was a problem hiding this comment.
Ah, then I agree with you, that has been a problem with the rust taint reach statistic.
👍
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
LGTM.
Getting DCA to show the numbers will require some (minor) changes in that repo. Let me know if you've hit problems there. Or I'm happy to do it, I'm quite familiar with how that stuff is wired.
Adds taint reach stats.
I've tested locally but there are still issues with getting DCA to show the numbers.