Skip to content

Apply unused_qualifications lint #14828

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

Merged
merged 2 commits into from
Aug 21, 2024
Merged

Apply unused_qualifications lint #14828

merged 2 commits into from
Aug 21, 2024

Conversation

EdJoPaTo
Copy link
Contributor

@EdJoPaTo EdJoPaTo commented Aug 19, 2024

Objective

Fixes #14782

Solution

Enable the lint and fix all upcoming hints (--fix). Also tried to figure out the false-positive (see review comment). Maybe split this PR up into multiple parts where only the last one enables the lint, so some can already be merged resulting in less many files touched / less potential for merge conflicts?

Currently, there are some cases where it might be easier to read the code with the qualifier, so perhaps remove the import of it and adapt its cases? In the current stage it's just a plain adoption of the suggestions in order to have a base to discuss.

Testing

cargo clippy and cargo run -p ci are happy.

Comment on lines +1 to +3
// Temporary workaround for impl_reflect!(Option/Result false-positive
#![allow(unused_qualifications)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabling the lint marks the following as to be improved but applying the suggestion (like --fix does) doesn't work. Also, I don't know how to ignore the lint in the proc_macro without #![allow for the whole module.

impl_reflect! {
#[type_path = "core::option"]
enum Option<T> {
None,
Some(T),
}
}
impl_reflect! {
#[type_path = "core::result"]
enum Result<T, E> {
Ok(T),
Err(E),
}
}

I neither don't fully understand the actual root cause of the error (as I don't really know what the proc_macro is doing here, not familiar with the code) nor know where to report this false-positive correctly to the Rust devs. As it's not a clippy lint not the clippy repo. Maybe directly in the rust repo as it's a false-positive there?

How should we proceed with this?

Copy link
Member

@alice-i-cecile alice-i-cecile Aug 19, 2024

Choose a reason for hiding this comment

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

I'm okay proceeding with this workaround, but we should open an issue for it (and link the issue in the source). Others may know better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree, there should be an issue, and it should be linked in the source next to the workaround.

For opening an issue there should be a somewhat minimal reproducible example. And I seem to understand too little to reduce this issue in some ways.
At the current state I don't feel confident in opening an issue about something when I can't even create a more minimal example of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect its related: rust-lang/rust#122519

One way of handling this might be to use explicit paths (::core instead of core) but I'm not sure how to do that here as the macro should handle that. Another suggestion might be to add the #[allow into the proc_macro generated code, but I don't know where to add that best. So I can't really test if it's really related to the linked issue.

Copy link
Contributor

Your PR increases Bevy Minimum Supported Rust Version. Please update the rust-version field in the root Cargo.toml file.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Very nice diff :) I like it!

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-Cross-Cutting Impacts the entire engine X-Contentious There are nontrivial implications that should be thought through S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 19, 2024
Copy link
Member

@TrialDragon TrialDragon left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just one issue with a, seeming, false positive.

@@ -599,7 +599,7 @@ trait DebugEnsureAligned {
impl<T: Sized> DebugEnsureAligned for *mut T {
#[track_caller]
fn debug_ensure_aligned(self) -> Self {
let align = core::mem::align_of::<T>();
let align = align_of::<T>();
Copy link
Member

@TrialDragon TrialDragon Aug 19, 2024

Choose a reason for hiding this comment

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

This one is a false positive I think; core::mem isn't imported here as far as I know, so this needs qualification, or to be pulled in in the use statements. It's why the CI failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The question is… why? Not only the std prelude, also the core prelude include align_of per default and the stable (non-MSRV) checks do not notice this. What's different between MSRV and stable? I highly doubt that the prelude changed between 1.79 which is the current bevy MSRV and 1.80 which is the current stable, so the error must be somewhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, 1.80 adds align_of to the prelude. I wouldn't have expected such a change and expected it to be tied to an edition. Which isn't the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added them explicitly to the use statements in 40237e1 (and plan on open an issue about this in the rust repo). Interestingly unused_imports (warn by default) doesn't seem to care about this on 1.80.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TrialDragon TrialDragon added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 20, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 21, 2024
Merged via the queue into bevyengine:main with commit 938d810 Aug 21, 2024
31 checks passed
@EdJoPaTo EdJoPaTo deleted the unused_qualifications branch August 21, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Cross-Cutting Impacts the entire engine C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable unused_qualifications lint at the workspace level
3 participants