tidy: Fix rustdoc warnings and add a tidy check for a common URL issue (#33366)

This change fixes all rustdoc errors and also adds a tidy check for a
very common rustdoc URL issue. Eventually rustdoc warnings should likely
cause the build to fail, but this catches those issues sooner in order
to not waste so much developer time.

Signed-off-by: Martin Robinson <mrobinson@igalia.com>

components/devtools/actors/process.rs

@@ -2,8 +2,9 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
-//! Liberally derived from the [Firefox JS implementation]
+//! Liberally derived from the [Firefox JS implementation].
-//! (https://searchfox.org/mozilla-central/source/devtools/server/actors/descriptors/process.js)
+//!
+//! [Firefox JS implementation]: https://searchfox.org/mozilla-central/source/devtools/server/actors/descriptors/process.js
 
 use std::net::TcpStream;
 

components/devtools/actors/root.rs

@@ -2,10 +2,12 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
-//! Liberally derived from the [Firefox JS implementation]
-//! (https://searchfox.org/mozilla-central/source/devtools/server/actors/root.js).
 //! Connection point for all new remote devtools interactions, providing lists of know actors
 //! that perform more specific actions (targets, addons, browser chrome, etc.)
+//!
+//! Liberally derived from the [Firefox JS implementation].
+//!
+//! [Firefox JS implementation]: https://searchfox.org/mozilla-central/source/devtools/server/actors/root.js
 
 use std::net::TcpStream;
 

components/devtools/actors/tab.rs

@@ -2,10 +2,12 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
-//! Liberally derived from the [Firefox JS implementation]
-//! (https://searchfox.org/mozilla-central/source/devtools/server/actors/descriptors/tab.js)
 //! Descriptor actor that represents a web view. It can link a tab to the corresponding watcher
 //! actor to enable inspection.
+//!
+//! Liberally derived from the [Firefox JS implementation].
+//!
+//! [Firefox JS implementation]: https://searchfox.org/mozilla-central/source/devtools/server/actors/descriptors/tab.js
 
 use std::net::TcpStream;
 

components/devtools/actors/watcher.rs

@@ -2,11 +2,13 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
-//! Liberally derived from the [Firefox JS implementation]
-//! (https://searchfox.org/mozilla-central/source/devtools/server/actors/watcher.js).
 //! The watcher is the main entry point when debugging an element. Right now only web views are supported.
 //! It talks to the devtools remote and lists the capabilities of the inspected target, and it serves
 //! as a bridge for messages between actors.
+//!
+//! Liberally derived from the [Firefox JS implementation].
+//!
+//! [Firefox JS implementation]: https://searchfox.org/mozilla-central/source/devtools/server/actors/descriptors/watcher.js
 
 use std::collections::HashMap;
 use std::net::TcpStream;

components/fonts/font_store.rs

@@ -184,7 +184,7 @@ impl WebRenderFontStore {
 /// have a complex set of fonts.
 ///
 /// This optimization is taken from:
-/// https://searchfox.org/mozilla-central/source/gfx/thebes/gfxFontEntry.cpp.
+/// <https://searchfox.org/mozilla-central/source/gfx/thebes/gfxFontEntry.cpp>.
 #[derive(Clone, Debug, Default)]
 struct SimpleFamily {
     regular: Option<FontTemplateRef>,

components/layout_2020/display_list/mod.rs

@@ -140,7 +140,7 @@ pub(crate) struct DisplayListBuilder<'a> {
     /// list building functions.
     current_scroll_node_id: ScrollTreeNodeId,
 
-    /// The current [ScrollNodeTreeId] for this [DisplayListBuilder]. This is necessary in addition
+    /// The current [ScrollTreeNodeId] for this [DisplayListBuilder]. This is necessary in addition
     /// to the [Self::current_scroll_node_id], because some pieces of fragments as backgrounds with
     /// `background-attachment: fixed` need to not scroll while the rest of the fragment does.
     current_reference_frame_scroll_node_id: ScrollTreeNodeId,

components/layout_2020/flow/inline/inline_box.rs

@@ -24,11 +24,11 @@ pub(crate) struct InlineBox {
     pub base_fragment_info: BaseFragmentInfo,
     #[serde(skip_serializing)]
     pub style: Arc<ComputedValues>,
-    /// The identifier of this inline box in the containing [`InlineFormattingContext`].
+    /// The identifier of this inline box in the containing [`super::InlineFormattingContext`].
     pub(super) identifier: InlineBoxIdentifier,
     pub is_first_fragment: bool,
     pub is_last_fragment: bool,
-    /// The index of the default font in the [`InlineFormattingContext`]'s font metrics store.
+    /// The index of the default font in the [`super::InlineFormattingContext`]'s font metrics store.
     /// This is initialized during IFC shaping.
     pub default_font_index: Option<usize>,
 }
@@ -58,12 +58,12 @@ impl InlineBox {
 
 #[derive(Debug, Default, Serialize)]
 pub(crate) struct InlineBoxes {
-    /// A collection of all inline boxes in a particular [`InlineFormattingContext`].
+    /// A collection of all inline boxes in a particular [`super::InlineFormattingContext`].
     inline_boxes: Vec<ArcRefCell<InlineBox>>,
 
     /// A list of tokens that represent the actual tree of inline boxes, while allowing
     /// easy traversal forward and backwards through the tree. This structure is also
-    /// stored in the [`InlineFormattingContext::inline_items`], but this version is
+    /// stored in the [`super::InlineFormattingContext::inline_items`], but this version is
     /// faster to iterate.
     inline_box_tree: Vec<InlineBoxTreePathToken>,
 }
@@ -182,7 +182,7 @@ pub(crate) struct InlineBoxIdentifier {
 
 pub(super) struct InlineBoxContainerState {
     /// The container state common to both [`InlineBox`] and the root of the
-    /// [`InlineFormattingContext`].
+    /// [`super::InlineFormattingContext`].
     pub base: InlineContainerState,
 
     /// The [`InlineBoxIdentifier`] of this inline container state. If this is the root

components/layout_2020/flow/inline/line.rs

@@ -130,11 +130,11 @@ pub(super) struct LineItemLayout<'layout_data, 'layout> {
     /// The state of the overall [`super::InlineFormattingContext`] layout.
     layout: &'layout mut InlineFormattingContextLayout<'layout_data>,
 
-    /// The set of [`super::LineItemLayoutInlineContainerState`] created while laying out items
+    /// The set of [`LineItemLayoutInlineContainerState`] created while laying out items
     /// on this line. This does not include the current level of recursion.
     pub state_stack: Vec<LineItemLayoutInlineContainerState>,
 
-    /// The current [`super::LineItemLayoutInlineContainerState`].
+    /// The current [`LineItemLayoutInlineContainerState`].
     pub current_state: LineItemLayoutInlineContainerState,
 
     /// The metrics of this line, which should remain constant throughout the

components/layout_2020/flow/inline/mod.rs

@@ -737,9 +737,8 @@ impl<'layout_dta> InlineFormattingContextLayout<'layout_dta> {
         self.inline_box_state_stack.push(inline_box_state);
     }
 
-    /// Finish laying out a particular [`InlineBox`] into line items. This will add the
+    /// Finish laying out a particular [`InlineBox`] into line items. This will
-    /// final [`InlineBoxLineItem`] to the state and pop its state off of
+    /// pop its state off of [`Self::inline_box_state_stack`].
-    /// [`Self::inline_box_state_stack`].
     fn finish_inline_box(&mut self) {
         let inline_box_state = match self.inline_box_state_stack.pop() {
             Some(inline_box_state) => inline_box_state,

components/script/dom/bindings/utils.rs

@@ -625,7 +625,7 @@ impl AsCCharPtrPtr for [u8] {
     }
 }
 
-/// https://searchfox.org/mozilla-central/rev/7279a1df13a819be254fd4649e07c4ff93e4bd45/dom/bindings/BindingUtils.cpp#3300
+/// <https://searchfox.org/mozilla-central/rev/7279a1df13a819be254fd4649e07c4ff93e4bd45/dom/bindings/BindingUtils.cpp#3300>
 pub unsafe extern "C" fn generic_static_promise_method(
     cx: *mut JSContext,
     argc: libc::c_uint,
@@ -646,7 +646,7 @@ pub unsafe extern "C" fn generic_static_promise_method(
 
 /// Coverts exception to promise rejection
 ///
-/// https://searchfox.org/mozilla-central/rev/b220e40ff2ee3d10ce68e07d8a8a577d5558e2a2/dom/bindings/BindingUtils.cpp#3315
+/// <https://searchfox.org/mozilla-central/rev/b220e40ff2ee3d10ce68e07d8a8a577d5558e2a2/dom/bindings/BindingUtils.cpp#3315>
 pub unsafe fn exception_to_promise(cx: *mut JSContext, rval: RawMutableHandleValue) -> bool {
     rooted!(in(cx) let mut exception = UndefinedValue());
     if !JS_GetPendingException(cx, exception.handle_mut()) {

components/script/dom/fontfaceset.rs

@@ -49,7 +49,7 @@ impl FontFaceSet {
 }
 
 impl FontFaceSetMethods for FontFaceSet {
-    /// https://drafts.csswg.org/css-font-loading/#dom-fontfaceset-ready
+    /// <https://drafts.csswg.org/css-font-loading/#dom-fontfaceset-ready>
     fn Ready(&self) -> Rc<Promise> {
         self.promise.clone()
     }

components/script/dom/gpubuffer.rs

@@ -216,7 +216,7 @@ impl GPUBufferMethods for GPUBuffer {
         }
     }
 
-    /// https://gpuweb.github.io/gpuweb/#dom-gpubuffer-destroy
+    /// <https://gpuweb.github.io/gpuweb/#dom-gpubuffer-destroy>
     fn Destroy(&self) {
         // Step 1
         self.Unmap();

components/script/dom/gpusupportedlimits.rs

@@ -202,7 +202,7 @@ impl GPUSupportedLimitsMethods for GPUSupportedLimits {
 pub fn set_limit(limits: &mut Limits, limit: &str, value: u64) -> bool {
     /// per spec defaults are lower bounds for values
     ///
-    /// https://www.w3.org/TR/webgpu/#limit-class-maximum
+    /// <https://www.w3.org/TR/webgpu/#limit-class-maximum>
     fn set_maximum<T>(limit: &mut T, value: u64) -> bool
     where
         T: Ord + Copy + TryFrom<u64> + UpperBounded,

components/script/dom/htmlcollection.rs

@@ -25,9 +25,9 @@ pub trait CollectionFilter: JSTraceable {
     fn filter<'a>(&self, elem: &'a Element, root: &'a Node) -> bool;
 }
 
-/// An optional u32, using u32::MAX to represent None.
+/// An optional `u32`, using `u32::MAX` to represent None.  It would be nicer
-/// It would be nicer just to use Option<u32> for this, but that would produce word
+/// just to use `Option<u32>`` for this, but that would produce word alignment
-/// alignment issues since Option<u32> uses 33 bits.
+/// issues since `Option<u32>`` uses 33 bits.
 #[derive(Clone, Copy, JSTraceable, MallocSizeOf)]
 struct OptionU32 {
     bits: u32,

components/script/dom/resizeobserver.rs

@@ -217,7 +217,7 @@ enum ObservationState {
     Skipped,
 }
 
-/// https://drafts.csswg.org/resize-observer/#resizeobservation
+/// <https://drafts.csswg.org/resize-observer/#resizeobservation>
 #[derive(JSTraceable, MallocSizeOf)]
 struct ResizeObservation {
     /// <https://drafts.csswg.org/resize-observer/#dom-resizeobservation-target>

components/script/dom/resizeobserverentry.rs

@@ -78,17 +78,17 @@ impl ResizeObserverEntry {
 }
 
 impl ResizeObserverEntryMethods for ResizeObserverEntry {
-    /// https://drafts.csswg.org/resize-observer/#dom-resizeobserverentry-target
+    /// <https://drafts.csswg.org/resize-observer/#dom-resizeobserverentry-target>
     fn Target(&self) -> DomRoot<Element> {
         DomRoot::from_ref(&*self.target)
     }
 
-    /// https://drafts.csswg.org/resize-observer/#dom-resizeobserverentry-contentrect
+    /// <https://drafts.csswg.org/resize-observer/#dom-resizeobserverentry-contentrect>
     fn ContentRect(&self) -> DomRoot<DOMRectReadOnly> {
         DomRoot::from_ref(&*self.content_rect)
     }
 
-    /// https://drafts.csswg.org/resize-observer/#dom-resizeobserverentry-borderboxsize
+    /// <https://drafts.csswg.org/resize-observer/#dom-resizeobserverentry-borderboxsize>
     fn BorderBoxSize(&self, cx: SafeJSContext) -> JSVal {
         let sizes: Vec<DomRoot<ResizeObserverSize>> = self
             .border_box_size
@@ -98,7 +98,7 @@ impl ResizeObserverEntryMethods for ResizeObserverEntry {
         to_frozen_array(sizes.as_slice(), cx)
     }
 
-    /// https://drafts.csswg.org/resize-observer/#dom-resizeobserverentry-contentboxsize
+    /// <https://drafts.csswg.org/resize-observer/#dom-resizeobserverentry-contentboxsize>
     fn ContentBoxSize(&self, cx: SafeJSContext) -> JSVal {
         let sizes: Vec<DomRoot<ResizeObserverSize>> = self
             .content_box_size
@@ -108,7 +108,7 @@ impl ResizeObserverEntryMethods for ResizeObserverEntry {
         to_frozen_array(sizes.as_slice(), cx)
     }
 
-    /// https://drafts.csswg.org/resize-observer/#dom-resizeobserverentry-devicepixelcontentboxsize
+    /// <https://drafts.csswg.org/resize-observer/#dom-resizeobserverentry-devicepixelcontentboxsize>
     fn DevicePixelContentBoxSize(&self, cx: SafeJSContext) -> JSVal {
         let sizes: Vec<DomRoot<ResizeObserverSize>> = self
             .device_pixel_content_box_size

components/webgpu/poll_thread.rs

@@ -42,7 +42,7 @@ pub(crate) struct Poller {
     handle: Option<JoinHandle<()>>,
     /// Lock for device maintain calls (in poll_all_devices and queue_submit)
     ///
-    /// This is workaround for wgpu deadlocks: https://github.com/gfx-rs/wgpu/issues/5572
+    /// This is workaround for wgpu deadlocks: <https://github.com/gfx-rs/wgpu/issues/5572>
     lock: Arc<Mutex<()>>,
 }
 

python/tidy/test.py

@@ -9,6 +9,7 @@
 
 import logging
 import os
+from typing import Iterable, Tuple
 import unittest
 
 from . import tidy
@@ -241,6 +242,41 @@ class CheckTidiness(unittest.TestCase):
         errors = tidy.collect_errors_for_files(iterFile('multiline_string.rs'), [], [tidy.check_rust], print_text=False)
         self.assertNoMoreErrors(errors)
 
+    def test_raw_url_in_rustdoc(self):
+        def assert_has_a_single_rustdoc_error(errors: Iterable[Tuple[int, str]]):
+            self.assertEqual(tidy.ERROR_RAW_URL_IN_RUSTDOC, next(errors)[1])
+            self.assertNoMoreErrors(errors)
+
+        errors = tidy.check_for_raw_urls_in_rustdoc(
+            "file.rs", 3,
+            b"/// https://google.com"
+        )
+        assert_has_a_single_rustdoc_error(errors)
+
+        errors = tidy.check_for_raw_urls_in_rustdoc(
+            "file.rs", 3,
+            b"//! (https://google.com)"
+        )
+        assert_has_a_single_rustdoc_error(errors)
+
+        errors = tidy.check_for_raw_urls_in_rustdoc(
+            "file.rs", 3,
+            b"/// <https://google.com>"
+        )
+        self.assertNoMoreErrors(errors)
+
+        errors = tidy.check_for_raw_urls_in_rustdoc(
+            "file.rs", 3,
+            b"/// [hi]: https://google.com"
+        )
+        self.assertNoMoreErrors(errors)
+
+        errors = tidy.check_for_raw_urls_in_rustdoc(
+            "file.rs", 3,
+            b"/// [hi](https://google.com)"
+        )
+        self.assertNoMoreErrors(errors)
+
 
 def run_tests():
     verbosity = 1 if logging.getLogger().level >= logging.WARN else 2

python/tidy/tidy.py

@@ -32,8 +32,11 @@ CONFIG_FILE_PATH = os.path.join(".", "servo-tidy.toml")
 WPT_CONFIG_INI_PATH = os.path.join(WPT_PATH, "config.ini")
 # regex source https://stackoverflow.com/questions/6883049/
 URL_REGEX = re.compile(br'https?://(?:[-\w.]|(?:%[\da-fA-F]{2}))+')
+UTF8_URL_REGEX = re.compile(r'https?://(?:[-\w.]|(?:%[\da-fA-F]{2}))+')
 CARGO_LOCK_FILE = os.path.join(TOPDIR, "Cargo.lock")
 
+ERROR_RAW_URL_IN_RUSTDOC = "Found raw link in rustdoc. Please escape it with angle brackets or use a markdown link."
+
 sys.path.append(os.path.join(WPT_PATH, "tests"))
 sys.path.append(os.path.join(WPT_PATH, "tests", "tools", "wptrunner"))
 
@@ -306,13 +309,39 @@ def check_whitespace(idx, line):
         yield (idx + 1, "CR on line")
 
 
-def check_by_line(file_name, lines):
+def check_for_raw_urls_in_rustdoc(file_name: str, idx: int, line: bytes):
+    """Check that rustdoc comments in Rust source code do not have raw URLs. These appear
+    as warnings when rustdoc is run. rustdoc warnings could be made fatal, but adding this
+    check as part of tidy catches this common problem without having to run rustdoc for all
+    of Servo."""
+    if not file_name.endswith(".rs"):
+        return
+
+    if b"///" not in line and b"//!" not in line:
+        return
+
+    # Types of URLS that are allowed:
+    #  - A URL surrounded by angle or square brackets.
+    #  - A markdown link.
+    #  - A URL as part of a markdown definition identifer.
+    #    [link text]: https://example.com
+    match = URL_REGEX.search(line)
+    if match and (
+            not line[match.start() - 1:].startswith(b"<")
+            and not line[match.start() - 1:].startswith(b"[")
+            and not line[match.start() - 2:].startswith(b"](")
+            and not line[match.start() - 3:].startswith(b"]: ")):
+        yield (idx + 1, ERROR_RAW_URL_IN_RUSTDOC)
+
+
+def check_by_line(file_name: str, lines: list[bytes]):
     for idx, line in enumerate(lines):
         errors = itertools.chain(
             check_length(file_name, idx, line),
             check_whitespace(idx, line),
             check_whatwg_specific_url(idx, line),
             check_whatwg_single_page_url(idx, line),
+            check_for_raw_urls_in_rustdoc(file_name, idx, line),
         )
 
         for error in errors: