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.

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

Warn the clippy::allow_attributes and clippy::allow_attributes_without_reason lints #17111

Closed
1 of 2 tasks
LikeLakers2 opened this issue Jan 3, 2025 · 1 comment · Fixed by #17374
Closed
1 of 2 tasks
Labels
A-Cross-Cutting Impacts the entire engine C-Code-Quality A section of code that is hard to understand or change C-Tracking-Issue An issue that collects information about a broad development initiative 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

@LikeLakers2
Copy link
Contributor

LikeLakers2 commented Jan 3, 2025

This is a tracking issue for warning on the clippy::allow_attributes and clippy::allow_attributes_without_reason lints across all bevy crates.

What are these?

clippy::allow_attributes

This lints against any instances of #[allow(...)], suggesting they be changed to #[expect(...)].

The use case for this is to avoid letting #[allow(...)]s pile up within Bevy's database, especially since #[expect(...)] is now available. By using #![warn(clippy::allow_attributes)], we can remove any unnecessary instances of #[allow(...)].

Do note that it's not possible to forbid this lint - there are instances where we are forced to #[allow(...)], as the lints may not always be triggered (such as in macros for tuples).

Notably, this does not lint against #![allow(...)] (notice the !), per the lint's description:

This lint only warns outer attributes (#[allow]), as inner attributes (#![allow]) are usually used to enable or disable lints on a global scale.

However, I will make an attempt to look for instances of #![allow(...)] anyways... if I remember.

See also:
https://rust-lang.github.io/rust-clippy/master/index.html#allow_attributes

clippy::allow_attributes_without_reason

This lints against any instances of #[allow(...)] or #[expect(...)] that do not include a reason = "..." field.

This ensures that if an expect or allow is used, it is given a reason - which helps to document why it's there in the first place.

Notably, this does not lint against #[warn(...)] or #[deny(...)].

See also:
https://rust-lang.github.io/rust-clippy/master/index.html#allow_attributes_without_reason

Checklist

PRs marked as drafts in the list below mean that something (i.e. another PR) blocking them. Once the blocking PRs are merged, each applicable PR will be updated to fix any conflicts, and marked as ready to merge.

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change C-Tracking-Issue An issue that collects information about a broad development initiative 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 Jan 3, 2025
github-merge-queue bot pushed a commit that referenced this issue Jan 5, 2025
…tributes_without_reason)]` (#17090)

# Objective
- #17111

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

No code changes have been made - except if a lint that was previously
`allow(...)`'d could be removed via small code changes. For example,
`unused_variables` can be handled by adding a `_` to the beginning of a
field's name.

## Testing
I ran `cargo clippy`, and received no errors.
github-merge-queue bot pushed a commit that referenced this issue Jan 5, 2025
…w_attributes_without_reason)]` (#17159)

# Objective
- #17111

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

No code changes have been made - except if a lint that was previously
`allow(...)`'d could be removed via small code changes. For example,
`unused_variables` can be handled by adding a `_` to the beginning of a
field's name.

## Testing
`cargo clippy`, `cargo clippy --package bevy_dev_tools` and cargo test
--package bevy_dev_tools` were run, and no errors were encountered.
(Except for one warning from bevy_sprite, but I plan to fix that when I
get to bevy_sprite)
github-merge-queue bot pushed a commit that referenced this issue Jan 6, 2025
…y::allow_attributes_without_reason)]` attributes to include a reason field pointing to the tracking issue (#17136)

# Objective
Ensure the deny lint attributes added as a result of #17111 point to the
tracking issue.

## Solution
Change all existing instances of:
```rust
#![deny(clippy::allow_attributes, clippy::allow_attributes_without_reason)]
```
to
```rust
#![deny(
    clippy::allow_attributes,
    clippy::allow_attributes_without_reason,
    reason = "See #17111; To be removed once all crates are in-line with these attributes"
)]
```

## Testing
N/A
github-merge-queue bot pushed a commit that referenced this issue Jan 6, 2025
…tributes_without_reason)]` (#17119)

# Objective
- #17111

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

No code changes have been made - except if a lint that was previously
`allow(...)`'d could be removed via small code changes. For example,
`unused_variables` can be handled by adding a `_` to the beginning of a
field's name.

## Testing
`cargo clippy` and `cargo test --package bevy_audio` were run, and no
errors were encountered.
github-merge-queue bot pushed a commit that referenced this issue Jan 6, 2025
…tributes_without_reason)]` (#17113)

# Objective
- #17111

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

No code changes have been made - except if a lint that was previously
`allow(...)`'d could be removed via small code changes. For example,
`unused_variables` can be handled by adding a `_` to the beginning of a
field's name.

## Testing
`cargo clippy` and `cargo test --package bevy_asset --features
multi_threaded` were run, and no errors were encountered.
github-merge-queue bot pushed a commit that referenced this issue Jan 6, 2025
…ttributes_without_reason)]` (Attempt 2) (#17184)

I broke the commit history on the other one,
#17160. Woops.

# Objective
- #17111

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

## Testing
`cargo clippy` and `cargo test --package bevy_sprite` were run, and no
errors were encountered.
github-merge-queue bot pushed a commit that referenced this issue Jan 6, 2025
…ow_attributes_without_reason)]` (#17186)

# Objective
- #17111

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

## Testing
`cargo clippy` and `cargo test --package bevy_diagnostic` were run, and
no errors were encountered.
github-merge-queue bot pushed a commit that referenced this issue Jan 6, 2025
…ttributes_without_reason)]` (#17194)

# Objective
- #17111

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

## Testing
`cargo clippy` and `cargo test --package bevy_render` were run, and no
errors were encountered.
github-merge-queue bot pushed a commit that referenced this issue Jan 7, 2025
…ibutes_without_reason)]` (#17214)

# Objective
- #17111

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

## Testing
`cargo clippy --tests` was run, and no errors were encountered.

I was expecting this crate to give more of a fight.
github-merge-queue bot pushed a commit that referenced this issue Jan 7, 2025
…ttributes_without_reason)]` (#17213)

# Objective
- #17111

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

## Testing
`cargo clippy --tests` was run, and no errors were encountered.
github-merge-queue bot pushed a commit that referenced this issue Jan 10, 2025
…w_attributes_without_reason)]` (#17284)

# Objective
- #17111

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

## Testing
`cargo clippy --tests --all-features --package bevy_transform` was run,
and no errors were encountered.
github-merge-queue bot pushed a commit that referenced this issue Jan 10, 2025
…ributes_without_reason)]` (#17285)

# Objective
- #17111

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

## Testing
`cargo clippy --tests --all-features --package bevy_text` was run, and
no errors were encountered.
github-merge-queue bot pushed a commit that referenced this issue Jan 10, 2025
…ributes_without_reason)]` (#17280)

# Objective
- #17111

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

## Testing
`cargo clippy --tests --all-features --package bevy_gltf` was run, and
no errors were encountered.
github-merge-queue bot pushed a commit that referenced this issue Jan 10, 2025
…ibutes_without_reason)]` (#17277)

# Objective
- #17111

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

## Testing
`cargo clippy --tests --package bevy_pbr` was run, and no errors were
encountered.
github-merge-queue bot pushed a commit that referenced this issue Jan 10, 2025
…tributes_without_reason)]` (#17278)

# Objective
- #17111

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

## Testing
`cargo clippy --tests --all-features --package bevy_scene` was run, and
no errors were encountered.
github-merge-queue bot pushed a commit that referenced this issue Jan 10, 2025
…tributes_without_reason)]` (#17288)

# Objective
- #17111

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

## Testing
Rust-analyzer did not return any errors once the deny was added.
github-merge-queue bot pushed a commit that referenced this issue Jan 11, 2025
…tributes_without_reason)]` (#17289)

# Objective
- #17111

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

## Testing
`cargo clippy --tests --package bevy_image` was run, and no errors were
encountered.

I could not run the above command with `--all-features` due to some
compilation errors with `bevy_core_pipeline` and `bevy_math` - but
hopefully CI catches anything I missed.

---------

Co-authored-by: Benjamin Brienen <[email protected]>
github-merge-queue bot pushed a commit that referenced this issue Jan 12, 2025
…_attributes_without_reason)]` (#17306)

# Objective
- #17111

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

## Testing
`cargo clippy --tests --all-features --package bevy_internal` was run,
and no errors were encountered.
github-merge-queue bot pushed a commit that referenced this issue Jan 12, 2025
…tributes_without_reason)]` (#17305)

# Objective
- #17111

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

## Testing
`cargo clippy --tests --all-features --package bevy_utils` was run, and
no errors were encountered.
github-merge-queue bot pushed a commit that referenced this issue Jan 12, 2025
…ttributes_without_reason)]` (#17303)

# Objective
- #17111

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

## Testing
`cargo clippy --tests --all-features --package bevy_remote` was run, and
no errors were encountered.
github-merge-queue bot pushed a commit that referenced this issue Jan 12, 2025
…attributes_without_reason)]` (#17302)

# Objective
- #17111

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

## Testing
`cargo clippy --tests --all-features --package bevy_picking` was run,
and no errors were encountered.
@LikeLakers2 LikeLakers2 changed the title Deny the clippy::allow_attributes and clippy::allow_attributes_without_reason lints Warn the clippy::allow_attributes and clippy::allow_attributes_without_reason lints Jan 12, 2025
@LikeLakers2
Copy link
Contributor Author

LikeLakers2 commented Jan 12, 2025

It didn't occur to me until now that deny may have been too aggressive. Thus, I've downgraded these lints to warns in #17320.

CI will still ensure any instances of these linting are taken care of.

github-merge-queue bot pushed a commit that referenced this issue Jan 14, 2025
…allow_attributes_without_reason)]` (#17137)

# Objective
- #17111

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

## Testing
`cargo clippy` and `cargo test --package bevy_core_pipeline` were run,
and no warnings were encountered.
github-merge-queue bot pushed a commit that referenced this issue Jan 14, 2025
…tributes_without_reason)]` (#17332)

# Objective
- #17111

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

## Testing
`cargo clippy --tests --all-features --package bevy_dylib` was run, and
no warnings were encountered.

I would've skipped over this crate if there weren't the two lint
attributes in it - might as well handle it now, y'know?
github-merge-queue bot pushed a commit that referenced this issue Jan 14, 2025
…low_attributes_without_reason)]` (#17323)

# Objective
- #17111

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

## Testing
`cargo clippy --tests --all-features --features
bevy_math/std,bevy_input/smol_str --package bevy_input_focus` was run,
and only an unrelated warning from `bevy_ecs` was encountered.

I could not test without the `bevy_math/std` feature due to compilation
errors with `glam`. Additionally, I had to use the `bevy_input/smol_str`
feature, as it appears some of `bevy_input_focus`' tests rely on that
feature. I will investigate these issues further, and make issues/PRs as
necessary.
github-merge-queue bot pushed a commit that referenced this issue Jan 14, 2025
…low_attributes_without_reason)]` (#17304)

# Objective
- #17111

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

## Testing
`cargo clippy --tests --all-features --package bevy_macro_utils` was
run, and no warnings were encountered.
github-merge-queue bot pushed a commit that referenced this issue Jan 14, 2025
…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.
github-merge-queue bot pushed a commit that referenced this issue Jan 15, 2025
…out_reason)]` to the workspace `Cargo.toml` (#17374)

# Objective
Fixes #17111

## Solution
Move `#![warn(clippy::allow_attributes,
clippy::allow_attributes_without_reason)]` to the workspace `Cargo.toml`

## Testing
Lots of CI testing, and local testing too.

---------

Co-authored-by: Benjamin Brienen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Cross-Cutting Impacts the entire engine C-Code-Quality A section of code that is hard to understand or change C-Tracking-Issue An issue that collects information about a broad development initiative 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
2 participants