jsc: require T: Send/T: Sync for JsCell<T>'s Send/Sync impls - #31500
Closed
robobun wants to merge 2 commits into
Closed
jsc: require T: Send/T: Sync for JsCell<T>'s Send/Sync impls#31500robobun wants to merge 2 commits into
T: Send/T: Sync for JsCell<T>'s Send/Sync impls#31500robobun wants to merge 2 commits into
Claude / Claude Code Review
completed
May 28, 2026 in 6m 53s
Code review found 2 potential issues
Found 6 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/jsc/JSCell.rs:133-136 |
Manual unsafe impl<T: Send> Send is now equivalent to auto-derivation |
| 🟡 Nit | src/jsc/JSCell.rs:244-246 |
Comment misnames the failure mode as 'conflicting implementations' |
Annotations
Check warning on line 136 in src/jsc/JSCell.rs
claude / Claude Code Review
Manual unsafe impl<T: Send> Send is now equivalent to auto-derivation
Nit: with the bound tightened to `T: Send`, this manual impl is now behaviorally identical to auto-derivation — `UnsafeCell<T>` only opts out of `Sync`, not `Send`, so `JsCell<T>: Send` iff `T: Send` even without this line. You could delete it and drop one `unsafe` to audit (the proof module would still pass), though keeping it for symmetry with the necessary `Sync` impl and the SAFETY doc is also a perfectly defensible choice.
Check warning on line 246 in src/jsc/JSCell.rs
claude / Claude Code Review
Comment misnames the failure mode as 'conflicting implementations'
Nit: the comment says the const "fails to compile with *conflicting implementations*", which is E0119's wording — but the two blanket impls have distinct `A` parameters and never overlap at definition time. The actual failure is **E0283** (ambiguous impl selection at the `NotSend<_>` use site), which the PR description and the test's `expect(output).not.toContain("E0283")` both name correctly. Consider rewording to "fails to compile with E0283 (ambiguous impl)" so the comment matches the test as
Loading