From 39bcf8e55438016fb86427f9b8a78b6a32a84126 Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Tue, 15 Dec 2020 23:18:03 +0100 Subject: [PATCH 1/3] Handle fatal errors when parsing doctests --- clippy_lints/src/doc.rs | 112 ++++++++++++++++++---------------- clippy_lints/src/lib.rs | 1 + tests/ui/needless_doc_main.rs | 6 ++ 3 files changed, 66 insertions(+), 53 deletions(-) diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index 55e4755c278..77203a286dd 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -451,66 +451,72 @@ fn check_doc<'a, Events: Iterator, Range, text: &str, span: Span) { - fn has_needless_main(code: &str) -> bool { - let filename = FileName::anon_source_code(code); + fn has_needless_main(cx: &LateContext<'_>, code: &str) -> bool { + rustc_driver::catch_fatal_errors(|| { + rustc_span::with_session_globals(cx.tcx.sess.edition(), || { + 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 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; + 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; }, - // 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 + 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 + }) + }) + .ok() + .unwrap_or_default() } - if has_needless_main(text) { + if has_needless_main(cx, text) { span_lint(cx, NEEDLESS_DOCTEST_MAIN, span, "needless `fn main` in doctest"); } } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index f06926fa91d..651b9e71131 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -27,6 +27,7 @@ extern crate rustc_ast; extern crate rustc_ast_pretty; extern crate rustc_attr; extern crate rustc_data_structures; +extern crate rustc_driver; extern crate rustc_errors; extern crate rustc_hir; extern crate rustc_hir_pretty; diff --git a/tests/ui/needless_doc_main.rs b/tests/ui/needless_doc_main.rs index 883683e08a2..52c84089fa8 100644 --- a/tests/ui/needless_doc_main.rs +++ b/tests/ui/needless_doc_main.rs @@ -128,6 +128,12 @@ fn bad_doctests() {} /// ``` fn no_false_positives() {} +/// Yields a parse error when interpreted as rust code: +/// ``` +/// r#"hi" +/// ``` +fn issue_6022() {} + fn main() { bad_doctests(); no_false_positives(); From 41b5ebebfdc583d9e3702dd61c8d897999d1e7f5 Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Wed, 16 Dec 2020 00:14:47 +0100 Subject: [PATCH 2/3] needless_doctest_main: add edition support --- clippy_lints/src/doc.rs | 26 ++++++++++++++++++-------- tests/ui/needless_doc_main.rs | 4 ++-- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index 77203a286dd..08134cc16c0 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -14,6 +14,7 @@ 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::edition::Edition; use rustc_span::source_map::{BytePos, FilePathMapping, MultiSpan, SourceMap, Span}; use rustc_span::{sym, FileName, Pos}; use std::io; @@ -377,7 +378,7 @@ fn check_attrs<'a>(cx: &LateContext<'_>, valid_idents: &FxHashSet, attrs check_doc(cx, valid_idents, events, &spans) } -const RUST_CODE: &[&str] = &["rust", "no_run", "should_panic", "compile_fail", "edition2018"]; +const RUST_CODE: &[&str] = &["rust", "no_run", "should_panic", "compile_fail"]; fn check_doc<'a, Events: Iterator, Range)>>( cx: &LateContext<'_>, @@ -400,13 +401,21 @@ fn check_doc<'a, Events: Iterator, Range { in_code = true; if let CodeBlockKind::Fenced(lang) = kind { - is_rust = - lang.is_empty() || !lang.contains("ignore") && lang.split(',').any(|i| RUST_CODE.contains(&i)); + let infos = lang.split(',').collect::>(); + is_rust = !infos.iter().any(|&i| i == "ignore") + && infos + .iter() + .any(|i| i.is_empty() || i.starts_with("edition") || RUST_CODE.contains(&i)); + edition = infos + .iter() + .find_map(|i| i.starts_with("edition").then(|| i[7..].parse::().ok())) + .flatten(); } }, End(CodeBlock(_)) => { @@ -436,7 +445,8 @@ fn check_doc<'a, Events: Iterator, Range, Range, text: &str, span: Span) { - fn has_needless_main(cx: &LateContext<'_>, code: &str) -> bool { +fn check_code(cx: &LateContext<'_>, text: &str, edition: Edition, span: Span) { + fn has_needless_main(code: &str, edition: Edition) -> bool { rustc_driver::catch_fatal_errors(|| { - rustc_span::with_session_globals(cx.tcx.sess.edition(), || { + rustc_span::with_session_globals(edition, || { let filename = FileName::anon_source_code(code); let sm = Lrc::new(SourceMap::new(FilePathMapping::empty())); @@ -516,7 +526,7 @@ fn check_code(cx: &LateContext<'_>, text: &str, span: Span) { .unwrap_or_default() } - if has_needless_main(cx, text) { + if has_needless_main(text, edition) { span_lint(cx, NEEDLESS_DOCTEST_MAIN, span, "needless `fn main` in doctest"); } } diff --git a/tests/ui/needless_doc_main.rs b/tests/ui/needless_doc_main.rs index 52c84089fa8..83e9bbaa3af 100644 --- a/tests/ui/needless_doc_main.rs +++ b/tests/ui/needless_doc_main.rs @@ -10,7 +10,7 @@ /// ``` /// /// With an explicit return type it should lint too -/// ``` +/// ```edition2015 /// fn main() -> () { /// unimplemented!(); /// } @@ -39,7 +39,7 @@ fn bad_doctests() {} /// ``` /// /// This shouldn't lint either, because main is async: -/// ``` +/// ```edition2018 /// async fn main() { /// assert_eq!(42, ANSWER); /// } From bb68ec6cfc76a6ec69d21c0f64dbc4aa99158958 Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Thu, 17 Dec 2020 15:24:44 +0100 Subject: [PATCH 3/3] Apply suggestion from PR review --- clippy_lints/src/doc.rs | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index 08134cc16c0..aba65532795 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -407,15 +407,18 @@ fn check_doc<'a, Events: Iterator, Range { in_code = true; if let CodeBlockKind::Fenced(lang) = kind { - let infos = lang.split(',').collect::>(); - is_rust = !infos.iter().any(|&i| i == "ignore") - && infos - .iter() - .any(|i| i.is_empty() || i.starts_with("edition") || RUST_CODE.contains(&i)); - edition = infos - .iter() - .find_map(|i| i.starts_with("edition").then(|| i[7..].parse::().ok())) - .flatten(); + for item in lang.split(',') { + if item == "ignore" { + is_rust = false; + break; + } + if let Some(stripped) = item.strip_prefix("edition") { + is_rust = true; + edition = stripped.parse::().ok(); + } else if item.is_empty() || RUST_CODE.contains(&item) { + is_rust = true; + } + } } }, End(CodeBlock(_)) => {