Auto merge of #4535 - rust-lang:unsafe-doc, r=flip1995

New lint: Require `# Safety` section in pub unsafe fn docs

changelog: add `missing_safety_doc` lint

This fixes #2207

CHANGELOG.md

@@ -1056,6 +1056,7 @@ Released 2018-09-13
 [`missing_const_for_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn
 [`missing_docs_in_private_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_docs_in_private_items
 [`missing_inline_in_public_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_inline_in_public_items
+[`missing_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc
 [`mistyped_literal_suffixes`]: https://rust-lang.github.io/rust-clippy/master/index.html#mistyped_literal_suffixes
 [`mixed_case_hex_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#mixed_case_hex_literals
 [`module_inception`]: https://rust-lang.github.io/rust-clippy/master/index.html#module_inception

README.md

@@ -6,7 +6,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 313 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 314 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

clippy_lints/Cargo.toml

@@ -27,7 +27,7 @@ semver = "0.9.0"
 serde = { version = "1.0", features = ["derive"] }
 toml = "0.5.3"
 unicode-normalization = "0.1"
-pulldown-cmark = "0.5.3"
+pulldown-cmark = "0.6.0"
 url = { version =  "2.1.0", features = ["serde"] } # cargo requires serde feat in its url dep
 # see https://github.com/rust-lang/rust/pull/63587#issuecomment-522343864
 if_chain = "1.0.0"

clippy_lints/src/doc.rs

@@ -34,6 +34,40 @@ declare_clippy_lint! {
     "presence of `_`, `::` or camel-case outside backticks in documentation"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for the doc comments of publicly visible
+    /// unsafe functions and warns if there is no `# Safety` section.
+    ///
+    /// **Why is this bad?** Unsafe functions should document their safety
+    /// preconditions, so that users can be sure they are using them safely.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Examples**:
+    /// ```rust
+    ///# type Universe = ();
+    /// /// This function should really be documented
+    /// pub unsafe fn start_apocalypse(u: &mut Universe) {
+    ///     unimplemented!();
+    /// }
+    /// ```
+    ///
+    /// At least write a line about safety:
+    ///
+    /// ```rust
+    ///# type Universe = ();
+    /// /// # Safety
+    /// ///
+    /// /// This function should not be called before the horsemen are ready.
+    /// pub unsafe fn start_apocalypse(u: &mut Universe) {
+    ///     unimplemented!();
+    /// }
+    /// ```
+    pub MISSING_SAFETY_DOC,
+    style,
+    "`pub unsafe fn` without `# Safety` docs"
+}
+
 #[allow(clippy::module_name_repetitions)]
 #[derive(Clone)]
 pub struct DocMarkdown {
@@ -46,15 +80,28 @@ impl DocMarkdown {
     }
 }
 
-impl_lint_pass!(DocMarkdown => [DOC_MARKDOWN]);
+impl_lint_pass!(DocMarkdown => [DOC_MARKDOWN, MISSING_SAFETY_DOC]);
 
 impl EarlyLintPass for DocMarkdown {
     fn check_crate(&mut self, cx: &EarlyContext<'_>, krate: &ast::Crate) {
         check_attrs(cx, &self.valid_idents, &krate.attrs);
     }
 
     fn check_item(&mut self, cx: &EarlyContext<'_>, item: &ast::Item) {
-        check_attrs(cx, &self.valid_idents, &item.attrs);
+        if check_attrs(cx, &self.valid_idents, &item.attrs) {
+            return;
+        }
+        // no safety header
+        if let ast::ItemKind::Fn(_, ref header, ..) = item.node {
+            if item.vis.node.is_pub() && header.unsafety == ast::Unsafety::Unsafe {
+                span_lint(
+                    cx,
+                    MISSING_SAFETY_DOC,
+                    item.span,
+                    "unsafe function's docs miss `# Safety` section",
+                );
+            }
+        }
     }
 }
 
@@ -115,7 +162,7 @@ pub fn strip_doc_comment_decoration(comment: &str, span: Span) -> (String, Vec<(
     panic!("not a doc-comment: {}", comment);
 }
 
-pub fn check_attrs<'a>(cx: &EarlyContext<'_>, valid_idents: &FxHashSet<String>, attrs: &'a [ast::Attribute]) {
+pub fn check_attrs<'a>(cx: &EarlyContext<'_>, valid_idents: &FxHashSet<String>, attrs: &'a [ast::Attribute]) -> bool {
     let mut doc = String::new();
     let mut spans = vec![];
 
@@ -129,7 +176,7 @@ pub fn check_attrs<'a>(cx: &EarlyContext<'_>, valid_idents: &FxHashSet<String>,
             }
         } else if attr.check_name(sym!(doc)) {
             // ignore mix of sugared and non-sugared doc
-            return;
+            return true; // don't trigger the safety check
         }
     }
 
@@ -140,57 +187,64 @@ pub fn check_attrs<'a>(cx: &EarlyContext<'_>, valid_idents: &FxHashSet<String>,
         current += offset_copy;
     }
 
-    if !doc.is_empty() {
-        let parser = pulldown_cmark::Parser::new(&doc).into_offset_iter();
-        // Iterate over all `Events` and combine consecutive events into one
-        let events = parser.coalesce(|previous, current| {
-            use pulldown_cmark::Event::*;
-
-            let previous_range = previous.1;
-            let current_range = current.1;
-
-            match (previous.0, current.0) {
-                (Text(previous), Text(current)) => {
-                    let mut previous = previous.to_string();
-                    previous.push_str(&current);
-                    Ok((Text(previous.into()), previous_range))
-                },
-                (previous, current) => Err(((previous, previous_range), (current, current_range))),
-            }
-        });
-        check_doc(cx, valid_idents, events, &spans);
+    if doc.is_empty() {
+        return false;
     }
+
+    let parser = pulldown_cmark::Parser::new(&doc).into_offset_iter();
+    // Iterate over all `Events` and combine consecutive events into one
+    let events = parser.coalesce(|previous, current| {
+        use pulldown_cmark::Event::*;
+
+        let previous_range = previous.1;
+        let current_range = current.1;
+
+        match (previous.0, current.0) {
+            (Text(previous), Text(current)) => {
+                let mut previous = previous.to_string();
+                previous.push_str(&current);
+                Ok((Text(previous.into()), previous_range))
+            },
+            (previous, current) => Err(((previous, previous_range), (current, current_range))),
+        }
+    });
+    check_doc(cx, valid_idents, events, &spans)
 }
 
 fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize>)>>(
     cx: &EarlyContext<'_>,
     valid_idents: &FxHashSet<String>,
     events: Events,
     spans: &[(usize, Span)],
-) {
+) -> bool {
+    // true if a safety header was found
     use pulldown_cmark::Event::*;
     use pulldown_cmark::Tag::*;
 
+    let mut safety_header = false;
     let mut in_code = false;
     let mut in_link = None;
+    let mut in_heading = false;
 
     for (event, range) in events {
         match event {
             Start(CodeBlock(_)) => in_code = true,
             End(CodeBlock(_)) => in_code = false,
             Start(Link(_, url, _)) => in_link = Some(url),
             End(Link(..)) => in_link = None,
-            Start(_tag) | End(_tag) => (),         // We don't care about other tags
-            Html(_html) | InlineHtml(_html) => (), // HTML is weird, just ignore it
-            SoftBreak | HardBreak | TaskListMarker(_) | Code(_) => (),
+            Start(Heading(_)) => in_heading = true,
+            End(Heading(_)) => in_heading = false,
+            Start(_tag) | End(_tag) => (), // We don't care about other tags
+            Html(_html) => (),             // HTML is weird, just ignore it
+            SoftBreak | HardBreak | TaskListMarker(_) | Code(_) | Rule => (),
             FootnoteReference(text) | Text(text) => {
                 if Some(&text) == in_link.as_ref() {
                     // Probably a link of the form `<http://example.com>`
                     // Which are represented as a link to "http://example.com" with
                     // text "http://example.com" by pulldown-cmark
                     continue;
                 }
-
+                safety_header |= in_heading && text.trim() == "Safety";
                 if !in_code {
                     let index = match spans.binary_search_by(|c| c.0.cmp(&range.start)) {
                         Ok(o) => o,
@@ -207,6 +261,7 @@ fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize
             },
         }
     }
+    safety_header
 }
 
 fn check_text(cx: &EarlyContext<'_>, valid_idents: &FxHashSet<String>, text: &str, span: Span) {

clippy_lints/src/lib.rs

@@ -708,6 +708,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con
         copies::IFS_SAME_COND,
         copies::IF_SAME_THEN_ELSE,
         derive::DERIVE_HASH_XOR_EQ,
+        doc::MISSING_SAFETY_DOC,
         double_comparison::DOUBLE_COMPARISONS,
         double_parens::DOUBLE_PARENS,
         drop_bounds::DROP_BOUNDS,
@@ -929,6 +930,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con
         block_in_if_condition::BLOCK_IN_IF_CONDITION_EXPR,
         block_in_if_condition::BLOCK_IN_IF_CONDITION_STMT,
         collapsible_if::COLLAPSIBLE_IF,
+        doc::MISSING_SAFETY_DOC,
         enum_variants::ENUM_VARIANT_NAMES,
         enum_variants::MODULE_INCEPTION,
         eq_op::OP_REF,

src/lintlist/mod.rs

@@ -6,7 +6,7 @@ pub use lint::Lint;
 pub use lint::LINT_LEVELS;
 
 // begin lint list, do not remove this comment, it’s used in `update_lints`
-pub const ALL_LINTS: [Lint; 313] = [
+pub const ALL_LINTS: [Lint; 314] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -1078,6 +1078,13 @@ pub const ALL_LINTS: [Lint; 313] = [
         deprecation: None,
         module: "missing_inline",
     },
+    Lint {
+        name: "missing_safety_doc",
+        group: "style",
+        desc: "`pub unsafe fn` without `# Safety` docs",
+        deprecation: None,
+        module: "doc",
+    },
     Lint {
         name: "mistyped_literal_suffixes",
         group: "correctness",

tests/ui/doc_unsafe.rs

@@ -0,0 +1,25 @@
+/// This is not sufficiently documented
+pub unsafe fn destroy_the_planet() {
+    unimplemented!();
+}
+
+/// This one is
+///
+/// # Safety
+///
+/// This function shouldn't be called unless the horsemen are ready
+pub unsafe fn apocalypse(universe: &mut ()) {
+    unimplemented!();
+}
+
+/// This is a private function, so docs aren't necessary
+unsafe fn you_dont_see_me() {
+    unimplemented!();
+}
+
+fn main() {
+    you_dont_see_me();
+    destroy_the_planet();
+    let mut universe = ();
+    apocalypse(&mut universe);
+}

tests/ui/doc_unsafe.stderr

@@ -0,0 +1,12 @@
+error: unsafe function's docs miss `# Safety` section
+  --> $DIR/doc_unsafe.rs:2:1
+   |
+LL | / pub unsafe fn destroy_the_planet() {
+LL | |     unimplemented!();
+LL | | }
+   | |_^
+   |
+   = note: `-D clippy::missing-safety-doc` implied by `-D warnings`
+
+error: aborting due to previous error
+