fix(thread_aware_macros): bound phantom-only generics by Send

[martinhavelka (wukchung)]

🤖 This description was written by an AI agent.

Problem

#[derive(ThreadAware)] failed to compile for any type whose generic parameter
appears only inside a PhantomData field:

#[derive(ThreadAware)]
struct DirectPhantom<T, U>(T, PhantomData<U>);

error[E0277]: `U` cannot be sent between threads safely
note: required because it appears within the type `PhantomData<U>`
note: required by a bound in `ThreadAware`

Bound collection returned early on PhantomData, so U gained no bound at all
and the generated impl could not satisfy the ThreadAware: Send supertrait.
Skipping the field in the relocation body is correct — a marker holds no value —
but skipping it also dropped the only bound that would have made the type Send.

The sole workaround was to widen the user's own public API to satisfy the derive:

struct Workaround<T, U: Send>(T, PhantomData<U>);

A second shape failed differently: PhantomData is recognized syntactically on
the field type, so a nested occurrence such as (PhantomData<T>,) was relocated
like any other field, and no ThreadAware impl for PhantomData existed to
satisfy it.

Change

The ThreadAware: Send obligation for fields that are never relocated is now
stated once, on Self, rather than derived per parameter:

• a parameter reachable through a relocated field is bound by ThreadAware, as before;
• if any field is present but never relocated — a PhantomData marker or a
  #[thread_aware(skip)] field — the impl gains where Self: Send;
• a marker nested inside a relocated field instead gets
  where PhantomData<X>: ThreadAware, which the compiler reduces through that
  marker's own impl.

Stating the obligation structurally rather than inferring it is what makes this
correct for every shape. Two narrower formulations were tried and are unsound:

Formulation	Fails on
bind each parameter named inside PhantomData<X> by Send	&'a T needs T: Sync; Arc<T> needs T: Send + Sync; [T] and <T as Tr>::Assoc reachable by no traversal at all
bind each unrelocated field type by Send	any type made Send by a manual unsafe impl — where *const T: Send is unprovable, so the impl compiles but no caller can use it

Self: Send is exactly the obligation the supertrait imposes, and it is
discharged either structurally or by a manual unsafe impl. Both of the shapes
in the table now work, including the raw-pointer variance marker and a skipped
Rc<T>.

Adds impl<T: ?Sized + Send> ThreadAware for PhantomData<T>, so a marker nested
in a tuple, array or reference relocates as a no-op. This also serves
hand-written generic code, not only derived impls.

Also removes bound suppression that matched only the final path segment: an
unrelated some_crate::ThreadAware was treated as this crate's trait, dropping a
bound the generated body needs. Comparison is now over every segment.

Compatibility

No API change, and no change for any existing consumer. Every crate that derives
ThreadAware — anyspawn, bytesbuf, cachet_memory, fetch — builds and
lints unchanged. Types that previously compiled continue to; types that
previously could not now do.

Two limitations are inherent and are documented on the derive. A macro cannot
resolve a path to the item it names, so both PhantomData and ThreadAware are
matched syntactically: a distinct type whose name ends in PhantomData is
treated as a marker, and a distinct trait named ThreadAware, referred to by
that bare name, is assumed to be this crate's. Qualifying either path
disambiguates. Relocating look-alikes unconditionally instead was tried and
reverted — it left bound inference and body generation disagreeing about which
fields are relocated.

Validation

cargo test -p thread_aware --lib                      117 passed
cargo test -p thread_aware --test derive_compiles      18 passed
cargo test -p thread_aware_macros_impl --lib           19 passed
cargo test -p thread_aware_macros_impl --test derive   30 passed
just package=thread_aware clippy                      clean
just package=thread_aware_macros_impl clippy          clean
just package=bytesbuf clippy                          clean
just package=thread_aware format                      no changes
cargo check -p thread_aware --no-default-features     clean

The derive was previously covered only by insta snapshots, which compare token
streams and never compile them. A snapshot can confirm the output matches what
was recorded, but here the recorded output was itself the defect, so the suite
passed while the feature was unusable. derive__generics_add_bounds.snap held
exactly the impl<T: ThreadAware, U> shape that fails to compile.

The compile-and-run tests in crates/thread_aware/tests/derive_compiles.rs link
the real crate and execute the expansion, which is why they catch this class of
bug. They cover phantom-only generics, nested PhantomData, PhantomData of a
shared reference, of Arc<T>, of an unsized slice and of an associated-type
projection, a parameter that is both relocated and phantom, a raw-pointer marker
and a skipped Rc<T> under manual unsafe impl Send, a skipped generic field,
an enum carrying a phantom-only parameter, and named enum variants under
deny(warnings). Two assert behaviour rather than compilation: that relocate
reaches every non-skipped field exactly once and no skipped one.

To confirm these are genuine regression tests, the source fixes were stashed and
the suite re-run. It reproduced the reported errors verbatim and compiled again
once the fixes were restored.

Snapshots remain, and now pin the bound-selection logic: pre-bound parameters,
lifetime and const parameters, an unrelated trait named ThreadAware, a
concrete marker argument that must produce no predicate, a repeated argument
that must produce only one, and an existing where clause the generated
predicate has to extend rather than replace.

Reviewer notes

Three snapshots were removed or rewritten because they pinned expansions that
cannot compile — &'a T and [u8; N] have no ThreadAware impl — or inputs
rustc rejects outright (struct UnusedParam<T, U>(T); is E0392). Those are the
same defect class as the bug this PR fixes, one level up.

Named enum variants bound every field by name while emitting no statement for
markers and skipped fields, so generated code carried an unused variable and
broke any consumer using deny(warnings) — on a shape this PR is what makes
compile in the first place. Such fields now bind as field: _.

Out of scope, worth a follow-up: trybuild cases pinning the diagnostics for
shapes that must keep failing, such as unions; and an audit of the other derive
macros in the workspace for the same snapshot-only coverage pattern.

AB#7745298

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (af4bdaa) to head (2d5c544).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #678   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files         543      543           
  Lines       60394    60431   +37     
=======================================
+ Hits        60394    60431   +37     

Flag	Coverage Δ
linux	94.1% <100.0%> (?)
linux-arm	94.1% <100.0%> (?)
windows	94.1% <100.0%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[martinhavelka (wukchung)]

🤖 Automated multi-model review: five independent reviewers (claude-opus-5, gpt-5.6-sol, claude-opus-4.7, gpt-5.5, grok-4.5), with every finding verified by compiling the shape in a worktree before posting. Roughly 18 raw findings, 5 survived verification. Refuted claims are not listed.

The core direction of the change is right. Splitting "reachable through a relocated field" from "named only inside PhantomData" is the correct distinction, ThreadAware-over-Send precedence is right for the simple PhantomData<T> case, the new impl<T: ?Sized + Send> ThreadAware for PhantomData<T> is sound with no overlapping impl, and ::core::marker::Send is the correct emitted path for no_std. Both originally reported shapes do now work.

What does not hold up is the per-parameter inference used to derive the Send bound - see the inline comment on collect_phantom_generics. It is correct for PhantomData<T>, PhantomData<(T,)> and PhantomData<[T; N]>, and wrong for references, Arc, slices and projections, all of which still fail to compile. A structural where X: Send predicate fixes every case at once and removes the need for the recursive walk entirely.

One item could not be anchored inline, because the file is not part of the diff:

[Medium] The derive rustdoc is now false. crates/thread_aware/src/lib.rs:239-240 still says generic type parameters "appearing in non-skipped fields automatically receive a ::thread_aware::ThreadAware bound (occurrences only inside PhantomData<..> are ignored)". Phantom-only parameters now receive a Send bound, which is a user-visible API change - a caller's T must now be Send. This is the only place the bound rule is documented, and the wrapper crate has no equivalent text. The "non-skipped fields" clause is inaccurate as well, per the note on collect_generics_in_fields.

No vote cast.

[Copilot]

Pull request overview

Fixes #[derive(ThreadAware)] for generics that are only mentioned inside PhantomData, by ensuring the generated impl adds an appropriate bound (Send instead of ThreadAware) and by providing a ThreadAware impl for PhantomData<T> itself so nested occurrences relocate as a no-op.

Changes:

• Update derive bound selection to distinguish generics reached via relocated fields (ThreadAware) vs PhantomData-only (Send).
• Add ThreadAware for PhantomData<T> (no-op relocate) with T: ?Sized + Send.
• Expand tests (compile-and-run + snapshots) to cover phantom-only generics and nested PhantomData shapes.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
crates/thread_aware/tests/derive_compiles.rs	Adds compile-and-run regression tests covering phantom-only generics and nested PhantomData.
crates/thread_aware/src/impls.rs	Adds ThreadAware impl for PhantomData<T> as a relocation no-op with T: Send.
crates/thread_aware_macros_impl/src/lib.rs	Implements generic-usage classification and adds Send bounds for phantom-only parameters.
crates/thread_aware_macros_impl/tests/derive.rs	Adds snapshot-driven tests for phantom-only bound behavior and additional shapes.
crates/thread_aware_macros_impl/tests/snapshots/derive__phantom_tuple_generic_gets_send_bound.snap	New snapshot asserting Send bound for tuple-contained phantom generic.
crates/thread_aware_macros_impl/tests/snapshots/derive__phantom_ref_generic_gets_send_bound.snap	New snapshot asserting Send bound for ref-contained phantom generic.
crates/thread_aware_macros_impl/tests/snapshots/derive__phantom_prebound_thread_aware_no_send.snap	New snapshot asserting no redundant Send when already ThreadAware.
crates/thread_aware_macros_impl/tests/snapshots/derive__phantom_prebound_send_no_dup.snap	New snapshot asserting no duplicate Send bound.
crates/thread_aware_macros_impl/tests/snapshots/derive__phantom_paren_generic_gets_send_bound.snap	New snapshot asserting Send bound for paren-contained phantom generic.
crates/thread_aware_macros_impl/tests/snapshots/derive__phantom_group_generic_gets_send_bound.snap	New snapshot asserting Send bound for group-contained phantom generic.
crates/thread_aware_macros_impl/tests/snapshots/derive__phantom_data_unnamed_fields.snap	Updates snapshot to show Send bound added for phantom-only generic.
crates/thread_aware_macros_impl/tests/snapshots/derive__phantom_data_named_fields.snap	Updates snapshot to show Send bound added for phantom-only generic.
crates/thread_aware_macros_impl/tests/snapshots/derive__phantom_array_generic_gets_send_bound.snap	New snapshot asserting Send bound for array-contained phantom generic.
crates/thread_aware_macros_impl/tests/snapshots/derive__generics_unused_param_gets_no_bound.snap	New snapshot asserting unused generic params remain unbounded.
crates/thread_aware_macros_impl/tests/snapshots/derive__generics_lifetime_and_const_params_untouched.snap	New snapshot asserting lifetime/const generics are untouched while type param is bounded.
crates/thread_aware_macros_impl/tests/snapshots/derive__generics_add_bounds.snap	Updates snapshot to add Send bound for phantom-only generic.
crates/thread_aware_macros_impl/tests/snapshots/derive__enum_unnamed_phantom_data.snap	Updates snapshot to add Send bounds for phantom-only enum generics.
crates/thread_aware_macros_impl/tests/snapshots/derive__enum_named_phantom_data.snap	Updates snapshot to add Send bounds for phantom-only enum generics.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

crates/thread_aware_macros_impl/tests/derive.rs:370

• In test code, prefer .unwrap() over .expect(...) so failures include the full backtrace without requiring a hand-written message (per repo guidelines). These new .expect(...) calls can be simplified.

    // A parameter used directly and inside `PhantomData` carries both bounds; treating
    // the two as mutually exclusive dropped the phantom one.
    let input = quote! {
        #[derive(ThreadAware)]
        struct RelocatedAndPhantom<'a, T: 'a>(T, core::marker::PhantomData<&'a T>);

crates/thread_aware_macros_impl/src/lib.rs:172

• collect_generics_in_fields currently walks every field’s type without considering #[thread_aware(skip)], but the generated relocation body explicitly skips those fields (see struct_gen/enum_gen). This means a generic that appears only in a skipped field will still be treated as “relocated” and get a ThreadAware bound, even though it’s never relocated. To match the derive semantics (and the PR description’s “reachable through a relocated field”), skipped fields should be excluded from usage.relocated and handled as “non-relocated but still must satisfy Send for ThreadAware: Send.”

/// Reports whether `ty` names any of the type's own generic parameters.
///
/// Scans tokens rather than matching on [`Type`] variants so that every shape is covered,
/// including qualified paths, bare functions and const-generic expressions. Over-reporting

[Copilot]

Pull request overview

Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.

Suppressed comments (1)

crates/thread_aware/src/lib.rs:249

• The docs here say the derive emits a Send predicate on the field type for PhantomData<..> markers, but the macro actually places the Send predicate on the PhantomData argument type (e.g. &'a T: Send / Arc<T>: Send) to preserve correct auto-trait semantics and to support nested PhantomData relocation via the new ThreadAware for PhantomData<_> impl. The text should be updated to match the generated bounds.

/// * A field that is never relocated - a `PhantomData<..>` marker, or one annotated with
///   `#[thread_aware(skip)]` - instead produces a `where` predicate requiring that field's
///   type to be [`Send`], which the `ThreadAware: Send` supertrait demands.
///
/// The `Send` obligation is placed on the field type itself rather than on the type

[Copilot]

Pull request overview

Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.

Suppressed comments (1)

crates/thread_aware/src/lib.rs:249

• The docs say non-relocated fields (including PhantomData<..>) add a where predicate requiring the field type to be Send, but the derive actually adds a predicate on the PhantomData argument type (e.g. &'a T: Send), not on PhantomData<&'a T>. This is a good implementation detail (it also enables PhantomData<X>: ThreadAware via the new ThreadAware for PhantomData<T> impl), but the documentation should match the generated bounds to avoid confusing users.

/// * A field that is never relocated - a `PhantomData<..>` marker, or one annotated with
///   `#[thread_aware(skip)]` - instead produces a `where` predicate requiring that field's
///   type to be [`Send`], which the `ThreadAware: Send` supertrait demands.
///
/// The `Send` obligation is placed on the field type itself rather than on the type

[Copilot]

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.

Suppressed comments (1)

crates/thread_aware/src/lib.rs:244

• The docs say a non-relocated field (including PhantomData<..>) “produces a where predicate requiring that field's type to be Send”. For PhantomData<X>, the derive actually adds the Send obligation on the argument type (e.g. X: Send / &'a T: Send / [T]: Send), not on PhantomData<X> itself. Tweaking this wording would make the docs match the generated bounds more precisely.

/// * A field that is never relocated - a `PhantomData<..>` marker, or one annotated with
///   `#[thread_aware(skip)]` - instead produces a `where` predicate requiring that field's
///   type to be [`Send`], which the `ThreadAware: Send` supertrait demands.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (1)

crates/thread_aware_macros_impl/src/lib.rs:174

• is_same_trait treats any single-segment ThreadAware bound as equivalent to the emitted ::thread_aware::ThreadAware. This is ambiguous: if a user has a different in-scope trait named ThreadAware (e.g. a local trait or a re-export under the same name), the derive may incorrectly suppress adding the required ::thread_aware::ThreadAware bound and the expansion will fail to type-check when it calls ::thread_aware::ThreadAware::relocate on a T that isn't bound by that trait. To avoid incorrect suppression, only treat paths whose segments match the emitted path as the same trait (accepting that this may introduce a redundant bound in the common use thread_aware::ThreadAware; T: ThreadAware case).

fn is_same_trait(candidate: &Path, emitted: &Path) -> bool {
    let candidate_idents: Vec<_> = candidate.segments.iter().map(|s| s.ident.to_string()).collect();
    let emitted_idents: Vec<_> = emitted.segments.iter().map(|s| s.ident.to_string()).collect();

    candidate_idents == emitted_idents || candidate_idents == ["ThreadAware"]
}

[Copilot]

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

crates/thread_aware/tests/zz_probe_bare.rs:3

• This file is labeled as a “Temporary review probe - not part of the PR”, but it is currently included as a new integration test. It should be removed from the PR to avoid shipping/maintaining ad-hoc probe coverage (and to avoid adding a test that doesn’t assert behavior).

// Temporary review probe - not part of the PR.
// Claim: a bare local trait named `ThreadAware` suppresses the generated bound.
use thread_aware_macros::ThreadAware;

crates/thread_aware/tests/zz_probe_mixed.rs:3

• This file is labeled as a “Temporary review probe - not part of the PR”, but it is currently included as a new integration test. It should be removed from the PR to avoid committing temporary probe code.

// Temporary review probe - not part of the PR.
// Claim: the `generics_lifetime_and_const_params_untouched` snapshot pins an
// expansion that does not compile.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.

Suppressed comments (2)

crates/thread_aware_macros_impl/src/lib.rs:242

• Same as above: this coverage(off) exclusion/comment appears stale now that the PR adds many tests specifically exercising collect_generics_in_type. Consider removing it so coverage reflects this code path.

#[cfg_attr(coverage_nightly, coverage(off))] // can't figure out how to get to 100% coverage of this function
fn collect_generics_in_type(ty: &Type, generic_idents: &HashSet<syn::Ident>, acc: &mut GenericUsage) -> syn::Result<()> {

crates/thread_aware_macros_impl/src/lib.rs:216

• The coverage(off) exclusion and the accompanying comment are now misleading (and they also remove these lines from the coverage gate). If the new tests truly cover this logic, this attribute should be removed so coverage accurately reflects regressions.

This issue also appears on line 241 of the same file.

#[cfg_attr(coverage_nightly, coverage(off))] // can't figure out how to get to 100% coverage of this function
fn collect_generics_in_fields(fields: &Fields, generics: &syn::Generics) -> syn::Result<GenericUsage> {

[martinhavelka (wukchung)]

🤖 Review of af4bdaa1...66156f4a. Six independent reviewers; every finding was re-verified against the current head before posting, and refuted or out-of-scope claims were dropped rather than passed on.

[Low] The title and description describe a mechanism the branch abandoned.

The title says "bound phantom-only generics by Send", and the body says "named only inside PhantomData — bound by Send". The final diff never bounds a phantom-only parameter. It emits where Self: ::core::marker::Send once per type when any field is unrelocated, plus where PhantomData<X>: ThreadAware for markers nested in a relocated field. The doc comment this PR adds to crates/thread_aware/src/lib.rs states that binding the parameters named inside a marker "would be unsound" — so the description currently advertises the original defect as the fix.

Stale details in the body:

• "Five snapshots changed" — 5 modified and 13 added.
• "185 passed" — cargo test -p thread_aware -p thread_aware_macros_impl reports 199 at this head.
• "a PhantomData nested in a tuple, array or reference relocates as a no-op" — arrays do not work at all; ThreadAware has no [T; N] impl (impls.rs:100).

Since the squashed title is what lands in the changelog, it is worth retitling around the actual mechanism — something like "state the Send obligation on Self".

Verified as correct, recorded because the design holds up well under attack: PhantomData<&'a T>, PhantomData<Arc<T>>, PhantomData<[T]>, PhantomData<T::Assoc>, PhantomData<fn(T)>, PhantomData<for<'a> fn(&'a T)>, PhantomData<PhantomData<T>>, PhantomData<dyn Send>, ?Sized parameters, default type parameters and recursive types all compile and instantiate. Stating the obligation on Self correctly defers it instead of over-binding, and the earlier per-parameter Send unsoundness is genuinely gone.

Out of scope — both reproduce identically at the merge base, so they are follow-ups rather than review comments: #[derive(ThreadAware)] on an empty enum fails with E0004 (match self {} is non-exhaustive for an inhabited &mut), and collect_generics_in_type treats the last segment of a qualified path such as concrete::T as a use of the generic T.

Test gap worth noting: impl ThreadAware for PhantomData<T> is the one impl in impls.rs with no unit test of its own, unlike every other impl in that file; it is exercised only transitively from thread_aware/tests/derive_compiles.rs.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.

Suppressed comments (1)

crates/thread_aware_macros_impl/tests/derive.rs:70

• The comment says this test ensures no duplicate ThreadAware bound when already present, but the expected expansion now does include both T: ThreadAware and T: ::thread_aware::ThreadAware (see updated snapshot). This is misleading and makes it harder to understand why the snapshot changed.

fn generics_prebound_bare_is_still_bound() {
    // Ensures no duplicate ThreadAware bound when already present.
    let input = quote! {

[Copilot]

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated no new comments.

[Evgenii (Vaiz)]

I'm not sure if we care about Send bound here. What if we remove it completely?

[Evgenii (Vaiz)]

I assume that there is no need for any special handling of phantom data because now it implements ThreadAware

[Ralf Biedert (ralfbiedert)]

🤖 Requesting changes for the PhantomData look-alike case: the derive can compile while silently omitting required relocation.

[Ralf Biedert (ralfbiedert)]

🤖 This assertion preserves a derive-contract violation: a qualified data-carrying type whose final segment is PhantomData is treated as the marker and silently receives zero relocations. The new Self: Send bound makes this compile while retaining cross-thread contention. Please restrict the special case to canonical core::marker::PhantomData / std::marker::PhantomData paths.