From e3f143ff0abfb10ffdcd53e1dba27322e76dc1f6 Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Wed, 2 Oct 2019 17:19:30 +0200 Subject: [PATCH] account for doc visibility --- clippy_dev/src/lib.rs | 2 +- clippy_lints/src/consts.rs | 2 +- clippy_lints/src/doc.rs | 78 +++++++++++++++++++++++++++------ clippy_lints/src/lib.rs | 2 +- clippy_lints/src/methods/mod.rs | 2 +- clippy_lints/src/types.rs | 4 +- tests/ui/doc_unsafe.rs | 60 ++++++++++++++++++++++--- tests/ui/doc_unsafe.stderr | 24 +++++++++- tests/ui/functions.rs | 2 +- tests/ui/new_without_default.rs | 2 +- 10 files changed, 151 insertions(+), 27 deletions(-) diff --git a/clippy_dev/src/lib.rs b/clippy_dev/src/lib.rs index b53a55799..7df7109c7 100644 --- a/clippy_dev/src/lib.rs +++ b/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> { lints .iter() diff --git a/clippy_lints/src/consts.rs b/clippy_lints/src/consts.rs index aeb82a0c8..dc70de485 100644 --- a/clippy_lints/src/consts.rs +++ b/clippy_lints/src/consts.rs @@ -324,7 +324,7 @@ impl<'c, 'cc> ConstEvalLateContext<'c, 'cc> { vec.iter().map(|elem| self.expr(elem)).collect::>() } - /// 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 { use rustc::mir::interpret::GlobalId; diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index bd9594274..0f95efe59 100644 --- a/clippy_lints/src/doc.rs +++ b/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, + in_trait_impl: bool, } impl DocMarkdown { pub fn new(valid_idents: FxHashSet) -> 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, attrs: &'a [ast::Attribute]) -> bool { +pub fn check_attrs<'a>(cx: &LateContext<'_, '_>, valid_idents: &FxHashSet, 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, } fn check_doc<'a, Events: Iterator, Range)>>( - cx: &EarlyContext<'_>, + cx: &LateContext<'_, '_>, valid_idents: &FxHashSet, events: Events, spans: &[(usize, Span)], @@ -283,6 +334,7 @@ fn check_doc<'a, Events: Iterator, Range, Range, 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, text: &str, span: Span) { +fn check_text(cx: &LateContext<'_, '_>, valid_idents: &FxHashSet, 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, 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). diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 10a14f3b9..48bf33b17 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -533,7 +533,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); diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index be233c6e6..3b208421b 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1895,7 +1895,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]); diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index e976b0557..b33d09121 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -1940,7 +1940,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. @@ -2118,7 +2118,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 { if let TyKind::Path(QPath::Resolved(None, ref path)) = hir_ty.kind { let params: Vec<_> = path diff --git a/tests/ui/doc_unsafe.rs b/tests/ui/doc_unsafe.rs index 7b26e86b4..98dbe2d4f 100644 --- a/tests/ui/doc_unsafe.rs +++ b/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(); + } } diff --git a/tests/ui/doc_unsafe.stderr b/tests/ui/doc_unsafe.stderr index d6d1cd2d4..468943068 100644 --- a/tests/ui/doc_unsafe.stderr +++ b/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 diff --git a/tests/ui/functions.rs b/tests/ui/functions.rs index f8de18dc6..7e2e083e2 100644 --- a/tests/ui/functions.rs +++ b/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) {} diff --git a/tests/ui/new_without_default.rs b/tests/ui/new_without_default.rs index dbc7d597b..781ea7bb1 100644 --- a/tests/ui/new_without_default.rs +++ b/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;