From e3f143ff0abfb10ffdcd53e1dba27322e76dc1f6 Mon Sep 17 00:00:00 2001
From: Andre Bogus <bogusandre@gmail.com>
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 b53a5579971..7df7109c75f 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<String, Vec<Self>> {
lints
.iter()
diff --git a/clippy_lints/src/consts.rs b/clippy_lints/src/consts.rs
index aeb82a0c8cd..dc70de48503 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::<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;
diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs
index bd959427403..0f95efe59f1 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<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).
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 10a14f3b906..48bf33b175f 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 be233c6e69a..3b208421b5e 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 e976b055791..b33d0912177 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<Self> {
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 7b26e86b40b..98dbe2d4f54 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 d6d1cd2d4fa..4689430684d 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 f8de18dc624..7e2e083e298 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 dbc7d597b2c..781ea7bb152 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;