remove several uses of unsafe (#8600)

This PR removes several uses of `unsafe`. I generally limited myself to
low hanging fruit that I could see. There are still a few remaining uses
of `unsafe` that looked a bit more difficult to remove (if possible at
all). But this gets rid of a good chunk of them.

I put each `unsafe` removal into its own commit with a justification for
why I did it. So I would encourage reviewing this PR commit-by-commit.
That way, we can legislate them independently. It's no problem to drop a
commit if we feel the `unsafe` should stay in that case.

crates/ruff_formatter/src/arguments.rs

@@ -1,23 +1,9 @@
 use super::{Buffer, Format, Formatter};
 use crate::FormatResult;
-use std::ffi::c_void;
-use std::marker::PhantomData;
 
-/// Mono-morphed type to format an object. Used by the [`crate::format`!], [`crate::format_args`!], and
+/// A convenience wrapper for representing a formattable argument.
-/// [`crate::write`!] macros.
-///
-/// This struct is similar to a dynamic dispatch (using `dyn Format`) because it stores a pointer to the value.
-/// However, it doesn't store the pointer to `dyn Format`'s vtable, instead it statically resolves the function
-/// pointer of `Format::format` and stores it in `formatter`.
 pub struct Argument<'fmt, Context> {
-    /// The value to format stored as a raw pointer where `lifetime` stores the value's lifetime.
+    value: &'fmt dyn Format<Context>,
-    value: *const c_void,
-
-    /// Stores the lifetime of the value. To get the most out of our dear borrow checker.
-    lifetime: PhantomData<&'fmt ()>,
-
-    /// The function pointer to `value`'s `Format::format` method
-    formatter: fn(*const c_void, &mut Formatter<'_, Context>) -> FormatResult<()>,
 }
 
 impl<Context> Clone for Argument<'_, Context> {
@@ -28,32 +14,19 @@ impl<Context> Clone for Argument<'_, Context> {
 impl<Context> Copy for Argument<'_, Context> {}
 
 impl<'fmt, Context> Argument<'fmt, Context> {
-    /// Called by the [ruff_formatter::format_args] macro. Creates a mono-morphed value for formatting
+    /// Called by the [ruff_formatter::format_args] macro.
-    /// an object.
     #[doc(hidden)]
     #[inline]
     pub fn new<F: Format<Context>>(value: &'fmt F) -> Self {
-        #[inline]
+        Self { value }
-        fn formatter<F: Format<Context>, Context>(
-            ptr: *const c_void,
-            fmt: &mut Formatter<Context>,
-        ) -> FormatResult<()> {
-            // SAFETY: Safe because the 'fmt lifetime is captured by the 'lifetime' field.
-            #[allow(unsafe_code)]
-            F::fmt(unsafe { &*ptr.cast::<F>() }, fmt)
-        }
-
-        Self {
-            value: (value as *const F).cast::<std::ffi::c_void>(),
-            lifetime: PhantomData,
-            formatter: formatter::<F, Context>,
-        }
     }
 
     /// Formats the value stored by this argument using the given formatter.
     #[inline]
+    // Seems to only be triggered on wasm32 and looks like a false positive?
+    #[allow(clippy::trivially_copy_pass_by_ref)]
     pub(super) fn format(&self, f: &mut Formatter<Context>) -> FormatResult<()> {
-        (self.formatter)(self.value, f)
+        self.value.fmt(f)
     }
 }
 

crates/ruff_formatter/src/builders.rs

@@ -2555,17 +2555,17 @@ pub struct BestFitting<'a, Context> {
 }
 
 impl<'a, Context> BestFitting<'a, Context> {
-    /// Creates a new best fitting IR with the given variants. The method itself isn't unsafe
+    /// Creates a new best fitting IR with the given variants.
-    /// but it is to discourage people from using it because the printer will panic if
+    ///
-    /// the slice doesn't contain at least the least and most expanded variants.
+    /// Callers are required to ensure that the number of variants given
+    /// is at least 2.
     ///
     /// You're looking for a way to create a `BestFitting` object, use the `best_fitting![least_expanded, most_expanded]` macro.
     ///
-    /// ## Safety
+    /// # Panics
-
+    ///
-    /// The slice must contain at least two variants.
+    /// When the slice contains less than two variants.
-    #[allow(unsafe_code)]
+    pub fn from_arguments_unchecked(variants: Arguments<'a, Context>) -> Self {
-    pub unsafe fn from_arguments_unchecked(variants: Arguments<'a, Context>) -> Self {
         assert!(
             variants.0.len() >= 2,
             "Requires at least the least expanded and most expanded variants"
@@ -2696,14 +2696,12 @@ impl<Context> Format<Context> for BestFitting<'_, Context> {
             buffer.write_element(FormatElement::Tag(EndBestFittingEntry));
         }
 
-        // SAFETY: The constructor guarantees that there are always at least two variants. It's, therefore,
+        // OK because the constructor guarantees that there are always at
-        // safe to call into the unsafe `from_vec_unchecked` function
+        // least two variants.
-        #[allow(unsafe_code)]
+        let variants = BestFittingVariants::from_vec_unchecked(buffer.into_vec());
-        let element = unsafe {
+        let element = FormatElement::BestFitting {
-            FormatElement::BestFitting {
+            variants,
-                variants: BestFittingVariants::from_vec_unchecked(buffer.into_vec()),
+            mode: self.mode,
-                mode: self.mode,
-            }
         };
 
         f.write_element(element);

crates/ruff_formatter/src/format_element.rs

@@ -332,17 +332,14 @@ pub enum BestFittingMode {
 pub struct BestFittingVariants(Box<[FormatElement]>);
 
 impl BestFittingVariants {
-    /// Creates a new best fitting IR with the given variants. The method itself isn't unsafe
+    /// Creates a new best fitting IR with the given variants.
-    /// but it is to discourage people from using it because the printer will panic if
+    ///
-    /// the slice doesn't contain at least the least and most expanded variants.
+    /// Callers are required to ensure that the number of variants given
+    /// is at least 2 when using `most_expanded` or `most_flag`.
     ///
     /// You're looking for a way to create a `BestFitting` object, use the `best_fitting![least_expanded, most_expanded]` macro.
-    ///
-    /// ## Safety
-    /// The slice must contain at least two variants.
     #[doc(hidden)]
-    #[allow(unsafe_code)]
+    pub fn from_vec_unchecked(variants: Vec<FormatElement>) -> Self {
-    pub unsafe fn from_vec_unchecked(variants: Vec<FormatElement>) -> Self {
         debug_assert!(
             variants
                 .iter()
@@ -351,12 +348,23 @@ impl BestFittingVariants {
                 >= 2,
             "Requires at least the least expanded and most expanded variants"
         );
-
         Self(variants.into_boxed_slice())
     }
 
     /// Returns the most expanded variant
+    ///
+    /// # Panics
+    ///
+    /// When the number of variants is less than two.
     pub fn most_expanded(&self) -> &[FormatElement] {
+        assert!(
+            self.as_slice()
+                .iter()
+                .filter(|element| matches!(element, FormatElement::Tag(Tag::StartBestFittingEntry)))
+                .count()
+                >= 2,
+            "Requires at least the least expanded and most expanded variants"
+        );
         self.into_iter().last().unwrap()
     }
 
@@ -365,7 +373,19 @@ impl BestFittingVariants {
     }
 
     /// Returns the least expanded variant
+    ///
+    /// # Panics
+    ///
+    /// When the number of variants is less than two.
     pub fn most_flat(&self) -> &[FormatElement] {
+        assert!(
+            self.as_slice()
+                .iter()
+                .filter(|element| matches!(element, FormatElement::Tag(Tag::StartBestFittingEntry)))
+                .count()
+                >= 2,
+            "Requires at least the least expanded and most expanded variants"
+        );
         self.into_iter().next().unwrap()
     }
 }

crates/ruff_formatter/src/macros.rs

@@ -329,10 +329,8 @@ macro_rules! format {
 #[macro_export]
 macro_rules! best_fitting {
     ($least_expanded:expr, $($tail:expr),+ $(,)?) => {{
-        #[allow(unsafe_code)]
+        // OK because the macro syntax requires at least two variants.
-        unsafe {
+        $crate::BestFitting::from_arguments_unchecked($crate::format_args!($least_expanded, $($tail),+))
-            $crate::BestFitting::from_arguments_unchecked($crate::format_args!($least_expanded, $($tail),+))
-        }
     }}
 }
 

crates/ruff_macros/src/newtype_index.rs

@@ -45,6 +45,9 @@ pub(super) fn generate_newtype_index(item: ItemStruct) -> syn::Result<proc_macro
                 // SAFETY:
                 // * The `value < u32::MAX` guarantees that the add doesn't overflow.
                 // * The `+ 1` guarantees that the index is not zero
+                //
+                // N.B. We have to use the unchecked variant here because we're
+                // in a const context and Option::unwrap isn't const yet.
                 #[allow(unsafe_code)]
                 Self(unsafe { std::num::NonZeroU32::new_unchecked((value as u32) + 1) })
             }
@@ -55,6 +58,9 @@ pub(super) fn generate_newtype_index(item: ItemStruct) -> syn::Result<proc_macro
                 // SAFETY:
                 // * The `value < u32::MAX` guarantees that the add doesn't overflow.
                 // * The `+ 1` guarantees that the index is larger than zero.
+                //
+                // N.B. We have to use the unchecked variant here because we're
+                // in a const context and Option::unwrap isn't const yet.
                 #[allow(unsafe_code)]
                 Self(unsafe { std::num::NonZeroU32::new_unchecked(value + 1) })
             }

crates/ruff_python_formatter/src/comments/map.rs

@@ -539,11 +539,11 @@ struct PartIndex(NonZeroU32);
 impl PartIndex {
     fn from_len(value: usize) -> Self {
         assert!(value < u32::MAX as usize);
-        // SAFETY:
+        // OK because:
         // * The `value < u32::MAX` guarantees that the add doesn't overflow.
         // * The `+ 1` guarantees that the index is not zero
-        #[allow(unsafe_code, clippy::cast_possible_truncation)]
+        #[allow(clippy::cast_possible_truncation)]
-        Self(unsafe { std::num::NonZeroU32::new_unchecked((value as u32) + 1) })
+        Self(std::num::NonZeroU32::new((value as u32) + 1).expect("valid value"))
     }
 
     fn value(self) -> usize {

crates/ruff_python_formatter/src/expression/binary_like.rs

@@ -1105,9 +1105,8 @@ impl OperatorIndex {
     fn new(index: usize) -> Self {
         assert_eq!(index % 2, 1, "Operator indices must be odd positions");
 
-        // SAFETY A value with a module 0 is guaranteed to never equal 0
+        // OK because a value with a modulo 1 is guaranteed to never equal 0
-        #[allow(unsafe_code)]
+        Self(NonZeroUsize::new(index).expect("valid index"))
-        Self(unsafe { NonZeroUsize::new_unchecked(index) })
     }
 
     const fn value(self) -> usize {

crates/ruff_python_literal/src/escape.rs

@@ -415,12 +415,10 @@ impl<'a> Escape for AsciiEscape<'a> {
     fn layout(&self) -> &EscapeLayout {
         &self.layout
     }
-    #[allow(unsafe_code)]
     fn write_source(&self, formatter: &mut impl std::fmt::Write) -> std::fmt::Result {
-        formatter.write_str(unsafe {
+        // OK because function must be called only when source is printable ascii characters.
-            // SAFETY: this function must be called only when source is printable ascii characters
+        let string = std::str::from_utf8(self.source).expect("ASCII bytes");
-            std::str::from_utf8_unchecked(self.source)
+        formatter.write_str(string)
-        })
     }
 
     #[cold]

crates/ruff_python_parser/src/string.rs

@@ -120,10 +120,8 @@ impl<'a> StringParser<'a> {
             len += 1;
         }
 
-        // SAFETY: radix_bytes is always going to be in the ASCII range.
+        // OK because radix_bytes is always going to be in the ASCII range.
-        #[allow(unsafe_code)]
+        let radix_str = std::str::from_utf8(&radix_bytes[..len]).expect("ASCII bytes");
-        let radix_str = unsafe { std::str::from_utf8_unchecked(&radix_bytes[..len]) };
-
         let value = u32::from_str_radix(radix_str, 8).unwrap();
         char::from_u32(value).unwrap()
     }

crates/ruff_source_file/src/newlines.rs

@@ -58,11 +58,7 @@ impl<'a> UniversalNewlineIterator<'a> {
 pub fn find_newline(text: &str) -> Option<(usize, LineEnding)> {
     let bytes = text.as_bytes();
     if let Some(position) = memchr2(b'\n', b'\r', bytes) {
-        // SAFETY: memchr guarantees to return valid positions
+        let line_ending = match bytes[position] {
-        #[allow(unsafe_code)]
-        let newline_character = unsafe { *bytes.get_unchecked(position) };
-
-        let line_ending = match newline_character {
             // Explicit branch for `\n` as this is the most likely path
             b'\n' => LineEnding::Lf,
             // '\r\n'