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

Migrate #[allow(...)] to #[expect(...)] #15118

Closed
wants to merge 22 commits into from
Closed

Migrate #[allow(...)] to #[expect(...)] #15118

wants to merge 22 commits into from

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Sep 9, 2024 •
edited
Loading

Objective

  • Rust 1.81 released the #[expect(...)] attribute, which works like #[allow(...)] but throws a warning if the lint isn't raised. This is preferred to #[allow(...)] because it tells us when it can be removed.
  • Closes Swap most uses of allow in lints to expect #15059.

Solution

Switch all applicable #[allow(...)] attributes to #[expect(...)] with a reason attribute.

Progress

  • bevy_ptr
  • bevy_utils
  • bevy_reflect
  • bevy_tasks
  • bevy_ecs
  • bevy_derive
  • bevy_app
  • bevy_reflect_derive
  • bevy_asset_macros
  • bevy_encase_derive
  • bevy_render_macros
  • bevy_state_macros
  • bevy_log
  • bevy_time
  • bevy_diagnostic
  • bevy_transform
  • bevy_asset
  • bevy_input
  • bevy_gilrs
  • bevy_window
  • bevy_winit
  • bevy_state
  • bevy_render
  • bevy_picking
  • bevy_core_pipeline
  • bevy_sprite
  • bevy_text
  • bevy_pbr
  • bevy_ui
  • bevy_gltf
  • bevy_gizmos
  • bevy_dev_tools
  • bevy_internal
  • bevy_dylib

Testing

  • cargo clippy --workspace --all-features

@BD103 BD103 added C-Code-Quality A section of code that is hard to understand or change A-Cross-Cutting Impacts the entire engine X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 9, 2024
MrGVSV
MrGVSV reviewed Sep 9, 2024
View reviewed changes
crates/bevy_reflect/derive/src/derive_data.rs
pub attrs: FieldAttributes,
/// The index of this variant within the enum.
#[allow(dead_code)]
#[expect(dead_code, reason = "TODO(MrGVSV): Why should this be kept?")]
Copy link
Member

Choose a reason for hiding this comment

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

It probably doesn't need to be kept anymore 😅

crates/bevy_reflect/derive/src/reflect_value.rs Outdated Show resolved Hide resolved
benfrankel
benfrankel reviewed Sep 10, 2024
View reviewed changes
crates/bevy_reflect/derive/src/derive_data.rs Outdated
@@ -922,8 +920,7 @@ pub(crate) enum ReflectTypePath<'a> {
generics: &'a Generics,
},
/// Any [`Type`] with only a defined `type_path` and `short_type_path`.
#[allow(dead_code)]
// Not currently used but may be useful in the future due to its generality.
#[expect(dead_code, reason = "Not current used but may be useful in the future due to its generality.")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[expect(dead_code, reason = "Not current used but may be useful in the future due to its generality.")]
#[expect(dead_code, reason = "Not currently used but may be useful in the future due to its generality.")]

crates/bevy_app/src/plugin.rs Outdated Show resolved Hide resolved
@BD103 BD103 added S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 13, 2024
@BD103
Copy link
Member Author

BD103 commented Sep 13, 2024

I'm going to put this PR up for adoption. I no longer have the energy to work on it, and there are still a lot of crates left. If you want to adopt this, my workflow looked like this:

  1. Run cargo clippy --workspace --message-format=short
  2. cd into the first crate that raises warnings.
  3. Run cargo watch -c -x clippy until warnings stop appearing.
  4. Run cargo clippy --no-default-features and cargo clippy --all-features for good measure.
  5. Repeat step 1 until there are no more warning.

@BD103 BD103 mentioned this pull request Sep 13, 2024
Open
@alice-i-cecile alice-i-cecile mentioned this pull request Sep 17, 2024
Merged
@richchurcher richchurcher mentioned this pull request Sep 19, 2024
Merged
@richchurcher
Copy link
Contributor

@BD103 I don't know if I can do the rest of this justice, but I figured I'd at least try to clean up the conflicts, it'd be a crime to see all that go to waste 😆 #15301

@BD103 BD103 deleted the allow-to-expect branch September 19, 2024 14:07
github-merge-queue bot pushed a commit that referenced this pull request Sep 20, 2024
@richchurcher @BD103
@benfrankel @chompaa
Allow to expect (adopted) (#15301)
fd329c0 
# Objective

> Rust 1.81 released the #[expect(...)] attribute, which works like
#[allow(...)] but throws a warning if the lint isn't raised. This is
preferred to #[allow(...)] because it tells us when it can be removed.

- Adopts the parts of #15118 that are complete, and updates the branch
so it can be merged.
- There were a few conflicts, let me know if I misjudged any of 'em.

Alice's
[recommendation](#15059 (comment))
seems well-taken, let's do this crate by crate now that @BD103 has done
the lion's share of this!

(Relates to, but doesn't yet completely finish #15059.)

Crates this _doesn't_ cover:

- bevy_input
- bevy_gilrs
- bevy_window
- bevy_winit
- bevy_state
- bevy_render
- bevy_picking
- bevy_core_pipeline
- bevy_sprite
- bevy_text
- bevy_pbr
- bevy_ui
- bevy_gltf
- bevy_gizmos
- bevy_dev_tools
- bevy_internal
- bevy_dylib

---------

Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com>
Co-authored-by: Ben Frankel <ben.frankel7@gmail.com>
Co-authored-by: Antony <antony.m.3012@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benfrankel benfrankel benfrankel left review comments

@MrGVSV MrGVSV MrGVSV left review comments

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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! X-Uncontroversial This work is generally agreed upon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Swap most uses of allow in lints to expect
5 participants
@BD103 @richchurcher @benfrankel @MrGVSV @alice-i-cecile