Skip to content

Commit

Permalink
bevy_ecs: Apply `#![warn(clippy::allow_attributes, clippy::allow_attr…
Browse files Browse the repository at this point in the history
…ibutes_without_reason)]` (#17335)

# Objective
- #17111

## Solution
Set the `clippy::allow_attributes` and
`clippy::allow_attributes_without_reason` lints to `warn`, and bring
`bevy_ecs` in line with the new restrictions.

## Testing
This PR is a WIP; testing will happen after it's finished.
  • Loading branch information
LikeLakers2 authored Jan 14, 2025
1 parent 706cdd5 commit 17c46f4
Show file tree
Hide file tree
Showing 36 changed files with 492 additions and 222 deletions.
10 changes: 8 additions & 2 deletions crates/bevy_ecs/src/archetype.rs
Original file line number Diff line number Diff line change
Expand Up @@ -789,7 +789,10 @@ pub struct Archetypes {
pub struct ArchetypeRecord {
/// Index of the component in the archetype's [`Table`](crate::storage::Table),
/// or None if the component is a sparse set component.
#[allow(dead_code)]
#[expect(
dead_code,
reason = "Currently unused, but planned to be used to implement a component index to improve performance of fragmenting relations."
)]
pub(crate) column: Option<usize>,
}

Expand Down Expand Up @@ -827,7 +830,10 @@ impl Archetypes {

/// Fetches the total number of [`Archetype`]s within the world.
#[inline]
#[allow(clippy::len_without_is_empty)] // the internal vec is never empty.
#[expect(
clippy::len_without_is_empty,
reason = "The internal vec is never empty"
)]
pub fn len(&self) -> usize {
self.archetypes.len()
}
Expand Down
45 changes: 34 additions & 11 deletions crates/bevy_ecs/src/bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,15 @@ impl<C: Component> DynamicBundle for C {

macro_rules! tuple_impl {
($(#[$meta:meta])* $($name: ident),*) => {
#[expect(
clippy::allow_attributes,
reason = "This is a tuple-related macro; as such, the lints below may not always apply."
)]
#[allow(
unused_mut,
unused_variables,
reason = "Zero-length tuples won't use any of the parameters."
)]
$(#[$meta])*
// SAFETY:
// - `Bundle::component_ids` calls `ids` for each component type in the
Expand All @@ -254,43 +263,57 @@ macro_rules! tuple_impl {
// - `Bundle::get_components` is called exactly once for each member. Relies on the above implementation to pass the correct
// `StorageType` into the callback.
unsafe impl<$($name: Bundle),*> Bundle for ($($name,)*) {
#[allow(unused_variables)]
fn component_ids(components: &mut Components, storages: &mut Storages, ids: &mut impl FnMut(ComponentId)){
$(<$name as Bundle>::component_ids(components, storages, ids);)*
}

#[allow(unused_variables)]
fn get_component_ids(components: &Components, ids: &mut impl FnMut(Option<ComponentId>)){
$(<$name as Bundle>::get_component_ids(components, ids);)*
}

#[allow(unused_variables, unused_mut)]
#[allow(clippy::unused_unit)]
#[allow(
clippy::unused_unit,
reason = "Zero-length tuples will generate a function body equivalent to `()`; however, this macro is meant for all applicable tuples, and as such it makes no sense to rewrite it just for that case."
)]
unsafe fn from_components<T, F>(ctx: &mut T, func: &mut F) -> Self
where
F: FnMut(&mut T) -> OwningPtr<'_>
{
#[allow(unused_unsafe)]
#[allow(
unused_unsafe,
reason = "Zero-length tuples will not run anything in the unsafe block. Additionally, rewriting this to move the () outside of the unsafe would require putting the safety comment inside the tuple, hurting readability of the code."
)]
// SAFETY: Rust guarantees that tuple calls are evaluated 'left to right'.
// https://doc.rust-lang.org/reference/expressions.html#evaluation-order-of-operands
unsafe { ($(<$name as Bundle>::from_components(ctx, func),)*) }
}

fn register_required_components(
_components: &mut Components,
_storages: &mut Storages,
_required_components: &mut RequiredComponents,
components: &mut Components,
storages: &mut Storages,
required_components: &mut RequiredComponents,
) {
$(<$name as Bundle>::register_required_components(_components, _storages, _required_components);)*
$(<$name as Bundle>::register_required_components(components, storages, required_components);)*
}
}

#[expect(
clippy::allow_attributes,
reason = "This is a tuple-related macro; as such, the lints below may not always apply."
)]
#[allow(
unused_mut,
unused_variables,
reason = "Zero-length tuples won't use any of the parameters."
)]
$(#[$meta])*
impl<$($name: Bundle),*> DynamicBundle for ($($name,)*) {
#[allow(unused_variables, unused_mut)]
#[inline(always)]
fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>)) {
#[allow(non_snake_case)]
#[allow(
non_snake_case,
reason = "The names of these variables are provided by the caller, not by us."
)]
let ($(mut $name,)*) = self;
$(
$name.get_components(&mut *func);
Expand Down
5 changes: 4 additions & 1 deletion crates/bevy_ecs/src/change_detection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,10 @@ impl<'w, T: Resource> Res<'w, T> {
///
/// Note that unless you actually need an instance of `Res<T>`, you should
/// prefer to just convert it to `&T` which can be freely copied.
#[allow(clippy::should_implement_trait)]
#[expect(
clippy::should_implement_trait,
reason = "As this struct derefs to the inner resource, a `Clone` trait implementation would interfere with the common case of cloning the inner content. (A similar case of this happening can be found with `std::cell::Ref::clone()`.)"
)]
pub fn clone(this: &Self) -> Self {
Self {
value: this.value,
Expand Down
11 changes: 5 additions & 6 deletions crates/bevy_ecs/src/entity/entity_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -382,25 +382,24 @@ impl<I: Iterator<Item: TrustedEntityBorrow> + Debug> Debug for UniqueEntityIter<
mod tests {
use alloc::{vec, vec::Vec};

#[allow(unused_imports)]
use crate::prelude::{Schedule, World};

#[allow(unused_imports)]
use crate::component::Component;
use crate::entity::Entity;
use crate::query::{QueryState, With};
use crate::system::Query;
use crate::world::Mut;
#[allow(unused_imports)]
use crate::{self as bevy_ecs};
#[allow(unused_imports)]
use crate::{entity::Entity, world::unsafe_world_cell};

use super::UniqueEntityIter;

#[derive(Component, Clone)]
pub struct Thing;

#[allow(clippy::iter_skip_zero)]
#[expect(
clippy::iter_skip_zero,
reason = "The `skip(0)` is used to ensure that the `Skip` iterator implements `EntitySet`, which is needed to pass the iterator as the `entities` parameter."
)]
#[test]
fn preserving_uniqueness() {
let mut world = World::new();
Expand Down
23 changes: 20 additions & 3 deletions crates/bevy_ecs/src/entity/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -592,7 +592,14 @@ impl Entities {
/// Reserve entity IDs concurrently.
///
/// Storage for entity generation and location is lazily allocated by calling [`flush`](Entities::flush).
#[allow(clippy::unnecessary_fallible_conversions)] // Because `IdCursor::try_from` may fail on 32-bit platforms.
#[expect(
clippy::allow_attributes,
reason = "`clippy::unnecessary_fallible_conversions` may not always lint."
)]
#[allow(
clippy::unnecessary_fallible_conversions,
reason = "`IdCursor::try_from` may fail on 32-bit platforms."
)]
pub fn reserve_entities(&self, count: u32) -> ReserveEntitiesIterator {
// Use one atomic subtract to grab a range of new IDs. The range might be
// entirely nonnegative, meaning all IDs come from the freelist, or entirely
Expand Down Expand Up @@ -786,7 +793,14 @@ impl Entities {
}

/// Ensure at least `n` allocations can succeed without reallocating.
#[allow(clippy::unnecessary_fallible_conversions)] // Because `IdCursor::try_from` may fail on 32-bit platforms.
#[expect(
clippy::allow_attributes,
reason = "`clippy::unnecessary_fallible_conversions` may not always lint."
)]
#[allow(
clippy::unnecessary_fallible_conversions,
reason = "`IdCursor::try_from` may fail on 32-bit platforms."
)]
pub fn reserve(&mut self, additional: u32) {
self.verify_flushed();

Expand Down Expand Up @@ -1178,7 +1192,10 @@ mod tests {
}

#[test]
#[allow(clippy::nonminimal_bool)] // This is intentionally testing `lt` and `ge` as separate functions.
#[expect(
clippy::nonminimal_bool,
reason = "This intentionally tests all possible comparison operators as separate functions; thus, we don't want to rewrite these comparisons to use different operators."
)]
fn entity_comparison() {
assert_eq!(
Entity::from_raw_and_generation(123, NonZero::<u32>::new(456).unwrap()),
Expand Down
1 change: 0 additions & 1 deletion crates/bevy_ecs/src/entity/visit_entities.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ mod tests {
ordered: Vec<Entity>,
unordered: HashSet<Entity>,
single: Entity,
#[allow(dead_code)]
#[visit_entities(ignore)]
not_an_entity: String,
}
Expand Down
1 change: 0 additions & 1 deletion crates/bevy_ecs/src/event/event_cursor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ impl<E: Event> Clone for EventCursor<E> {
}
}

#[allow(clippy::len_without_is_empty)] // Check fails since the is_empty implementation has a signature other than `(&self) -> bool`
impl<E: Event> EventCursor<E> {
/// See [`EventReader::read`](super::EventReader::read)
pub fn read<'a>(&'a mut self, events: &'a Events<E>) -> EventIterator<'a, E> {
Expand Down
2 changes: 0 additions & 2 deletions crates/bevy_ecs/src/event/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,6 @@ mod tests {
assert!(last.is_none(), "EventMutator should be empty");
}

#[allow(clippy::iter_nth_zero)]
#[test]
fn test_event_reader_iter_nth() {
use bevy_ecs::prelude::*;
Expand All @@ -595,7 +594,6 @@ mod tests {
schedule.run(&mut world);
}

#[allow(clippy::iter_nth_zero)]
#[test]
fn test_event_mutator_iter_nth() {
use bevy_ecs::prelude::*;
Expand Down
5 changes: 4 additions & 1 deletion crates/bevy_ecs/src/identifier/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,10 @@ mod tests {

#[rustfmt::skip]
#[test]
#[allow(clippy::nonminimal_bool)] // This is intentionally testing `lt` and `ge` as separate functions.
#[expect(
clippy::nonminimal_bool,
reason = "This intentionally tests all possible comparison operators as separate functions; thus, we don't want to rewrite these comparisons to use different operators."
)]
fn id_comparison() {
assert!(Identifier::new(123, 456, IdKind::Entity).unwrap() == Identifier::new(123, 456, IdKind::Entity).unwrap());
assert!(Identifier::new(123, 456, IdKind::Placeholder).unwrap() == Identifier::new(123, 456, IdKind::Placeholder).unwrap());
Expand Down
61 changes: 42 additions & 19 deletions crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,14 @@
// FIXME(11590): remove this once the lint is fixed
#![allow(unsafe_op_in_unsafe_fn)]
// TODO: remove once Edition 2024 is released
#![allow(dependency_on_unit_never_type_fallback)]
#![expect(
unsafe_op_in_unsafe_fn,
reason = "See #11590. To be removed once all applicable unsafe code has an unsafe block with a safety comment."
)]
#![cfg_attr(
test,
expect(
dependency_on_unit_never_type_fallback,
reason = "See #17340. To be removed once Edition 2024 is released"
)
)]
#![doc = include_str!("../README.md")]
#![cfg_attr(
any(docsrs, docsrs_dep),
Expand All @@ -11,7 +18,12 @@
)
)]
#![cfg_attr(any(docsrs, docsrs_dep), feature(doc_auto_cfg, rustdoc_internals))]
#![allow(unsafe_code)]
#![expect(unsafe_code, reason = "Unsafe code is used to improve performance.")]
#![warn(
clippy::allow_attributes,
clippy::allow_attributes_without_reason,
reason = "See #17111; To be removed once all crates are in-line with these attributes"
)]
#![doc(
html_logo_url = "https://bevyengine.org/assets/icon.png",
html_favicon_url = "https://bevyengine.org/assets/icon.png"
Expand Down Expand Up @@ -55,7 +67,10 @@ pub use bevy_ptr as ptr;
///
/// This includes the most common types in this crate, re-exported for your convenience.
pub mod prelude {
#[expect(deprecated)]
#[expect(
deprecated,
reason = "`crate::schedule::apply_deferred` is considered deprecated; however, it may still be used by crates which consume `bevy_ecs`, so its removal here may cause confusion. It is intended to be removed in the Bevy 0.17 cycle."
)]
#[doc(hidden)]
pub use crate::{
bundle::Bundle,
Expand Down Expand Up @@ -147,9 +162,8 @@ mod tests {
#[derive(Component, Debug, PartialEq, Eq, Clone, Copy)]
struct C;

#[allow(dead_code)]
#[derive(Default)]
struct NonSendA(usize, PhantomData<*mut ()>);
struct NonSendA(PhantomData<*mut ()>);

#[derive(Component, Clone, Debug)]
struct DropCk(Arc<AtomicUsize>);
Expand All @@ -166,8 +180,10 @@ mod tests {
}
}

// TODO: The compiler says the Debug and Clone are removed during dead code analysis. Investigate.
#[allow(dead_code)]
#[expect(
dead_code,
reason = "This struct is used to test how `Drop` behavior works in regards to SparseSet storage, and as such is solely a wrapper around `DropCk` to make it use the SparseSet storage. Because of this, the inner field is intentionally never read."
)]
#[derive(Component, Clone, Debug)]
#[component(storage = "SparseSet")]
struct DropCkSparse(DropCk);
Expand Down Expand Up @@ -2641,42 +2657,49 @@ mod tests {
World::new().register_component::<A>();
}

// These structs are primarily compilation tests to test the derive macros. Because they are
// never constructed, we have to manually silence the `dead_code` lint.
#[allow(dead_code)]
#[expect(
dead_code,
reason = "This struct is used as a compilation test to test the derive macros, and as such is intentionally never constructed."
)]
#[derive(Component)]
struct ComponentA(u32);

#[allow(dead_code)]
#[expect(
dead_code,
reason = "This struct is used as a compilation test to test the derive macros, and as such is intentionally never constructed."
)]
#[derive(Component)]
struct ComponentB(u32);

#[allow(dead_code)]
#[derive(Bundle)]
struct Simple(ComponentA);

#[allow(dead_code)]
#[derive(Bundle)]
struct Tuple(Simple, ComponentB);

#[allow(dead_code)]
#[derive(Bundle)]
struct Record {
field0: Simple,
field1: ComponentB,
}

#[allow(dead_code)]
#[derive(Component, VisitEntities, VisitEntitiesMut)]
struct MyEntities {
entities: Vec<Entity>,
another_one: Entity,
maybe_entity: Option<Entity>,
#[expect(
dead_code,
reason = "This struct is used as a compilation test to test the derive macros, and as such this field is intentionally never used."
)]
#[visit_entities(ignore)]
something_else: String,
}

#[allow(dead_code)]
#[expect(
dead_code,
reason = "This struct is used as a compilation test to test the derive macros, and as such is intentionally never constructed."
)]
#[derive(Component, VisitEntities, VisitEntitiesMut)]
struct MyEntitiesTuple(Vec<Entity>, Entity, #[visit_entities(ignore)] usize);
}
Loading

0 comments on commit 17c46f4

Please sign in to comment.