refactor: addr_of_mut to OnceLock#2486
Merged
HerringtonDarkholme merged 2 commits intoFeb 22, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request converts unsafe global mutable statics to thread-safe OnceLock-based lazy initialization patterns across the CLI and dynamic language modules. The changes remove unsafe blocks and make registration functions safe by eliminating mutable static state. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2486 +/- ##
=======================================
Coverage 88.14% 88.15%
=======================================
Files 108 108
Lines 17876 17874 -2
=======================================
- Hits 15757 15756 -1
+ Misses 2119 2118 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/cli/src/lang/injection.rs (1)
54-75: Missingdebug_assert!for double-registration, unlikelang_globs.rs.In
lang_globs.rs(line 17–20), there is adebug_assert!(LANG_GLOBS.get().is_none(), "language globs already registered")to catch accidental double-registration in debug builds. This file lacks a similar guard. Ifregister_injetablesis called twice,.set().ok()silently discards the new data, and line 61's.get().expect(...)would read the stale first registration — a subtle data-loss scenario.Consider adding a
debug_assert!for consistency:Proposed fix
pub fn register_injetables(injections: Vec<SerializableInjection>) -> Result<()> { + debug_assert!( + LANG_INJECTIONS.get().is_none(), + "language injections already registered" + ); let mut injectable = HashMap::new();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/lang/injection.rs` around lines 54 - 75, Add a debug-only assertion at the start of register_injetables to ensure we aren't double-registering: call debug_assert!(LANG_INJECTIONS.get().is_none(), "language injections already registered") (and optionally do the same for INJECTABLE_LANGS if you want both guards) before building and setting the maps so a debug build will catch accidental second registrations instead of silently dropping the new data.crates/cli/src/lang/lang_globs.rs (1)
153-165: Test uses the globalOnceLock— may conflict with other tests callingregister().
OnceLockcan only be set once per process. Since Rust's test harness runs tests as threads in a single process, if any other test in this crate (or integration tests) also callsregister(), thedebug_assert!will fire or data will be silently dropped. This is a pre-existing test-isolation concern with global state, not introduced by this PR — just worth noting as a known limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/lang/lang_globs.rs` around lines 153 - 165, The test uses the global OnceLock LANG_GLOBS via register(), which can only be set once per process and will conflict with other tests; modify the test or code so tests don't rely on global state: either add a cfg(test)-only helper to reset or initialize LANG_GLOBS (exposing a test_reset or test_set function) and call that from test_merge_with_globs, or refactor register()/LANG_GLOBS to allow injection of a local globs instance (e.g., accept a Globs param or provide a non-global merge_globs_in_place) so the test can operate on a per-test Globs instance instead of calling the global register(); update test_merge_with_globs to use the new helper/injectable API and reference the existing register(), LANG_GLOBS, and merge_globs symbols when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/dynamic/src/lib.rs`:
- Around line 157-159: Remove the incorrect "/// # Safety" doc heading above the
non-unsafe function register and instead document the one-time-call requirement
as a normal doc comment; update the comment block for pub fn register(regs:
Vec<Registration>) -> Result<(), DynamicLangError> so the note "The register
function should be called exactly once before use." appears as a regular
documentation line (not under a Safety section) immediately above the function
signature.
---
Nitpick comments:
In `@crates/cli/src/lang/injection.rs`:
- Around line 54-75: Add a debug-only assertion at the start of
register_injetables to ensure we aren't double-registering: call
debug_assert!(LANG_INJECTIONS.get().is_none(), "language injections already
registered") (and optionally do the same for INJECTABLE_LANGS if you want both
guards) before building and setting the maps so a debug build will catch
accidental second registrations instead of silently dropping the new data.
In `@crates/cli/src/lang/lang_globs.rs`:
- Around line 153-165: The test uses the global OnceLock LANG_GLOBS via
register(), which can only be set once per process and will conflict with other
tests; modify the test or code so tests don't rely on global state: either add a
cfg(test)-only helper to reset or initialize LANG_GLOBS (exposing a test_reset
or test_set function) and call that from test_merge_with_globs, or refactor
register()/LANG_GLOBS to allow injection of a local globs instance (e.g., accept
a Globs param or provide a non-global merge_globs_in_place) so the test can
operate on a per-test Globs instance instead of calling the global register();
update test_merge_with_globs to use the new helper/injectable API and reference
the existing register(), LANG_GLOBS, and merge_globs symbols when making the
change.
9dd8b43 to
2ccf176
Compare
HerringtonDarkholme
approved these changes
Feb 22, 2026
Member
|
Thanks! |
nicopauss
pushed a commit
to Intersec/lib-common
that referenced
this pull request
Apr 1, 2026
##### [\`v0.41.0\`](https://github.com/ast-grep/ast-grep/blob/HEAD/CHANGELOG.md#0410) - chore(deps): update pyo3/maturin-action action to v1.50.0 [`#2465`](ast-grep/ast-grep#2465) - fix(deps): update rust-wasm-bindgen monorepo [`#2491`](ast-grep/ast-grep#2491) - chore(deps): update rust crate toml\_edit to v0.25.3 [`#2469`](ast-grep/ast-grep#2469) - chore(deps): update dependency web-tree-sitter to ^0.26.0 [`#2492`](ast-grep/ast-grep#2492) - refactor: addr\_of\_mut to OnceLock [`#2486`](ast-grep/ast-grep#2486) - feat: support ast-grep-wasm [`#2484`](ast-grep/ast-grep#2484) - chore(deps): update rust crate clap to v4.5.60 [`#2483`](ast-grep/ast-grep#2483) - chore(deps): update rust crate anyhow to v1.0.102 [`#2489`](ast-grep/ast-grep#2489) - chore(deps): update dependency oxlint to v1.49.0 [`#2488`](ast-grep/ast-grep#2488) - feat: make default rule id to filename [`#2482`](ast-grep/ast-grep#2482) - chore(deps): update dependency oxlint to v1.48.0 [`#2481`](ast-grep/ast-grep#2481) - chore(deps): update rust crate napi-derive to v3.5.2 [`#2475`](ast-grep/ast-grep#2475) - chore(deps): update rust crate napi to v3.8.3 [`#2474`](ast-grep/ast-grep#2474) - chore(deps): update rust crate futures to v0.3.32 [`#2476`](ast-grep/ast-grep#2476) - chore(deps): update rust crate clap to v4.5.58 [`#2470`](ast-grep/ast-grep#2470) - fix: parse each language injection region as independent tree [`#2479`](ast-grep/ast-grep#2479) - chore(deps): update dependency [@types/node](https://github.com/types/node) to v24.10.13 [`#2467`](ast-grep/ast-grep#2467) - chore(deps): update rust crate clap\_complete to v4.5.66 [`#2471`](ast-grep/ast-grep#2471) - chore(deps): update rust crate predicates to v3.1.4 [`#2472`](ast-grep/ast-grep#2472) - chore(deps): update dependency oxlint to v1.47.0 [`#2468`](ast-grep/ast-grep#2468) - chore(deps): update rust crate tempfile to v3.25.0 [`#2466`](ast-grep/ast-grep#2466) - fix(deps): update rust crate toml\_edit to 0.25.0 [`#2473`](ast-grep/ast-grep#2473) - chore(deps): update rust crate tree-sitter to v0.26.5 [`#2458`](ast-grep/ast-grep#2458) - chore(deps): update dependency [@types/node](https://github.com/types/node) to v24.10.12 [`#2438`](ast-grep/ast-grep#2438) - chore(deps): update rust crate schemars to v1.2.1 [`#2457`](ast-grep/ast-grep#2457) - chore(deps): update rust crate clap to v4.5.57 [`#2455`](ast-grep/ast-grep#2455) - chore(deps): update rust crate anyhow to v1.0.101 [`#2462`](ast-grep/ast-grep#2462) - chore(deps): update rust crate inquire to v0.9.3 [`#2463`](ast-grep/ast-grep#2463) - chore(deps): update dependency oxlint to v1.43.0 [`#2459`](ast-grep/ast-grep#2459) - chore(deps): update rust crate regex to v1.12.3 [`#2461`](ast-grep/ast-grep#2461) - chore(deps): update rust crate clap to v4.5.55 [`#2454`](ast-grep/ast-grep#2454) - chore(deps): update dependency oxlint to v1.42.0 [`#2452`](ast-grep/ast-grep#2452) - chore(deps): update dependency oxlint to v1.41.0 [`#2447`](ast-grep/ast-grep#2447) - chore(deps): update rust crate assert\_cmd to v2.1.2 [`#2437`](ast-grep/ast-grep#2437) - chore(deps): update rust crate thiserror to v2.0.18 [`#2446`](ast-grep/ast-grep#2446) - chore(deps): update dependency oxlint to v1.39.0 [`#2442`](ast-grep/ast-grep#2442) - chore(deps): update rust crate inquire to v0.9.2 [`#2445`](ast-grep/ast-grep#2445) - fix: improve max diagnostic impl to use one atomic op [`781ec81`](ast-grep/ast-grep@781ec81) - fix: udpate schema [`e645fdf`](ast-grep/ast-grep@e645fdf) - fix: move dumping to core crate [`0b2acb9`](ast-grep/ast-grep@0b2acb9) Renovate-Branch: renovate/2024.6-ast-grep-cli-0.x Change-Id: I88da2bd1f360bebda04663520c30423d0cb7ddd1 Priv-Id: 72123de3a05b89754a6bf040b3bd2708f3d9a9e2
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I have noticed that
addr_of_mutcan be replaced byOnceLockin order to removeunsafe.In fact, the mutation is made only once during project setup if i'm not wrong.
Summary by CodeRabbit