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.

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

Jump to bottom

Fix clippy::needless_lifetimes for most crates #15907

Closed
wants to merge 1 commit into from
Closed

Fix clippy::needless_lifetimes for most crates #15907

wants to merge 1 commit into from

Conversation

clarfonthey
Copy link
Contributor

Part "2" for #15906. This fixes clippy::needless_lifetimes for all crates except bevy_reflect and bevy_ecs, to reduce the amount of code to review.

I say "2" in quotes because all the lint fixes can be merged separately, and don't depend on each other. Although all of them are required to truly make ci lints pass without errors.

@alice-i-cecile alice-i-cecile added A-Cross-Cutting Impacts the entire engine S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Code-Quality A section of code that is hard to understand or change labels Oct 14, 2024
alice-i-cecile
alice-i-cecile reviewed Oct 14, 2024
View reviewed changes
crates/bevy_audio/src/audio_output.rs
@@ -58,7 +58,7 @@ pub struct PlaybackRemoveMarker;
pub(crate) struct EarPositions<'w, 's> {
pub(crate) query: Query<'w, 's, (Entity, &'static GlobalTransform, &'static SpatialListener)>,
}
impl<'w, 's> EarPositions<'w, 's> {
impl EarPositions<'_, '_> {
Copy link
Member

Choose a reason for hiding this comment

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

This is substantially less clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide a reasoning, @alice-i-cecile? I agree with your other ones, but this one seems to me like an improvement to conciseness with no loss in clarity.

alice-i-cecile
alice-i-cecile reviewed Oct 14, 2024
View reviewed changes
crates/bevy_hierarchy/src/hierarchy.rs
@@ -176,7 +176,7 @@ fn despawn_descendants_inner<'v, 'w>(
world
}

impl<'w> DespawnRecursiveExt for EntityWorldMut<'w> {
impl DespawnRecursiveExt for EntityWorldMut<'_> {
Copy link
Member

Choose a reason for hiding this comment

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

This is much less clear. The 'w lifetime has semantic meaning here.

alice-i-cecile
alice-i-cecile reviewed Oct 14, 2024
View reviewed changes
crates/bevy_hierarchy/src/query_extension.rs
@@ -214,7 +214,7 @@ where
}
}

impl<'w, 's, D: QueryData, F: QueryFilter> Iterator for DescendantIter<'w, 's, D, F>
impl<'w, D: QueryData, F: QueryFilter> Iterator for DescendantIter<'w, '_, D, F>
Copy link
Member

Choose a reason for hiding this comment

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

I really dislike this change too.

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 like this change because there is still a lifetime parameter, so it's weird to switch between eliding and not eliding.

alice-i-cecile
alice-i-cecile reviewed Oct 14, 2024
View reviewed changes
crates/bevy_macro_utils/src/symbol.rs
@@ -11,7 +11,7 @@ impl PartialEq<Symbol> for Ident {
}
}

impl<'a> PartialEq<Symbol> for &'a Ident {
impl PartialEq<Symbol> for &Ident {
Copy link
Member

Choose a reason for hiding this comment

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

Good change.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Oct 14, 2024
@alice-i-cecile
Copy link
Member

After, looking at the 3 PRs in this group, I don't think we should do this. These are a serious downgrade in some cases (actual named lifetimes + 'w/'s), neutral in most cases and positive in a very small fraction of cases. There's important information being lost here in favor of a meaningless anonymous lifetime.

I'd prefer to close these PRs and blanket ignore the lint if it gets stabilized. I'll file an issue on the clippy repo too.

@alice-i-cecile alice-i-cecile mentioned this pull request Oct 14, 2024
Open
BenjaminBrienen
BenjaminBrienen approved these changes Oct 14, 2024
View reviewed changes
crates/bevy_hierarchy/src/query_extension.rs
@@ -214,7 +214,7 @@ where
}
}

impl<'w, 's, D: QueryData, F: QueryFilter> Iterator for DescendantIter<'w, 's, D, F>
impl<'w, D: QueryData, F: QueryFilter> Iterator for DescendantIter<'w, '_, D, F>
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 like this change because there is still a lifetime parameter, so it's weird to switch between eliding and not eliding.

crates/bevy_hierarchy/src/query_extension.rs
@@ -259,7 +259,7 @@ where
}
}

impl<'w, 's, D: QueryData, F: QueryFilter> Iterator for DescendantDepthFirstIter<'w, 's, D, F>
impl<'w, D: QueryData, F: QueryFilter> Iterator for DescendantDepthFirstIter<'w, '_, D, F>
Copy link
Contributor

Choose a reason for hiding this comment

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

.

crates/bevy_hierarchy/src/query_extension.rs
@@ -298,7 +298,7 @@ where
}
}

impl<'w, 's, D: QueryData, F: QueryFilter> Iterator for AncestorIter<'w, 's, D, F>
impl<'w, D: QueryData, F: QueryFilter> Iterator for AncestorIter<'w, '_, D, F>
Copy link
Contributor

Choose a reason for hiding this comment

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

.

crates/bevy_tasks/src/task_pool.rs
@@ -602,7 +602,7 @@ pub struct Scope<'scope, 'env: 'scope, T> {
env: PhantomData<&'env mut &'env ()>,
}

impl<'scope, 'env, T: Send + 'scope> Scope<'scope, 'env, T> {
impl<'scope, T: Send + 'scope> Scope<'scope, '_, T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

.

crates/bevy_tasks/src/task_pool.rs
@@ -655,7 +655,7 @@ impl<'scope, 'env, T: Send + 'scope> Scope<'scope, 'env, T> {
}
}

impl<'scope, 'env, T> Drop for Scope<'scope, 'env, T>
impl<'scope, T> Drop for Scope<'scope, '_, T>
Copy link
Contributor

Choose a reason for hiding this comment

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

.

crates/bevy_ui/src/ghost_hierarchy.rs
@@ -30,7 +30,7 @@ pub struct UiRootNodes<'w, 's> {
ui_children: UiChildren<'w, 's>,
}

impl<'w, 's> UiRootNodes<'w, 's> {
impl<'s> UiRootNodes<'_, 's> {
Copy link
Contributor

Choose a reason for hiding this comment

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

.

@BenjaminBrienen
Copy link
Contributor

I'm going to checkout this commit and play with the code a bit to see if I can do better than clippy at eliding the remaining lifetime parameters. Probably not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile left review comments

@BenjaminBrienen BenjaminBrienen BenjaminBrienen approved these changes

Assignees
No one assigned
Labels
A-Cross-Cutting Impacts the entire engine C-Code-Quality A section of code that is hard to understand or change S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@clarfonthey @alice-i-cecile @BenjaminBrienen