fix: enable clippy as_conversions lint across workspace by arianbeh · Pull Request #5577 · firecracker-microvm/firecracker

arianbeh · 2025-12-10T23:48:45Z

Changes

Enabled the clippy::as_conversions lint at the workspace level by updating
Cargo.toml.
Resolved all resulting as_conversions violations across the workspace by:
- Replacing unsafe pointer casts with std::ptr::from_ref / std::ptr::from_mut
  where appropriate (e.g. in cpu-template-helper).
- Replacing silent integer casts with explicit conversions such as
  i64::from(), u64::try_from(), and usize::try_from().
- Using localized #[allow(clippy::as_conversions)] in low-level or const
  contexts where as is intentional and required (e.g., enum-to-byte
  encoding in ACPI AML, seccompiler const fns, and libc constant conversions).
- Cleaning up comparisons and length/size conversions to avoid silent
  truncation.
Updated the necessary code paths in:
- cpu-template-helper
- utils::time
- acpi-tables (AML, DSDT, MADT, XSDT)
- seccompiler (bindings, types, lib)
- rebase-snap
- jailer (resource_limits)
- snapshot-editor
- firecracker (main and examples)
- vmm (library, benches, and tests)

Reason

This PR implements the final step of #3161, consolidating several
cast-related Clippy lints into as_conversions and fixing all violations.

Enabling as_conversions ensures:

Integer and pointer casts are either:
- Performed via explicit, safe conversion APIs, or
- Clearly scoped behind #[allow] where they are intentional and unavoidable.
Silent truncation, wraparound, and lossy conversions are avoided.
The workspace stays clean under RUSTFLAGS="-Dwarnings".
Future code introducing unsafe as casts will be flagged consistently.

All changes preserve existing behavior while making the code safer and more
explicit about type boundaries.

Verification

Locally ran:

RUSTFLAGS="-Dwarnings" cargo clippy --all --all-targets
cargo fmt

All packages build clean with no Clippy warnings.

I was not able to run tools/devtool checkbuild or tools/devtool checkstyle
because /dev/kvm is not available in my environment.

Closes #3161.

JamesC1305 · 2025-12-19T09:39:38Z

Hi @arianbeh,

Thank you for your contribution, this definitely looks like a good QoL change for us.

The only thing I'd suggest as an improvement is to use repr(u8) on enums you're defining From for. We can then use#[allow(clippy::as_conversions)] to allow a single as cast inside the from function. This doesn't prevent us using as entirely, but it makes our use case explicit, which should prevent any unwanted consequences whilst preventing unnecessary code bloat.

I'll leave a review on your PR and point it our :)

JamesC1305 · 2026-01-07T17:20:44Z

Hi @arianbeh,

It looks like your changes are not compiling (https://buildkite.com/firecracker/firecracker-pr/builds/15430#019b98cb-47c2-4d53-8b05-b4cefbbac739/L698). Could you please fix this and squash it into your original commit?

JamesC1305 · 2026-01-15T11:11:03Z

Hi @arianbeh,

I notice you haven't engaged with this PR lately. Are you still making changes, or are you waiting for me to run tests?

arianbeh · 2026-01-15T18:29:17Z

Hi @arianbeh,

I notice you haven't engaged with this PR lately. Are you still making changes, or are you waiting for me to run tests?

Hey, I did fix the compile issue and I belive I pushed it. I also accpeted your earlier comments believing it would add those to the code.

codecov · 2026-01-16T12:31:43Z

Codecov Report

❌ Patch coverage is 69.04762% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.03%. Comparing base (326fc46) to head (cf662ba).
⚠️ Report is 5 commits behind head on main.

⚠️ Current head cf662ba differs from pull request most recent head f84fd96

Please upload reports for the commit f84fd96 to get more accurate results.

Files with missing lines	Patch %	Lines
src/seccompiler/src/types.rs	0.00%	8 Missing ⚠️
src/acpi-tables/src/aml.rs	88.88%	3 Missing ⚠️
src/seccompiler/src/bindings.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5577   +/-   ##
=======================================
  Coverage   83.02%   83.03%           
=======================================
  Files         276      276           
  Lines       29428    29445   +17     
=======================================
+ Hits        24433    24450   +17     
  Misses       4995     4995

Flag	Coverage Δ
5.10-m5n.metal	`83.31% <69.04%> (+0.01%)`	⬆️
5.10-m6a.metal	`82.63% <69.04%> (+<0.01%)`	⬆️
5.10-m6g.metal	`80.05% <59.52%> (+0.01%)`	⬆️
5.10-m6i.metal	`83.31% <69.04%> (+0.01%)`	⬆️
5.10-m7a.metal-48xl	`82.63% <69.04%> (+<0.01%)`	⬆️
5.10-m7g.metal	`80.05% <59.52%> (+0.01%)`	⬆️
5.10-m7i.metal-24xl	`83.28% <69.04%> (+<0.01%)`	⬆️
5.10-m7i.metal-48xl	`83.28% <69.04%> (+<0.01%)`	⬆️
5.10-m8g.metal-24xl	`80.04% <59.52%> (+<0.01%)`	⬆️
5.10-m8g.metal-48xl	`80.04% <59.52%> (+0.01%)`	⬆️
5.10-m8i.metal-48xl	`83.28% <69.04%> (+<0.01%)`	⬆️
5.10-m8i.metal-96xl	`83.28% <69.04%> (+<0.01%)`	⬆️
6.1-m5n.metal	`83.34% <69.04%> (+0.01%)`	⬆️
6.1-m6a.metal	`82.66% <69.04%> (+<0.01%)`	⬆️
6.1-m6g.metal	`80.04% <59.52%> (+<0.01%)`	⬆️
6.1-m6i.metal	`83.34% <69.04%> (+0.01%)`	⬆️
6.1-m7a.metal-48xl	`82.65% <69.04%> (+0.01%)`	⬆️
6.1-m7g.metal	`80.04% <59.52%> (+<0.01%)`	⬆️
6.1-m7i.metal-24xl	`83.35% <69.04%> (+0.01%)`	⬆️
6.1-m7i.metal-48xl	`83.35% <69.04%> (+<0.01%)`	⬆️
6.1-m8g.metal-24xl	`80.04% <59.52%> (+<0.01%)`	⬆️
6.1-m8g.metal-48xl	`80.04% <59.52%> (+<0.01%)`	⬆️
6.1-m8i.metal-48xl	`83.35% <69.04%> (+<0.01%)`	⬆️
6.1-m8i.metal-96xl	`83.35% <69.04%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

JamesC1305 · 2026-01-16T12:38:46Z

Hi @arianbeh,

It looks like style checks are failing due to rust formatting and commit messages. Rust formatting can be fixed using devtool fmt, and the best way to fix the commits would be to merge them into your original fix commit and force push. You can then run devtool checkstyle to verify that the tests pass. Thank you!

arianbeh · 2026-01-27T21:46:17Z

Hi @arianbeh,

It looks like style checks are failing due to rust formatting and commit messages. Rust formatting can be fixed using devtool fmt, and the best way to fix the commits would be to merge them into your original fix commit and force push. You can then run devtool checkstyle to verify that the tests pass. Thank you!

Hey @JamesC1305 I am sorry I have not been active since your last comment. Due to some personal circumstance I have not been able to work on the issue. I will still try my best to adress the style check issue but I am not sure how long it is going to take me.

JamesC1305 · 2026-02-09T12:41:24Z

Hi @arianbeh,

Thanks again for taking a look at this. I notice that your commit seems to have picked up some of the changes from the main branch. When you next have the chance, would you be able to drop these from your commit, so that it only includes the changes you intend to make?

Feel free to reach out if you need any support on this :)

arianbeh · 2026-02-09T18:16:16Z

Hi @arianbeh,

Thanks again for taking a look at this. I notice that your commit seems to have picked up some of the changes from the main branch. When you next have the chance, would you be able to drop these from your commit, so that it only includes the changes you intend to make?

Feel free to reach out if you need any support on this :)

Hey @JamesC1305,

Thanks for letting me know. That is my bad I didn't realize I picked up some stuff from main. I would really appreciate some guidance on how to address this issue since I am not sure how to identiy the exta bits of code.

Thank you,

JamesC1305 · 2026-02-11T17:13:53Z

Hi @arianbeh,

No worries :) My first suggestion would probably to look at git's reflog. More info can be found on that here. With that, you should (hopefully) be able to get a hash for the old state of your commit which did not include the rebase changes from main.

You can look at the reflog using git reflog. Once you find a state which you think works, you can check it out by using git checkout <HASH>. After reviewing the new state of the code, you can set that state to be the tip of the branch using git checkout <BRANCHNAME> followed bygit reset --hard <HASH>, and finally pushing that using git push --force-with-lease.

Take some care, as once you execute git reset, the branch will be modified, but if something goes wrong, the reflog should contain the state before resetting, so you can always revert back :).

Let me know if you need any more info/help with this.

JamesC1305 · 2026-03-25T15:59:55Z

Hi @arianbeh,

Just wanted to ping you to see if you've had a chance to review my last message? Let me know if there's anything more we can help with :)

arianbeh · 2026-03-25T23:01:05Z

Hi @arianbeh,

Just wanted to ping you to see if you've had a chance to review my last message? Let me know if there's anything more we can help with :)

Hey @JamesC1305,

I apologize for not being active recently. I just cleaned up my commit. I made sure there are no warnings and that it passes the style checks.

Please let me know if i missed smth or if any other issues pop up. :)

…ixed style check errors Signed-off-by: arianbeh <arianbehmard@gmail.com>

JamesC1305 · 2026-03-30T10:18:27Z

Hi @arianbeh,

I've just taken another look at your PR. Firstly, the commit message is a bit too long, which caused a style failure in the last buildkite run. Would you please be able to provide a shorter commit message?

Second of all, it would be good to propagate and handle some of the conversion errors if possible. In a few places, you call try_from and immediately unwrap. Whilst this raises errors that an as conversion would silently ignore, it would be good to check the try_from's you've added to see if these errors can be handled in a non-fatal way that would allow the VMM to continue executing.

Lastly, in the cases where it makes sense to allow as conversions, I think it would be better to scope down the allow attribute to the specific lint being ignored. as_conversions is a set of multiple individual lints. Scoping down the allow attribute to refer to just the particular lint that clippy has an issue with will help with auditability, and will throw a warning if the use of as changes in the future. You can see the full list of lints that as_conversions covers here: https://rust-lang.github.io/rust-clippy/stable/index.html#as_conversions

Stale review.

JamesC1305 self-assigned this Dec 17, 2025

JamesC1305 previously requested changes Dec 19, 2025

View reviewed changes

Comment thread src/acpi-tables/src/aml.rs

Comment thread src/acpi-tables/src/aml.rs

Comment thread src/acpi-tables/src/dsdt.rs

Comment thread src/acpi-tables/src/aml.rs

JamesC1305 added the Status: Awaiting author Indicates that an issue or pull request requires author action label Jan 9, 2026

arianbeh force-pushed the use-clippy-as-conversions branch from d36ea75 to 3d7bf22 Compare January 28, 2026 00:05

arianbeh requested review from Manciukic, kalyazin, pb8o and xmarcalx as code owners January 28, 2026 00:05

arianbeh force-pushed the use-clippy-as-conversions branch from 3d7bf22 to a6dd03b Compare January 28, 2026 16:40

arianbeh force-pushed the use-clippy-as-conversions branch from a6dd03b to 7780a11 Compare March 25, 2026 22:58

fix: enable clippy::as_conversions lint, fix resulting warnings and f…

f84fd96

…ixed style check errors Signed-off-by: arianbeh <arianbehmard@gmail.com>

arianbeh force-pushed the use-clippy-as-conversions branch from cf662ba to f84fd96 Compare March 27, 2026 21:10

Conversation

arianbeh commented Dec 10, 2025

Changes

Reason

Verification

Uh oh!

JamesC1305 commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JamesC1305 commented Jan 7, 2026

Uh oh!

JamesC1305 commented Jan 15, 2026

Uh oh!

arianbeh commented Jan 15, 2026

Uh oh!

codecov Bot commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JamesC1305 commented Jan 16, 2026

Uh oh!

arianbeh commented Jan 27, 2026

Uh oh!

JamesC1305 commented Feb 9, 2026

Uh oh!

arianbeh commented Feb 9, 2026

Uh oh!

JamesC1305 commented Feb 11, 2026

Uh oh!

JamesC1305 commented Mar 25, 2026

Uh oh!

arianbeh commented Mar 25, 2026

Uh oh!

JamesC1305 commented Mar 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JamesC1305 commented Dec 19, 2025 •

edited

Loading

codecov Bot commented Jan 16, 2026 •

edited

Loading