docs/rust/clippy.md - chromium/src.git - Git at Google

 # 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)
	# 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)