diff --git a/CHANGELOG.md b/CHANGELOG.md index abe975fa42b..2e9b755caa0 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 5a109fcc2cc..b440e267efe 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -140,6 +140,7 @@ 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, diff --git a/clippy_lints/src/doc/mod.rs b/clippy_lints/src/doc/mod.rs index a11d5622b4d..ba452775015 100644 --- a/clippy_lints/src/doc/mod.rs +++ b/clippy_lints/src/doc/mod.rs @@ -199,6 +199,39 @@ "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 @@ pub fn new(valid_idents: &[String], check_private_items: bool) -> Self { 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, Range, Range)> = Vec::new(); @@ -530,6 +565,8 @@ fn check_doc<'a, Events: Iterator, Range, Range { 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, Range, text: &str, edition: Edition, range: Range, fragments: Fragments<'_>) { - fn has_needless_main(code: String, edition: Edition) -> bool { +fn get_test_spans(item: &Item, test_attr_spans: &mut Vec>) { + 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, + 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>) { 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 @@ fn has_needless_main(code: String, edition: Edition) -> bool { 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 @@ fn has_needless_main(code: String, edition: Edition) -> bool { 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 @@ fn has_needless_main(code: String, edition: Edition) -> bool { // 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"); + } + } } diff --git a/tests/ui/test_attr_in_doctest.rs b/tests/ui/test_attr_in_doctest.rs new file mode 100644 index 00000000000..4c904f7a09a --- /dev/null +++ b/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() {} diff --git a/tests/ui/test_attr_in_doctest.stderr b/tests/ui/test_attr_in_doctest.stderr new file mode 100644 index 00000000000..605259f3bfb --- /dev/null +++ b/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 +