From 4f2fc39b74ff3eacd28b7438964a5256e26f494f Mon Sep 17 00:00:00 2001 From: overlookmotel <557937+overlookmotel@users.noreply.github.com> Date: Wed, 26 Feb 2025 02:05:35 +0000 Subject: [PATCH] chore(allocator, data_structures, lexer, traverse): enable `undocumented_unsafe_blocks` clippy lint (#9363) Enable `undocumented_unsafe_blocks` clippy lint rule, and fix warnings it raises. --- Cargo.toml | 2 +- crates/oxc_allocator/src/allocator_api2.rs | 5 +++-- crates/oxc_data_structures/src/stack/common.rs | 2 ++ crates/oxc_parser/src/lexer/byte_handlers.rs | 1 + crates/oxc_parser/src/lexer/source.rs | 17 ++++++++++++----- crates/oxc_traverse/src/context/ancestry.rs | 1 + crates/oxc_traverse/src/context/mod.rs | 1 + 7 files changed, 21 insertions(+), 8 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 7304fc379cbf5..c89d77bfe0a54 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -51,7 +51,7 @@ rc_buffer = "warn" rc_mutex = "warn" rest_pat_in_fully_bound_structs = "warn" unnecessary_safety_comment = "warn" -undocumented_unsafe_blocks = "allow" # FIXME +undocumented_unsafe_blocks = "warn" infinite_loop = "warn" map_with_unused_argument_over_ranges = "warn" unused_result_ok = "warn" diff --git a/crates/oxc_allocator/src/allocator_api2.rs b/crates/oxc_allocator/src/allocator_api2.rs index b41af94b1b0da..332443417965e 100644 --- a/crates/oxc_allocator/src/allocator_api2.rs +++ b/crates/oxc_allocator/src/allocator_api2.rs @@ -1,5 +1,6 @@ -// All methods just delegate to `bumpalo`, so all marked `#[inline(always)]` -#![expect(clippy::inline_always)] +// All methods just delegate to `bumpalo`, so all marked `#[inline(always)]`. +// All have same safety preconditions of `bumpalo` methods of the same name. +#![expect(clippy::inline_always, clippy::undocumented_unsafe_blocks)] use std::{alloc::Layout, ptr::NonNull}; diff --git a/crates/oxc_data_structures/src/stack/common.rs b/crates/oxc_data_structures/src/stack/common.rs index e10e7e8219792..ba3286362d269 100644 --- a/crates/oxc_data_structures/src/stack/common.rs +++ b/crates/oxc_data_structures/src/stack/common.rs @@ -61,6 +61,7 @@ pub trait StackCommon: StackCapacity { let old_layout = unsafe { Self::layout_for(self.capacity_bytes()) }; // Grow allocation. + // SAFETY: // `start` and `end` are boundaries of the allocation (`alloc` and `grow` ensure that). // So `old_start_ptr` and `old_layout` accurately describe the current allocation. // `grow` creates new allocation with byte size double what it currently is, or caps it @@ -270,6 +271,7 @@ unsafe fn grow( // `layout_for` produces a layout with `T`'s alignment, so `new_ptr` is aligned for `T`. let new_ptr = unsafe { alloc::realloc(old_start.as_ptr(), old_layout, new_capacity_bytes) }; let Some(new_start) = NonNull::new(new_ptr) else { + // SAFETY: See above let new_layout = unsafe { Layout::from_size_align_unchecked(new_capacity_bytes, old_layout.align()) }; alloc::handle_alloc_error(new_layout); diff --git a/crates/oxc_parser/src/lexer/byte_handlers.rs b/crates/oxc_parser/src/lexer/byte_handlers.rs index d449fd834fafe..3057e3b7339ac 100644 --- a/crates/oxc_parser/src/lexer/byte_handlers.rs +++ b/crates/oxc_parser/src/lexer/byte_handlers.rs @@ -11,6 +11,7 @@ impl Lexer<'_> { /// * `byte` must be next byte of source code, corresponding to current position of `lexer.source`. /// * Only `BYTE_HANDLERS` for ASCII characters may use the `ascii_byte_handler!()` macro. pub(super) unsafe fn handle_byte(&mut self, byte: u8) -> Kind { + // SAFETY: Caller guarantees to uphold safety invariants unsafe { BYTE_HANDLERS[byte as usize](self) } } } diff --git a/crates/oxc_parser/src/lexer/source.rs b/crates/oxc_parser/src/lexer/source.rs index 7f7c33a49e3e7..1e596adac3946 100644 --- a/crates/oxc_parser/src/lexer/source.rs +++ b/crates/oxc_parser/src/lexer/source.rs @@ -279,8 +279,10 @@ impl<'a> Source<'a> { debug_assert!(end.ptr <= self.end); // Check `start` and `end` are on UTF-8 character boundaries. // SAFETY: Above assertions ensure `start` and `end` are valid to read from if not at EOF. - debug_assert!(start.ptr == self.end || !is_utf8_cont_byte(unsafe { start.read() })); - debug_assert!(end.ptr == self.end || !is_utf8_cont_byte(unsafe { end.read() })); + unsafe { + debug_assert!(start.ptr == self.end || !is_utf8_cont_byte(start.read())); + debug_assert!(end.ptr == self.end || !is_utf8_cont_byte(end.read())); + } // SAFETY: Caller guarantees `start` is not after `end`. // `SourcePosition`s can only be created from a `Source`. @@ -541,12 +543,13 @@ impl<'a> Source<'a> { /// Caller must ensure `Source` is not at end of file. #[inline] pub(super) unsafe fn peek_byte_unchecked(&self) -> u8 { + debug_assert!(self.ptr >= self.start && self.ptr < self.end); + // SAFETY: Caller guarantees `ptr` is before `end` (i.e. not at end of file). // Methods of this type provide no way to allow `ptr` to be before `start`. // `Source`'s invariants guarantee that `self.start` - `self.end` contains allocated memory. // `Source::new` takes an immutable ref `&str`, guaranteeing that the memory `self.ptr` // addresses cannot be aliased by a `&mut` ref as long as `Source` exists. - debug_assert!(self.ptr >= self.start && self.ptr < self.end); unsafe { self.position().read() } } } @@ -592,6 +595,7 @@ impl SourcePosition<'_> { /// just not past it. #[inline] pub(super) unsafe fn add(self, n: usize) -> Self { + // SAFETY: Caller guarantees that `add` will not go out of bounds unsafe { Self::new(self.ptr.add(n)) } } @@ -603,6 +607,7 @@ impl SourcePosition<'_> { /// of `Source` this `SourcePosition` was created from. #[inline] pub(super) unsafe fn sub(self, n: usize) -> Self { + // SAFETY: Caller guarantees that `sub` will not go out of bounds unsafe { Self::new(self.ptr.sub(n)) } } @@ -623,6 +628,8 @@ impl SourcePosition<'_> { /// because if a `&mut` reference existed, that would violate Rust's aliasing rules. #[inline] pub(super) unsafe fn read(self) -> u8 { + debug_assert!(!self.ptr.is_null()); + // SAFETY: // Caller guarantees `self` is not at end of source text. // `Source` is created from a valid `&str`, so points to allocated, initialized memory. @@ -630,7 +637,6 @@ impl SourcePosition<'_> { // to the same memory can exist, as that would violate Rust's aliasing rules. // Pointer is "dereferenceable" by definition as a `u8` is 1 byte and cannot span multiple objects. // Alignment is not relevant as `u8` is aligned on 1 (i.e. no alignment requirements). - debug_assert!(!self.ptr.is_null()); unsafe { *self.ptr.as_ref().unwrap_unchecked() } } @@ -641,6 +647,8 @@ impl SourcePosition<'_> { /// i.e. if source length is 10, `self` must be on position 8 max. #[inline] pub(super) unsafe fn read2(self) -> [u8; 2] { + debug_assert!(!self.ptr.is_null()); + // SAFETY: // Caller guarantees `self` is not at no later than 2 bytes before end of source text. // `Source` is created from a valid `&str`, so points to allocated, initialized memory. @@ -648,7 +656,6 @@ impl SourcePosition<'_> { // to the same memory can exist, as that would violate Rust's aliasing rules. // Pointer is "dereferenceable" by definition as a `u8` is 1 byte and cannot span multiple objects. // Alignment is not relevant as `u8` is aligned on 1 (i.e. no alignment requirements). - debug_assert!(!self.ptr.is_null()); #[expect(clippy::ptr_as_ptr)] unsafe { let p = self.ptr as *const [u8; 2]; diff --git a/crates/oxc_traverse/src/context/ancestry.rs b/crates/oxc_traverse/src/context/ancestry.rs index cff55e4253d18..b6419be1d6192 100644 --- a/crates/oxc_traverse/src/context/ancestry.rs +++ b/crates/oxc_traverse/src/context/ancestry.rs @@ -194,6 +194,7 @@ impl<'a> TraverseAncestry<'a> { #[expect(clippy::ptr_as_ptr, clippy::ref_as_ptr)] pub(crate) unsafe fn retag_stack(&mut self, ty: AncestorType) { debug_assert!(self.stack.len() >= 2); + // SAFETY: Caller must uphold the safety invariants unsafe { *(self.stack.last_mut() as *mut _ as *mut AncestorType) = ty }; } } diff --git a/crates/oxc_traverse/src/context/mod.rs b/crates/oxc_traverse/src/context/mod.rs index 7fdd4bc476870..d024bb9e0eb78 100644 --- a/crates/oxc_traverse/src/context/mod.rs +++ b/crates/oxc_traverse/src/context/mod.rs @@ -649,6 +649,7 @@ impl<'a> TraverseCtx<'a> { /// This method must not be public outside this crate, or consumer could break safety invariants. #[inline] pub(crate) unsafe fn retag_stack(&mut self, ty: AncestorType) { + // SAFETY: Caller muct uphold safety constraints unsafe { self.ancestry.retag_stack(ty) }; }