Auto merge of #4611 - rust-lang:doc-visibility, r=flip1995

account for doc visibility

This fixes #4608.

Also I noticed that the lint failed to look at trait and impl items. There's a small bit of fallout in the code, too, but not enough to warrant its own commit.

changelog: check docs of trait items and impl items, also make `missing_safety_doc` account for visibility

clippy_dev/src/lib.rs

@@ -57,7 +57,7 @@ impl Lint {
         lints.filter(|l| l.deprecation.is_none() && !l.is_internal())
     }
 
-    /// Returns the lints in a HashMap, grouped by the different lint groups
+    /// Returns the lints in a `HashMap`, grouped by the different lint groups
     pub fn by_lint_group(lints: &[Self]) -> HashMap<String, Vec<Self>> {
         lints
             .iter()

clippy_lints/src/consts.rs

@@ -324,7 +324,7 @@ impl<'c, 'cc> ConstEvalLateContext<'c, 'cc> {
         vec.iter().map(|elem| self.expr(elem)).collect::<Option<_>>()
     }
 
-    /// Lookup a possibly constant expression from a ExprKind::Path.
+    /// Lookup a possibly constant expression from a `ExprKind::Path`.
     fn fetch_path(&mut self, qpath: &QPath, id: HirId) -> Option<Constant> {
         use rustc::mir::interpret::GlobalId;
 

clippy_lints/src/doc.rs

@@ -1,11 +1,12 @@
 use crate::utils::span_lint;
 use itertools::Itertools;
 use pulldown_cmark;
-use rustc::lint::{EarlyContext, EarlyLintPass, LintArray, LintPass};
+use rustc::hir;
+use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
 use rustc::{declare_tool_lint, impl_lint_pass};
 use rustc_data_structures::fx::FxHashSet;
 use std::ops::Range;
-use syntax::ast;
+use syntax::ast::Attribute;
 use syntax::source_map::{BytePos, Span};
 use syntax_pos::Pos;
 use url::Url;
@@ -100,28 +101,78 @@ declare_clippy_lint! {
 #[derive(Clone)]
 pub struct DocMarkdown {
     valid_idents: FxHashSet<String>,
+    in_trait_impl: bool,
 }
 
 impl DocMarkdown {
     pub fn new(valid_idents: FxHashSet<String>) -> Self {
-        Self { valid_idents }
+        Self {
+            valid_idents,
+            in_trait_impl: false,
+        }
     }
 }
 
 impl_lint_pass!(DocMarkdown => [DOC_MARKDOWN, MISSING_SAFETY_DOC, NEEDLESS_DOCTEST_MAIN]);
 
-impl EarlyLintPass for DocMarkdown {
-    fn check_crate(&mut self, cx: &EarlyContext<'_>, krate: &ast::Crate) {
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for DocMarkdown {
+    fn check_crate(&mut self, cx: &LateContext<'a, 'tcx>, krate: &'tcx hir::Crate) {
         check_attrs(cx, &self.valid_idents, &krate.attrs);
     }
 
-    fn check_item(&mut self, cx: &EarlyContext<'_>, item: &ast::Item) {
+    fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::Item) {
         if check_attrs(cx, &self.valid_idents, &item.attrs) {
             return;
         }
         // no safety header
-        if let ast::ItemKind::Fn(_, ref header, ..) = item.kind {
-            if item.vis.node.is_pub() && header.unsafety == ast::Unsafety::Unsafe {
+        match item.kind {
+            hir::ItemKind::Fn(_, ref header, ..) => {
+                if cx.access_levels.is_exported(item.hir_id) && header.unsafety == hir::Unsafety::Unsafe {
+                    span_lint(
+                        cx,
+                        MISSING_SAFETY_DOC,
+                        item.span,
+                        "unsafe function's docs miss `# Safety` section",
+                    );
+                }
+            },
+            hir::ItemKind::Impl(_, _, _, _, ref trait_ref, ..) => {
+                self.in_trait_impl = trait_ref.is_some();
+            },
+            _ => {},
+        }
+    }
+
+    fn check_item_post(&mut self, _cx: &LateContext<'a, 'tcx>, item: &'tcx hir::Item) {
+        if let hir::ItemKind::Impl(..) = item.kind {
+            self.in_trait_impl = false;
+        }
+    }
+
+    fn check_trait_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::TraitItem) {
+        if check_attrs(cx, &self.valid_idents, &item.attrs) {
+            return;
+        }
+        // no safety header
+        if let hir::TraitItemKind::Method(ref sig, ..) = item.kind {
+            if cx.access_levels.is_exported(item.hir_id) && sig.header.unsafety == hir::Unsafety::Unsafe {
+                span_lint(
+                    cx,
+                    MISSING_SAFETY_DOC,
+                    item.span,
+                    "unsafe function's docs miss `# Safety` section",
+                );
+            }
+        }
+    }
+
+    fn check_impl_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::ImplItem) {
+        if check_attrs(cx, &self.valid_idents, &item.attrs) || self.in_trait_impl {
+            return;
+        }
+        // no safety header
+        if let hir::ImplItemKind::Method(ref sig, ..) = item.kind {
+            if cx.access_levels.is_exported(item.hir_id) && sig.header.unsafety == hir::Unsafety::Unsafe {
                 span_lint(
                     cx,
                     MISSING_SAFETY_DOC,
@@ -190,7 +241,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]) -> bool {
+pub fn check_attrs<'a>(cx: &LateContext<'_, '_>, valid_idents: &FxHashSet<String>, attrs: &'a [Attribute]) -> bool {
     let mut doc = String::new();
     let mut spans = vec![];
 
@@ -240,7 +291,7 @@ pub fn check_attrs<'a>(cx: &EarlyContext<'_>, valid_idents: &FxHashSet<String>,
 }
 
 fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize>)>>(
-    cx: &EarlyContext<'_>,
+    cx: &LateContext<'_, '_>,
     valid_idents: &FxHashSet<String>,
     events: Events,
     spans: &[(usize, Span)],
@@ -283,6 +334,7 @@ fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize
                 } else {
                     // Adjust for the beginning of the current `Event`
                     let span = span.with_lo(span.lo() + BytePos::from_usize(range.start - begin));
+
                     check_text(cx, valid_idents, &text, span);
                 }
             },
@@ -291,13 +343,13 @@ fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize
     safety_header
 }
 
-fn check_code(cx: &EarlyContext<'_>, text: &str, span: Span) {
+fn check_code(cx: &LateContext<'_, '_>, text: &str, span: Span) {
     if text.contains("fn main() {") {
         span_lint(cx, NEEDLESS_DOCTEST_MAIN, span, "needless `fn main` in doctest");
     }
 }
 
-fn check_text(cx: &EarlyContext<'_>, valid_idents: &FxHashSet<String>, text: &str, span: Span) {
+fn check_text(cx: &LateContext<'_, '_>, valid_idents: &FxHashSet<String>, text: &str, span: Span) {
     for word in text.split(|c: char| c.is_whitespace() || c == '\'') {
         // Trim punctuation as in `some comment (see foo::bar).`
         //                                                   ^^
@@ -320,7 +372,7 @@ fn check_text(cx: &EarlyContext<'_>, valid_idents: &FxHashSet<String>, text: &st
     }
 }
 
-fn check_word(cx: &EarlyContext<'_>, word: &str, span: Span) {
+fn check_word(cx: &LateContext<'_, '_>, word: &str, span: Span) {
     /// Checks if a string is camel-case, i.e., contains at least two uppercase
     /// letters (`Clippy` is ok) and one lower-case letter (`NASA` is ok).
     /// Plurals are also excluded (`IDs` is ok).

clippy_lints/src/lib.rs

@@ -535,7 +535,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con
             conf.blacklisted_names.iter().cloned().collect()
     ));
     reg.register_late_lint_pass(box functions::Functions::new(conf.too_many_arguments_threshold, conf.too_many_lines_threshold));
-    reg.register_early_lint_pass(box doc::DocMarkdown::new(conf.doc_valid_idents.iter().cloned().collect()));
+    reg.register_late_lint_pass(box doc::DocMarkdown::new(conf.doc_valid_idents.iter().cloned().collect()));
     reg.register_late_lint_pass(box neg_multiply::NegMultiply);
     reg.register_early_lint_pass(box unsafe_removed_from_name::UnsafeNameRemoval);
     reg.register_late_lint_pass(box mem_discriminant::MemDiscriminant);

clippy_lints/src/methods/mod.rs

@@ -1886,7 +1886,7 @@ fn lint_iter_nth<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr, iter_ar
 }
 
 fn lint_get_unwrap<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr, get_args: &'tcx [hir::Expr], is_mut: bool) {
-    // Note: we don't want to lint `get_mut().unwrap` for HashMap or BTreeMap,
+    // Note: we don't want to lint `get_mut().unwrap` for `HashMap` or `BTreeMap`,
     // because they do not implement `IndexMut`
     let mut applicability = Applicability::MachineApplicable;
     let expr_ty = cx.tables.expr_ty(&get_args[0]);

clippy_lints/src/types.rs

@@ -1986,7 +1986,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for InvalidUpcastComparisons {
 declare_clippy_lint! {
     /// **What it does:** Checks for public `impl` or `fn` missing generalization
     /// over different hashers and implicitly defaulting to the default hashing
-    /// algorithm (SipHash).
+    /// algorithm (`SipHash`).
     ///
     /// **Why is this bad?** `HashMap` or `HashSet` with custom hashers cannot be
     /// used with them.
@@ -2164,7 +2164,7 @@ enum ImplicitHasherType<'tcx> {
 }
 
 impl<'tcx> ImplicitHasherType<'tcx> {
-    /// Checks that `ty` is a target type without a BuildHasher.
+    /// Checks that `ty` is a target type without a `BuildHasher`.
     fn new<'a>(cx: &LateContext<'a, 'tcx>, hir_ty: &hir::Ty) -> Option<Self> {
         if let TyKind::Path(QPath::Resolved(None, ref path)) = hir_ty.kind {
             let params: Vec<_> = path

tests/ui/doc_unsafe.rs

@@ -17,9 +17,59 @@ unsafe fn you_dont_see_me() {
     unimplemented!();
 }
 
-fn main() {
-    you_dont_see_me();
-    destroy_the_planet();
-    let mut universe = ();
-    apocalypse(&mut universe);
+mod private_mod {
+    pub unsafe fn only_crate_wide_accessible() {
+        unimplemented!();
+    }
+
+    pub unsafe fn republished() {
+        unimplemented!();
+    }
+}
+
+pub use private_mod::republished;
+
+pub trait UnsafeTrait {
+    unsafe fn woefully_underdocumented(self);
+
+    /// # Safety
+    unsafe fn at_least_somewhat_documented(self);
+}
+
+pub struct Struct;
+
+impl UnsafeTrait for Struct {
+    unsafe fn woefully_underdocumented(self) {
+        // all is well
+    }
+
+    unsafe fn at_least_somewhat_documented(self) {
+        // all is still well
+    }
+}
+
+impl Struct {
+    pub unsafe fn more_undocumented_unsafe() -> Self {
+        unimplemented!();
+    }
+
+    /// # Safety
+    pub unsafe fn somewhat_documented(&self) {
+        unimplemented!();
+    }
+
+    unsafe fn private(&self) {
+        unimplemented!();
+    }
+}
+
+#[allow(clippy::let_unit_value)]
+fn main() {
+    unsafe {
+        you_dont_see_me();
+        destroy_the_planet();
+        let mut universe = ();
+        apocalypse(&mut universe);
+        private_mod::only_crate_wide_accessible();
+    }
 }

tests/ui/doc_unsafe.stderr

@@ -8,5 +8,27 @@ LL | | }
    |
    = note: `-D clippy::missing-safety-doc` implied by `-D warnings`
 
-error: aborting due to previous error
+error: unsafe function's docs miss `# Safety` section
+  --> $DIR/doc_unsafe.rs:25:5
+   |
+LL | /     pub unsafe fn republished() {
+LL | |         unimplemented!();
+LL | |     }
+   | |_____^
+
+error: unsafe function's docs miss `# Safety` section
+  --> $DIR/doc_unsafe.rs:33:5
+   |
+LL |     unsafe fn woefully_underdocumented(self);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: unsafe function's docs miss `# Safety` section
+  --> $DIR/doc_unsafe.rs:52:5
+   |
+LL | /     pub unsafe fn more_undocumented_unsafe() -> Self {
+LL | |         unimplemented!();
+LL | |     }
+   | |_____^
+
+error: aborting due to 4 previous errors
 

tests/ui/functions.rs

@@ -1,6 +1,6 @@
 #![warn(clippy::all)]
 #![allow(dead_code)]
-#![allow(unused_unsafe)]
+#![allow(unused_unsafe, clippy::missing_safety_doc)]
 
 // TOO_MANY_ARGUMENTS
 fn good(_one: u32, _two: u32, _three: &str, _four: bool, _five: f32, _six: f32, _seven: bool) {}

tests/ui/new_without_default.rs

@@ -1,5 +1,5 @@
 #![feature(const_fn)]
-#![allow(dead_code)]
+#![allow(dead_code, clippy::missing_safety_doc)]
 #![warn(clippy::new_without_default)]
 
 pub struct Foo;