From e70507ca403c9475a92b3c1b8230fad08c9c7ab2 Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Sun, 8 Sep 2024 08:04:19 -0700 Subject: [PATCH] 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 --- components/devtools/actors/process.rs | 5 +-- components/devtools/actors/root.rs | 6 ++-- components/devtools/actors/tab.rs | 6 ++-- components/devtools/actors/watcher.rs | 6 ++-- components/fonts/font_store.rs | 2 +- components/layout_2020/display_list/mod.rs | 2 +- .../layout_2020/flow/inline/inline_box.rs | 10 +++--- components/layout_2020/flow/inline/line.rs | 4 +-- components/layout_2020/flow/inline/mod.rs | 5 ++- components/script/dom/bindings/utils.rs | 4 +-- components/script/dom/fontfaceset.rs | 2 +- components/script/dom/gpubuffer.rs | 2 +- components/script/dom/gpusupportedlimits.rs | 2 +- components/script/dom/htmlcollection.rs | 6 ++-- components/script/dom/resizeobserver.rs | 2 +- components/script/dom/resizeobserverentry.rs | 10 +++--- components/webgpu/poll_thread.rs | 2 +- python/tidy/test.py | 36 +++++++++++++++++++ python/tidy/tidy.py | 31 +++++++++++++++- 19 files changed, 107 insertions(+), 36 deletions(-) diff --git a/components/devtools/actors/process.rs b/components/devtools/actors/process.rs index 2e40e598003..6f9d0ca60f0 100644 --- a/components/devtools/actors/process.rs +++ b/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] -//! (https://searchfox.org/mozilla-central/source/devtools/server/actors/descriptors/process.js) +//! Liberally derived from the [Firefox JS implementation]. +//! +//! [Firefox JS implementation]: https://searchfox.org/mozilla-central/source/devtools/server/actors/descriptors/process.js use std::net::TcpStream; diff --git a/components/devtools/actors/root.rs b/components/devtools/actors/root.rs index cbe0dad1beb..d6a4f2e4edd 100644 --- a/components/devtools/actors/root.rs +++ b/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; diff --git a/components/devtools/actors/tab.rs b/components/devtools/actors/tab.rs index b4c228d9559..1d1e9ee19cf 100644 --- a/components/devtools/actors/tab.rs +++ b/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; diff --git a/components/devtools/actors/watcher.rs b/components/devtools/actors/watcher.rs index c74a82d421e..7454cc725f6 100644 --- a/components/devtools/actors/watcher.rs +++ b/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; diff --git a/components/fonts/font_store.rs b/components/fonts/font_store.rs index e5287b5bf88..d4b59792b44 100644 --- a/components/fonts/font_store.rs +++ b/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. +/// . #[derive(Clone, Debug, Default)] struct SimpleFamily { regular: Option, diff --git a/components/layout_2020/display_list/mod.rs b/components/layout_2020/display_list/mod.rs index 14e18694835..9d93410e00f 100644 --- a/components/layout_2020/display_list/mod.rs +++ b/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, diff --git a/components/layout_2020/flow/inline/inline_box.rs b/components/layout_2020/flow/inline/inline_box.rs index a8406611098..13e33381fe1 100644 --- a/components/layout_2020/flow/inline/inline_box.rs +++ b/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, - /// 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, } @@ -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>, /// 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, } @@ -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 diff --git a/components/layout_2020/flow/inline/line.rs b/components/layout_2020/flow/inline/line.rs index 93032b9b102..468d8fda825 100644 --- a/components/layout_2020/flow/inline/line.rs +++ b/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, - /// The current [`super::LineItemLayoutInlineContainerState`]. + /// The current [`LineItemLayoutInlineContainerState`]. pub current_state: LineItemLayoutInlineContainerState, /// The metrics of this line, which should remain constant throughout the diff --git a/components/layout_2020/flow/inline/mod.rs b/components/layout_2020/flow/inline/mod.rs index 3eddfb3bbbe..fa0d28e8c8e 100644 --- a/components/layout_2020/flow/inline/mod.rs +++ b/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 - /// final [`InlineBoxLineItem`] to the state and pop its state off of - /// [`Self::inline_box_state_stack`]. + /// Finish laying out a particular [`InlineBox`] into line items. This will + /// pop its state off of [`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, diff --git a/components/script/dom/bindings/utils.rs b/components/script/dom/bindings/utils.rs index 8b6f25d1f80..c3f6bf66971 100644 --- a/components/script/dom/bindings/utils.rs +++ b/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 +/// 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 +/// 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()) { diff --git a/components/script/dom/fontfaceset.rs b/components/script/dom/fontfaceset.rs index 6c3d498cbec..d9b05b29e60 100644 --- a/components/script/dom/fontfaceset.rs +++ b/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 + /// fn Ready(&self) -> Rc { self.promise.clone() } diff --git a/components/script/dom/gpubuffer.rs b/components/script/dom/gpubuffer.rs index 43cdabee98e..488ef614b92 100644 --- a/components/script/dom/gpubuffer.rs +++ b/components/script/dom/gpubuffer.rs @@ -216,7 +216,7 @@ impl GPUBufferMethods for GPUBuffer { } } - /// https://gpuweb.github.io/gpuweb/#dom-gpubuffer-destroy + /// fn Destroy(&self) { // Step 1 self.Unmap(); diff --git a/components/script/dom/gpusupportedlimits.rs b/components/script/dom/gpusupportedlimits.rs index ace3f777d29..f86dd5aaf20 100644 --- a/components/script/dom/gpusupportedlimits.rs +++ b/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 + /// fn set_maximum(limit: &mut T, value: u64) -> bool where T: Ord + Copy + TryFrom + UpperBounded, diff --git a/components/script/dom/htmlcollection.rs b/components/script/dom/htmlcollection.rs index 24e71f482e5..f48715f158a 100644 --- a/components/script/dom/htmlcollection.rs +++ b/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. -/// It would be nicer just to use Option for this, but that would produce word -/// alignment issues since Option uses 33 bits. +/// An optional `u32`, using `u32::MAX` to represent None. It would be nicer +/// just to use `Option`` for this, but that would produce word alignment +/// issues since `Option`` uses 33 bits. #[derive(Clone, Copy, JSTraceable, MallocSizeOf)] struct OptionU32 { bits: u32, diff --git a/components/script/dom/resizeobserver.rs b/components/script/dom/resizeobserver.rs index ff8032418ae..bb41c10bfee 100644 --- a/components/script/dom/resizeobserver.rs +++ b/components/script/dom/resizeobserver.rs @@ -217,7 +217,7 @@ enum ObservationState { Skipped, } -/// https://drafts.csswg.org/resize-observer/#resizeobservation +/// #[derive(JSTraceable, MallocSizeOf)] struct ResizeObservation { /// diff --git a/components/script/dom/resizeobserverentry.rs b/components/script/dom/resizeobserverentry.rs index e437d88dd37..f3afec9f26b 100644 --- a/components/script/dom/resizeobserverentry.rs +++ b/components/script/dom/resizeobserverentry.rs @@ -78,17 +78,17 @@ impl ResizeObserverEntry { } impl ResizeObserverEntryMethods for ResizeObserverEntry { - /// https://drafts.csswg.org/resize-observer/#dom-resizeobserverentry-target + /// fn Target(&self) -> DomRoot { DomRoot::from_ref(&*self.target) } - /// https://drafts.csswg.org/resize-observer/#dom-resizeobserverentry-contentrect + /// fn ContentRect(&self) -> DomRoot { DomRoot::from_ref(&*self.content_rect) } - /// https://drafts.csswg.org/resize-observer/#dom-resizeobserverentry-borderboxsize + /// fn BorderBoxSize(&self, cx: SafeJSContext) -> JSVal { let sizes: Vec> = 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 + /// fn ContentBoxSize(&self, cx: SafeJSContext) -> JSVal { let sizes: Vec> = 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 + /// fn DevicePixelContentBoxSize(&self, cx: SafeJSContext) -> JSVal { let sizes: Vec> = self .device_pixel_content_box_size diff --git a/components/webgpu/poll_thread.rs b/components/webgpu/poll_thread.rs index 060b5171ffb..b4b93de6912 100644 --- a/components/webgpu/poll_thread.rs +++ b/components/webgpu/poll_thread.rs @@ -42,7 +42,7 @@ pub(crate) struct Poller { handle: Option>, /// 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: lock: Arc>, } diff --git a/python/tidy/test.py b/python/tidy/test.py index 1d7ce3fbfc8..5bdcbddd090 100644 --- a/python/tidy/test.py +++ b/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"/// " + ) + 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 diff --git a/python/tidy/tidy.py b/python/tidy/tidy.py index b57a127c102..88e92b80922 100644 --- a/python/tidy/tidy.py +++ b/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: