Skip to content

Conversation

@ADD-SP
Copy link
Member

@ADD-SP ADD-SP commented Oct 25, 2025

Closes #7707

@ADD-SP ADD-SP added C-maintenance Category: PRs that clean code up or issues documenting cleanup. A-tokio Area: The main tokio crate labels Oct 25, 2025
@github-actions github-actions bot added R-loom-current-thread Run loom current-thread tests on this PR R-loom-multi-thread Run loom multi-thread tests on this PR R-loom-sync Run loom sync tests on this PR R-loom-time-driver Run loom time driver tests on this PR labels Oct 25, 2025
Comment on lines +286 to +288
unsafe {
self.assume_init(cnt);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

/// Additionally, there must be no concurrent mutations.
pub(crate) unsafe fn unsync_load(&self) -> u16 {
core::ptr::read(self.inner.get() as *const u16)
unsafe { core::ptr::read(self.inner.get() as *const u16) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

/// Additionally, there must be no concurrent mutations.
pub(crate) unsafe fn unsync_load(&self) -> u32 {
core::ptr::read(self.inner.get() as *const u32)
unsafe { core::ptr::read(self.inner.get() as *const u32) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

/// Additionally, there must be no concurrent mutations.
pub(crate) unsafe fn unsync_load(&self) -> usize {
core::ptr::read(self.inner.get() as *const usize)
unsafe { core::ptr::read(self.inner.get() as *const usize) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

F: FnMut() -> io::Result<()> + Send + Sync + 'static,
{
self.std.pre_exec(f);
unsafe { self.std.pre_exec(f) };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

@ADD-SP ADD-SP force-pushed the add_sp/deny-unsafe_op_in_unsafe_fn branch 2 times, most recently from 1f8c154 to f72239f Compare October 25, 2025 13:05
/// Must be called with the same `Synced` instance returned by `Inject::new`
pub(crate) unsafe fn pop(&self, synced: &mut Synced) -> Option<task::Notified<T>> {
self.pop_n(synced, 1).next()
unsafe { self.pop_n(synced, 1).next() }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

F: FnOnce(&Self) -> R,
{
crate::runtime::context::with_trace(f)
unsafe { crate::runtime::context::with_trace(f) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”


unsafe fn from_raw(ptr: NonNull<Header>) -> Task<S> {
Task::from_raw(ptr)
unsafe { Task::from_raw(ptr) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”


unsafe fn pointers(target: NonNull<Header>) -> NonNull<linked_list::Pointers<Header>> {
self::core::Trailer::addr_of_owned(Header::get_trailer(target))
unsafe { self::core::Trailer::addr_of_owned(Header::get_trailer(target)) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

@ADD-SP ADD-SP force-pushed the add_sp/deny-unsafe_op_in_unsafe_fn branch from f72239f to e2e4749 Compare October 25, 2025 13:08

pub(crate) unsafe fn add_entry(&mut self, item: TimerHandle) {
let slot = slot_for(item.registered_when(), self.level);
let slot = slot_for(unsafe { item.registered_when() }, self.level);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since almost all methods on TimerHandle is unsafe, and both of them share the same safety requirement as follow, it doesn't helpful to add additional "Safety" here.

/// An `TimerHandle` is the (non-enforced) "unique" pointer from the driver to the
/// timer entry. Generally, at most one `TimerHandle` exists for a timer at a time
/// (enforced by the timer state machine).
///
/// SAFETY: An `TimerHandle` is essentially a raw pointer, and the usual caveats
/// of pointer safety apply. In particular, `TimerHandle` does not itself enforce
/// that the timer does still exist; however, normally an `TimerHandle` is created
/// immediately before registering the timer, and is consumed when firing the
/// timer, to help minimize mistakes. Still, because `TimerHandle` cannot enforce
/// memory safety, all operations are unsafe.
#[derive(Debug)]
pub(crate) struct TimerHandle {
inner: NonNull<TimerShared>,
}

item: TimerHandle,
) -> Result<u64, (TimerHandle, InsertError)> {
let when = item.sync_when();
let when = unsafe { item.sync_when() };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since almost all methods on TimerHandle is unsafe, and both of them share the same safety requirement as follow, it doesn't helpful to add additional "Safety" here.

/// An `TimerHandle` is the (non-enforced) "unique" pointer from the driver to the
/// timer entry. Generally, at most one `TimerHandle` exists for a timer at a time
/// (enforced by the timer state machine).
///
/// SAFETY: An `TimerHandle` is essentially a raw pointer, and the usual caveats
/// of pointer safety apply. In particular, `TimerHandle` does not itself enforce
/// that the timer does still exist; however, normally an `TimerHandle` is created
/// immediately before registering the timer, and is consumed when firing the
/// timer, to help minimize mistakes. Still, because `TimerHandle` cannot enforce
/// memory safety, all operations are unsafe.
#[derive(Debug)]
pub(crate) struct TimerHandle {
inner: NonNull<TimerShared>,
}

Comment on lines 646 to 691
pub(super) unsafe fn set_expiration(&self, tick: u64) {
self.inner.as_ref().set_expiration(tick);
unsafe {
self.inner.as_ref().set_expiration(tick);
}
}

/// Attempts to mark this entry as pending. If the expiration time is after
/// `not_after`, however, returns an Err with the current expiration time.
///
/// If an `Err` is returned, the `registered_when` value will be updated to this
/// new expiration time.
///
/// SAFETY: The caller must ensure that the handle remains valid, the driver
/// lock is held, and that the timer is not in any wheel linked lists.
/// After returning Ok, the entry must be added to the pending list.
pub(super) unsafe fn mark_pending(&self, not_after: u64) -> Result<(), u64> {
match self.inner.as_ref().state.mark_pending(not_after) {
match unsafe { self.inner.as_ref().state.mark_pending(not_after) } {
Ok(()) => {
// mark this as being on the pending queue in registered_when
self.inner.as_ref().set_registered_when(STATE_DEREGISTERED);
unsafe {
self.inner.as_ref().set_registered_when(STATE_DEREGISTERED);
}
Ok(())
}
Err(tick) => {
self.inner.as_ref().set_registered_when(tick);
unsafe {
self.inner.as_ref().set_registered_when(tick);
}
Err(tick)
}
}
}

/// Attempts to transition to a terminal state. If the state is already a
/// terminal state, does nothing.
///
/// Because the entry might be dropped after the state is moved to a
/// terminal state, this function consumes the handle to ensure we don't
/// access the entry afterwards.
///
/// Returns the last-registered waker, if any.
///
/// SAFETY: The driver lock must be held while invoking this function, and
/// the entry must not be in any wheel linked lists.
pub(super) unsafe fn fire(self, completed_state: TimerResult) -> Option<Waker> {
self.inner.as_ref().state.fire(completed_state)
unsafe { self.inner.as_ref().state.fire(completed_state) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since almost all methods on TimerHandle is unsafe, and both of them share the same safety requirement as follow, it doesn't helpful to add additional "Safety" here.

/// An `TimerHandle` is the (non-enforced) "unique" pointer from the driver to the
/// timer entry. Generally, at most one `TimerHandle` exists for a timer at a time
/// (enforced by the timer state machine).
///
/// SAFETY: An `TimerHandle` is essentially a raw pointer, and the usual caveats
/// of pointer safety apply. In particular, `TimerHandle` does not itself enforce
/// that the timer does still exist; however, normally an `TimerHandle` is created
/// immediately before registering the timer, and is consumed when firing the
/// timer, to help minimize mistakes. Still, because `TimerHandle` cannot enforce
/// memory safety, all operations are unsafe.
#[derive(Debug)]
pub(crate) struct TimerHandle {
inner: NonNull<TimerShared>,
}

#[cfg(all(tokio_unstable, feature = "tracing"))]
let future = crate::util::trace::task(future, "task", meta, id.as_u64());
self.inner.spawn_local(future, id, meta.spawned_at)
unsafe { self.inner.spawn_local(future, id, meta.spawned_at) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

/// The pointer must have been created by [`Self::into_raw`].
unsafe fn from_raw(ptr: *const ()) -> Arc<Inner> {
Arc::from_raw(ptr as *const Inner)
unsafe { Arc::from_raw(ptr as *const Inner) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

Comment on lines +328 to +331
unsafe {
Arc::increment_strong_count(raw as *const Inner);
}
unsafe { unparker_to_raw_waker(Inner::from_raw(raw)) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

/// The pointer must have been created by [`Inner::into_raw`].
unsafe fn drop_waker(raw: *const ()) {
drop(Inner::from_raw(raw));
drop(unsafe { Inner::from_raw(raw) });
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

/// The pointer must have been created by [`Inner::into_raw`].
unsafe fn wake(raw: *const ()) {
let unparker = Inner::from_raw(raw);
let unparker = unsafe { Inner::from_raw(raw) };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

Comment on lines +354 to +356
unsafe {
(*raw).unpark();
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

// SAFETY: The OnceCell must not be empty.
unsafe fn get_unchecked(&self) -> &T {
&*self.value.with(|ptr| (*ptr).as_ptr())
unsafe { &*self.value.with(|ptr| (*ptr).as_ptr()) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

/// if it is set, then only the receiver may call this method.
unsafe fn consume_value(&self) -> Option<T> {
self.value.with_mut(|ptr| (*ptr).take())
self.value.with_mut(|ptr| unsafe { (*ptr).take() })
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

/// if it is set, then only the receiver may call this method.
unsafe fn has_value(&self) -> bool {
self.value.with(|ptr| (*ptr).is_some())
self.value.with(|ptr| unsafe { (*ptr).is_some() })
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

// SAFETY: The SetOnce must not be empty.
unsafe fn get_unchecked(&self) -> &T {
&*self.value.with(|ptr| (*ptr).as_ptr())
unsafe { &*self.value.with(|ptr| (*ptr).as_ptr()) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is very short, and it’s clear enough for developers to reason about the safety requirements, so we don’t need additional "Safety" comments here”

@ADD-SP ADD-SP force-pushed the add_sp/deny-unsafe_op_in_unsafe_fn branch 2 times, most recently from 0c2f1d2 to 2413a5d Compare October 25, 2025 13:22
@ADD-SP ADD-SP marked this pull request as draft October 25, 2025 13:23
@ADD-SP ADD-SP force-pushed the add_sp/deny-unsafe_op_in_unsafe_fn branch from 2413a5d to b975c86 Compare October 25, 2025 13:24
@ADD-SP ADD-SP force-pushed the add_sp/deny-unsafe_op_in_unsafe_fn branch from b975c86 to 9409eaf Compare October 25, 2025 13:29
@ADD-SP ADD-SP changed the title tokio enable the unsafe_op_in_unsafe_fn lint at the crate level tokio: enable the unsafe_op_in_unsafe_fn lint at the crate level Oct 25, 2025
@ADD-SP ADD-SP marked this pull request as ready for review October 25, 2025 13:45
impl<$($gen)*> $struct_name {$(
#[doc = "# Safety"]
#[doc = ""]
#[doc = "The `self` pointer must be valid."]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Should it say self or me here ?

// Safety: `ptr` was created from a `Header` pointer in function `waker_ref`.
let ptr = unsafe { NonNull::new_unchecked(ptr as *mut Header) };
// TODO; replace to #[expect(unsafe_op_in_unsafe_fn)] after bumping MSRV to 1.81.0.
#[cfg_attr(not(all(tokio_unstable, feature = "tracing")), allow(unused_unsafe))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows unused_unsafe but the TODO says expect(unsafe_op_in_unsafe_fn)

// Safety: `ptr` was created from a `Header` pointer in function `waker_ref`.
let ptr = unsafe { NonNull::new_unchecked(ptr as *mut Header) };
// TODO; replace to #[expect(unsafe_op_in_unsafe_fn)] after bumping MSRV to 1.81.0.
#[cfg_attr(not(all(tokio_unstable, feature = "tracing")), allow(unused_unsafe))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows unused_unsafe but the TODO says expect(unsafe_op_in_unsafe_fn)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-tokio Area: The main tokio crate C-maintenance Category: PRs that clean code up or issues documenting cleanup. R-loom-current-thread Run loom current-thread tests on this PR R-loom-multi-thread Run loom multi-thread tests on this PR R-loom-sync Run loom sync tests on this PR R-loom-time-driver Run loom time driver tests on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable the unsafe_op_in_unsafe_fn lint

2 participants