From d4b246bb117d451e34f1692fa34f9574b3d169e4 Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Wed, 4 Sep 2024 16:39:23 -0700 Subject: [PATCH] rustdoc: unify the short-circuit on all lints This is a bit of an experiment to see if it improves perf. --- src/librustdoc/passes/lint.rs | 33 ++- src/librustdoc/passes/lint/bare_urls.rs | 68 +++-- .../passes/lint/check_code_block_syntax.rs | 6 +- src/librustdoc/passes/lint/html_tags.rs | 258 +++++++++--------- .../passes/lint/redundant_explicit_links.rs | 7 +- .../passes/lint/unescaped_backticks.rs | 12 +- .../passes/lint/unportable_markdown.rs | 12 +- 7 files changed, 189 insertions(+), 207 deletions(-) diff --git a/src/librustdoc/passes/lint.rs b/src/librustdoc/passes/lint.rs index bc804a340bf..4da5d8f0e06 100644 --- a/src/librustdoc/passes/lint.rs +++ b/src/librustdoc/passes/lint.rs @@ -27,12 +27,33 @@ pub(crate) fn run_lints(krate: Crate, cx: &mut DocContext<'_>) -> Crate { impl<'a, 'tcx> DocVisitor for Linter<'a, 'tcx> { fn visit_item(&mut self, item: &Item) { - bare_urls::visit_item(self.cx, item); - check_code_block_syntax::visit_item(self.cx, item); - html_tags::visit_item(self.cx, item); - unescaped_backticks::visit_item(self.cx, item); - redundant_explicit_links::visit_item(self.cx, item); - unportable_markdown::visit_item(self.cx, item); + let Some(hir_id) = DocContext::as_local_hir_id(self.cx.tcx, item.item_id) else { + // If non-local, no need to check anything. + return; + }; + let dox = item.doc_value(); + if !dox.is_empty() { + let may_have_link = dox.contains(&[':', '['][..]); + let may_have_block_comment_or_html = dox.contains(&['<', '>']); + // ~~~rust + // // This is a real, supported commonmark syntax for block code + // ~~~ + let may_have_code = dox.contains(&['~', '`', '\t'][..]) || dox.contains(" "); + if may_have_link { + bare_urls::visit_item(self.cx, item, hir_id, &dox); + redundant_explicit_links::visit_item(self.cx, item, hir_id); + } + if may_have_code { + check_code_block_syntax::visit_item(self.cx, item, &dox); + unescaped_backticks::visit_item(self.cx, item, hir_id, &dox); + } + if may_have_block_comment_or_html { + html_tags::visit_item(self.cx, item, hir_id, &dox); + unportable_markdown::visit_item(self.cx, item, hir_id, &dox); + } else if may_have_link { + unportable_markdown::visit_item(self.cx, item, hir_id, &dox); + } + } self.visit_item_recur(item) } diff --git a/src/librustdoc/passes/lint/bare_urls.rs b/src/librustdoc/passes/lint/bare_urls.rs index 22051dd954d..bac0e07f1c1 100644 --- a/src/librustdoc/passes/lint/bare_urls.rs +++ b/src/librustdoc/passes/lint/bare_urls.rs @@ -8,6 +8,7 @@ use pulldown_cmark::{Event, Parser, Tag}; use regex::Regex; use rustc_errors::Applicability; +use rustc_hir::HirId; use rustc_resolve::rustdoc::source_span_for_markdown_range; use tracing::trace; @@ -15,50 +16,43 @@ use crate::core::DocContext; use crate::html::markdown::main_body_opts; -pub(super) fn visit_item(cx: &DocContext<'_>, item: &Item) { - let Some(hir_id) = DocContext::as_local_hir_id(cx.tcx, item.item_id) else { - // If non-local, no need to check anything. - return; +pub(super) fn visit_item(cx: &DocContext<'_>, item: &Item, hir_id: HirId, dox: &str) { + let report_diag = |cx: &DocContext<'_>, msg: &'static str, range: Range| { + let sp = source_span_for_markdown_range(cx.tcx, &dox, &range, &item.attrs.doc_strings) + .unwrap_or_else(|| item.attr_span(cx.tcx)); + cx.tcx.node_span_lint(crate::lint::BARE_URLS, hir_id, sp, |lint| { + lint.primary_message(msg) + .note("bare URLs are not automatically turned into clickable links") + .multipart_suggestion( + "use an automatic link instead", + vec![ + (sp.shrink_to_lo(), "<".to_string()), + (sp.shrink_to_hi(), ">".to_string()), + ], + Applicability::MachineApplicable, + ); + }); }; - let dox = item.doc_value(); - if !dox.is_empty() { - let report_diag = |cx: &DocContext<'_>, msg: &'static str, range: Range| { - let sp = source_span_for_markdown_range(cx.tcx, &dox, &range, &item.attrs.doc_strings) - .unwrap_or_else(|| item.attr_span(cx.tcx)); - cx.tcx.node_span_lint(crate::lint::BARE_URLS, hir_id, sp, |lint| { - lint.primary_message(msg) - .note("bare URLs are not automatically turned into clickable links") - .multipart_suggestion( - "use an automatic link instead", - vec![ - (sp.shrink_to_lo(), "<".to_string()), - (sp.shrink_to_hi(), ">".to_string()), - ], - Applicability::MachineApplicable, - ); - }); - }; - let mut p = Parser::new_ext(&dox, main_body_opts()).into_offset_iter(); + let mut p = Parser::new_ext(&dox, main_body_opts()).into_offset_iter(); - while let Some((event, range)) = p.next() { - match event { - Event::Text(s) => find_raw_urls(cx, &s, range, &report_diag), - // We don't want to check the text inside code blocks or links. - Event::Start(tag @ (Tag::CodeBlock(_) | Tag::Link { .. })) => { - while let Some((event, _)) = p.next() { - match event { - Event::End(end) - if mem::discriminant(&end) == mem::discriminant(&tag.to_end()) => - { - break; - } - _ => {} + while let Some((event, range)) = p.next() { + match event { + Event::Text(s) => find_raw_urls(cx, &s, range, &report_diag), + // We don't want to check the text inside code blocks or links. + Event::Start(tag @ (Tag::CodeBlock(_) | Tag::Link { .. })) => { + while let Some((event, _)) = p.next() { + match event { + Event::End(end) + if mem::discriminant(&end) == mem::discriminant(&tag.to_end()) => + { + break; } + _ => {} } } - _ => {} } + _ => {} } } } diff --git a/src/librustdoc/passes/lint/check_code_block_syntax.rs b/src/librustdoc/passes/lint/check_code_block_syntax.rs index 977c0953336..1b2431a629b 100644 --- a/src/librustdoc/passes/lint/check_code_block_syntax.rs +++ b/src/librustdoc/passes/lint/check_code_block_syntax.rs @@ -15,10 +15,8 @@ use crate::core::DocContext; use crate::html::markdown::{self, RustCodeBlock}; -pub(crate) fn visit_item(cx: &DocContext<'_>, item: &clean::Item) { - if let Some(def_id) = item.item_id.as_local_def_id() - && let Some(dox) = &item.opt_doc_value() - { +pub(crate) fn visit_item(cx: &DocContext<'_>, item: &clean::Item, dox: &str) { + if let Some(def_id) = item.item_id.as_local_def_id() { let sp = item.attr_span(cx.tcx); let extra = crate::html::markdown::ExtraInfo::new(cx.tcx, def_id, sp); for code_block in markdown::rust_code_blocks(dox, &extra) { diff --git a/src/librustdoc/passes/lint/html_tags.rs b/src/librustdoc/passes/lint/html_tags.rs index 6f9e9d36a5c..223174838ad 100644 --- a/src/librustdoc/passes/lint/html_tags.rs +++ b/src/librustdoc/passes/lint/html_tags.rs @@ -5,159 +5,149 @@ use std::str::CharIndices; use pulldown_cmark::{BrokenLink, Event, LinkType, Parser, Tag, TagEnd}; +use rustc_hir::HirId; use rustc_resolve::rustdoc::source_span_for_markdown_range; use crate::clean::*; use crate::core::DocContext; use crate::html::markdown::main_body_opts; -pub(crate) fn visit_item(cx: &DocContext<'_>, item: &Item) { +pub(crate) fn visit_item(cx: &DocContext<'_>, item: &Item, hir_id: HirId, dox: &str) { let tcx = cx.tcx; - let Some(hir_id) = DocContext::as_local_hir_id(tcx, item.item_id) - // If non-local, no need to check anything. - else { - return; - }; - let dox = item.doc_value(); - if !dox.is_empty() { - let report_diag = |msg: String, range: &Range, is_open_tag: bool| { - let sp = match source_span_for_markdown_range(tcx, &dox, range, &item.attrs.doc_strings) + let report_diag = |msg: String, range: &Range, is_open_tag: bool| { + let sp = match source_span_for_markdown_range(tcx, &dox, range, &item.attrs.doc_strings) { + Some(sp) => sp, + None => item.attr_span(tcx), + }; + tcx.node_span_lint(crate::lint::INVALID_HTML_TAGS, hir_id, sp, |lint| { + use rustc_lint_defs::Applicability; + + lint.primary_message(msg); + + // If a tag looks like ``, it might actually be a generic. + // We don't try to detect stuff `` because that's not valid HTML, + // and we don't try to detect stuff `` because that's not valid Rust. + let mut generics_end = range.end; + if let Some(Some(mut generics_start)) = (is_open_tag + && dox[..generics_end].ends_with('>')) + .then(|| extract_path_backwards(&dox, range.start)) { - Some(sp) => sp, - None => item.attr_span(tcx), - }; - tcx.node_span_lint(crate::lint::INVALID_HTML_TAGS, hir_id, sp, |lint| { - use rustc_lint_defs::Applicability; - - lint.primary_message(msg); - - // If a tag looks like ``, it might actually be a generic. - // We don't try to detect stuff `` because that's not valid HTML, - // and we don't try to detect stuff `` because that's not valid Rust. - let mut generics_end = range.end; - if let Some(Some(mut generics_start)) = (is_open_tag - && dox[..generics_end].ends_with('>')) - .then(|| extract_path_backwards(&dox, range.start)) + while generics_start != 0 + && generics_end < dox.len() + && dox.as_bytes()[generics_start - 1] == b'<' + && dox.as_bytes()[generics_end] == b'>' { - while generics_start != 0 - && generics_end < dox.len() - && dox.as_bytes()[generics_start - 1] == b'<' - && dox.as_bytes()[generics_end] == b'>' - { - generics_end += 1; - generics_start -= 1; - if let Some(new_start) = extract_path_backwards(&dox, generics_start) { - generics_start = new_start; - } - if let Some(new_end) = extract_path_forward(&dox, generics_end) { - generics_end = new_end; - } + generics_end += 1; + generics_start -= 1; + if let Some(new_start) = extract_path_backwards(&dox, generics_start) { + generics_start = new_start; } if let Some(new_end) = extract_path_forward(&dox, generics_end) { generics_end = new_end; } - let generics_sp = match source_span_for_markdown_range( - tcx, - &dox, - &(generics_start..generics_end), - &item.attrs.doc_strings, - ) { - Some(sp) => sp, - None => item.attr_span(tcx), - }; - // Sometimes, we only extract part of a path. For example, consider this: - // - // <[u32] as IntoIter>::Item - // ^^^^^ unclosed HTML tag `u32` - // - // We don't have any code for parsing fully-qualified trait paths. - // In theory, we could add it, but doing it correctly would require - // parsing the entire path grammar, which is problematic because of - // overlap between the path grammar and Markdown. - // - // The example above shows that ambiguity. Is `[u32]` intended to be an - // intra-doc link to the u32 primitive, or is it intended to be a slice? - // - // If the below conditional were removed, we would suggest this, which is - // not what the user probably wants. - // - // <[u32] as `IntoIter`>::Item - // - // We know that the user actually wants to wrap the whole thing in a code - // block, but the only reason we know that is because `u32` does not, in - // fact, implement IntoIter. If the example looks like this: - // - // <[Vec] as IntoIter::Item - // - // The ideal fix would be significantly different. - if (generics_start > 0 && dox.as_bytes()[generics_start - 1] == b'<') - || (generics_end < dox.len() && dox.as_bytes()[generics_end] == b'>') - { - return; - } - // multipart form is chosen here because ``Vec`` would be confusing. - lint.multipart_suggestion( - "try marking as source code", - vec![ - (generics_sp.shrink_to_lo(), String::from("`")), - (generics_sp.shrink_to_hi(), String::from("`")), - ], - Applicability::MaybeIncorrect, - ); } - }); - }; - - let mut tags = Vec::new(); - let mut is_in_comment = None; - let mut in_code_block = false; - - let link_names = item.link_names(&cx.cache); - - let mut replacer = |broken_link: BrokenLink<'_>| { - if let Some(link) = - link_names.iter().find(|link| *link.original_text == *broken_link.reference) - { - Some((link.href.as_str().into(), link.new_text.to_string().into())) - } else if matches!( - &broken_link.link_type, - LinkType::Reference | LinkType::ReferenceUnknown - ) { - // If the link is shaped [like][this], suppress any broken HTML in the [this] part. - // The `broken_intra_doc_links` will report typos in there anyway. - Some(( - broken_link.reference.to_string().into(), - broken_link.reference.to_string().into(), - )) - } else { - None - } - }; - - let p = Parser::new_with_broken_link_callback(&dox, main_body_opts(), Some(&mut replacer)) - .into_offset_iter(); - - for (event, range) in p { - match event { - Event::Start(Tag::CodeBlock(_)) => in_code_block = true, - Event::Html(text) | Event::InlineHtml(text) if !in_code_block => { - extract_tags(&mut tags, &text, range, &mut is_in_comment, &report_diag) + if let Some(new_end) = extract_path_forward(&dox, generics_end) { + generics_end = new_end; } - Event::End(TagEnd::CodeBlock) => in_code_block = false, - _ => {} + let generics_sp = match source_span_for_markdown_range( + tcx, + &dox, + &(generics_start..generics_end), + &item.attrs.doc_strings, + ) { + Some(sp) => sp, + None => item.attr_span(tcx), + }; + // Sometimes, we only extract part of a path. For example, consider this: + // + // <[u32] as IntoIter>::Item + // ^^^^^ unclosed HTML tag `u32` + // + // We don't have any code for parsing fully-qualified trait paths. + // In theory, we could add it, but doing it correctly would require + // parsing the entire path grammar, which is problematic because of + // overlap between the path grammar and Markdown. + // + // The example above shows that ambiguity. Is `[u32]` intended to be an + // intra-doc link to the u32 primitive, or is it intended to be a slice? + // + // If the below conditional were removed, we would suggest this, which is + // not what the user probably wants. + // + // <[u32] as `IntoIter`>::Item + // + // We know that the user actually wants to wrap the whole thing in a code + // block, but the only reason we know that is because `u32` does not, in + // fact, implement IntoIter. If the example looks like this: + // + // <[Vec] as IntoIter::Item + // + // The ideal fix would be significantly different. + if (generics_start > 0 && dox.as_bytes()[generics_start - 1] == b'<') + || (generics_end < dox.len() && dox.as_bytes()[generics_end] == b'>') + { + return; + } + // multipart form is chosen here because ``Vec`` would be confusing. + lint.multipart_suggestion( + "try marking as source code", + vec![ + (generics_sp.shrink_to_lo(), String::from("`")), + (generics_sp.shrink_to_hi(), String::from("`")), + ], + Applicability::MaybeIncorrect, + ); } - } + }); + }; - for (tag, range) in tags.iter().filter(|(t, _)| { - let t = t.to_lowercase(); - !ALLOWED_UNCLOSED.contains(&t.as_str()) - }) { - report_diag(format!("unclosed HTML tag `{tag}`"), range, true); - } + let mut tags = Vec::new(); + let mut is_in_comment = None; + let mut in_code_block = false; - if let Some(range) = is_in_comment { - report_diag("Unclosed HTML comment".to_string(), &range, false); + let link_names = item.link_names(&cx.cache); + + let mut replacer = |broken_link: BrokenLink<'_>| { + if let Some(link) = + link_names.iter().find(|link| *link.original_text == *broken_link.reference) + { + Some((link.href.as_str().into(), link.new_text.to_string().into())) + } else if matches!(&broken_link.link_type, LinkType::Reference | LinkType::ReferenceUnknown) + { + // If the link is shaped [like][this], suppress any broken HTML in the [this] part. + // The `broken_intra_doc_links` will report typos in there anyway. + Some(( + broken_link.reference.to_string().into(), + broken_link.reference.to_string().into(), + )) + } else { + None } + }; + + let p = Parser::new_with_broken_link_callback(&dox, main_body_opts(), Some(&mut replacer)) + .into_offset_iter(); + + for (event, range) in p { + match event { + Event::Start(Tag::CodeBlock(_)) => in_code_block = true, + Event::Html(text) | Event::InlineHtml(text) if !in_code_block => { + extract_tags(&mut tags, &text, range, &mut is_in_comment, &report_diag) + } + Event::End(TagEnd::CodeBlock) => in_code_block = false, + _ => {} + } + } + + for (tag, range) in tags.iter().filter(|(t, _)| { + let t = t.to_lowercase(); + !ALLOWED_UNCLOSED.contains(&t.as_str()) + }) { + report_diag(format!("unclosed HTML tag `{tag}`"), range, true); + } + + if let Some(range) = is_in_comment { + report_diag("Unclosed HTML comment".to_string(), &range, false); } } diff --git a/src/librustdoc/passes/lint/redundant_explicit_links.rs b/src/librustdoc/passes/lint/redundant_explicit_links.rs index 0a90c039dfb..9c37e11349a 100644 --- a/src/librustdoc/passes/lint/redundant_explicit_links.rs +++ b/src/librustdoc/passes/lint/redundant_explicit_links.rs @@ -24,12 +24,7 @@ struct LinkData { display_link: String, } -pub(crate) fn visit_item(cx: &DocContext<'_>, item: &Item) { - let Some(hir_id) = DocContext::as_local_hir_id(cx.tcx, item.item_id) else { - // If non-local, no need to check anything. - return; - }; - +pub(crate) fn visit_item(cx: &DocContext<'_>, item: &Item, hir_id: HirId) { let hunks = prepare_to_doc_link_resolution(&item.attrs.doc_strings); for (item_id, doc) in hunks { if let Some(item_id) = item_id.or(item.def_id()) diff --git a/src/librustdoc/passes/lint/unescaped_backticks.rs b/src/librustdoc/passes/lint/unescaped_backticks.rs index a6c8db16f82..d79f682a580 100644 --- a/src/librustdoc/passes/lint/unescaped_backticks.rs +++ b/src/librustdoc/passes/lint/unescaped_backticks.rs @@ -4,6 +4,7 @@ use pulldown_cmark::{BrokenLink, Event, Parser}; use rustc_errors::Diag; +use rustc_hir::HirId; use rustc_lint_defs::Applicability; use rustc_resolve::rustdoc::source_span_for_markdown_range; @@ -11,17 +12,8 @@ use crate::core::DocContext; use crate::html::markdown::main_body_opts; -pub(crate) fn visit_item(cx: &DocContext<'_>, item: &Item) { +pub(crate) fn visit_item(cx: &DocContext<'_>, item: &Item, hir_id: HirId, dox: &str) { let tcx = cx.tcx; - let Some(hir_id) = DocContext::as_local_hir_id(tcx, item.item_id) else { - // If non-local, no need to check anything. - return; - }; - - let dox = item.doc_value(); - if dox.is_empty() { - return; - } let link_names = item.link_names(&cx.cache); let mut replacer = |broken_link: BrokenLink<'_>| { diff --git a/src/librustdoc/passes/lint/unportable_markdown.rs b/src/librustdoc/passes/lint/unportable_markdown.rs index 87fe0055883..f8368a866c8 100644 --- a/src/librustdoc/passes/lint/unportable_markdown.rs +++ b/src/librustdoc/passes/lint/unportable_markdown.rs @@ -12,6 +12,7 @@ use std::collections::{BTreeMap, BTreeSet}; +use rustc_hir::HirId; use rustc_lint_defs::Applicability; use rustc_resolve::rustdoc::source_span_for_markdown_range; use {pulldown_cmark as cmarkn, pulldown_cmark_old as cmarko}; @@ -19,17 +20,8 @@ use crate::clean::Item; use crate::core::DocContext; -pub(crate) fn visit_item(cx: &DocContext<'_>, item: &Item) { +pub(crate) fn visit_item(cx: &DocContext<'_>, item: &Item, hir_id: HirId, dox: &str) { let tcx = cx.tcx; - let Some(hir_id) = DocContext::as_local_hir_id(tcx, item.item_id) else { - // If non-local, no need to check anything. - return; - }; - - let dox = item.doc_value(); - if dox.is_empty() { - return; - } // P1: unintended strikethrough was fixed by requiring single-tildes to flank // the same way underscores do, so nothing is done here