Conversation
jerrysxie
commented
Dec 23, 2025
- Add exceptions for test code and modules with limited dependencies
There was a problem hiding this comment.
Pull request overview
This PR introduces strict Clippy lints at the workspace level to prevent panic-inducing operations (unwrap, expect, panic, etc.) while adding targeted exceptions for test code and specific service modules with limited dependencies.
Key changes:
- Added comprehensive panic-related Clippy lints at workspace level (deny by default)
- Configured service crates to inherit workspace lints
- Added lint allowances for test modules and specific production services
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
Cargo.toml |
Adds workspace-level Clippy lints denying panic operations (unwrap, expect, panic, todo, etc.) along with performance and correctness categories |
thermal-service/Cargo.toml |
Enables workspace lint inheritance |
thermal-service/src/lib.rs |
Allows todo and unwrap_used for this service module |
keyboard-service/Cargo.toml |
Enables workspace lint inheritance |
keyboard-service/src/lib.rs |
Allows expect_used, indexing_slicing, panic_in_result_fn, and unwrap_used |
espi-service/src/lib.rs |
Allows expect_used, indexing_slicing, panic, and unwrap_used |
debug-service/src/lib.rs |
Allows expect_used, indexing_slicing, and unwrap_used |
partition-manager/partition-manager/src/test/mod.rs |
Allows panic and unwrap_used for test module |
partition-manager/generation/src/tests/mod.rs |
Allows unwrap_used for test module |
embedded-service/src/type_c/event.rs |
Allows unwrap_used for test module |
embedded-service/src/ipc/deferred.rs |
Allows unwrap_used for test module |
embedded-service/src/intrusive_list.rs |
Allows unwrap_used on specific test functions |
embedded-service/src/hid/mod.rs |
Allows unwrap_used for test module |
embedded-service/src/hid/command.rs |
Allows indexing_slicing and unwrap_used for test module |
embedded-service/src/ec_type/protocols/mctp.rs |
Allows panic on specific test function that uses explicit panic |
embedded-service/src/ec_type/mod.rs |
Allows unwrap_used for test module |
embedded-service/src/buffer.rs |
Allows unwrap_used for test module |
embedded-service/src/broadcaster/immediate.rs |
Allows unwrap_used for test module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
269050e to
e54a096
Compare
* Add exceptions for test code and modules with limited dependencies
e54a096 to
cfa0626
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Could also consider placing this in the lib.rs of each crate to blanket allow panics in tests:
#![cfg_attr(
test,
allow(
clippy::unwrap_used,
clippy::expect_used,
clippy::unreachable,
clippy::unimplemented,
clippy::todo,
clippy::panic,
clippy::panic_in_result_fn,
clippy::indexing_slicing
)
)]Though this would also allow panics anywhere in the crate if testing is enabled, so technically different (but since a regular cargo clippy would catch them maybe not a big deal).
But if the goal is to be as minimally selective as we can in allowing panics even in tests the current way is better.
That is a good alternative. Right now, let's be as minimal as possible in regard to panics but in the future if doing this in the test code becames too much of a pain then we can switch to this approach. |