gate(#10944): the asserted-global rule could not see static mut - #11004
proggeramlug wants to merge 1 commit into
Conversation
Review finding on #10947. `_STATIC` required the identifier immediately after `static`, so a `static mut` declaration never matched at all; and had it matched, `_SHARED_TY` would have excluded it anyway, because a `static mut` is usually a plain integer or array rather than an Atomic or a lock. That is a hole exactly where the hazard is worst. An `AtomicU64` read under contention gives a wrong count; racing on a `static mut` is undefined behaviour. The one shape the rule most needed to catch was the one shape it structurally could not. Fixed by capturing an optional `mut` and treating its presence as sufficient on its own -- a `static mut` is shared mutable state by definition, so it does not have to argue its way past a type filter. Latent today, and stated as such rather than claimed as a catch: the tree's only two `static mut` declarations (ohos_napi.rs) are asserted by no test, so the baseline stays at 62 entries and this commit changes no current verdict. It closes the gap before one arrives. The line anchor keeps `&'static mut` references out: the three in test helpers begin with `let` or `fn`, not `static`. Checked against all five real occurrences in the tree plus four constructed near-misses. Self-test gains a `static mut` case that fails without the fix.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe asserted-global scanner now captures ChangesMutable Static Detection
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The scanner now detects asserted static mut declarations, with self-test coverage added. No merge-blocking production impact is identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Landed on main in merge train 257 (#11039, v0.5.1640), main Carried at head This train was split by blast radius after an earlier 35-PR assembly hit five gap regressions: it carries only PRs touching no lowering path. Trains rebase-merge, so commits get new SHAs and GitHub cannot mark this merged. Closed as landed. |
1 participant
Review finding on PR #10947, which closed unmerged after its work landed on main as
fc7108496b. The finding was never applied, so the gap is live onmaintoday atscripts/global_sink_isolation.py:594. This is that fix, rebased onto main.The gap
_STATICrequired the identifier immediately afterstatic, so astatic mutdeclaration never matched at all. And had it matched,_SHARED_TYwould have excluded it anyway — astatic mutis typically a plain integer or array, not anAtomic*or a lock.Two independent reasons the rule could not see it, in the one place it most needed to. A contended
AtomicU64read gives a wrong count; racing on astatic mutis undefined behaviour. The gate's hole sat exactly where the hazard is worst.The fix
Capture an optional
mutand treat its presence as sufficient on its own. Astatic mutis shared mutable state by definition — it does not have to argue its way past a type filter.Stated as latent, not claimed as a catch
The tree's only two
static mutdeclarations (ohos_napi.rs:804,:876) are asserted by no test. The baseline stays at 62 entries and this changes no current verdict. It closes the gap before one arrives.Checks
&'static mutreferences out — the three in test helpers (sweep_page_tally.rs:61,concat_site.rs:14,full_rebuild_skip.rs:42) begin withletorfn, notstatic. Verified against all five real occurrences in the tree plus four constructed near-misses; 8/8 discriminate correctly.--self-test: 20 checks, 0 failures. Gate: 0 hazards, 6 allowlisted, 120 statics classified, 62 asserted (baseline 62).an assertedstatic mutwas NOT reported); restoring the fix returns it to green.Summary by CodeRabbit