bevy_asset: Apply #![deny(clippy::allow_attributes, clippy::allow_attributes_without_reason)]
#17113
Conversation
| #[expect( | ||
| dead_code, | ||
| reason = "This exists to ensure that `#[derive(Asset)]` works on enums. The inner variants are known not to be used." | ||
| )] |
There was a problem hiding this comment.
I have no idea why this only lints on the enum test case, despite the struct test cases also being unused.
| #[derive(Asset, TypePath, Debug)] | ||
| struct A(usize); | ||
| struct A; |
There was a problem hiding this comment.
In case you're wondering: I checked and double-checked to make sure that removing these usize fields wouldn't affect tests.
If they affect tests, I didn't see it during my testing.
There was a problem hiding this comment.
I don't think the tests are meant to test specifically unit struct assets. Unit struct assets don't sound very useful anyway.
There was a problem hiding this comment.
Sure, but... The tests compile and finish successfully either way.
Really, the (usize) only confuses people into thinking that we're somehow constructing instances of the structs. Except, we're not - the structs are only used as type parameters.
If we start needing a value, they can be added back.
There was a problem hiding this comment.
It's true that as far as I can see there seems to be no behavior in the tests hinging on the structs having any fields. However, idk if they're relevant for TypeId calculations.
It would be best to ask someone who wrote this or knows about reflection.
There was a problem hiding this comment.
@IQuick143 I don't believe they're relevant for TypeId calculations: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=99bbd15c51d685da51caa2a4beecf51b
I mean, it appears that changing one to include (usize) does change its TypeId - but two unit structs still have two different TypeIds.
There was a problem hiding this comment.
In that case, I think this fix is good :)
| #[derive(Asset, TypePath, Debug)] | ||
| struct A(usize); | ||
| struct A; |
There was a problem hiding this comment.
I don't think the tests are meant to test specifically unit struct assets. Unit struct assets don't sound very useful anyway.
|
In benches, we silence dead code warnings due to similar dead_code unit tests in this PR. |
|
It's fine to me if we fix the dead code warning by removing the fields, but I wonder if it might make sense to instead use those fields in the unit tests. If not, nevermind me. |
|
@BenjaminBrienen Adding a new test that uses those fields would probably make more sense - but the addition of tests is out-of-scope for this PR. |
LikeLakers2
changed the title
#[deny(clippy::allow_attributes, clippy::allow_attributes_without_reason)]#![deny(clippy::allow_attributes, clippy::allow_attributes_without_reason)]
TimJentzsch
added
C-Code-Quality
LikeLakers2
force-pushed
the
lint/deny_allow_and_without_reason/bevy_asset
branch
from
a1bf6e0 to
53bc757
Compare
IQuick143
added
S-Ready-For-Final-Review
Objective
clippy::allow_attributesandclippy::allow_attributes_without_reasonlints #17111Solution
Set the
clippy::allow_attributesandclippy::allow_attributes_without_reasonlints todeny, and bringbevy_assetin 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_variablescan be handled by adding a_to the beginning of a field's name.Testing
cargo clippyandcargo test --package bevy_asset --features multi_threadedwere run, and no errors were encountered.