Improve broken link to catch more cases and span point to whole link

maxcnunes · maxcnunes · commit ed3271d102c6 · 2024-11-18T15:43:31.000-03:00
diff --git a/clippy_lints/src/doc/broken_link.rs b/clippy_lints/src/doc/broken_link.rs
@@ -11,51 +11,108 @@ pub fn check(cx: &LateContext<'_>, attrs: &[Attribute]) {
     }
 }
 
+/// Scan AST attributes looking up in doc comments for broken links
+/// which rustdoc won't be able to properly create link tags later.
+#[derive(Debug)]
 struct BrokenLinkLoader {
+    /// List of spans for detected broken links.
     spans_broken_link: Vec<Span>,
+
+    /// Mark it has detected a link and it is processing whether
+    /// or not it is broken.
     active: bool,
+
+    /// Keep track of the span for the processing broken link.
+    active_span: Option<Span>,
+
+    /// Keep track where exactly the link definition has started in the code.
+    active_pos_start: u32,
+
+    /// Mark it is processing the link text tag.
     processing_link_text: bool,
+
+    /// Mark it is processing the link url tag.
     processing_link_url: bool,
+
+    /// Mark it is reading the url tag content. It will be false if the loader
+    /// got to the url tag processing, but all the chars read so far were just
+    /// whitespaces.
+    reading_link_url: bool,
+
+    /// Mark the url url isn't empty, but it still being processed in a new line.
+    reading_link_url_new_line: bool,
+
+    /// Mark the current link url is broken across multiple lines.
+    url_multiple_lines: bool,
+
+    /// Mark the link's span start position.
     start: u32,
 }
 
 impl BrokenLinkLoader {
+    /// Return spans of broken links.
     fn collect_spans_broken_link(attrs: &[Attribute]) -> Vec<Span> {
         let mut loader = BrokenLinkLoader {
             spans_broken_link: vec![],
             active: false,
+            active_pos_start: 0,
+            active_span: None,
             processing_link_text: false,
             processing_link_url: false,
+            reading_link_url: false,
+            reading_link_url_new_line: false,
+            url_multiple_lines: false,
             start: 0_u32,
         };
         loader.scan_attrs(attrs);
         loader.spans_broken_link
     }
 
     fn scan_attrs(&mut self, attrs: &[Attribute]) {
-        for attr in attrs {
+        for idx in 0..attrs.len() {
+            let attr = &attrs[idx];
             if let AttrKind::DocComment(_com_kind, sym) = attr.kind
                 && let AttrStyle::Outer = attr.style
             {
                 self.scan_line(sym.as_str(), attr.span);
+            } else {
+                if idx > 0 && self.active && self.processing_link_url {
+                    let prev_attr = &attrs[idx - 1];
+                    let prev_end_line = prev_attr.span.hi().0;
+                    self.record_broken_link(prev_end_line);
+                }
+                self.reset_lookup();
             }
         }
+
+        if self.active && self.processing_link_url {
+            let last_end_line = attrs.last().unwrap().span.hi().0;
+            self.record_broken_link(last_end_line);
+            self.reset_lookup();
+        }
     }
 
-    fn scan_line(&mut self, the_str: &str, attr_span: Span) {
+    fn scan_line(&mut self, line: &str, attr_span: Span) {
         // Note that we specifically need the char _byte_ indices here, not the positional indexes
         // within the char array to deal with multi-byte characters properly. `char_indices` does
         // exactly that. It provides an iterator over tuples of the form `(byte position, char)`.
-        let char_indices: Vec<_> = the_str.char_indices().collect();
+        let char_indices: Vec<_> = line.char_indices().collect();
 
-        let mut no_url_curr_line = true;
+        self.reading_link_url_new_line = self.reading_link_url;
 
         for (pos, c) in char_indices {
+            if pos == 0 && c.is_whitespace() {
+                // ignore prefix whitespace on comments
+                continue;
+            }
+
             if !self.active {
                 if c == '[' {
                     self.processing_link_text = true;
                     self.active = true;
-                    self.start = attr_span.lo().0 + u32::try_from(pos).unwrap();
+                    // +3 skips the opening delimiter
+                    self.active_pos_start = attr_span.lo().0 + u32::try_from(pos).unwrap() + 3;
+                    self.active_span = Some(attr_span);
                 }
                 continue;
             }
@@ -73,37 +130,57 @@ impl BrokenLinkLoader {
                 } else {
                     // not a real link, start lookup over again
                     self.reset_lookup();
-                    no_url_curr_line = true;
                 }
                 continue;
             }
 
             if c == ')' {
+                // record full broken link tag
+                if self.url_multiple_lines {
+                    // +3 skips the opening delimiter and +1 to include the closing parethesis
+                    let pos_end = attr_span.lo().0 + u32::try_from(pos).unwrap() + 4;
+                    self.record_broken_link(pos_end);
+                    self.reset_lookup();
+                }
                 self.reset_lookup();
-                no_url_curr_line = true;
-            } else if no_url_curr_line && c != ' ' {
-                no_url_curr_line = false;
+                continue;
             }
-        }
-
-        // If it got at the end of the line and it still processing a link part,
-        // it means this is a broken link.
-        if self.active && self.processing_link_url && !no_url_curr_line {
-            let pos_end_line = u32::try_from(the_str.len()).unwrap() - 1;
 
-            // +3 skips the opening delimiter
-            let start = BytePos(self.start + 3);
-            let end = start + BytePos(pos_end_line);
+            if self.reading_link_url && c.is_whitespace() {
+                let pos_end = u32::try_from(pos).unwrap();
+                self.record_broken_link(pos_end);
+                self.reset_lookup();
+                continue;
+            }
 
-            let com_span = Span::new(start, end, attr_span.ctxt(), attr_span.parent());
+            if !c.is_whitespace() {
+                self.reading_link_url = true;
+            }
 
-            self.spans_broken_link.push(com_span);
+            if self.reading_link_url_new_line {
+                self.url_multiple_lines = true;
+            }
         }
     }
 
     fn reset_lookup(&mut self) {
-        self.processing_link_url = false;
         self.active = false;
         self.start = 0;
+        self.processing_link_text = false;
+        self.processing_link_url = false;
+        self.reading_link_url = false;
+        self.reading_link_url_new_line = false;
+        self.url_multiple_lines = false;
+    }
+
+    fn record_broken_link(&mut self, pos_end: u32) {
+        if let Some(attr_span) = self.active_span {
+            let start = BytePos(self.active_pos_start);
+            let end = BytePos(pos_end);
+
+            let com_span = Span::new(start, end, attr_span.ctxt(), attr_span.parent());
+
+            self.spans_broken_link.push(com_span);
+        }
     }
 }
diff --git a/tests/ui/doc_broken_link.rs b/tests/ui/doc_broken_link.rs
@@ -1,27 +1,41 @@
 #![warn(clippy::doc_broken_link)]
 
-fn main() {
-    doc_valid_link();
-    doc_valid_link_broken_title();
-    doc_valid_link_broken_url_tag();
-    doc_invalid_link_broken_url_scheme_part();
-    doc_invalid_link_broken_url_host_part();
-}
+fn main() {}
+
+pub struct FakeType {}
 
 /// Test valid link, whole link single line.
 /// [doc valid link](https://test.fake/doc_valid_link)
 pub fn doc_valid_link() {}
 
-/// Test valid link, title tag broken across multiple lines.
+/// Test valid link, whole link single line but it has special chars such as brackets and
+/// parenthesis. [doc invalid link url invalid char](https://test.fake/doc_valid_link_url_invalid_char?foo[bar]=1&bar(foo)=2)
+pub fn doc_valid_link_url_invalid_char() {}
+
+/// Test valid link, text tag broken across multiple lines.
 /// [doc invalid link broken
-/// title](https://test.fake/doc_valid_link_broken_title)
-pub fn doc_valid_link_broken_title() {}
+/// text](https://test.fake/doc_valid_link_broken_text)
+pub fn doc_valid_link_broken_text() {}
+
+/// Test valid link, url tag broken across multiple lines, but
+/// the whole url part in a single line.
+/// [doc valid link broken url tag two lines first](https://test.fake/doc_valid_link_broken_url_tag_two_lines_first
+/// )
+pub fn doc_valid_link_broken_url_tag_two_lines_first() {}
 
 /// Test valid link, url tag broken across multiple lines, but
 /// the whole url part in a single line.
-/// [doc valid link broken url tag](
-/// https://test.fake/doc_valid_link_broken_url_tag)
-pub fn doc_valid_link_broken_url_tag() {}
+/// [doc valid link broken url tag two lines second](
+/// https://test.fake/doc_valid_link_broken_url_tag_two_lines_second)
+pub fn doc_valid_link_broken_url_tag_two_lines_second() {}
+
+/// Test valid link, url tag broken across multiple lines, but
+/// the whole url part in a single line, but the closing pharentesis
+/// in a third line.
+/// [doc valid link broken url tag three lines](
+/// https://test.fake/doc_valid_link_broken_url_tag_three_lines
+/// )
+pub fn doc_valid_link_broken_url_tag_three_lines() {}
 
 /// Test invalid link, url part broken across multiple lines.
 /// [doc invalid link broken url scheme part part](https://
@@ -35,14 +49,25 @@ pub fn doc_invalid_link_broken_url_scheme_part() {}
 //~^^ ERROR: possible broken doc link
 pub fn doc_invalid_link_broken_url_host_part() {}
 
+/// Test invalid link, url part broken across multiple lines.
+/// [doc invalid link broken url host part](
+/// https://test.fake/doc_invalid_link_missing_close_parenthesis
+//~^^ ERROR: possible broken doc link
+pub fn doc_invalid_link_missing_close_parenthesis() {}
+
+// NOTE: We don't test doc links where the url is broken accross
+// multiple lines in the path part because that is something
+// rustdoc itself will check and warn about it.
+pub fn doc_invalid_link_broken_url_path_part() {}
+
 /// This might be considered a link false positive
 /// and should be ignored by this lint rule:
-/// Example of referencing some code with brackets [T].
+/// Example of referencing some code with brackets [FakeType].
 pub fn doc_ignore_link_false_positive_1() {}
 
 /// This might be considered a link false positive
 /// and should be ignored by this lint rule:
-/// [`T`]. Continue text after brackets,
+/// [`FakeType`]. Continue text after brackets,
 /// then (something in
 /// parenthesis).
 pub fn doc_ignore_link_false_positive_2() {}
diff --git a/tests/ui/doc_broken_link.stderr b/tests/ui/doc_broken_link.stderr
@@ -1,17 +1,29 @@
 error: possible broken doc link
-  --> tests/ui/doc_broken_link.rs:27:5
+  --> tests/ui/doc_broken_link.rs:41:5
    |
-LL | /// [doc invalid link broken url scheme part part](https://
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL |   /// [doc invalid link broken url scheme part part](https://
+   |  _____^
+LL | | /// test.fake/doc_invalid_link_broken_url_scheme_part)
+   | |______________________________________________________^
    |
    = note: `-D clippy::doc-broken-link` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::doc_broken_link)]`
 
 error: possible broken doc link
-  --> tests/ui/doc_broken_link.rs:33:5
+  --> tests/ui/doc_broken_link.rs:47:5
    |
-LL | /// [doc invalid link broken url host part](https://test
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL |   /// [doc invalid link broken url host part](https://test
+   |  _____^
+LL | | /// .fake/doc_invalid_link_broken_url_host_part)
+   | |________________________________________________^
 
-error: aborting due to 2 previous errors
+error: possible broken doc link
+  --> tests/ui/doc_broken_link.rs:53:5
+   |
+LL |   /// [doc invalid link broken url host part](
+   |  _____^
+LL | | /// https://test.fake/doc_invalid_link_missing_close_parenthesis
+   | |________________________________________________________________^
+
+error: aborting due to 3 previous errors
 
