pre-commit: enable check and clippy#4947
Conversation
7df11af to
b7644a7
Compare
|
@copilot please try and fix the following clippy violations: |
... Implemented the requested clippy fixes in commit This includes the evdev-rs fixes ( |
|
@mattkae remaining violation: |
There was a problem hiding this comment.
Pull request overview
This PR extends the repository’s pre-commit/CI enforcement by running Rust workspace validation (cargo check and cargo clippy) and includes several small Rust refactors/cleanups to satisfy clippy and general linting.
Changes:
- Enable
cargo checkandcargo clippy(plus a clippy fix hook) in.pre-commit-config.yaml. - Update the pre-commit GitHub Actions workflow to install additional system dependencies needed for Rust checks.
- Apply minor Rust refactors across the Wayland build script and
evdev-rsto reduce warnings and improve idiomatic usage (e.g., slice args,matches!, iterator cleanups).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/server/frontend_wayland/wayland_rs/build_script/wayland_server_generation.rs |
Use slice parameters instead of &Vec for generated server code helpers. |
src/server/frontend_wayland/wayland_rs/build_script/protocol_parser.rs |
Minor control-flow simplification in global-interface detection logic. |
src/server/frontend_wayland/wayland_rs/build_script/protocol_middleware_generation.rs |
Use slice parameters instead of &Vec for middleware generation. |
src/server/frontend_wayland/wayland_rs/build_script/main.rs |
Propagate slice-based APIs and apply small iterator/matcher cleanups. |
src/server/frontend_wayland/wayland_rs/build_script/helpers.rs |
Minor cleanup for create_dir_all argument usage. |
src/server/frontend_wayland/wayland_rs/build_script/ffi_generation.rs |
Update FFI generator to accept slices and iterate without moving. |
src/server/frontend_wayland/wayland_rs/build_script/cpp_builder.rs |
Small idiomatic Rust cleanups (e.g., push('\n'), field init shorthand). |
src/platforms/evdev-rs/src/platform.rs |
Clippy-driven refactors; adds #[allow(clippy::arc_with_non_send_sync)] in two places. |
src/platforms/evdev-rs/src/libinput_interface.rs |
Remove unused imports to satisfy clippy. |
src/platforms/evdev-rs/src/lib.rs |
Doc/comment formatting and minor return-style cleanup. |
src/platforms/evdev-rs/src/event_processing.rs |
Clippy-driven refactors (slice args, ?, or_default, loop/typing cleanups). |
src/platforms/evdev-rs/src/device.rs |
Idiomatic operator/return/initializer cleanups and signature simplification. |
.pre-commit-config.yaml |
Enable cargo-check and cargo-clippy hooks (including a clippy-fix variant). |
.github/workflows/pre-commit.yaml |
Install extra apt packages so Rust checks can run in CI. |
Comments suppressed due to low confidence (1)
src/platforms/evdev-rs/src/platform.rs:284
- Same as above: this
#[allow(clippy::arc_with_non_send_sync)]should ideally be accompanied by a short justification comment, or replaced withRcif the state is guaranteed to be single-threaded. Leaving it unexplained makes it easy to accidentally introduce real cross-thread usage later.
#[allow(clippy::arc_with_non_send_sync)]
pub fn create_input_device(&mut self, device_id: i32) -> Box<LibinputDevice> {
| @@ -66,6 +64,7 @@ impl PlatformRs { | |||
| /// was already started. The caller must only schedule `assign_seat()` when | |||
| /// this returns `true` to avoid calling `udev_assign_seat()` on an already- | |||
| /// assigned libinput context. | |||
There was a problem hiding this comment.
Here's a possible reference to use:
https://github.com/rust-lang/rust-clippy/issues?q=is%3Aissue%20state%3Aopen%20arc_with_non_send_sync
There was a problem hiding this comment.
Ooof. On closer inspection, this is not a false-positive, but a (potentially) real problem that's hidden under a couple of layers of indirection — LibInputDeviceState contains a LibInput that contains an Option<Rc<dyn LibInputInterface + 'static>>, and that is unsafe to send across threads (even wrapped in a Mutex), because you can .clone() an Rc<> from a shared reference (that's the point), and if you do that on two different threads then the reference counting can race (leading to either a memory leak or use-after-free, depending).
So this is something we should fix, rather than #[allow()].
There was a problem hiding this comment.
@copilot suggest a fix per above analysis
There was a problem hiding this comment.
Implemented a fix in 9006b2e (with core refactor in f37eb3b): libinput is no longer stored inside the shared Arc<Mutex<LibinputDeviceState>>. PlatformRs now owns libinput separately, event-processing functions take &mut Libinput explicitly, and fallback initialization now creates both libinput and shared state together to keep them consistent.
No UI changes were made, so screenshot is not applicable.
There was a problem hiding this comment.
Yup! I see what you mean. It seems that copilot figured out a decent solution of:
- Moving it out of the state
- Just piping it down where and when it's needed
RAOF
left a comment
There was a problem hiding this comment.
All the changes look good to me.
| @@ -66,6 +64,7 @@ impl PlatformRs { | |||
| /// was already started. The caller must only schedule `assign_seat()` when | |||
| /// this returns `true` to avoid calling `udev_assign_seat()` on an already- | |||
| /// assigned libinput context. | |||
There was a problem hiding this comment.
Ooof. On closer inspection, this is not a false-positive, but a (potentially) real problem that's hidden under a couple of layers of indirection — LibInputDeviceState contains a LibInput that contains an Option<Rc<dyn LibInputInterface + 'static>>, and that is unsafe to send across threads (even wrapped in a Mutex), because you can .clone() an Rc<> from a shared reference (that's the point), and if you do that on two different threads then the reference counting can race (leading to either a memory leak or use-after-free, depending).
So this is something we should fix, rather than #[allow()].
Agent-Logs-Url: https://github.com/canonical/mir/sessions/81dc8167-6849-4c1a-a6b1-677a1040fa1c Co-authored-by: Saviq <231077+Saviq@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/canonical/mir/sessions/a08d2b85-be6b-4d0c-9564-91ac07fd9595 Co-authored-by: Saviq <231077+Saviq@users.noreply.github.com>
Agent-Logs-Url: https://github.com/canonical/mir/sessions/a08d2b85-be6b-4d0c-9564-91ac07fd9595 Co-authored-by: Saviq <231077+Saviq@users.noreply.github.com>
Heh - but we have one there! I think clippy is unable to reason about it in the unsafe extern C++ block... |
|
@Saviq Evdev-rs is fixed, but I am tempted to leave wayland-rs for later, since so much of it is currently in flux |
Sure, just make clippy pass by suppressing what it's unhappy with. |
What's new?
pre-commit runs
cargo checkandcargo clippy.How to test
CI
Checklist