X-Git-Url: https://git.lizzy.rs/?a=blobdiff_plain;f=clippy_lints%2Fsrc%2Fdoc.rs;h=8f8d8ad96bbee14a9eb714cc0d05c23fd40efa9e;hb=e5a5b0a0774625eebbe7b29c67b49dc6431544d1;hp=413645a091ffc82f046a1aef35bde653b1f4169e;hpb=a9d002c8a53be53aab6c754b011fb699e2f51aa0;p=rust.git diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index 413645a091f..8f8d8ad96bb 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -1,11 +1,14 @@ -use crate::utils::span_lint; +use crate::utils::{match_type, paths, return_ty, span_lint}; use itertools::Itertools; use pulldown_cmark; -use rustc::lint::{EarlyContext, EarlyLintPass, LintArray, LintPass}; -use rustc::{declare_tool_lint, lint_array}; +use rustc::hir; +use rustc::impl_lint_pass; +use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use rustc_data_structures::fx::FxHashSet; -use syntax::ast; -use syntax::source_map::{BytePos, Span}; +use rustc_session::declare_tool_lint; +use std::ops::Range; +use syntax::ast::{AttrKind, Attribute}; +use syntax::source_map::{BytePos, MultiSpan, Span}; use syntax_pos::Pos; use url::Url; @@ -26,60 +29,189 @@ /// /// Do something with the foo_bar parameter. See also /// /// that::other::module::foo. /// // ^ `foo_bar` and `that::other::module::foo` should be ticked. - /// fn doit(foo_bar) { .. } + /// fn doit(foo_bar: usize) {} /// ``` pub DOC_MARKDOWN, pedantic, "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" +} + +declare_clippy_lint! { + /// **What it does:** Checks the doc comments of publicly visible functions that + /// return a `Result` type and warns if there is no `# Errors` section. + /// + /// **Why is this bad?** Documenting the type of errors that can be returned from a + /// function can help callers write code to handle the errors appropriately. + /// + /// **Known problems:** None. + /// + /// **Examples:** + /// + /// Since the following function returns a `Result` it has an `# Errors` section in + /// its doc comment: + /// + /// ```rust + ///# use std::io; + /// /// # Errors + /// /// + /// /// Will return `Err` if `filename` does not exist or the user does not have + /// /// permission to read it. + /// pub fn read(filename: String) -> io::Result { + /// unimplemented!(); + /// } + /// ``` + pub MISSING_ERRORS_DOC, + pedantic, + "`pub fn` returns `Result` without `# Errors` in doc comment" +} + +declare_clippy_lint! { + /// **What it does:** Checks for `fn main() { .. }` in doctests + /// + /// **Why is this bad?** The test can be shorter (and likely more readable) + /// if the `fn main()` is left implicit. + /// + /// **Known problems:** None. + /// + /// **Examples:** + /// ``````rust + /// /// An example of a doctest with a `main()` function + /// /// + /// /// # Examples + /// /// + /// /// ``` + /// /// fn main() { + /// /// // this needs not be in an `fn` + /// /// } + /// /// ``` + /// fn needless_main() { + /// unimplemented!(); + /// } + /// `````` + pub NEEDLESS_DOCTEST_MAIN, + style, + "presence of `fn main() {` in code examples" +} + +#[allow(clippy::module_name_repetitions)] #[derive(Clone)] -pub struct Doc { +pub struct DocMarkdown { valid_idents: FxHashSet, + in_trait_impl: bool, } -impl Doc { +impl DocMarkdown { pub fn new(valid_idents: FxHashSet) -> Self { - Self { valid_idents } + Self { + valid_idents, + in_trait_impl: false, + } } } -impl LintPass for Doc { - fn get_lints(&self) -> LintArray { - lint_array![DOC_MARKDOWN] - } +impl_lint_pass!(DocMarkdown => [DOC_MARKDOWN, MISSING_SAFETY_DOC, MISSING_ERRORS_DOC, NEEDLESS_DOCTEST_MAIN]); - fn name(&self) -> &'static str { - "DocMarkdown" +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); } -} -impl EarlyLintPass for Doc { - fn check_crate(&mut self, cx: &EarlyContext<'_>, krate: &ast::Crate) { - check_attrs(cx, &self.valid_idents, &krate.attrs); + fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::Item) { + let headers = check_attrs(cx, &self.valid_idents, &item.attrs); + match item.kind { + hir::ItemKind::Fn(ref sig, ..) => { + lint_for_missing_headers(cx, item.hir_id, item.span, sig, headers); + }, + hir::ItemKind::Impl(_, _, _, _, ref trait_ref, ..) => { + self.in_trait_impl = trait_ref.is_some(); + }, + _ => {}, + } } - fn check_item(&mut self, cx: &EarlyContext<'_>, item: &ast::Item) { - check_attrs(cx, &self.valid_idents, &item.attrs); + 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; + } } -} -struct Parser<'a> { - parser: pulldown_cmark::Parser<'a>, -} + fn check_trait_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::TraitItem) { + let headers = check_attrs(cx, &self.valid_idents, &item.attrs); + if let hir::TraitItemKind::Method(ref sig, ..) = item.kind { + lint_for_missing_headers(cx, item.hir_id, item.span, sig, headers); + } + } -impl<'a> Parser<'a> { - fn new(parser: pulldown_cmark::Parser<'a>) -> Self { - Self { parser } + fn check_impl_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::ImplItem) { + let headers = check_attrs(cx, &self.valid_idents, &item.attrs); + if self.in_trait_impl { + return; + } + if let hir::ImplItemKind::Method(ref sig, ..) = item.kind { + lint_for_missing_headers(cx, item.hir_id, item.span, sig, headers); + } } } -impl<'a> Iterator for Parser<'a> { - type Item = (usize, pulldown_cmark::Event<'a>); - - fn next(&mut self) -> Option { - let offset = self.parser.get_offset(); - self.parser.next().map(|event| (offset, event)) +fn lint_for_missing_headers<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + hir_id: hir::HirId, + span: impl Into + Copy, + sig: &hir::FnSig, + headers: DocHeaders, +) { + if !cx.access_levels.is_exported(hir_id) { + return; // Private functions do not require doc comments + } + if !headers.safety && sig.header.unsafety == hir::Unsafety::Unsafe { + span_lint( + cx, + MISSING_SAFETY_DOC, + span, + "unsafe function's docs miss `# Safety` section", + ); + } + if !headers.errors && match_type(cx, return_ty(cx, hir_id), &paths::RESULT) { + span_lint( + cx, + MISSING_ERRORS_DOC, + span, + "docs for function returning `Result` missing `# Errors` section", + ); } } @@ -90,6 +222,7 @@ fn next(&mut self) -> Option { /// need to keep track of /// the spans but this function is inspired from the later. #[allow(clippy::cast_possible_truncation)] +#[must_use] pub fn strip_doc_comment_decoration(comment: &str, span: Span) -> (String, Vec<(usize, Span)>) { // one-line comments lose their prefix const ONELINERS: &[&str] = &["///!", "///", "//!", "//"]; @@ -140,21 +273,29 @@ 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]) { +#[derive(Copy, Clone)] +struct DocHeaders { + safety: bool, + errors: bool, +} + +fn check_attrs<'a>(cx: &LateContext<'_, '_>, valid_idents: &FxHashSet, attrs: &'a [Attribute]) -> DocHeaders { let mut doc = String::new(); let mut spans = vec![]; for attr in attrs { - if attr.is_sugared_doc { - if let Some(ref current) = attr.value_str() { - let current = current.to_string(); - let (current, current_spans) = strip_doc_comment_decoration(¤t, attr.span); - spans.extend_from_slice(¤t_spans); - doc.push_str(¤t); - } - } else if attr.check_name("doc") { + if let AttrKind::DocComment(ref comment) = attr.kind { + let comment = comment.to_string(); + let (comment, current_spans) = strip_doc_comment_decoration(&comment, attr.span); + spans.extend_from_slice(¤t_spans); + doc.push_str(&comment); + } else if attr.check_name(sym!(doc)) { // ignore mix of sugared and non-sugared doc - return; + // don't trigger the safety or errors check + return DocHeaders { + safety: true, + errors: true, + }; } } @@ -165,48 +306,62 @@ pub fn check_attrs<'a>(cx: &EarlyContext<'_>, valid_idents: &FxHashSet, current += offset_copy; } - if !doc.is_empty() { - let parser = Parser::new(pulldown_cmark::Parser::new(&doc)); - let parser = parser.coalesce(|x, y| { - use pulldown_cmark::Event::*; - - let x_offset = x.0; - let y_offset = y.0; - - match (x.1, y.1) { - (Text(x), Text(y)) => { - let mut x = x.into_owned(); - x.push_str(&y); - Ok((x_offset, Text(x.into()))) - }, - (x, y) => Err(((x_offset, x), (y_offset, y))), - } - }); - check_doc(cx, valid_idents, parser, &spans); + if doc.is_empty() { + return DocHeaders { + safety: false, + errors: 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(¤t); + 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)>>( - cx: &EarlyContext<'_>, +fn check_doc<'a, Events: Iterator, Range)>>( + cx: &LateContext<'_, '_>, valid_idents: &FxHashSet, - docs: Events, + events: Events, spans: &[(usize, Span)], -) { +) -> DocHeaders { + // true if a safety header was found use pulldown_cmark::Event::*; use pulldown_cmark::Tag::*; + let mut headers = DocHeaders { + safety: false, + errors: false, + }; let mut in_code = false; let mut in_link = None; + let mut in_heading = false; - for (offset, event) in docs { + for (event, range) in events { match event { - Start(CodeBlock(_)) | Start(Code) => in_code = true, - End(CodeBlock(_)) | End(Code) => in_code = false, - Start(Link(link, _)) => in_link = Some(link), - 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 => (), + Start(CodeBlock(_)) => in_code = true, + End(CodeBlock(_)) => in_code = false, + Start(Link(_, url, _)) => in_link = Some(url), + End(Link(..)) => in_link = None, + 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 `` @@ -214,26 +369,34 @@ fn check_doc<'a, Events: Iterator)>>( // text "http://example.com" by pulldown-cmark continue; } - - if !in_code { - let index = match spans.binary_search_by(|c| c.0.cmp(&offset)) { - Ok(o) => o, - Err(e) => e - 1, - }; - - let (begin, span) = spans[index]; - + headers.safety |= in_heading && text.trim() == "Safety"; + headers.errors |= in_heading && text.trim() == "Errors"; + let index = match spans.binary_search_by(|c| c.0.cmp(&range.start)) { + Ok(o) => o, + Err(e) => e - 1, + }; + let (begin, span) = spans[index]; + if in_code { + check_code(cx, &text, span); + } else { // Adjust for the beginning of the current `Event` - let span = span.with_lo(span.lo() + BytePos::from_usize(offset - begin)); + let span = span.with_lo(span.lo() + BytePos::from_usize(range.start - begin)); check_text(cx, valid_idents, &text, span); } }, } } + headers +} + +fn check_code(cx: &LateContext<'_, '_>, text: &str, span: Span) { + if text.contains("fn main() {") && !(text.contains("static") || 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).` // ^^ @@ -256,7 +419,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).