Auto merge of #11170 - tgross35:undocumented-unsafe-blocks-defaults, r=Centri3

bors · bors · commit 07dc32a178ab · 2023-08-09T21:17:25.000Z
Change defaults of `accept-comment-above-statement` and `accept-comment-above-attributes` This patch sets the two configuration options for `undocumented_unsafe_blocks` to `true` by default: these are `accept-comment-above-statement` and `accept-comment-above-attributes`. Having these values `false` by default prevents what many users would consider clean code, e.g. placing the `// SAFETY:` comment above a single-line functino call, rather than directly next to the argument. This was originally discussed in #11162 changelog: [`undocumented_unsafe_blocks`]: set `accept-comment-above-statement` and `accept-comment-above-attributes` to `true` by default.
diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md
@@ -703,7 +703,7 @@ Minimum chars an ident can have, anything below or equal to this will be linted.
 ## `accept-comment-above-statement`
 Whether to accept a safety comment to be placed above the statement containing the `unsafe` block
 
-**Default Value:** `false` (`bool`)
+**Default Value:** `true` (`bool`)
 
 ---
 **Affected lints:**
@@ -713,7 +713,7 @@ Whether to accept a safety comment to be placed above the statement containing t
 ## `accept-comment-above-attributes`
 Whether to accept a safety comment to be placed above the attributes for the `unsafe` block
 
-**Default Value:** `false` (`bool`)
+**Default Value:** `true` (`bool`)
 
 ---
 **Affected lints:**
@@ -750,4 +750,3 @@ Which crates to allow absolute paths from
 **Affected lints:**
 * [`absolute_paths`](https://rust-lang.github.io/rust-clippy/master/index.html#absolute_paths)
 
-
diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs
@@ -542,11 +542,11 @@ define_Conf! {
     /// Lint: UNDOCUMENTED_UNSAFE_BLOCKS.
     ///
     /// Whether to accept a safety comment to be placed above the statement containing the `unsafe` block
-    (accept_comment_above_statement: bool = false),
+    (accept_comment_above_statement: bool = true),
     /// Lint: UNDOCUMENTED_UNSAFE_BLOCKS.
     ///
     /// Whether to accept a safety comment to be placed above the attributes for the `unsafe` block
-    (accept_comment_above_attributes: bool = false),
+    (accept_comment_above_attributes: bool = true),
     /// Lint: UNNECESSARY_RAW_STRING_HASHES.
     ///
     /// Whether to allow `r#""#` when `r""` can be used
diff --git a/tests/ui-toml/undocumented_unsafe_blocks/clippy.toml b/tests/ui-toml/undocumented_unsafe_blocks/clippy.toml
@@ -1,2 +1,2 @@
-accept-comment-above-statement = true
-accept-comment-above-attributes = true
+accept-comment-above-statement = false
+accept-comment-above-attributes = false
diff --git a/tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs b/tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs
@@ -491,7 +491,7 @@ unsafe impl CrateRoot for () {}
 // SAFETY: ok
 unsafe impl CrateRoot for (i32) {}
 
-fn issue_9142() {
+fn nested_block_separation_issue_9142() {
     // SAFETY: ok
     let _ =
         // we need this comment to avoid rustfmt putting
@@ -518,49 +518,49 @@ pub const unsafe fn a_const_function_with_a_very_long_name_to_break_the_line() -
     2
 }
 
-fn issue_10832() {
-    // Safety: A safety comment
+fn separate_line_from_let_issue_10832() {
+    // Safety: A separate safety comment that will warn
     let _some_variable_with_a_very_long_name_to_break_the_line =
         unsafe { a_function_with_a_very_long_name_to_break_the_line() };
 
-    // Safety: Another safety comment
+    // Safety: Another separate safety comment that will warn
     const _SOME_CONST_WITH_A_VERY_LONG_NAME_TO_BREAK_THE_LINE: u32 =
         unsafe { a_const_function_with_a_very_long_name_to_break_the_line() };
 
-    // Safety: Yet another safety comment
+    // Safety: Yet another separate safety comment that will warn
     static _SOME_STATIC_WITH_A_VERY_LONG_NAME_TO_BREAK_THE_LINE: u32 =
         unsafe { a_const_function_with_a_very_long_name_to_break_the_line() };
 }
 
-fn issue_8679<T: Copy>() {
-    // SAFETY:
+fn above_expr_attribute_issue_8679<T: Copy>() {
+    // SAFETY: this should fail, above attribute
     #[allow(unsafe_code)]
     unsafe {}
 
-    // SAFETY:
+    // SAFETY: this should fail, above attribute
     #[expect(unsafe_code, reason = "totally safe")]
     unsafe {
         *std::ptr::null::<T>()
     };
 
-    // Safety: A safety comment
+    // Safety: A safety comment that should fail
     #[allow(unsafe_code)]
     let _some_variable_with_a_very_long_name_to_break_the_line =
         unsafe { a_function_with_a_very_long_name_to_break_the_line() };
 
-    // Safety: Another safety comment
+    // Safety: Another safety comment that should fail
     #[allow(unsafe_code)]
     const _SOME_CONST_WITH_A_VERY_LONG_NAME_TO_BREAK_THE_LINE: u32 =
         unsafe { a_const_function_with_a_very_long_name_to_break_the_line() };
 
-    // Safety: Yet another safety comment
+    // Safety: Yet another safety comment that should fail
     #[allow(unsafe_code)]
     static _SOME_STATIC_WITH_A_VERY_LONG_NAME_TO_BREAK_THE_LINE: u32 =
         unsafe { a_const_function_with_a_very_long_name_to_break_the_line() };
 
     // SAFETY:
     #[allow(unsafe_code)]
-    // This also works I guess
+    // This shouldn't work either
     unsafe {}
 }
 
diff --git a/tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.stderr b/tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.stderr
@@ -276,6 +276,14 @@ LL | unsafe impl CrateRoot for () {}
    |
    = help: consider adding a safety comment on the preceding line
 
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:499:9
+   |
+LL |         unsafe {};
+   |         ^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
 error: statement has unnecessary safety comment
   --> $DIR/undocumented_unsafe_blocks.rs:502:5
    |
@@ -310,5 +318,77 @@ LL |             let bar = unsafe {};
    |
    = help: consider adding a safety comment on the preceding line
 
-error: aborting due to 35 previous errors
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:524:9
+   |
+LL |         unsafe { a_function_with_a_very_long_name_to_break_the_line() };
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:528:9
+   |
+LL |         unsafe { a_const_function_with_a_very_long_name_to_break_the_line() };
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:532:9
+   |
+LL |         unsafe { a_const_function_with_a_very_long_name_to_break_the_line() };
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:538:5
+   |
+LL |     unsafe {}
+   |     ^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:542:5
+   |
+LL |     unsafe {
+   |     ^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:549:9
+   |
+LL |         unsafe { a_function_with_a_very_long_name_to_break_the_line() };
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:554:9
+   |
+LL |         unsafe { a_const_function_with_a_very_long_name_to_break_the_line() };
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:559:9
+   |
+LL |         unsafe { a_const_function_with_a_very_long_name_to_break_the_line() };
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:564:5
+   |
+LL |     unsafe {}
+   |     ^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: aborting due to 45 previous errors
 
diff --git a/tests/ui/undocumented_unsafe_blocks.rs b/tests/ui/undocumented_unsafe_blocks.rs
@@ -1,7 +1,8 @@
 //@aux-build:proc_macro_unsafe.rs:proc-macro
 
 #![warn(clippy::undocumented_unsafe_blocks, clippy::unnecessary_safety_comment)]
-#![allow(clippy::let_unit_value, clippy::missing_safety_doc)]
+#![allow(deref_nullptr, clippy::let_unit_value, clippy::missing_safety_doc)]
+#![feature(lint_reasons)]
 
 extern crate proc_macro_unsafe;
 
@@ -490,7 +491,7 @@ unsafe impl CrateRoot for () {}
 // SAFETY: ok
 unsafe impl CrateRoot for (i32) {}
 
-fn issue_9142() {
+fn nested_block_separation_issue_9142() {
     // SAFETY: ok
     let _ =
         // we need this comment to avoid rustfmt putting
@@ -517,18 +518,51 @@ pub const unsafe fn a_const_function_with_a_very_long_name_to_break_the_line() -
     2
 }
 
-fn issue_10832() {
-    // Safety: A safety comment. But it will warn anyways
+fn separate_line_from_let_issue_10832() {
+    // SAFETY:
     let _some_variable_with_a_very_long_name_to_break_the_line =
         unsafe { a_function_with_a_very_long_name_to_break_the_line() };
 
-    // Safety: Another safety comment. But it will warn anyways
+    // SAFETY:
     const _SOME_CONST_WITH_A_VERY_LONG_NAME_TO_BREAK_THE_LINE: u32 =
         unsafe { a_const_function_with_a_very_long_name_to_break_the_line() };
 
-    // Safety: Yet another safety comment. But it will warn anyways
+    // SAFETY:
     static _SOME_STATIC_WITH_A_VERY_LONG_NAME_TO_BREAK_THE_LINE: u32 =
         unsafe { a_const_function_with_a_very_long_name_to_break_the_line() };
 }
 
+fn above_expr_attribute_issue_8679<T: Copy>() {
+    // SAFETY:
+    #[allow(unsafe_code)]
+    unsafe {}
+
+    // SAFETY:
+    #[expect(unsafe_code, reason = "totally safe")]
+    unsafe {
+        *std::ptr::null::<T>()
+    };
+
+    // Safety: A safety comment
+    #[allow(unsafe_code)]
+    let _some_variable_with_a_very_long_name_to_break_the_line =
+        unsafe { a_function_with_a_very_long_name_to_break_the_line() };
+
+    // Safety: Another safety comment
+    #[allow(unsafe_code)]
+    const _SOME_CONST_WITH_A_VERY_LONG_NAME_TO_BREAK_THE_LINE: u32 =
+        unsafe { a_const_function_with_a_very_long_name_to_break_the_line() };
+
+    // Safety: Yet another safety comment with multiple attributes
+    #[allow(unsafe_code)]
+    #[allow(non_upper_case_globals)]
+    static _some_static_with_a_very_long_name_to_break_the_line: u32 =
+        unsafe { a_const_function_with_a_very_long_name_to_break_the_line() };
+
+    // SAFETY:
+    #[allow(unsafe_code)]
+    // This also works I guess
+    unsafe {}
+}
+
 fn main() {}
diff --git a/tests/ui/undocumented_unsafe_blocks.stderr b/tests/ui/undocumented_unsafe_blocks.stderr
