Uplift `undocumented_unsafe_blocks` to `pedantic`

[tgross35]

Description

missing_safety_doc is warn by default, I think it makes sense to have its counterpart undocumented_unsafe_blocks at least be in pedantic. The other doc related lints like missing_panic_docs are in pedantic, so it seems to be a suitible place.

@rustbot label +A-category

[tgross35]

I think that if this is uplifted, accept-comment-above-statement and accept-comment-above-attributes should both default to true, to help keep sanity.

[Centri3]

missing_safety_doc and missing_panic_docs are a bit of an unfair comparison, safety docs are for the user of your crate and let them know what safety invariants they must uphold, whilst SAFETY: comments help you explain why it's sound to contributors; sometimes, it's also pretty unnecessary (stuff like // SAFETY: the caller must uphold the invariants described in the safety docs or the like, there's tons of those around libstd that don't really provide much clarity).

Stuff like the winapi as well would likely be very unnecessary as a good portion of it can be utilized somewhat care-free (despite being unsafe). You'd have to document each and every call, which would likely just be noise.

I'm definitely for making the 2 config options true by default though 👍

[tgross35]

missing_safety_doc and missing_panic_docs are a bit of an unfair comparison, safety docs are for the user of your crate and let them know what safety invariants they must uphold, whilst SAFETY: comments help you explain why it's sound to contributors; sometimes, it's also pretty unnecessary (stuff like // SAFETY: the caller must uphold the invariants described in the safety docs or the like, there's tons of those around libstd that don't really provide much clarity).

Stuff like the winapi as well would likely be very unnecessary as a good portion of it can be utilized somewhat care-free (despite being unsafe). You'd have to document each and every call, which would likely just be noise.

I do get that the comparison isn't exactly apples to apples - but Clippy of course has a ton of lints that are dedicated to easier development and maintenance. Not that that alone is the strongest justification, but even a simple comment like // SAFETY: FFI call with no preconditions goes a long way toward easily answering the question of why something that says unsafe is, well, safe.

I'm definitely for making the 2 config options true by default though 👍

Great! I can submit a PR for this later

[Centri3]

but Clippy of course has a ton of lints that are dedicated to easier development and maintenance

If a codebase did it from the get go then yeah it would help a ton, but I feel like applying this retroactively to any codebase that bulk-enables the pedantic group is a bit far. If they don't decide to just allow the lint, they'd have a lot to go through; and even then, likely wouldn't have the full picture of why a certain operation is sound. I feel like that would do more harm than good, potentially comments that are just // SAFETY: TODO or ones that are outright wrong. Maybe I'm being a bit pedantic though ^^

but even a simple comment like // SAFETY: FFI call with no preconditions goes a long way toward easily answering the question of why something that says unsafe is, well, safe.

I agree with this :)

[ojeda]

but even a simple comment like // SAFETY: FFI call with no preconditions goes a long way toward easily answering the question of why something that says unsafe is, well, safe.

Yeah, and having a comment gives extra information: if there is no comment, a reader doesn't know if it is because it was supposedly "trivial" or because it was missing (and thus there may be other requirements).

Thus always having comments, even if they just say it is a FFI call (or similar), can be helpful. That is why we want to always have a comment in the kernel.

I feel like that would do more harm than good, potentially comments that are just // SAFETY: TODO

Those can be useful! For instance, in the kernel, we had some unsafe blocks without them, but we discussed enabling the lint even if it required to add a few TODOs for the moment, precisely so that we don't forget to add new ones. Of course, it is best to simply fill them, and in our case we don't have many, but other projects may have too many to fix right away.

or ones that are outright wrong.

Wrong (and outdated) comments are, indeed, always a problem, but it seems orthogonal. And if it is a SAFETY comment, chances are the call may not be sound to begin with. In Rust for Linux we discussed over the years how to deal with this sort of issue and how to homogenize SAFETY comments, and recently @y86-dev has actually started to work on prototyping how this could be formalized/automated to some degree and how it would look like.

Having said that, I agree with the problem of adding things like this to groups that people may be already using. In fact, relatedly, we wanted to perhaps stop using groups so that (by the time we stop pinning the compiler version) we can guarantee new Clippy versions do not break the kernel builds (since we do -D for groups). Is there a way for Clippy to ignore lints that were added starting with a certain version? If not, I will open an issue (msrv does not do that). I also considered doing -W for the groups, and -D individual lints of the group manually, but at that point probably we will want to add/enable individual lints manually anyway.

[Centri3]

Those can be useful! For instance, in the kernel, we had some unsafe blocks without them, but we discussed enabling the lint even if it required to add a few TODOs for the moment, precisely so that we don't forget to add new ones. Of course, it is best to simply fill them, and in our case we don't have many, but other projects may have too many to fix right away.

In cases where the lint's usage is enforced I feel like simply having a warning (i.e., not adding SAFETY: TODO is much nicer, rather than grepping for the countless variations of SAFETY: TODO. There's also SAFETY: <blank> I suppose, which was what I originally wrote then changed to TODO. I've left a few too many of those lying around 🙃

but it seems orthogonal.

I disagree, as I was bringing up that they would be hastily added without much forethought, which is not a good combo with unsafe. In other cases though I agree.

since we do -D for groups

I also considered doing -W for the groups

It's probably just easier to -W yeah, perhaps not the most ideal but CI could -D some extra groups using RUSTFLAGS if I'm not misunderstanding

[ojeda]

In cases where the lint's usage is enforced I feel like simply having a warning (i.e., not adding SAFETY: TODO is much nicer, rather than grepping for the countless variations of SAFETY: TODO. There's also SAFETY: <blank> I suppose, which was what I originally wrote then changed to TODO. I've left a few too many of those lying around upside_down_face

That is fine too. In the kernel we can't really do that because warnings are to be fixed (i.e. the ones enabled by default) and writing the SAFETY comments is a requirement.

I disagree, as I was bringing up that they would be hastily added without much forethought, which is not a good combo with unsafe. In other cases though I agree.

That is up to the review/quality process of the project in particular. But, again, I agree putting something like this in "pedantic" is a fairly big hammer. Perhaps it should be done, but it is something that should be discussed widely, I would assume.

It's probably just easier to -W yeah, perhaps not the most ideal but CI could -D some extra groups using RUSTFLAGS if I'm not misunderstanding

What we do in compile testing is to make warnings be errors (in Rust with -Dwarnings), which if I understand correctly, it is similar to what you are suggesting. CIs focused on runtime-testing in general do not want -Werror, because they do not want to block whatever they are testing only due to a random warning that appeared given some combination of toolchain versions, kernel configs, etc.

Having said that, if we can make something -D directly, even better. When I setup the original list I started with an inflexible/strict approach to see how much we could get away with, and then relax as needed.

[flip1995]

Is there a way for Clippy to ignore lints that were added starting with a certain version? If not, I will open an issue

Oh, Jon Gjengset also asked about this at RustNationUK at the start of the year. I think this is a good idea. Please open an issue about it.

[ojeda]

Oh, Jon Gjengset also asked about this at RustNationUK at the start of the year. I think this is a good idea. Please open an issue about it.

Done! #11227