clippy annotation reporter #1049
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BenchmarksComparisonBenchmark execution time: 2025-06-01 03:04:12 Comparing candidate commit 9a877f9 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 52 metrics, 2 unstable metrics. CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
Group 13
BaselineOmitted due to size. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1049 +/- ##
==========================================
- Coverage 71.01% 70.99% -0.03%
==========================================
Files 329 329
Lines 49888 49888
==========================================
- Hits 35430 35420 -10
- Misses 14458 14468 +10
🚀 New features to boost your workflow:
|
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
ekump
force-pushed
the
ekump/clippy-annotation-action
branch
from
4dae85a to
a1c1a9d
Compare
ekump
force-pushed
the
ekump/clippy-annotation-action
branch
from
4db63c3 to
c0d22c4
Compare
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
ekump
force-pushed
the
ekump/clippy-annotation-action
branch
6 times, most recently
from
196838e to
78c37d6
Compare
ekump
force-pushed
the
ekump/clippy-annotation-action
branch
from
92c3830 to
145fb3f
Compare
ekump
force-pushed
the
ekump/clippy-annotation-action
branch
4 times, most recently
from
f56f485 to
3242198
Compare
ekump
force-pushed
the
ekump/clippy-annotation-action
branch
from
cce6240 to
a0b7788
Compare
This is not part of the libdatadog workspace. It's a github action that will run for CI. It reports counts of clippy allow annotations for both changed files in the PR and a repo overall. The goal is to call attention to excessive usage of allows, which could be a signal of reduced code quality. This action should be moved to its own repo when confident of its functionality.
reviews the files in the repo to parse and count the allow annotations usage. It also compares changed files to their base to determine the diffs of counts.
combines env vars and CLI args to get relevant info about the PR being analyzed.
finished report and comments it on the PR. it checks for existing comments and updates them instead of repeatedly posting a new comment for every PR change.
data from the analyzer and generates the text of the report that will be posted on the PR.
ekump
force-pushed
the
ekump/clippy-annotation-action
branch
from
a0b7788 to
9a877f9
Compare
ekump
changed the title
| shell: bash | ||
| run: | | ||
| cd ${{ github.action_path }} | ||
| cargo build --release |
There was a problem hiding this comment.
Not sure how this is usually done, but should we cache the binary as it probably won't change very often.
There was a problem hiding this comment.
The build time relative to the rest of CI is very short. I don't think it's an issue in the short-term. Once we move this to a separate repo the action should be pre-built.
|
|
||
| let rule_pattern = rules.join("|"); | ||
| let regex = Regex::new(&format!( | ||
| r"#\s*\[\s*allow\s*\(\s*clippy\s*::\s*({})\s*\)\s*\]", |
There was a problem hiding this comment.
It could be useful to allow non-clippy lints for things like "missing_docs". WDYT ?
There was a problem hiding this comment.
100% agree, the missing docs one would be quite useful. I think we can do it in a follow-up PR. There are probably a handful of features we'll want to add.
| annotations: &[ClippyAnnotation], | ||
| ) -> HashMap<Rc<String>, usize> { | ||
| let mut counts = HashMap::new(); | ||
| let mut crate_cache: HashMap<String, Rc<String>> = HashMap::new(); |
There was a problem hiding this comment.
Would it be to naive to assume all files in sub directory of the same cargo.toml are in the same crate (may be wrong in workspace though) ? Are at least cache based on directory. This could reduce the number of calls to get_crate_for_file.
There was a problem hiding this comment.
You can have crates within crates, even when the parent isn't a workspace. And as you mentioned we'd need extra logic to handle the top level workspace cargo.
I think this implementation is a good starting point (at least for libdatadog), but we likely will want to evolve the logic for matching files to crates. Ideally, it would be nice if we could leverage cargo metadata more for this rather than file paths.
After compilation, the reporter takes just a couple of seconds to run for all of libdatadog (including time communicating with GitHub). I think we'd be ok adding better caching later.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds a github action that scans the repo for usages of clippy allow annotations and reports changes across all crates in the repo.
If the action fails, it should not fail CI and block PRs being merged. If for any reason this action causes issues, feel free to disable it. This report shouldn't impede anyone's work in libdatadog.
Motivation
We want to reduce the chance of libdatadog panicking as much as possible. #915 enabled clippy warnings for panic macros in all crates. This PR will start reporting the number of annotations to bring attention to new additions of allow annotations or PRs that reduce the use of the annotations.
Additional Notes
This code is not part of the libdatadog workspace. It's an independent crate. It's included in this repo as it is expected we will need to make tweaks and fix bugs. Once we feel it is stable it should be moved to a separate repo and treated like any other github action we use.
allow annotations from unit tests for the reporter are currently showing up in the report. The option to filter certain file paths or crates will be added in a follow up PR.
Apologies for the size of the PR. I didn't anticipate it being this large.
Here is what the report looks like when crates have been changed:
How to test the change?
Unit tests were added where possible. I verified the report by adding extra allows and observing changes in the report.