Skip to content

refactor(semantic): implement Send and Sync for ScopingCell#13042

Merged
graphite-app[bot] merged 1 commit intomainfrom
08-13-refactor_semantic_implement_send_and_sync_for_scopingcell_
Aug 19, 2025
Merged

refactor(semantic): implement Send and Sync for ScopingCell#13042
graphite-app[bot] merged 1 commit intomainfrom
08-13-refactor_semantic_implement_send_and_sync_for_scopingcell_

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Aug 13, 2025

Previous PRs removed unsound Sync impl for Allocator (#13033) and Send impl for Vec (#13041).

Previously ScopingCell was Send and Sync (and therefore Scoping was too). The recent PRs mean that ScopingCell lost those traits too.

This PR implements both traits again for ScopingCell by restricting its API slightly, so now it is Send and Sync again, but this time in a manner which maintains soundness.

The comments in the code outline the logic of why I believe it to be sound.

Rolldown relies on ScopingCell being Send and Sync. After this PR, this stack does not require any changes in Rolldown. I've checked that cargo ck in Rolldown passes when using the version of Oxc from this branch.

@github-actions github-actions bot added A-semantic Area - Semantic C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Aug 13, 2025
Copy link
Member Author

overlookmotel commented Aug 13, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@codspeed-hq
Copy link

codspeed-hq bot commented Aug 13, 2025

CodSpeed Instrumentation Performance Report

Merging #13042 will not alter performance

Comparing 08-13-refactor_semantic_implement_send_and_sync_for_scopingcell_ (cc067c2) with 08-18-fix_allocator_remove_unsound_send_impl_and_tighten_sync_requirement_for_hashmap_ (f490d27)

Summary

✅ 34 untouched benchmarks

@overlookmotel overlookmotel marked this pull request as ready for review August 13, 2025 00:41
@overlookmotel overlookmotel requested a review from Dunqing as a code owner August 13, 2025 00:41
@overlookmotel overlookmotel force-pushed the 08-13-refactor_semantic_implement_send_and_sync_for_scopingcell_ branch from 76d7ef7 to 94907c8 Compare August 13, 2025 01:37
Copy link
Member

@Dunqing Dunqing left a comment

Choose a reason for hiding this comment

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

Great!

@graphite-app graphite-app bot force-pushed the 08-12-fix_allocator_remove_unsound_send_impl_and_tighten_sync_requirements_for_vec_ branch from 1529968 to 49299c7 Compare August 13, 2025 11:53
@graphite-app graphite-app bot requested review from Sysix and camc314 as code owners August 13, 2025 11:53
@graphite-app graphite-app bot force-pushed the 08-13-refactor_semantic_implement_send_and_sync_for_scopingcell_ branch from 94907c8 to b85beb1 Compare August 13, 2025 11:54
@overlookmotel overlookmotel changed the base branch from 08-12-fix_allocator_remove_unsound_send_impl_and_tighten_sync_requirements_for_vec_ to graphite-base/13042 August 18, 2025 23:53
@overlookmotel overlookmotel force-pushed the 08-13-refactor_semantic_implement_send_and_sync_for_scopingcell_ branch from b85beb1 to fd22653 Compare August 18, 2025 23:55
@overlookmotel overlookmotel changed the base branch from graphite-base/13042 to 08-18-fix_allocator_remove_unsound_send_impl_and_tighten_sync_requirement_for_hashmap_ August 18, 2025 23:55
@overlookmotel overlookmotel force-pushed the 08-13-refactor_semantic_implement_send_and_sync_for_scopingcell_ branch from fd22653 to 32c7632 Compare August 19, 2025 00:12
@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Aug 19, 2025
@graphite-app
Copy link
Contributor

graphite-app bot commented Aug 19, 2025

Merge activity

…3042)

Previous PRs removed unsound `Sync` impl for `Allocator` (#13033) and `Send` impl for `Vec` (#13041).

Previously `ScopingCell` was `Send` and `Sync` (and therefore `Scoping` was too). The recent PRs mean that `ScopingCell` lost those traits too.

This PR implements both traits again for `ScopingCell` by restricting its API slightly, so now it is `Send` and `Sync` again, but this time in a manner which maintains soundness.

The comments in the code outline the logic of why I believe it to be sound.

Rolldown relies on `ScopingCell` being `Send` and `Sync`. After this PR, this stack does not require any changes in Rolldown. I've checked that `cargo ck` in Rolldown passes when using the version of Oxc from this branch.
@graphite-app graphite-app bot force-pushed the 08-18-fix_allocator_remove_unsound_send_impl_and_tighten_sync_requirement_for_hashmap_ branch from 1b86fbb to f490d27 Compare August 19, 2025 21:38
@graphite-app graphite-app bot force-pushed the 08-13-refactor_semantic_implement_send_and_sync_for_scopingcell_ branch from 32c7632 to cc067c2 Compare August 19, 2025 21:38
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Aug 19, 2025
Base automatically changed from 08-18-fix_allocator_remove_unsound_send_impl_and_tighten_sync_requirement_for_hashmap_ to main August 19, 2025 21:45
@graphite-app graphite-app bot merged commit cc067c2 into main Aug 19, 2025
26 checks passed
@graphite-app graphite-app bot deleted the 08-13-refactor_semantic_implement_send_and_sync_for_scopingcell_ branch August 19, 2025 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-semantic Area - Semantic C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants