skiplist: Remove the 'static bounds by taiki-e · Pull Request #1238 · crossbeam-rs/crossbeam

taiki-e · 2026-02-22T09:43:55Z

Closes #205

Copilot

Pull request overview

This PR removes the K: 'static / V: 'static bounds from SkipMap/SkipSet mutation APIs by switching SkipMap to pin against either the global collector (when K/V don’t need drop) or a per-map Collector plus per-thread handles.

Changes:

Remove 'static bounds from SkipMap/SkipSet and corresponding entry APIs.
Introduce per-SkipMap collector selection + per-thread pinning via thread_local::ThreadLocal.
Add regression tests demonstrating non-'static key/value usage compiles.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
crossbeam-skiplist/src/map.rs	Adds local pinning (`ThreadLocal` handles) and removes `'static` bounds from public map APIs.
crossbeam-skiplist/src/set.rs	Removes `'static` bounds for set APIs (delegates to `SkipMap`).
crossbeam-skiplist/src/base.rs	Makes `SkipList::{new,with_comparator}` unsafe and removes `'static` bounds from base methods; exposes collector to crate.
crossbeam-skiplist/Cargo.toml	Adds optional `thread_local` dependency enabled by `std`.
crossbeam-skiplist/tests/map.rs	Adds a non-`'static` compilation regression test.
crossbeam-skiplist/tests/set.rs	Adds a non-`'static` compilation regression test.
crossbeam-skiplist/tests/base.rs	Updates tests to call the now-unsafe `SkipList::new` safely via a helper.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-22T09:56:50Z

 impl<K, V> SkipList<K, V> {
    /// Returns a new, empty skip list.
-    pub fn new(collector: Collector) -> Self {
-        Self::with_comparator(collector, Default::default())
+    ///
+    /// # Safety
+    ///
+    /// If `mem::needs_drop::<K>` or/and `mem::needs_drop::<V>` is true,
+    /// `collector` must destroy garbage within the lifetime of `K` or/and `V`.
+    ///
+    /// The easiest way to ensure this is to request `K: 'static` and `V: 'static` bounds.
+    pub unsafe fn new(collector: Collector) -> Self {
+        // SAFETY: the caller must uphold the safety contract.
+        unsafe { Self::with_comparator(collector, Default::default()) }
    }
 }

 impl<K, V, C> SkipList<K, V, C> {
    /// Returns a new, empty skip list using the given comparator.
-    pub fn with_comparator(collector: Collector, comparator: C) -> Self {
+    ///
+    /// # Safety
+    ///
+    /// If `mem::needs_drop::<K>` or/and `mem::needs_drop::<V>` is true,
+    /// `collector` must destroy garbage within the lifetime of `K` or/and `V`.
+    ///
+    /// The easiest way to ensure this is to request `K: 'static` and `V: 'static` bounds.
+    pub unsafe fn with_comparator(collector: Collector, comparator: C) -> Self {
        Self {


SkipList::new/with_comparator becoming unsafe is a public API breaking change (the type is re-exported from the crate). Consider providing a safe constructor that preserves the previous ergonomics (e.g., a safe new that requires K: 'static, V: 'static and/or !needs_drop::<K/V>()), and keep the new behavior behind an unsafe “unchecked” constructor. Also consider documenting this change prominently (e.g., in the changelog) since callers will now need unsafe just to construct a SkipList.

Copilot · 2026-02-22T09:56:51Z

 }

+struct LocalHandle(epoch::LocalHandle);
+// TODO


LocalHandle contains an epoch::LocalHandle but is given an unsafe impl Send with a // TODO marker and no soundness rationale. Since Send is an unsafe contract, this needs an explicit safety comment explaining why it’s safe to move/drop these handles across threads in this usage, or (preferably) avoid the unsafe impl (e.g., by using a handle type that is already Send / adjusting the storage strategy so the handle never needs to cross threads).

Suggested change

// TODO

// SAFETY:

// `LocalHandle` is only ever stored inside `ThreadLocal<LocalHandle>` (see the `handles`

// field of `SkipMap`). The `thread_local` crate ensures that each per-thread value is

// created, accessed, and dropped on the same thread that owns it. As a result, even

// though `LocalHandle` is marked as `Send` to satisfy `ThreadLocal`'s bounds, the

// wrapped `epoch::LocalHandle` is never moved to or used from a different thread than

// the one that created it, preserving the thread-affinity guarantees required by

// `epoch::LocalHandle`.

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-22T12:44:58Z

 pub struct SkipMap<K, V, C = BasicComparator> {
    inner: base::SkipList<K, V, C>,
+    handles: ThreadLocal<LocalHandle>,
 }


Field drop order is currently inner then handles, but handles stores LocalHandles registered with inner.collector. Dropping inner (and thus its Collector) before dropping handles can make dropping those LocalHandles invalid/UB. Fix by reordering fields so handles is declared before inner, or implement Drop (or use ManuallyDrop) to ensure handles are dropped before inner.

Copilot · 2026-02-22T12:44:58Z


    /// The `Collector` associated with this skip list.
-    collector: Collector,
+    pub(crate) collector: Collector,


Making collector a pub(crate) field increases coupling and makes it easier to accidentally rely on internal invariants. Prefer keeping the field private and exposing a narrow pub(crate) accessor (e.g., fn collector(&self) -> &Collector) or a dedicated registration/pinning helper on SkipList so only the intended operations are possible.

Copilot · 2026-02-22T12:44:59Z

+fn new_() {
+    new::<i32, i32>(epoch::default_collector().clone());
+    new::<String, Box<i32>>(epoch::default_collector().clone());
 }


The helper new forces the test function to be named new_, which is a bit opaque. Consider renaming the helper to something more specific (e.g., new_skiplist_static) so the test can remain fn new() without the underscore.

Copilot · 2026-02-22T12:44:59Z

+    /// If `mem::needs_drop::<K>` or/and `mem::needs_drop::<V>` is true,
+    /// `collector` must destroy garbage within the lifetime of `K` or/and `V`.


In these doc comments, "or/and" is grammatically awkward. Consider changing to "and/or" (or rephrasing) for clarity.

Suggested change

/// If `mem::needs_drop::<K>` or/and `mem::needs_drop::<V>` is true,

/// `collector` must destroy garbage within the lifetime of `K` or/and `V`.

/// If `mem::needs_drop::<K>` or `mem::needs_drop::<V>` is true,

/// `collector` must destroy garbage within the lifetime of `K` and/or `V`.

taiki-e added the crossbeam-skiplist label Feb 22, 2026

taiki-e requested a review from Copilot February 22, 2026 09:44

taiki-e added the breaking change label Feb 22, 2026

Copilot started reviewing on behalf of taiki-e February 22, 2026 09:45 View session

taiki-e force-pushed the taiki-e/skiplist-non-static branch from 7b04c6b to 0939c55 Compare February 22, 2026 09:45

Copilot AI reviewed Feb 22, 2026

View reviewed changes

taiki-e force-pushed the taiki-e/skiplist-non-static branch 2 times, most recently from 67c187a to a8e00f7 Compare February 22, 2026 12:30

skiplist: Remove the 'static bounds

6207923

taiki-e force-pushed the taiki-e/skiplist-non-static branch from a8e00f7 to 6207923 Compare February 22, 2026 12:37

taiki-e requested a review from Copilot February 22, 2026 12:38

Copilot AI reviewed Feb 22, 2026

View reviewed changes

Copilot started reviewing on behalf of taiki-e February 22, 2026 12:45 View session

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

skiplist: Remove the 'static bounds#1238

skiplist: Remove the 'static bounds#1238
taiki-e wants to merge 1 commit into
mainfrom
taiki-e/skiplist-non-static

taiki-e commented Feb 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

-// TODO
+// SAFETY:
+// `LocalHandle` is only ever stored inside `ThreadLocal<LocalHandle>` (see the `handles`
+// field of `SkipMap`). The `thread_local` crate ensures that each per-thread value is
+// created, accessed, and dropped on the same thread that owns it. As a result, even
+// though `LocalHandle` is marked as `Send` to satisfy `ThreadLocal`'s bounds, the
+// wrapped `epoch::LocalHandle` is never moved to or used from a different thread than
+// the one that created it, preserving the thread-affinity guarantees required by
+// `epoch::LocalHandle`.

		/// If `mem::needs_drop::<K>` or/and `mem::needs_drop::<V>` is true,
		/// `collector` must destroy garbage within the lifetime of `K` or/and `V`.

Uh oh!

Conversation

taiki-e commented Feb 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants