Skip to content

bevy_asset: Apply #![deny(clippy::allow_attributes, clippy::allow_attributes_without_reason)] #17113

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.

Sign up for GitHub

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 3 commits into from
Jan 6, 2025
Merged

bevy_asset: Apply #![deny(clippy::allow_attributes, clippy::allow_attributes_without_reason)] #17113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7d3aa4b
Apply `#[deny(clippy::allow_attributes, clippy::allow_attributes_with…
LikeLakers2 Jan 3, 2025
d6d99aa
Update the deny to link to the tracking issue
LikeLakers2 Jan 3, 2025
53bc757
Remove outdated comment
LikeLakers2 Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions crates/bevy_asset/src/asset_changed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ pub struct AssetChangedState<A: AsAssetId> {
_asset: PhantomData<fn(A)>,
}

#[allow(unsafe_code)]
#[expect(unsafe_code, reason = "WorldQuery is an unsafe trait.")]
/// SAFETY: `ROQueryFetch<Self>` is the same as `QueryFetch<Self>`
unsafe impl<A: AsAssetId> WorldQuery for AssetChanged<A> {
type Item<'w> = ();
Expand Down Expand Up @@ -269,7 +269,7 @@ unsafe impl<A: AsAssetId> WorldQuery for AssetChanged<A> {
}
}

#[allow(unsafe_code)]
#[expect(unsafe_code, reason = "QueryFilter is an unsafe trait.")]
/// SAFETY: read-only access
unsafe impl<A: AsAssetId> QueryFilter for AssetChanged<A> {
const IS_ARCHETYPAL: bool = false;
Expand Down
5 changes: 4 additions & 1 deletion crates/bevy_asset/src/handle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,8 +551,11 @@ mod tests {
}

/// Typed and Untyped `Handles` should be orderable amongst each other and themselves
#[allow(clippy::cmp_owned)]
#[test]
#[expect(
clippy::cmp_owned,
reason = "This lints on the assertion that a typed handle converted to an untyped handle maintains its ordering compared to an untyped handle. While the conversion would normally be useless, we need to ensure that converted handles maintain their ordering, making the conversion necessary here."
)]
fn ordering() {
assert!(UUID_1 < UUID_2);

Expand Down
12 changes: 9 additions & 3 deletions crates/bevy_asset/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,11 @@
//! This trait mirrors [`AssetLoader`] in structure, and works in tandem with [`AssetWriter`](io::AssetWriter), which mirrors [`AssetReader`](io::AssetReader).

#![expect(missing_docs, reason = "Not all docs are written yet, see #3492.")]
#![deny(
clippy::allow_attributes,
clippy::allow_attributes_without_reason,
reason = "See #17111; To be removed once all crates are in-line with these attributes"
)]
#![cfg_attr(docsrs, feature(doc_auto_cfg))]
#![doc(
html_logo_url = "https://bevyengine.org/assets/icon.png",
Expand Down Expand Up @@ -1767,8 +1772,11 @@ mod tests {
#[derive(Asset, TypePath)]
pub struct TestAsset;

#[allow(dead_code)]
#[derive(Asset, TypePath)]
#[expect(
dead_code,
reason = "This exists to ensure that `#[derive(Asset)]` works on enums. The inner variants are known not to be used."
)]
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 have no idea why this only lints on the enum test case, despite the struct test cases also being unused.

pub enum EnumTestAsset {
Unnamed(#[dependency] Handle<TestAsset>),
Named {
Expand All @@ -1783,7 +1791,6 @@ mod tests {
Empty,
}

#[allow(dead_code)]
#[derive(Asset, TypePath)]
pub struct StructTestAsset {
#[dependency]
Expand All @@ -1792,7 +1799,6 @@ mod tests {
embedded: TestAsset,
}

#[allow(dead_code)]
#[derive(Asset, TypePath)]
pub struct TupleTestAsset(#[dependency] Handle<TestAsset>);
}
1 change: 0 additions & 1 deletion crates/bevy_asset/src/processor/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,6 @@ impl AssetProcessor {
self.set_state(ProcessorState::Finished).await;
}

#[allow(unused)]
#[cfg(all(not(target_arch = "wasm32"), feature = "multi_threaded"))]
async fn process_assets_internal<'scope>(
&'scope self,
Expand Down
10 changes: 3 additions & 7 deletions crates/bevy_asset/src/server/loaders.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,18 +342,14 @@ mod tests {

use super::*;

// The compiler notices these fields are never read and raises a dead_code lint which kill CI.
#[allow(dead_code)]
#[derive(Asset, TypePath, Debug)]
struct A(usize);
struct A;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case you're wondering: I checked and double-checked to make sure that removing these usize fields wouldn't affect tests.

If they affect tests, I didn't see it during my testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the tests are meant to test specifically unit struct assets. Unit struct assets don't sound very useful anyway.

Copy link
Contributor Author

@LikeLakers2 LikeLakers2 Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, but... The tests compile and finish successfully either way.

Really, the (usize) only confuses people into thinking that we're somehow constructing instances of the structs. Except, we're not - the structs are only used as type parameters.

If we start needing a value, they can be added back.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's true that as far as I can see there seems to be no behavior in the tests hinging on the structs having any fields. However, idk if they're relevant for TypeId calculations.
It would be best to ask someone who wrote this or knows about reflection.

Copy link
Contributor Author

@LikeLakers2 LikeLakers2 Jan 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@IQuick143 I don't believe they're relevant for TypeId calculations: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=99bbd15c51d685da51caa2a4beecf51b

I mean, it appears that changing one to include (usize) does change its TypeId - but two unit structs still have two different TypeIds.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, I think this fix is good :)


#[allow(dead_code)]
#[derive(Asset, TypePath, Debug)]
struct B(usize);
struct B;

#[allow(dead_code)]
#[derive(Asset, TypePath, Debug)]
struct C(usize);
struct C;

struct Loader<A: Asset, const N: usize, const E: usize> {
sender: Sender<()>,
Expand Down
Loading