Auto merge of #5912 - ebroto:needless_doctest_main_improvements, r=Manishearth,flip1995

Parse doctests in needless_doctest_main

This switches from text-based search to running the parser to avoid false positives. Inspired by how [rustdoc](3f3250500f/src/librustdoc/test.rs (L366)) handles this and by #4729.

cc @llogiq

changelog: Fix multiple false positives in [`needless_doctest_main`].

Fixes #5879
Fixes #4906
Fixes #5103
Fixes #4698

clippy_lints/src/doc.rs

@@ -1,16 +1,22 @@
 use crate::utils::{implements_trait, is_entrypoint_fn, is_type_diagnostic_item, return_ty, span_lint};
 use if_chain::if_chain;
 use itertools::Itertools;
-use rustc_ast::ast::{AttrKind, Attribute};
+use rustc_ast::ast::{Async, AttrKind, Attribute, FnRetTy, ItemKind};
 use rustc_ast::token::CommentKind;
 use rustc_data_structures::fx::FxHashSet;
+use rustc_data_structures::sync::Lrc;
+use rustc_errors::emitter::EmitterWriter;
+use rustc_errors::Handler;
 use rustc_hir as hir;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::lint::in_external_macro;
 use rustc_middle::ty;
+use rustc_parse::maybe_new_parser_from_source_str;
+use rustc_session::parse::ParseSess;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
-use rustc_span::source_map::{BytePos, MultiSpan, Span};
-use rustc_span::Pos;
+use rustc_span::source_map::{BytePos, FilePathMapping, MultiSpan, SourceMap, Span};
+use rustc_span::{FileName, Pos};
+use std::io;
 use std::ops::Range;
 use url::Url;
 
@@ -431,10 +437,67 @@ fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize
     headers
 }
 
-static LEAVE_MAIN_PATTERNS: &[&str] = &["static", "fn main() {}", "extern crate", "async fn main() {"];
-
 fn check_code(cx: &LateContext<'_>, text: &str, span: Span) {
-    if text.contains("fn main() {") && !LEAVE_MAIN_PATTERNS.iter().any(|p| text.contains(p)) {
+    fn has_needless_main(code: &str) -> bool {
+        let filename = FileName::anon_source_code(code);
+
+        let sm = Lrc::new(SourceMap::new(FilePathMapping::empty()));
+        let emitter = EmitterWriter::new(box io::sink(), None, false, false, false, None, false);
+        let handler = Handler::with_emitter(false, None, box emitter);
+        let sess = ParseSess::with_span_handler(handler, sm);
+
+        let mut parser = match maybe_new_parser_from_source_str(&sess, filename, code.into()) {
+            Ok(p) => p,
+            Err(errs) => {
+                for mut err in errs {
+                    err.cancel();
+                }
+                return false;
+            },
+        };
+
+        let mut relevant_main_found = false;
+        loop {
+            match parser.parse_item() {
+                Ok(Some(item)) => match &item.kind {
+                    // Tests with one of these items are ignored
+                    ItemKind::Static(..)
+                    | ItemKind::Const(..)
+                    | ItemKind::ExternCrate(..)
+                    | ItemKind::ForeignMod(..) => return false,
+                    // We found a main function ...
+                    ItemKind::Fn(_, sig, _, Some(block)) if item.ident.name == sym!(main) => {
+                        let is_async = matches!(sig.header.asyncness, Async::Yes{..});
+                        let returns_nothing = match &sig.decl.output {
+                            FnRetTy::Default(..) => true,
+                            FnRetTy::Ty(ty) if ty.kind.is_unit() => true,
+                            _ => false,
+                        };
+
+                        if returns_nothing && !is_async && !block.stmts.is_empty() {
+                            // This main function should be linted, but only if there are no other functions
+                            relevant_main_found = true;
+                        } else {
+                            // This main function should not be linted, we're done
+                            return false;
+                        }
+                    },
+                    // Another function was found; this case is ignored too
+                    ItemKind::Fn(..) => return false,
+                    _ => {},
+                },
+                Ok(None) => break,
+                Err(mut e) => {
+                    e.cancel();
+                    return false;
+                },
+            }
+        }
+
+        relevant_main_found
+    }
+
+    if has_needless_main(text) {
         span_lint(cx, NEEDLESS_DOCTEST_MAIN, span, "needless `fn main` in doctest");
     }
 }

tests/ui/needless_doc_main.rs

@@ -9,8 +9,14 @@
 /// }
 /// ```
 ///
-/// This should, too.
+/// With an explicit return type it should lint too
+/// ```
+/// fn main() -> () {
+///     unimplemented!();
+/// }
+/// ```
 ///
+/// This should, too.
 /// ```rust
 /// fn main() {
 ///     unimplemented!();
@@ -18,7 +24,6 @@
 /// ```
 ///
 /// This one too.
-///
 /// ```no_run
 /// fn main() {
 ///     unimplemented!();
@@ -33,6 +38,20 @@ fn bad_doctests() {}
 /// fn main(){}
 /// ```
 ///
+/// This shouldn't lint either, because main is async:
+/// ```
+/// async fn main() {
+///     assert_eq!(42, ANSWER);
+/// }
+/// ```
+///
+/// Same here, because the return type is not the unit type:
+/// ```
+/// fn main() -> Result<()> {
+///     Ok(())
+/// }
+/// ```
+///
 /// This shouldn't lint either, because there's a `static`:
 /// ```
 /// static ANSWER: i32 = 42;
@@ -42,6 +61,15 @@ fn bad_doctests() {}
 /// }
 /// ```
 ///
+/// This shouldn't lint either, because there's a `const`:
+/// ```
+/// fn main() {
+///     assert_eq!(42, ANSWER);
+/// }
+///
+/// const ANSWER: i32 = 42;
+/// ```
+///
 /// Neither should this lint because of `extern crate`:
 /// ```
 /// #![feature(test)]
@@ -51,8 +79,41 @@ fn bad_doctests() {}
 /// }
 /// ```
 ///
-/// We should not lint ignored examples:
+/// Neither should this lint because it has an extern block:
+/// ```
+/// extern {}
+/// fn main() {
+///     unimplemented!();
+/// }
+/// ```
 ///
+/// This should not lint because there is another function defined:
+/// ```
+/// fn fun() {}
+///
+/// fn main() {
+///     unimplemented!();
+/// }
+/// ```
+///
+/// We should not lint inside raw strings ...
+/// ```
+/// let string = r#"
+/// fn main() {
+///     unimplemented!();
+/// }
+/// "#;
+/// ```
+///
+/// ... or comments
+/// ```
+/// // fn main() {
+/// //     let _inception = 42;
+/// // }
+/// let _inception = 42;
+/// ```
+///
+/// We should not lint ignored examples:
 /// ```rust,ignore
 /// fn main() {
 ///     unimplemented!();
@@ -60,7 +121,6 @@ fn bad_doctests() {}
 /// ```
 ///
 /// Or even non-rust examples:
-///
 /// ```text
 /// fn main() {
 ///     is what starts the program

tests/ui/needless_doc_main.stderr

@@ -7,16 +7,22 @@ LL | /// fn main() {
    = note: `-D clippy::needless-doctest-main` implied by `-D warnings`
 
 error: needless `fn main` in doctest
-  --> $DIR/needless_doc_main.rs:15:4
+  --> $DIR/needless_doc_main.rs:14:4
+   |
+LL | /// fn main() -> () {
+   |    ^^^^^^^^^^^^^^^^^^
+
+error: needless `fn main` in doctest
+  --> $DIR/needless_doc_main.rs:21:4
    |
 LL | /// fn main() {
    |    ^^^^^^^^^^^^
 
 error: needless `fn main` in doctest
-  --> $DIR/needless_doc_main.rs:23:4
+  --> $DIR/needless_doc_main.rs:28:4
    |
 LL | /// fn main() {
    |    ^^^^^^^^^^^^
 
-error: aborting due to 3 previous errors
+error: aborting due to 4 previous errors