# Clippy in Chromium ## How to run Clippy lints against Rust code in Chromium When building locally, Clippy is disabled by default to avoid adding build overhead to Chromium engineers primarily working with C, C++, or Java. However, Clippy warnings are enforced by the CQ. Therefore, it is strongly recommended that engineers working with Rust code enable Clippy warnings by setting `enable_rust_clippy = true` in their `args.gn`. Doing so allows you to fix Clippy warnings as they occur, rather than waiting to discover them until after uploading a CL and running it through the CQ. It also lets you verify that you've fixed all the problems when rebuilding locally. ## Chromium policy Chromium has a relaxed, non-strict Clippy policy. Clippy is a tool that hopefully helps to improve the quality of Chromium code, but ultimately we trust the judgement of Chromium engineers. * We hope that *most* Clippy violations point out actual opportunities for improving the code (e.g. catching common mistakes, teaching idiomatic Rust patterns, etc.). * If a Clippy lint doesn’t seem useful in a certain scenario, then use your best judgement and feel free to suppress the lint using the `#[allow(...)]` macro. - We recommend leaving a short comment that will help future readers understand why the lint was suppressed. - We recommend consulting with the [@security team](https://groups.google.com/u/1/a/chromium.org/g/security) before suppressing `unsafe`-related lints like [`clippy::missing_safety_doc`](https://rust-lang.github.io/rust-clippy/master/index.html?search=safe#missing_safety_doc) or [`clippy::clippy::undocumented_unsafe_blocks`](https://rust-lang.github.io/rust-clippy/master/index.html?search=safe#undocumented_unsafe_blocks). - We recommend applying suppression to a single item, but it is okay to suppress at a module, or crate level if it makes sense (e.g. if all functions in a given test module have to violate a certain lint) * If a Clippy lint is frequently suppressed, then one option on the table is disabling the lint globally in Chromium - please talk with [@chrome-rust](https://groups.google.com/a/google.com/g/chrome-rust) if you find yourself oftentimes disabling a particular lint. For a list of Clippy lints that apply by default to Chromium targets please see the `//build/config/compiler:default_clippy_lints` target. ## Known issues * https://crbug.com/472338477: Can't invoke Clippy on a proc-macro crate * https://github.com/rust-lang/rust-clippy/issues/16317: `clippy::missing_safety_doc` triggers on `cxx::bridge`-generated code ([Example `#[allow(...)]` used as a workaround](https://source.chromium.org/chromium/chromium/src/+/main:mojo/public/rust/sequences/cxx.rs;l=24-25;drc=3abf739f4b711e4339c793af95ab482ca3a6c7fc)) ## Clippy coverage in Chromium CQ/CI Clippy is enabled on a subset of CQ bots: `android-arm64-rel`, `linux-chromeos-rel`, `linux-rel`, `mac-rel`, `win-rel`. TODO(https://crbug.com/41484295): Add other bots and target platforms if additional code needs to be covered by Clippy (`ios-simulator`?). ## Future work Next steps: * Figure out how Chromium-specific lints can be added * Consider opting into additional lints. Examples: - [`undocumented_unsafe_blocks`](https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks) - [`multiple_unsafe_ops_per_block`](https://rust-lang.github.io/rust-clippy/master/index.html?groups=restriction#multiple_unsafe_ops_per_block) ## Automatically applying fix suggestions ### IDE integration TODO: See if/how this works + write this section. ### Command-line When build fails because of Clippy, then a note toward the end spells out a command for using `build/rust/apply_fixes.py` to apply machine-applicable fix suggestions from this particular Clippy invocation. Example: ``` $ autoninja -C out/rel ... ... error: this expression creates a reference which is immediately dereferenced by the compiler --> ../../third_party/blink/renderer/core/xml/parser/xml_ffi.rs:271:33 | 271 | q_name.push_str(&pref); | ^^^^^ help: change this to: `pref` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow = note: `-D clippy::needless-borrow` implied by `-D clippy::style` = help: to override `-D clippy::style` add `#[allow(clippy::needless_borrow)]` error: aborting due to 1 previous error NOTE: See `//docs/rust/clippy.md` for more Clippy info. NOTE: To apply machine-applicable fix suggestions (if any) run: build/rust/apply_fixes.py out/rel clippy-driver obj/third_party/blink/renderer/core/xml_ffi.rustflags.json ... $ build/rust/apply_fixes.py out/rel clippy-driver obj/third_party/blink/renderer/core/xml_ffi.rustflags.json Invoking clippy-driver to repro the build problem... Looking for machine-applicable fixes... Applying fixes to `../../third_party/blink/renderer/core/xml/parser/xml_ffi.rs`... Applying a fix on line 271... $ git diff diff --git a/third_party/blink/renderer/core/xml/parser/xml_ffi.rs b/third_party/blink/renderer/core/xml/parser/xml_ffi.rs index 707c74bf87784..b02eaf1266d2c 100644 --- a/third_party/blink/renderer/core/xml/parser/xml_ffi.rs +++ b/third_party/blink/renderer/core/xml/parser/xml_ffi.rs @@ -268,7 +268,7 @@ fn attributes_next<'a>( match &name.prefix { Some(pref) => { let mut q_name = String::with_capacity(pref.len() + 1 + name.local_name.len()); - q_name.push_str(&pref); + q_name.push_str(pref); q_name.push(':'); q_name.push_str(&name.local_name); attribute_view.as_mut().Populate( ``` ## Implementation detail: When exactly does Clippy run? When `enable_rust_clippy = true` then by default each Rust target (e.g. `rust_static_library("foo")`) will have a [`validations`](https://gn.googlesource.com/gn/+/main/docs/reference.md#var_validations) dependency edge to an `action("foo_clippy")` target. This means that `foo_clippy` will be invoked whenever `foo` is built (when `ninja` is asked to build `foo` directly or when `foo` is transitively needed). OTOH, targets that depend on `foo` will not be blocked waiting for completion of `foo_clippy`. If you want to minimize overhead of Clippy even further, you can consider setting `enable_rust_clippy_eager = false`. This removes the `validations` dependency edge described above, so that building `foo` no longer triggers `foo_clippy`. This mode is discouraged, because it means that Clippy failures will go undetected until a full build (e.g. until CQ). Nevertheless, this mode may be useful when wanting to quickly iterate on code. ## References * Tracking bug: https://crbug.com/41484295/dependencies * Initial design doc: ["Clippy in Chromium"](https://docs.google.com/document/d/1waxby3UnWPhU_CjKFQlQGUTMmkb0qTZK2gYwqo4KPd0/edit?usp=sharing) * Technical deep dive: ["Clippy in Chromium: dep edge"](https://docs.google.com/document/d/1Ki9gwPv43p4G65cYoaPOdv3GIqHNpYHAPsXNp0Zdmjk/edit?usp=sharing)