Auto merge of #77119 - GuillaumeGomez:unclosed-html-tag-lint, r=jyn514

Unclosed html tag lint Part of #67799. I think `@ollie27` will be interested (`@Manishearth` too since they opened the issue ;) ). r? `@jyn514`
2020-10-07 09:56:51 +00:00 · 2020-10-07 09:56:51 +00:00 · 8ae3b50976
parent 5296ac6565 d3b7b7e23a
commit 8ae3b50976
11 changed files with 428 additions and 5 deletions
--- a/compiler/rustc_lint/src/lib.rs
+++ b/compiler/rustc_lint/src/lib.rs
@ -64,8 +64,8 @@ use rustc_middle::ty::query::Providers;
 use rustc_middle::ty::TyCtxt;
 use rustc_session::lint::builtin::{
    BARE_TRAIT_OBJECTS, BROKEN_INTRA_DOC_LINKS, ELIDED_LIFETIMES_IN_PATHS,
-    EXPLICIT_OUTLIVES_REQUIREMENTS, INVALID_CODEBLOCK_ATTRIBUTES, MISSING_DOC_CODE_EXAMPLES,
-    PRIVATE_DOC_TESTS,
+    EXPLICIT_OUTLIVES_REQUIREMENTS, INVALID_CODEBLOCK_ATTRIBUTES, INVALID_HTML_TAGS,
+    MISSING_DOC_CODE_EXAMPLES, PRIVATE_DOC_TESTS,
 };
 use rustc_span::symbol::{Ident, Symbol};
 use rustc_span::Span;
@ -311,7 +311,8 @@ fn register_builtins(store: &mut LintStore, no_interleave_lints: bool) {
        PRIVATE_INTRA_DOC_LINKS,
        INVALID_CODEBLOCK_ATTRIBUTES,
        MISSING_DOC_CODE_EXAMPLES,
-        PRIVATE_DOC_TESTS
+        PRIVATE_DOC_TESTS,
+        INVALID_HTML_TAGS
    );

    // Register renamed and removed lints.
--- a/compiler/rustc_session/src/lint/builtin.rs
+++ b/compiler/rustc_session/src/lint/builtin.rs
@ -1881,6 +1881,16 @@ declare_lint! {
    "detects code samples in docs of private items not documented by rustdoc"
 }

+declare_lint! {
+    /// The `invalid_html_tags` lint detects invalid HTML tags. This is a
+    /// `rustdoc` only lint, see the documentation in the [rustdoc book].
+    ///
+    /// [rustdoc book]: ../../../rustdoc/lints.html#invalid_html_tags
+    pub INVALID_HTML_TAGS,
+    Allow,
+    "detects invalid HTML tags in doc comments"
+}
+
 declare_lint! {
    /// The `where_clauses_object_safety` lint detects for [object safety] of
    /// [where clauses].
@ -2699,6 +2709,7 @@ declare_lint_pass! {
        INVALID_CODEBLOCK_ATTRIBUTES,
        MISSING_CRATE_LEVEL_DOCS,
        MISSING_DOC_CODE_EXAMPLES,
+        INVALID_HTML_TAGS,
        PRIVATE_DOC_TESTS,
        WHERE_CLAUSES_OBJECT_SAFETY,
        PROC_MACRO_DERIVE_RESOLUTION_FALLBACK,
--- a/src/doc/rustdoc/src/lints.md
+++ b/src/doc/rustdoc/src/lints.md
@ -250,3 +250,38 @@ warning: unknown attribute `should-panic`. Did you mean `should_panic`?

 In the example above, the correct form is `should_panic`. This helps detect
 typo mistakes for some common attributes.
+
+## invalid_html_tags
+
+This lint is **allowed by default** and is **nightly-only**. It detects unclosed
+or invalid HTML tags. For example:
+
+```rust
+#![warn(invalid_html_tags)]
+
+/// <h1>
+/// </script>
+pub fn foo() {}
+```
+
+Which will give:
+
+```text
+warning: unopened HTML tag `script`
+ --> foo.rs:1:1
+  |
+1 | / /// <h1>
+2 | | /// </script>
+  | |_____________^
+  |
+  = note: `#[warn(invalid_html_tags)]` on by default
+
+warning: unclosed HTML tag `h1`
+ --> foo.rs:1:1
+  |
+1 | / /// <h1>
+2 | | /// </script>
+  | |_____________^
+
+warning: 2 warnings emitted
+```
--- a/src/librustdoc/core.rs
+++ b/src/librustdoc/core.rs
@ -328,6 +328,7 @@ pub fn run_core(
    let private_doc_tests = rustc_lint::builtin::PRIVATE_DOC_TESTS.name;
    let no_crate_level_docs = rustc_lint::builtin::MISSING_CRATE_LEVEL_DOCS.name;
    let invalid_codeblock_attributes_name = rustc_lint::builtin::INVALID_CODEBLOCK_ATTRIBUTES.name;
+    let invalid_html_tags = rustc_lint::builtin::INVALID_HTML_TAGS.name;
    let renamed_and_removed_lints = rustc_lint::builtin::RENAMED_AND_REMOVED_LINTS.name;
    let unknown_lints = rustc_lint::builtin::UNKNOWN_LINTS.name;

@ -340,6 +341,7 @@ pub fn run_core(
        private_doc_tests.to_owned(),
        no_crate_level_docs.to_owned(),
        invalid_codeblock_attributes_name.to_owned(),
+        invalid_html_tags.to_owned(),
        renamed_and_removed_lints.to_owned(),
        unknown_lints.to_owned(),
    ];
--- a/src/librustdoc/html/markdown.rs
+++ b/src/librustdoc/html/markdown.rs
@ -43,7 +43,7 @@ use pulldown_cmark::{html, BrokenLink, CodeBlockKind, CowStr, Event, Options, Pa
 #[cfg(test)]
 mod tests;

-fn opts() -> Options {
+pub(crate) fn opts() -> Options {
    Options::ENABLE_TABLES | Options::ENABLE_FOOTNOTES | Options::ENABLE_STRIKETHROUGH
 }

--- a/src/librustdoc/passes/html_tags.rs
+++ b/src/librustdoc/passes/html_tags.rs
@ -0,0 +1,190 @@
+use super::{span_of_attrs, Pass};
+use crate::clean::*;
+use crate::core::DocContext;
+use crate::fold::DocFolder;
+use crate::html::markdown::opts;
+use core::ops::Range;
+use pulldown_cmark::{Event, Parser};
+use rustc_feature::UnstableFeatures;
+use rustc_session::lint;
+
+pub const CHECK_INVALID_HTML_TAGS: Pass = Pass {
+    name: "check-invalid-html-tags",
+    run: check_invalid_html_tags,
+    description: "detects invalid HTML tags in doc comments",
+};
+
+struct InvalidHtmlTagsLinter<'a, 'tcx> {
+    cx: &'a DocContext<'tcx>,
+}
+
+impl<'a, 'tcx> InvalidHtmlTagsLinter<'a, 'tcx> {
+    fn new(cx: &'a DocContext<'tcx>) -> Self {
+        InvalidHtmlTagsLinter { cx }
+    }
+}
+
+pub fn check_invalid_html_tags(krate: Crate, cx: &DocContext<'_>) -> Crate {
+    if !UnstableFeatures::from_environment().is_nightly_build() {
+        krate
+    } else {
+        let mut coll = InvalidHtmlTagsLinter::new(cx);
+
+        coll.fold_crate(krate)
+    }
+}
+
+const ALLOWED_UNCLOSED: &[&str] = &[
+    "area", "base", "br", "col", "embed", "hr", "img", "input", "keygen", "link", "meta", "param",
+    "source", "track", "wbr",
+];
+
+fn drop_tag(
+    tags: &mut Vec<(String, Range<usize>)>,
+    tag_name: String,
+    range: Range<usize>,
+    f: &impl Fn(&str, &Range<usize>),
+) {
+    let tag_name_low = tag_name.to_lowercase();
+    if let Some(pos) = tags.iter().rposition(|(t, _)| t.to_lowercase() == tag_name_low) {
+        // If the tag is nested inside a "<script>" or a "<style>" tag, no warning should
+        // be emitted.
+        let should_not_warn = tags.iter().take(pos + 1).any(|(at, _)| {
+            let at = at.to_lowercase();
+            at == "script" || at == "style"
+        });
+        for (last_tag_name, last_tag_span) in tags.drain(pos + 1..) {
+            if should_not_warn {
+                continue;
+            }
+            let last_tag_name_low = last_tag_name.to_lowercase();
+            if ALLOWED_UNCLOSED.iter().any(|&at| at == &last_tag_name_low) {
+                continue;
+            }
+            // `tags` is used as a queue, meaning that everything after `pos` is included inside it.
+            // So `<h2><h3></h2>` will look like `["h2", "h3"]`. So when closing `h2`, we will still
+            // have `h3`, meaning the tag wasn't closed as it should have.
+            f(&format!("unclosed HTML tag `{}`", last_tag_name), &last_tag_span);
+        }
+        // Remove the `tag_name` that was originally closed
+        tags.pop();
+    } else {
+        // It can happen for example in this case: `<h2></script></h2>` (the `h2` tag isn't required
+        // but it helps for the visualization).
+        f(&format!("unopened HTML tag `{}`", tag_name), &range);
+    }
+}
+
+fn extract_tag(
+    tags: &mut Vec<(String, Range<usize>)>,
+    text: &str,
+    range: Range<usize>,
+    f: &impl Fn(&str, &Range<usize>),
+) {
+    let mut iter = text.char_indices().peekable();
+
+    while let Some((start_pos, c)) = iter.next() {
+        if c == '<' {
+            let mut tag_name = String::new();
+            let mut is_closing = false;
+            let mut prev_pos = start_pos;
+            loop {
+                let (pos, c) = match iter.peek() {
+                    Some((pos, c)) => (*pos, *c),
+                    // In case we reached the of the doc comment, we want to check that it's an
+                    // unclosed HTML tag. For example "/// <h3".
+                    None => (prev_pos, '\0'),
+                };
+                prev_pos = pos;
+                // Checking if this is a closing tag (like `</a>` for `<a>`).
+                if c == '/' && tag_name.is_empty() {
+                    is_closing = true;
+                } else if c.is_ascii_alphanumeric() {
+                    tag_name.push(c);
+                } else {
+                    if !tag_name.is_empty() {
+                        let mut r =
+                            Range { start: range.start + start_pos, end: range.start + pos };
+                        if c == '>' {
+                            // In case we have a tag without attribute, we can consider the span to
+                            // refer to it fully.
+                            r.end += 1;
+                        }
+                        if is_closing {
+                            // In case we have "</div >" or even "</div         >".
+                            if c != '>' {
+                                if !c.is_whitespace() {
+                                    // It seems like it's not a valid HTML tag.
+                                    break;
+                                }
+                                let mut found = false;
+                                for (new_pos, c) in text[pos..].char_indices() {
+                                    if !c.is_whitespace() {
+                                        if c == '>' {
+                                            r.end = range.start + new_pos + 1;
+                                            found = true;
+                                        }
+                                        break;
+                                    }
+                                }
+                                if !found {
+                                    break;
+                                }
+                            }
+                            drop_tag(tags, tag_name, r, f);
+                        } else {
+                            tags.push((tag_name, r));
+                        }
+                    }
+                    break;
+                }
+                iter.next();
+            }
+        }
+    }
+}
+
+impl<'a, 'tcx> DocFolder for InvalidHtmlTagsLinter<'a, 'tcx> {
+    fn fold_item(&mut self, item: Item) -> Option<Item> {
+        let hir_id = match self.cx.as_local_hir_id(item.def_id) {
+            Some(hir_id) => hir_id,
+            None => {
+                // If non-local, no need to check anything.
+                return self.fold_item_recur(item);
+            }
+        };
+        let dox = item.attrs.collapsed_doc_value().unwrap_or_default();
+        if !dox.is_empty() {
+            let cx = &self.cx;
+            let report_diag = |msg: &str, range: &Range<usize>| {
+                let sp = match super::source_span_for_markdown_range(cx, &dox, range, &item.attrs) {
+                    Some(sp) => sp,
+                    None => span_of_attrs(&item.attrs).unwrap_or(item.source.span()),
+                };
+                cx.tcx.struct_span_lint_hir(lint::builtin::INVALID_HTML_TAGS, hir_id, sp, |lint| {
+                    lint.build(msg).emit()
+                });
+            };
+
+            let mut tags = Vec::new();
+
+            let p = Parser::new_ext(&dox, opts()).into_offset_iter();
+
+            for (event, range) in p {
+                match event {
+                    Event::Html(text) => extract_tag(&mut tags, &text, range, &report_diag),
+                    _ => {}
+                }
+            }
+
+            for (tag, range) in tags.iter().filter(|(t, _)| {
+                let t = t.to_lowercase();
+                ALLOWED_UNCLOSED.iter().find(|&&at| at == t).is_none()
+            }) {
+                report_diag(&format!("unclosed HTML tag `{}`", tag), range);
+            }
+        }
+
+        self.fold_item_recur(item)
+    }
+}
--- a/src/librustdoc/passes/mod.rs
+++ b/src/librustdoc/passes/mod.rs
@ -45,6 +45,9 @@ pub use self::check_code_block_syntax::CHECK_CODE_BLOCK_SYNTAX;
 mod calculate_doc_coverage;
 pub use self::calculate_doc_coverage::CALCULATE_DOC_COVERAGE;

+mod html_tags;
+pub use self::html_tags::CHECK_INVALID_HTML_TAGS;
+
 /// A single pass over the cleaned documentation.
 ///
 /// Runs in the compiler context, so it has access to types and traits and the like.
@ -87,6 +90,7 @@ pub const PASSES: &[Pass] = &[
    CHECK_CODE_BLOCK_SYNTAX,
    COLLECT_TRAIT_IMPLS,
    CALCULATE_DOC_COVERAGE,
+    CHECK_INVALID_HTML_TAGS,
 ];

 /// The list of passes run by default.
@ -100,6 +104,7 @@ pub const DEFAULT_PASSES: &[ConditionalPass] = &[
    ConditionalPass::new(STRIP_PRIV_IMPORTS, WhenDocumentPrivate),
    ConditionalPass::always(COLLECT_INTRA_DOC_LINKS),
    ConditionalPass::always(CHECK_CODE_BLOCK_SYNTAX),
+    ConditionalPass::always(CHECK_INVALID_HTML_TAGS),
    ConditionalPass::always(PROPAGATE_DOC_CFG),
 ];

--- a/src/test/rustdoc-ui/invalid-html-tags.rs
+++ b/src/test/rustdoc-ui/invalid-html-tags.rs
@ -0,0 +1,75 @@
+#![deny(invalid_html_tags)]
+
+//! <p>💩<p>
+//~^ ERROR unclosed HTML tag `p`
+//~^^ ERROR unclosed HTML tag `p`
+
+/// <img><input>
+/// <script>
+/// <img><input>
+/// </script>
+/// <unknown>
+//~^ ERROR unclosed HTML tag `unknown`
+/// < ok
+/// <script>
+//~^ ERROR unclosed HTML tag `script`
+pub fn foo() {}
+
+/// <h1>
+///   <h2>
+//~^ ERROR unclosed HTML tag `h2`
+///     <h3>
+//~^ ERROR unclosed HTML tag `h3`
+/// </h1>
+/// </hello>
+//~^ ERROR unopened HTML tag `hello`
+pub fn bar() {}
+
+/// <div>
+///    <br/> <p>
+//~^ ERROR unclosed HTML tag `p`
+/// </div>
+pub fn a() {}
+
+/// <div>
+///   <p>
+///      <div></div>
+///   </p>
+/// </div>
+pub fn b() {}
+
+/// <div style="hello">
+//~^ ERROR unclosed HTML tag `div`
+///   <h3>
+//~^ ERROR unclosed HTML tag `h3`
+/// <script
+//~^ ERROR unclosed HTML tag `script`
+pub fn c() {}
+
+// Unclosed tags shouldn't warn if they are nested inside a <script> elem.
+/// <script>
+///   <h3><div>
+/// </script>
+/// <script>
+///   <div>
+///     <p>
+///   </div>
+/// </script>
+pub fn d() {}
+
+// Unclosed tags shouldn't warn if they are nested inside a <style> elem.
+/// <style>
+///   <h3><div>
+/// </style>
+/// <stYle>
+///   <div>
+///     <p>
+///   </div>
+/// </style>
+pub fn e() {}
+
+// Closing tags need to have ">" at the end, otherwise it's not a closing tag!
+/// <div></div >
+/// <div></div
+//~^ ERROR unclosed HTML tag `div`
+pub fn f() {}
--- a/src/test/rustdoc-ui/invalid-html-tags.stderr
+++ b/src/test/rustdoc-ui/invalid-html-tags.stderr
@ -0,0 +1,80 @@
+error: unclosed HTML tag `p`
+  --> $DIR/invalid-html-tags.rs:3:5
+   |
+LL | //! <p>💩<p>
+   |     ^^^
+   |
+note: the lint level is defined here
+  --> $DIR/invalid-html-tags.rs:1:9
+   |
+LL | #![deny(invalid_html_tags)]
+   |         ^^^^^^^^^^^^^^^^^
+
+error: unclosed HTML tag `p`
+  --> $DIR/invalid-html-tags.rs:3:9
+   |
+LL | //! <p>💩<p>
+   |          ^^^
+
+error: unclosed HTML tag `unknown`
+  --> $DIR/invalid-html-tags.rs:11:5
+   |
+LL | /// <unknown>
+   |     ^^^^^^^^^
+
+error: unclosed HTML tag `script`
+  --> $DIR/invalid-html-tags.rs:14:5
+   |
+LL | /// <script>
+   |     ^^^^^^^^
+
+error: unclosed HTML tag `h2`
+  --> $DIR/invalid-html-tags.rs:19:7
+   |
+LL | ///   <h2>
+   |       ^^^^
+
+error: unclosed HTML tag `h3`
+  --> $DIR/invalid-html-tags.rs:21:9
+   |
+LL | ///     <h3>
+   |         ^^^^
+
+error: unopened HTML tag `hello`
+  --> $DIR/invalid-html-tags.rs:24:5
+   |
+LL | /// </hello>
+   |     ^^^^^^^^
+
+error: unclosed HTML tag `p`
+  --> $DIR/invalid-html-tags.rs:29:14
+   |
+LL | ///    <br/> <p>
+   |              ^^^
+
+error: unclosed HTML tag `div`
+  --> $DIR/invalid-html-tags.rs:41:5
+   |
+LL | /// <div style="hello">
+   |     ^^^^
+
+error: unclosed HTML tag `h3`
+  --> $DIR/invalid-html-tags.rs:43:7
+   |
+LL | ///   <h3>
+   |       ^^^^
+
+error: unclosed HTML tag `script`
+  --> $DIR/invalid-html-tags.rs:45:5
+   |
+LL | /// <script
+   |     ^^^^^^
+
+error: unclosed HTML tag `div`
+  --> $DIR/invalid-html-tags.rs:73:5
+   |
+LL | /// <div></div
+   |     ^^^^^
+
+error: aborting due to 12 previous errors
+
--- a/src/test/rustdoc-ui/lint-group.rs
+++ b/src/test/rustdoc-ui/lint-group.rs
@ -22,3 +22,8 @@ pub fn no_doctest() {} //~^ ERROR missing code example in this documentation
 /// println!("sup");
 /// ```
 fn private_doctest() {} //~^^^^^ ERROR documentation test in private item
+
+/// <unknown>
+//~^ ERROR unclosed HTML tag `unknown`
+//~^^ ERROR missing code example
+pub fn c() {}
--- a/src/test/rustdoc-ui/lint-group.stderr
+++ b/src/test/rustdoc-ui/lint-group.stderr
@ -28,6 +28,12 @@ LL | #![deny(rustdoc)]
   |         ^^^^^^^
   = note: `#[deny(private_doc_tests)]` implied by `#[deny(rustdoc)]`

+error: missing code example in this documentation
+  --> $DIR/lint-group.rs:26:1
+   |
+LL | /// <unknown>
+   | ^^^^^^^^^^^^^
+
 error: unresolved link to `error`
  --> $DIR/lint-group.rs:9:29
   |
@ -42,5 +48,18 @@ LL | #![deny(rustdoc)]
   = note: `#[deny(broken_intra_doc_links)]` implied by `#[deny(rustdoc)]`
   = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`

-error: aborting due to 3 previous errors
+error: unclosed HTML tag `unknown`
+  --> $DIR/lint-group.rs:26:5
+   |
+LL | /// <unknown>
+   |     ^^^^^^^^^
+   |
+note: the lint level is defined here
+  --> $DIR/lint-group.rs:7:9
+   |
+LL | #![deny(rustdoc)]
+   |         ^^^^^^^
+   = note: `#[deny(invalid_html_tags)]` implied by `#[deny(rustdoc)]`
+
+error: aborting due to 5 previous errors