enable clippy::undocumented_unsafe_blocks, clippy::unnecessary_safety_comment

[andrewgazelka]

No description provided.

[TestingPlant]

I believe we should enable clippy::undocumented_unsafe_blocks instead to require a safety comment since safety connects are more common

[maciek134]

@TestingPlant the difference is that #![deny(unsafe_code) will not compile unless you #[expect(unsafe_code)], while clippy::undocumented_unsafe_blocks can be ignored by just not running clippy.

[andrewgazelka]

@TestingPlant the difference is that #[deny(unsafe_code) will not compile unless you #[expect(unsafe_used)], while clippy::undocumented_unsafe_blocks can be ignored by just not running clippy.

Clippy is checked in CI → one cannot merge unless clippy passes. So it should be effectively the same.

The benefit of #[expect(...)] is clippy/check will not pass if the unsafe code is removed and the reasoning is still there (as it does not need to be.

[maciek134]

Clippy is checked in CI → one cannot merge unless clippy passes.

Unless one overrides CI, or an unrelated change to CI breaks the check. Both is best anyway imo, safety concerns documented on the unsafe block stay there no matter what happens with git hosting.

[andrewgazelka]

Clippy is checked in CI → one cannot merge unless clippy passes.

Unless one overrides CI, or an unrelated change to CI breaks the check. Both is best anyway imo, safety concerns documented on the unsafe block stay there no matter what happens with git hosting.

ok these are exotic scenarios that will either be rare or not happen with proper reviewing.

However, I agree with you in the sense of using

#[expect]

even though I do not agree with your logic here really.

[TestingPlant]

The benefit of #[expect(...)] is clippy/check will not pass if the unsafe code is removed and the reasoning is still there (as it does not need to be.

To clarify, I'm referring to enabling clippy::undocumented_unsafe_blocks by default through the Cargo.toml.

If the safety comment is still there and the unsafe block is removed, there will be a warning from clippy::unnecessary_safety_comment. I don't see any advantages from using #[expect(unsafe_used, reason ="")] instead of requiring safety comments through clippy::undocumented_unsafe_blocks, and safety comments is the standard way of justifying unsafe code in Rust.

[andrewgazelka]

The benefit of #[expect(...)] is clippy/check will not pass if the unsafe code is removed and the reasoning is still there (as it does not need to be.

To clarify, I'm referring to enabling clippy::undocumented_unsafe_blocks by default through the Cargo.toml.

If the safety comment is still there and the unsafe block is removed, there will be a warning from clippy::unnecessary_safety_comment. I don't see any advantages from using #[expect(unsafe_used, reason ="")] instead of requiring safety comments through clippy::undocumented_unsafe_blocks, and safety comments is the standard way of justifying unsafe code in Rust.

ah ok i wasnt aware there was a negative lint as well. ok i am fine with //safety comment then

[andrewgazelka]

ok I changed the issue title as per @TestingPlant 's recommendation