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

Enable workspace lints #2741

Merged
merged 5 commits into from
Dec 12, 2023
Merged

Enable workspace lints #2741

merged 5 commits into from
Dec 12, 2023

Conversation

kennykerr
Copy link
Collaborator

As suggested in #2739, I'm enabling workspace lints to more easily enable non-default Rust and Clippy lints.

I've started with only elided-lifetimes-in-paths as some of the others, like rust_2018_idioms, has some false positives that prevent them from being turned on for this workspace.

Fixes: #2739

@ChrisDenton
Copy link
Collaborator

If it's only a single case, would adding an #[allow(unused_extern_crates)] // false positive be worth it so you still get everything else linted?

@kennykerr
Copy link
Collaborator Author

Seems to work!

@ChrisDenton
Copy link
Collaborator

Is the reason for the false positive that CI doesn't always use features that use it? E.g. clippy is run with no features at all

cargo clippy -p windows-sys &&

Copy link
Collaborator

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, this PR lgtm.

@kennykerr
Copy link
Collaborator Author

Is the reason for the false positive that CI doesn't always use features that use it? E.g. clippy is run with no features at all

cargo clippy -p windows-sys &&

Yes, it isn't used in all cases. It is used by some features and by the optional implement and interface macros.

@kennykerr kennykerr merged commit a6bdfc8 into master Dec 12, 2023
47 checks passed
@kennykerr kennykerr deleted the workspace.lints branch December 12, 2023 22:12
Comment on lines +10 to 11
#[allow(unused_extern_crates)]
extern crate self as windows_sys;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not seem that this is needed for anything; the lint is valid. There is nothing inside sys / windows referring to (::)windows_sys:: or (::)windows:: respectively. Could these lines just be removed altogether?

Copy link
Contributor

@MarijnS95 MarijnS95 Dec 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err, I meant to say that:

  1. On windows_sys I don't seem to get a false-positive here, because it's actually used;
  2. On windows I only get a few errors originating from the windows_implement macro, which expects the windows crate to be available as an external crate (which is interesting for a macro that generates code to be used both inside and outside of a crate).
    EDIT: It's actually including ::windows::core::. Wasn't there a request before to use ::windows_core:: here so that the macro can also be used without the windows crate (at the downside that windows+implement users now need both crates)?

Copy link
Collaborator

@ChrisDenton ChrisDenton Dec 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

> cargo clippy -p windows-sys
    Checking windows-sys v0.52.0 (R:\windows-rs\crates\libs\sys)
warning: unused extern crate
  --> crates\libs\sys\src\lib.rs:11:1
   |
11 | extern crate self as windows_sys;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove it
   |
   = note: `-W unused-extern-crates` implied by `-W rust-2018-idioms`
   = help: to override `-W rust-2018-idioms` add `#[allow(unused_extern_crates)]`

warning: `windows-sys` (lib) generated 1 warning (run `cargo clippy --fix --lib -p windows-sys` to apply 1 suggestion)
    Finished dev [unoptimized + debuginfo] target(s) in 2m 49s

> cargo clippy -p windows-sys --features Win32
    Checking windows-sys v0.52.0 (R:\windows-rs\crates\libs\sys)
    Finished dev [unoptimized + debuginfo] target(s) in 1.41s

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the console highlight:

> cargo clippy -p windows-sys
    Checking windows-sys v0.52.0 (R:\windows-rs\crates\libs\sys)
warning: unused extern crate
  --> crates\libs\sys\src\lib.rs:11:1
   |
11 | extern crate self as windows_sys;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove it
   |
   = note: `-W unused-extern-crates` implied by `-W rust-2018-idioms`
   = help: to override `-W rust-2018-idioms` add `#[allow(unused_extern_crates)]`

warning: `windows-sys` (lib) generated 1 warning (run `cargo clippy --fix --lib -p windows-sys` to apply 1 suggestion)
    Finished dev [unoptimized + debuginfo] target(s) in 2m 49s

> cargo clippy -p windows-sys --features Win32
    Checking windows-sys v0.52.0 (R:\windows-rs\crates\libs\sys)
    Finished dev [unoptimized + debuginfo] target(s) in 1.41s

Indeed, it is conditionally used :(

@kennykerr
Copy link
Collaborator Author

On windows I only get a few errors originating from the windows_implement macro, which expects the windows crate to be available as an external crate (which is interesting for a macro that generates code to be used both inside and outside of a crate).
EDIT: It's actually including ::windows::core::. Wasn't there a request before to use ::windows_core:: here so that the macro can also be used without the windows crate (at the downside that windows+implement users now need both crates)?

If there's a simpler solution, I'd love to hear it. I'd like to make the macros depend only on the windows_core crate but that makes it less convenient for what I'd consider is the majority of developers.

@MarijnS95
Copy link
Contributor

If there's a simpler solution, I'd love to hear it. I'd like to make the macros depend only on the windows_core crate but that makes it less convenient for what I'd consider is the majority of developers.

Those two desires conflict, unfortunately. We can either use ::windows_core:: import and drop the extern crate self as windows, which makes the macros depend on only windows_core. Or leave it for what it is.

There is a special crate that is able to process renames in Cargo.toml (https://crates.io/crates/proc-macro-crate), maybe such a technique can be used to understand whether windows, windows_core, or both (and if so, what they might have been renamed to!) are part of the current crate dependency, and switch between ::windows::core and ::windows_core:: based on that?

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.

Deny rust_2018_idioms and unused_qualifications in crate roots
3 participants