Auto merge of #11872 - llogiq:test-attr-in-doctest, r=xFrednet

add lint against unit tests in doctests

During RustLab, Alice Ryhl brought to my attention that the Andoid team stumbled over the fact that if one attempts to write a unit test within a doctest, it will be summarily ignored. So this lint should help people wondering why their tests won't run.

---

changelog: New lint: [`test_attr_in_doctest`]
[#11872](#11872)

CHANGELOG.md

@@ -5545,6 +5545,7 @@ Released 2018-09-13
 [`tabs_in_doc_comments`]: https://rust-lang.github.io/rust-clippy/master/index.html#tabs_in_doc_comments
 [`temporary_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_assignment
 [`temporary_cstring_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_cstring_as_ptr
+[`test_attr_in_doctest`]: https://rust-lang.github.io/rust-clippy/master/index.html#test_attr_in_doctest
 [`tests_outside_test_module`]: https://rust-lang.github.io/rust-clippy/master/index.html#tests_outside_test_module
 [`to_digit_is_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_digit_is_some
 [`to_string_in_display`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_display

clippy_lints/src/declared_lints.rs

@@ -140,6 +140,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::doc::MISSING_SAFETY_DOC_INFO,
     crate::doc::NEEDLESS_DOCTEST_MAIN_INFO,
     crate::doc::SUSPICIOUS_DOC_COMMENTS_INFO,
+    crate::doc::TEST_ATTR_IN_DOCTEST_INFO,
     crate::doc::UNNECESSARY_SAFETY_DOC_INFO,
     crate::double_parens::DOUBLE_PARENS_INFO,
     crate::drop_forget_ref::DROP_NON_DROP_INFO,

clippy_lints/src/doc/mod.rs

@@ -199,6 +199,39 @@ declare_clippy_lint! {
     "presence of `fn main() {` in code examples"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for `#[test]` in doctests unless they are marked with
+    /// either `ignore`, `no_run` or `compile_fail`.
+    ///
+    /// ### Why is this bad?
+    /// Code in examples marked as `#[test]` will somewhat
+    /// surprisingly not be run by `cargo test`. If you really want
+    /// to show how to test stuff in an example, mark it `no_run` to
+    /// make the intent clear.
+    ///
+    /// ### Examples
+    /// ```no_run
+    /// /// An example of a doctest with a `main()` function
+    /// ///
+    /// /// # Examples
+    /// ///
+    /// /// ```
+    /// /// #[test]
+    /// /// fn equality_works() {
+    /// ///     assert_eq!(1_u8, 1);
+    /// /// }
+    /// /// ```
+    /// fn test_attr_in_doctest() {
+    ///     unimplemented!();
+    /// }
+    /// ```
+    #[clippy::version = "1.40.0"]
+    pub TEST_ATTR_IN_DOCTEST,
+    suspicious,
+    "presence of `#[test]` in code examples"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Detects the syntax `['foo']` in documentation comments (notice quotes instead of backticks)
@@ -329,6 +362,7 @@ impl_lint_pass!(Documentation => [
     MISSING_ERRORS_DOC,
     MISSING_PANICS_DOC,
     NEEDLESS_DOCTEST_MAIN,
+    TEST_ATTR_IN_DOCTEST,
     UNNECESSARY_SAFETY_DOC,
     SUSPICIOUS_DOC_COMMENTS
 ]);
@@ -515,6 +549,7 @@ fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize
     let mut in_heading = false;
     let mut is_rust = false;
     let mut no_test = false;
+    let mut ignore = false;
     let mut edition = None;
     let mut ticks_unbalanced = false;
     let mut text_to_check: Vec<(CowStr<'_>, Range<usize>)> = Vec::new();
@@ -530,6 +565,8 @@ fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize
                             break;
                         } else if item == "no_test" {
                             no_test = true;
+                        } else if item == "no_run" || item == "compile_fail" {
+                            ignore = true;
                         }
                         if let Some(stripped) = item.strip_prefix("edition") {
                             is_rust = true;
@@ -543,6 +580,7 @@ fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize
             End(CodeBlock(_)) => {
                 in_code = false;
                 is_rust = false;
+                ignore = false;
             },
             Start(Link(_, url, _)) => in_link = Some(url),
             End(Link(..)) => in_link = None,
@@ -596,7 +634,7 @@ fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize
                 if in_code {
                     if is_rust && !no_test {
                         let edition = edition.unwrap_or_else(|| cx.tcx.sess.edition());
-                        needless_doctest_main::check(cx, &text, edition, range.clone(), fragments);
+                        needless_doctest_main::check(cx, &text, edition, range.clone(), fragments, ignore);
                     }
                 } else {
                     if in_link.is_some() {

clippy_lints/src/doc/needless_doctest_main.rs

@@ -1,9 +1,9 @@
 use std::ops::Range;
 use std::{io, thread};
 
-use crate::doc::NEEDLESS_DOCTEST_MAIN;
+use crate::doc::{NEEDLESS_DOCTEST_MAIN, TEST_ATTR_IN_DOCTEST};
 use clippy_utils::diagnostics::span_lint;
-use rustc_ast::{Async, Fn, FnRetTy, ItemKind};
+use rustc_ast::{Async, Fn, FnRetTy, Item, ItemKind};
 use rustc_data_structures::sync::Lrc;
 use rustc_errors::emitter::EmitterWriter;
 use rustc_errors::Handler;
@@ -13,14 +13,33 @@ use rustc_parse::parser::ForceCollect;
 use rustc_session::parse::ParseSess;
 use rustc_span::edition::Edition;
 use rustc_span::source_map::{FilePathMapping, SourceMap};
-use rustc_span::{sym, FileName};
+use rustc_span::{sym, FileName, Pos};
 
 use super::Fragments;
 
-pub fn check(cx: &LateContext<'_>, text: &str, edition: Edition, range: Range<usize>, fragments: Fragments<'_>) {
-    fn has_needless_main(code: String, edition: Edition) -> bool {
+fn get_test_spans(item: &Item, test_attr_spans: &mut Vec<Range<usize>>) {
+    test_attr_spans.extend(
+        item.attrs
+            .iter()
+            .find(|attr| attr.has_name(sym::test))
+            .map(|attr| attr.span.lo().to_usize()..item.ident.span.hi().to_usize()),
+    );
+}
+
+pub fn check(
+    cx: &LateContext<'_>,
+    text: &str,
+    edition: Edition,
+    range: Range<usize>,
+    fragments: Fragments<'_>,
+    ignore: bool,
+) {
+    // return whether the code contains a needless `fn main` plus a vector of byte position ranges
+    // of all `#[test]` attributes in not ignored code examples
+    fn check_code_sample(code: String, edition: Edition, ignore: bool) -> (bool, Vec<Range<usize>>) {
         rustc_driver::catch_fatal_errors(|| {
             rustc_span::create_session_globals_then(edition, || {
+                let mut test_attr_spans = vec![];
                 let filename = FileName::anon_source_code(&code);
 
                 let fallback_bundle =
@@ -35,17 +54,21 @@ pub fn check(cx: &LateContext<'_>, text: &str, edition: Edition, range: Range<us
                     Ok(p) => p,
                     Err(errs) => {
                         drop(errs);
-                        return false;
+                        return (false, test_attr_spans);
                     },
                 };
 
                 let mut relevant_main_found = false;
+                let mut eligible = true;
                 loop {
                     match parser.parse_item(ForceCollect::No) {
                         Ok(Some(item)) => match &item.kind {
                             ItemKind::Fn(box Fn {
                                 sig, body: Some(block), ..
                             }) if item.ident.name == sym::main => {
+                                if !ignore {
+                                    get_test_spans(&item, &mut test_attr_spans);
+                                }
                                 let is_async = matches!(sig.header.asyncness, Async::Yes { .. });
                                 let returns_nothing = match &sig.decl.output {
                                     FnRetTy::Default(..) => true,
@@ -58,27 +81,34 @@ pub fn check(cx: &LateContext<'_>, text: &str, edition: Edition, range: Range<us
                                     relevant_main_found = true;
                                 } else {
                                     // This main function should not be linted, we're done
-                                    return false;
+                                    eligible = false;
+                                }
+                            },
+                            // Another function was found; this case is ignored for needless_doctest_main
+                            ItemKind::Fn(box Fn { .. }) => {
+                                eligible = false;
+                                if !ignore {
+                                    get_test_spans(&item, &mut test_attr_spans);
                                 }
                             },
                             // Tests with one of these items are ignored
                             ItemKind::Static(..)
                             | ItemKind::Const(..)
                             | ItemKind::ExternCrate(..)
-                            | ItemKind::ForeignMod(..)
-                            // Another function was found; this case is ignored
-                            | ItemKind::Fn(..) => return false,
+                            | ItemKind::ForeignMod(..) => {
+                                eligible = false;
+                            },
                             _ => {},
                         },
                         Ok(None) => break,
                         Err(e) => {
                             e.cancel();
-                            return false;
+                            return (false, test_attr_spans);
                         },
                     }
                 }
 
-                relevant_main_found
+                (relevant_main_found & eligible, test_attr_spans)
             })
         })
         .ok()
@@ -90,11 +120,16 @@ pub fn check(cx: &LateContext<'_>, text: &str, edition: Edition, range: Range<us
     // Because of the global session, we need to create a new session in a different thread with
     // the edition we need.
     let text = text.to_owned();
-    if thread::spawn(move || has_needless_main(text, edition))
+    let (has_main, test_attr_spans) = thread::spawn(move || check_code_sample(text, edition, ignore))
         .join()
-        .expect("thread::spawn failed")
-        && let Some(span) = fragments.span(cx, range.start..range.end - trailing_whitespace)
-    {
+        .expect("thread::spawn failed");
+    if has_main && let Some(span) = fragments.span(cx, range.start..range.end - trailing_whitespace) {
         span_lint(cx, NEEDLESS_DOCTEST_MAIN, span, "needless `fn main` in doctest");
     }
+    for span in test_attr_spans {
+        let span = (range.start + span.start)..(range.start + span.end);
+        if let Some(span) = fragments.span(cx, span) {
+            span_lint(cx, TEST_ATTR_IN_DOCTEST, span, "unit tests in doctest are not executed");
+        }
+    }
 }

tests/ui/test_attr_in_doctest.rs

@@ -0,0 +1,51 @@
+/// This is a test for `#[test]` in doctests
+///
+/// # Examples
+///
+/// ```
+/// #[test]
+/// fn should_be_linted() {
+///     assert_eq!(1, 1);
+/// }
+/// ```
+///
+/// Make sure we catch multiple tests in one example,
+/// and show that we really parse the attr:
+///
+/// ```
+/// #[test]
+/// fn should_also_be_linted() {
+///     #[cfg(test)]
+///     assert!(true);
+/// }
+///
+/// #[test]
+/// fn should_be_linted_too() {
+///     assert_eq!("#[test]", "
+///     #[test]
+///     ");
+/// }
+/// ```
+///
+/// We don't catch examples that aren't run:
+///
+/// ```ignore
+/// #[test]
+/// fn ignored() { todo!() }
+/// ```
+///
+/// ```no_run
+/// #[test]
+/// fn ignored() { todo!() }
+/// ```
+///
+/// ```compile_fail
+/// #[test]
+/// fn ignored() { Err(()) }
+/// ```
+///
+/// ```txt
+/// #[test]
+/// fn not_even_rust() { panic!("Ouch") }
+/// ```
+fn test_attr_in_doctests() {}

tests/ui/test_attr_in_doctest.stderr

@@ -0,0 +1,29 @@
+error: unit tests in doctest are not executed
+  --> $DIR/test_attr_in_doctest.rs:6:5
+   |
+LL |   /// #[test]
+   |  _____^
+LL | | /// fn should_be_linted() {
+   | |_______________________^
+   |
+   = note: `-D clippy::test-attr-in-doctest` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::test_attr_in_doctest)]`
+
+error: unit tests in doctest are not executed
+  --> $DIR/test_attr_in_doctest.rs:16:5
+   |
+LL |   /// #[test]
+   |  _____^
+LL | | /// fn should_also_be_linted() {
+   | |____________________________^
+
+error: unit tests in doctest are not executed
+  --> $DIR/test_attr_in_doctest.rs:22:5
+   |
+LL |   /// #[test]
+   |  _____^
+LL | | /// fn should_be_linted_too() {
+   | |___________________________^
+
+error: aborting due to 3 previous errors
+