Skip to content

Swap most uses of allow in lints to expect #15059

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
Open
alice-i-cecile opened this issue Sep 5, 2024 · 10 comments
Open

Swap most uses of allow in lints to expect #15059

alice-i-cecile opened this issue Sep 5, 2024 · 10 comments
Assignees
@bas-ie
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-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@alice-i-cecile
Copy link
Member

What problem does this solve or what need does it fill?

With the advent of Rust 1.81, we can now distinguish between lints that we haven't gotten around to fixing and those where the lint is wrong.

What solution would you like?

Most localized lints in Bevy should be expect: the only exception that comes to mind is missing_docs.

Leave the Cargo.toml lints set as allow, as funny as it would be to set a crate-level expect(type_complexity).
@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! A-Cross-Cutting Impacts the entire engine D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Sep 5, 2024
@alice-i-cecile
Copy link
Member Author

I'd also like to turn on the new allow_attributes_without_reason lint as part of this.

@BD103
Copy link
Member

BD103 commented Sep 5, 2024

I'll take a stab at this!

@BD103 BD103 mentioned this issue Sep 9, 2024
Closed
34 tasks
@BD103
Copy link
Member

BD103 commented Sep 13, 2024

If someone wants to work on this, I got a lot of progress done in #15118. Feel free to adopt it and follow my advice here for finishing it.

@alice-i-cecile
Copy link
Member Author

My suggestion for those tackling this is to handle this one crate at a time: that will ensure it's easier to complete and review smaller bits of work.

This was referenced Sep 19, 2024
Merged
Closed
github-merge-queue bot pushed a commit that referenced this issue Sep 20, 2024
@bas-ie @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 <[email protected]>
Co-authored-by: Ben Frankel <[email protected]>
Co-authored-by: Antony <[email protected]>
@bas-ie
Copy link
Contributor

bas-ie commented Sep 21, 2024 •
edited
Loading

Copying BD103's instructions/workflow for ease of reference. Also note that crate-level expect/missing_docs should be fixed in an upcoming Rust version.

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.

(Note that the current bug doesn't surface until --all-targets is specified, so usually worth adding that to the workflow. See #15321.)

@bas-ie
Copy link
Contributor

bas-ie commented Sep 21, 2024 •
edited
Loading

Keeping a list here for my own reference: these were not covered by the previous PR.

  • bevy_core_pipeline has crate-level allow, needs crate-level expect later
  • bevy_dev_tools no warnings, already looks pretty covered
  • bevy_dylib already covered
  • bevy_gilrs already covered
  • bevy_gizmos none found
  • bevy_gltf
  • bevy_input no warnings, looks pretty well-covered
  • bevy_internal
  • bevy_pbr
  • bevy_picking
  • bevy_render
  • bevy_sprite
  • bevy_state
  • bevy_text
  • bevy_ui
  • bevy_window
  • bevy_winit

@bas-ie
Copy link
Contributor

bas-ie commented Sep 27, 2024

Just tracking things for myself... it looks like stable Rust 1.83 is 62 days away, so I have a reminder to revisit this end of November when we can rely on missing_docs working as expected.

@bas-ie
Copy link
Contributor

bas-ie commented Nov 29, 2024

Looking into this again...

@bas-ie bas-ie self-assigned this Nov 29, 2024
@bas-ie bas-ie mentioned this issue Nov 30, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Dec 16, 2024
@bas-ie
Rust 1.83, allow -> expect (missing_docs) (#16561)
f2719f5 
# Objective

We were waiting for 1.83 to address most of these, due to a bug with
`missing_docs` and `expect`. Relates to, but does not entirely complete,
#15059.

## Solution

- Upgrade to 1.83
- Switch `allow(missing_docs)` to `expect(missing_docs)`
- Remove a few now-unused `allow`s along the way, or convert to `expect`
ecoskey pushed a commit to ecoskey/bevy that referenced this issue Jan 6, 2025
@bas-ie @ecoskey
Rust 1.83, allow -> expect (missing_docs) (bevyengine#16561)
2503154 
# Objective

We were waiting for 1.83 to address most of these, due to a bug with
`missing_docs` and `expect`. Relates to, but does not entirely complete,
bevyengine#15059.

## Solution

- Upgrade to 1.83
- Switch `allow(missing_docs)` to `expect(missing_docs)`
- Remove a few now-unused `allow`s along the way, or convert to `expect`
mrchantey pushed a commit to mrchantey/bevy that referenced this issue Feb 4, 2025
@bas-ie @mrchantey
Rust 1.83, allow -> expect (missing_docs) (bevyengine#16561)
fb7fc88 
# Objective

We were waiting for 1.83 to address most of these, due to a bug with
`missing_docs` and `expect`. Relates to, but does not entirely complete,
bevyengine#15059.

## Solution

- Upgrade to 1.83
- Switch `allow(missing_docs)` to `expect(missing_docs)`
- Remove a few now-unused `allow`s along the way, or convert to `expect`
@alice-i-cecile
Copy link
Member Author

@bas-ie, are we fully done with this at this point?

@bas-ie
Copy link
Contributor

bas-ie commented Feb 5, 2025

I don't think so. I haven't looked specifically lately, but I think we just got the instances that were in BD's original PR and a few extras. I'll go hunting for more 👀

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

@bas-ie bas-ie

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-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Migrate #[allow(...)] to #[expect(...)] BD103/bevy
3 participants
@bas-ie @alice-i-cecile @BD103