Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to clean up some lints and enable more #923

Merged
merged 9 commits into from
Apr 1, 2024
Merged

Try to clean up some lints and enable more #923

merged 9 commits into from
Apr 1, 2024

Conversation

psychon
Copy link
Owner

@psychon psychon commented Mar 17, 2024

I came across the unused_extern_crates lint, which is allow-by-default. It is however part of the rust_2018_idioms lint group, which we are already denying.

Anyway, this discovery led me down a linting rabbit hole.

There is the unused_import_braces lint which arguable lints against something that rustfmt already deals with. Both the lint and rustfmt deal with the braces in "use test::{A};". Thus, this lint can just be dropped.

The "unused" lint group includes "unused_must_use" and some other things that sound helpful, so let's just switch to the more generic lint. This found some unused_macro_rules in x11rb/src/tracing.rs, which need to be allowed, so this lint can just be denied and not forbidden. But some of the lints that we forbid are part of the "unused" group and #[deny]ing something that was previously #[forbid]en is an error. Thus, I am switching the order of these and move all the #[deny]s up.

Next, I thought that "unused_qualifications" should be part of "unused", but it actually is not. The following code warns about the allows for unused_extern_crates and unused_must_use, but not unused_qualifications, so this is not part of the unused group. Same for unused_results.

#![forbid(unused)]
#![allow(unused_extern_crates)]
#![allow(unused_must_use)]
#![allow(unused_qualifications)]
#![allow(unused_results)]

I now feel confused about lints. Is this PR still a good idea? Does it improve anything?

I came across the unused_extern_crates lint, which is allow-by-default.
It is however part of the rust_2018_idioms lint group, which we are
already denying.

Anyway, this discovery led me down a linting rabbit hole.

There is the unused_import_braces lint which arguable lints against
something that rustfmt already deals with. Both the lint and rustfmt
deal with the braces in  "use test::{A};". Thus, this lint can just be
dropped.

The "unused" lint group includes "unused_must_use" and some other things
that sound helpful, so let's just switch to the more generic lint. This
found some unused_macro_rules in x11rb/src/tracing.rs, which need to be
allowed, so this lint can just be denied and not forbidden. But some of
the lints that we forbid are part of the "unused" group and #[deny]ing
something that was previously #[forbid]en is an error. Thus, I am
switching the order of these and move all the #[deny]s up.

Next, I thought that "unused_qualifications" should be part of "unused",
but it actually is not. The following code warns about the allows for
unused_extern_crates and unused_must_use, but not unused_qualifications,
so this is not part of the unused group. Same for unused_results.

    #![forbid(unused)]
    #![allow(unused_extern_crates)]
    #![allow(unused_must_use)]
    #![allow(unused_qualifications)]
    #![allow(unused_results)]

Signed-off-by: Uli Schlachter <psychon@znc.in>
Copy link

codecov bot commented Mar 17, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 13.06%. Comparing base (08ce9fa) to head (6551db1).

Files Patch % Lines
x11rb/src/properties.rs 0.00% 2 Missing ⚠️
x11rb-protocol/src/resource_manager/mod.rs 0.00% 1 Missing ⚠️
x11rb/src/rust_connection/packet_reader.rs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #923      +/-   ##
==========================================
- Coverage   13.06%   13.06%   -0.01%     
==========================================
  Files         191      191              
  Lines      136668   136678      +10     
==========================================
  Hits        17860    17860              
- Misses     118808   118818      +10     
Flag Coverage Δ
tests 13.06% <33.33%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Since Rust 2021, TryFrom and TryInto are part of the prelude.

Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Drop is part of the prelude.

Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
TryFrom, TryInto, and drop are all part of the prelude in Rust 2021.

Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
@psychon psychon merged commit 2b3a646 into master Apr 1, 2024
22 of 23 checks passed
@psychon psychon deleted the lints branch April 1, 2024 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant