| Message ID | 20241108180139.117112-9-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: improved integration with cargo | expand |
Paolo Bonzini <pbonzini@redhat.com> writes: > Many lints that default to allow can be helpful in detecting bugs or > keeping the code style homogeneous. Add them liberally, though perhaps > not as liberally as in hw/char/pl011/src/lib.rs. In particular, enabling > entire groups can be problematic because of bitrot when new links are > added in the future. > > For Clippy, this is actually a feature that is only present in Cargo > 1.74.0 but, since we are not using Cargo to *build* QEMU, only developers > will need a new-enough cargo and only to run tools such as clippy. > The requirement does not apply to distros that are building QEMU. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > rust/Cargo.toml | 66 +++++++++++++++++++++++++++++++++++ > rust/hw/char/pl011/src/lib.rs | 18 ++-------- > rust/qemu-api/src/bindings.rs | 6 ++-- > 3 files changed, 71 insertions(+), 19 deletions(-) > > diff --git a/rust/Cargo.toml b/rust/Cargo.toml > index 1ff8f5c2781..43cca33a8d8 100644 > --- a/rust/Cargo.toml > +++ b/rust/Cargo.toml > @@ -19,3 +19,69 @@ unknown_lints = "allow" > > # Prohibit code that is forbidden in Rust 2024 > unsafe_op_in_unsafe_fn = "deny" > + [snip] > + > +# nice to have, but cannot be enabled yet > +#wildcard_imports = "deny" > + > +# these may have false positives > +#option_if_let_else = "deny" > +cognitive_complexity = "deny" Just to confirm, CC <= 25 is to be enforced for all methods, right? -- Best Regards Junjie Mao
On 11/13/24 08:14, Junjie Mao wrote: > > Paolo Bonzini <pbonzini@redhat.com> writes: > >> Many lints that default to allow can be helpful in detecting bugs or >> keeping the code style homogeneous. Add them liberally, though perhaps >> not as liberally as in hw/char/pl011/src/lib.rs. In particular, enabling >> entire groups can be problematic because of bitrot when new links are >> added in the future. >> >> For Clippy, this is actually a feature that is only present in Cargo >> 1.74.0 but, since we are not using Cargo to *build* QEMU, only developers >> will need a new-enough cargo and only to run tools such as clippy. >> The requirement does not apply to distros that are building QEMU. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> rust/Cargo.toml | 66 +++++++++++++++++++++++++++++++++++ >> rust/hw/char/pl011/src/lib.rs | 18 ++-------- >> rust/qemu-api/src/bindings.rs | 6 ++-- >> 3 files changed, 71 insertions(+), 19 deletions(-) >> >> diff --git a/rust/Cargo.toml b/rust/Cargo.toml >> index 1ff8f5c2781..43cca33a8d8 100644 >> --- a/rust/Cargo.toml >> +++ b/rust/Cargo.toml >> @@ -19,3 +19,69 @@ unknown_lints = "allow" >> >> # Prohibit code that is forbidden in Rust 2024 >> unsafe_op_in_unsafe_fn = "deny" >> + > [snip] >> + >> +# nice to have, but cannot be enabled yet >> +#wildcard_imports = "deny" >> + >> +# these may have false positives >> +#option_if_let_else = "deny" >> +cognitive_complexity = "deny" > > Just to confirm, CC <= 25 is to be enforced for all methods, right? I wanted an opinion on that. option_if_let_else has been more of a pain than a benefit, sometimes it suggests code that is worse or does not even compile. So far I've never had any cognitive_complexity error show up, but pl011 used it so I have kept it in Cargo.toml. If we start having too many #[allow()] for cognitive_complexity we can remove it; for many of the others, instead, we might even change deny to forbid. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 11/13/24 08:14, Junjie Mao wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> Many lints that default to allow can be helpful in detecting bugs or >>> keeping the code style homogeneous. Add them liberally, though perhaps >>> not as liberally as in hw/char/pl011/src/lib.rs. In particular, enabling >>> entire groups can be problematic because of bitrot when new links are >>> added in the future. >>> >>> For Clippy, this is actually a feature that is only present in Cargo >>> 1.74.0 but, since we are not using Cargo to *build* QEMU, only developers >>> will need a new-enough cargo and only to run tools such as clippy. >>> The requirement does not apply to distros that are building QEMU. >>> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>> --- >>> rust/Cargo.toml | 66 +++++++++++++++++++++++++++++++++++ >>> rust/hw/char/pl011/src/lib.rs | 18 ++-------- >>> rust/qemu-api/src/bindings.rs | 6 ++-- >>> 3 files changed, 71 insertions(+), 19 deletions(-) >>> >>> diff --git a/rust/Cargo.toml b/rust/Cargo.toml >>> index 1ff8f5c2781..43cca33a8d8 100644 >>> --- a/rust/Cargo.toml >>> +++ b/rust/Cargo.toml >>> @@ -19,3 +19,69 @@ unknown_lints = "allow" >>> >>> # Prohibit code that is forbidden in Rust 2024 >>> unsafe_op_in_unsafe_fn = "deny" >>> + >> [snip] >>> + >>> +# nice to have, but cannot be enabled yet >>> +#wildcard_imports = "deny" >>> + >>> +# these may have false positives >>> +#option_if_let_else = "deny" >>> +cognitive_complexity = "deny" >> Just to confirm, CC <= 25 is to be enforced for all methods, right? > > I wanted an opinion on that. option_if_let_else has been more of a pain than a > benefit, sometimes it suggests code that is worse or does not even compile. > > So far I've never had any cognitive_complexity error show up, but pl011 used it > so I have kept it in Cargo.toml. If we start having too many #[allow()] for > cognitive_complexity we can remove it; for many of the others, instead, we might > even change deny to forbid. Agree. The most common case I have seen with a high CC is a long switch/match statement, which should not be too many. For the time being it should be a useful hint for complexity control. Reviewed-by: Junjie Mao <junjie.mao@hotmail.com> > > Paolo -- Best Regards Junjie Mao
diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 1ff8f5c2781..43cca33a8d8 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -19,3 +19,69 @@ unknown_lints = "allow" # Prohibit code that is forbidden in Rust 2024 unsafe_op_in_unsafe_fn = "deny" + +[workspace.lints.rustdoc] +broken_intra_doc_links = "deny" +invalid_html_tags = "deny" +invalid_rust_codeblocks = "deny" +bare_urls = "deny" +unescaped_backticks = "deny" +redundant_explicit_links = "deny" + +[workspace.lints.clippy] +# default-warn lints +result_unit_err = "allow" +too_many_arguments = "allow" +upper_case_acronyms = "allow" + +# default-allow lints +as_ptr_cast_mut = "deny" +as_underscore = "deny" +assertions_on_result_states = "deny" +bool_to_int_with_if = "deny" +borrow_as_ptr = "deny" +cast_lossless = "deny" +dbg_macro = "deny" +debug_assert_with_mut_call = "deny" +derive_partial_eq_without_eq = "deny" +doc_markdown = "deny" +empty_structs_with_brackets = "deny" +ignored_unit_patterns = "deny" +implicit_clone = "deny" +macro_use_imports = "deny" +missing_const_for_fn = "deny" +missing_safety_doc = "deny" +multiple_crate_versions = "deny" +mut_mut = "deny" +needless_bitwise_bool = "deny" +needless_pass_by_ref_mut = "deny" +no_effect_underscore_binding = "deny" +option_option = "deny" +or_fun_call = "deny" +ptr_as_ptr = "deny" +ptr_cast_constness = "deny" +pub_underscore_fields = "deny" +redundant_clone = "deny" +redundant_closure_for_method_calls = "deny" +redundant_else = "deny" +redundant_pub_crate = "deny" +ref_binding_to_reference = "deny" +ref_option_ref = "deny" +return_self_not_must_use = "deny" +same_name_method = "deny" +semicolon_inside_block = "deny" +significant_drop_in_scrutinee = "deny" +significant_drop_tightening = "deny" +suspicious_operation_groupings = "deny" +transmute_ptr_to_ptr = "deny" +transmute_undefined_repr = "deny" +type_repetition_in_bounds = "deny" +unused_self = "deny" +used_underscore_binding = "deny" + +# nice to have, but cannot be enabled yet +#wildcard_imports = "deny" + +# these may have false positives +#option_if_let_else = "deny" +cognitive_complexity = "deny" diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs index cd0a49acb91..3fa178cded0 100644 --- a/rust/hw/char/pl011/src/lib.rs +++ b/rust/hw/char/pl011/src/lib.rs @@ -14,28 +14,14 @@ //! the [`registers`] module for register types. #![deny( - rustdoc::broken_intra_doc_links, - rustdoc::redundant_explicit_links, clippy::correctness, clippy::suspicious, clippy::complexity, clippy::perf, clippy::cargo, clippy::nursery, - clippy::style, - // restriction group - clippy::dbg_macro, - clippy::as_underscore, - clippy::assertions_on_result_states, - // pedantic group - clippy::doc_markdown, - clippy::borrow_as_ptr, - clippy::cast_lossless, - clippy::option_if_let_else, - clippy::missing_const_for_fn, - clippy::cognitive_complexity, - clippy::missing_safety_doc, - )] + clippy::style +)] #![allow(clippy::result_unit_err)] extern crate bilge; diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs index 1dac310594d..972b1f1ee90 100644 --- a/rust/qemu-api/src/bindings.rs +++ b/rust/qemu-api/src/bindings.rs @@ -7,10 +7,10 @@ non_snake_case, non_upper_case_globals, unsafe_op_in_unsafe_fn, + clippy::pedantic, + clippy::restriction, + clippy::style, clippy::missing_const_for_fn, - clippy::too_many_arguments, - clippy::approx_constant, - clippy::use_self, clippy::useless_transmute, clippy::missing_safety_doc )]
Many lints that default to allow can be helpful in detecting bugs or keeping the code style homogeneous. Add them liberally, though perhaps not as liberally as in hw/char/pl011/src/lib.rs. In particular, enabling entire groups can be problematic because of bitrot when new links are added in the future. For Clippy, this is actually a feature that is only present in Cargo 1.74.0 but, since we are not using Cargo to *build* QEMU, only developers will need a new-enough cargo and only to run tools such as clippy. The requirement does not apply to distros that are building QEMU. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- rust/Cargo.toml | 66 +++++++++++++++++++++++++++++++++++ rust/hw/char/pl011/src/lib.rs | 18 ++-------- rust/qemu-api/src/bindings.rs | 6 ++-- 3 files changed, 71 insertions(+), 19 deletions(-)