feat(dylints): add ban_env_var_set_after_import + require_oncelock_install_before_use (#840)#892
Conversation
…stall_before_use (#840) Adds two convention-based dylints under dylints/ mirroring the established pattern from dylints/ban_runtime_new_outside_main/: - ban_env_var_set_after_import: bans std::env::set_var outside main.rs / src/bin / tests / #[cfg(test)]. Motivated by the Python paths.py / cli.py FBUILD_DEV_MODE order-of-init bug captured in the project memory. - require_oncelock_install_before_use: bans std::sync::OnceLock::set and OnceLock::set_blocking outside the same allow-list. Convention shortcut for the "Approaches to investigate -> convention-based static check" suggestion in #840. Both lints document the convention-vs-MIR limitation: they catch the *attempt* at the call site, not the actual "set after read" dataflow. The MIR-level check is tracked as a follow-up on #840 ("MIR for completeness later"). Wiring: - root Cargo.toml [workspace] exclude updated with both dirs - ci/hooks/crate_guard.py APPROVED_CRATE_DIRS updated with both dirs No existing dylint sources touched. Pre-existing call-site violations are not allow-listed; maintainer can fix in follow-up or annotate with per-call #[allow(...)] attributes. Closes #840
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
✨ 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 |
PR #892's audit identified 3 production OnceLock::set sites in `crates/fbuild-core/src/containment.rs` (GLOBAL_GROUP + Windows JOB installs) and `crates/fbuild-build/src/compile_backend.rs` (install_global). All three are one-shot install paths called from the binary entry points before any consumer reaches the OnceLock — the order-of-init bug class the lint targets is structurally impossible there. Add them to the path-based allowlist so the new lint ships at the intended Deny level without breaking the existing build. New non-install OnceLock::set call sites outside these files (or outside main.rs / src/bin / tests / cfg(test)) will trip the lint as designed.
Summary
Adds two convention-based dylints under
dylints/mirroring the established pattern fromdylints/ban_runtime_new_outside_main/. Both target the same bug class: process-global state that gets initialized in the wrong order between modules. The motivating example from #840 is the Pythonpaths.py/cli.pyFBUILD_DEV_MODEorder-of-init bug captured in the project memory.Approach: convention-based, not MIR dataflow
Per #840's "Approaches to investigate -> convention-based static check" recommendation. These lints catch the attempt at the call site, not the actual "set after read" dataflow. The latter requires MIR-level dataflow across the whole crate graph and is the "MIR for completeness later" follow-up tracked on #840. Both READMEs document this limitation explicitly.
Lint surface
ban_env_var_set_after_importBANNED_PATHS = [["std", "env", "set_var"]]Bans
std::env::set_varoutside:**/main.rs**/src/bin/****/tests/**#[cfg(test)]Rationale: env-var mutation has order-of-init hazards. If a module already read the variable into a
static/OnceLockbefore thisset_varran, that captured value sticks. Onlymain()(and tests, which manage their own process state) may mutate env vars.require_oncelock_install_before_useBANNED_PATHS = [["std", "sync", "OnceLock", "set"], ["std", "sync", "OnceLock", "set_blocking"]]Bans both
OnceLock::setandOnceLock::set_blockingoutside the same allow-list. Rationale:OnceLockis single-assignment; if a reader already filled the lock viaget_or_init, the late.set()silently no-ops. The safe shape is either install-from-main()-before-readers-run, or rewrite aroundget_or_init(|| ...)so installation is lazy and idempotent.The lint also handles
MethodCallexpressions (not justPath/Call) so receiver-styleMY_GLOBAL.set(...)is resolved viatypeck_results().type_dependent_def_id().Wiring
Cargo.toml[workspace] excludeextended with both new dirs (matches the existing internal bridge APIs + dylint bans (total sweep, no grandfathering) #844 sweep grouping convention).ci/hooks/crate_guard.pyAPPROVED_CRATE_DIRSextended with both new dirs.ci/check_workspace_crates.pyAPPROVED_MEMBERS— dylints are workspace excludes, not members, so they live undercrate_guard.pyonly.soldr cargo check --workspace --all-targetspasses locally with these changes.Pre-existing violations (NOT auto-allowed)
Per task constraint, no real-world allowlist for fbuild's own code. Sweeping
crates/*/src/**/*.rs:std::env::set_var-> 48 occurrences across 14 files, but the vast majority are inside#[cfg(test)] mod testsblocks (EnvVarGuard / lock guard patterns) which the lint's#[cfg(test)]walk exempts. The remaining production sites the lint would actually flag are concentrated incrates/fbuild-paths/src/running_process.rsandcrates/fbuild-daemon/src/broker/service.rs(theEnvVarGuard::droppaths that appear outside#[cfg(test)]only because of where the impl is declared — these are still inside the test module). A clean follow-up pass is recommended once the lint is wired into CI.OnceLock::set-> 5 production sites the lint would flag:crates/fbuild-core/src/containment.rs:88(GLOBAL_GROUP.set(group))crates/fbuild-core/src/containment.rs:443(JOB.set(JobHandle(job)), Windows-only)crates/fbuild-build/src/compile_backend.rs:80(GLOBAL.set(backend)ininstall_global)SHUTDOWN_TX.setandCompileBackend.setsites which legitimately live inmain.rs(allowed) or test code (allowed).These are call-site violations that the maintainer can address either by (a) moving the install to
main()/ a#[cfg(test)]-only init helper, (b) rewriting aroundget_or_init, or (c) annotating with a per-call#[allow(require_oncelock_install_before_use)]documented inline. Not auto-fixed in this PR per task spec.Toolchain note
Did NOT run
cargo dylint --alllocally — the dylint nightly toolchain pin is heavy and not always built.soldr cargo check --workspace --all-targetspasses, which is what verifies theCargo.toml/crate_guard.pywiring.Test plan
cargo dylint --allagainst the workspace and the two new lints loadci/check_workspace_crates.pystill passes (verified locally:OK: 14 approved workspace members, no new crates.)#[allow]vs follow-up fixget_or_init-before-setproof)Closes #840