add lint against unit tests in doctests

Author: llogiq

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,36 @@ declare_clippy_lint! {
     "presence of `fn main() {` in code examples"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for `#[test]` in doctests
+    ///
+    /// ### Why is this bad?
+    /// Code in examples marked as `#[test]` will somewhat
+    /// surprisingly not be run by `cargo test`.
+    ///
+    /// ### 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)
@@ -330,6 +360,7 @@ impl_lint_pass!(DocMarkdown => [
     MISSING_ERRORS_DOC,
     MISSING_PANICS_DOC,
     NEEDLESS_DOCTEST_MAIN,
+    TEST_ATTR_IN_DOCTEST,
     UNNECESSARY_SAFETY_DOC,
     SUSPICIOUS_DOC_COMMENTS
 ]);

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,24 @@ 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;
 
+fn get_test_spans(item: &Item, test_attr_spans: &mut Vec<Range<usize>>) {
+    test_attr_spans.extend(
+        item.attrs
+            .iter()
+            .filter(|attr| attr.has_name(sym::test))
+            .map(|attr| attr.span.lo().to_usize()..attr.span.hi().to_usize()),
+    );
+}
+
 pub fn check(cx: &LateContext<'_>, text: &str, edition: Edition, range: Range<usize>, fragments: Fragments<'_>) {
-    fn has_needless_main(code: String, edition: Edition) -> bool {
+    fn check_code_sample(code: String, edition: Edition) -> (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 +45,19 @@ 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 => {
+                                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 +70,32 @@ 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;
+                                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 +107,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))
         .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,29 @@
+/// 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]
+///     ");
+/// }
+/// ```
+fn test_attr_in_doctests() {}

tests/ui/test_attr_in_doctest.stderr

@@ -0,0 +1,23 @@
+error: unit tests in doctest are not executed
+  --> $DIR/test_attr_in_doctest.rs:6:5
+   |
+LL | /// #[test]
+   |     ^^^^^^^
+   |
+   = 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]
+   |     ^^^^^^^
+
+error: unit tests in doctest are not executed
+  --> $DIR/test_attr_in_doctest.rs:22:5
+   |
+LL | /// #[test]
+   |     ^^^^^^^
+
+error: aborting due to 3 previous errors
+