From 8d4eac3fa435a5fc7ac9732f7c60174d167301cd Mon Sep 17 00:00:00 2001 From: zackees Date: Tue, 30 Jun 2026 11:49:57 -0700 Subject: [PATCH 1/2] feat(dylints): add ban_env_var_set_after_import + require_oncelock_install_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 --- Cargo.toml | 5 + ci/hooks/crate_guard.py | 6 + .../ban_env_var_set_after_import/Cargo.toml | 18 ++ .../ban_env_var_set_after_import/README.md | 50 ++++ .../rust-toolchain.toml | 3 + .../src/README.md | 14 + .../src/allowlist.txt | 6 + .../ban_env_var_set_after_import/src/lib.rs | 252 +++++++++++++++++ .../Cargo.toml | 18 ++ .../README.md | 56 ++++ .../rust-toolchain.toml | 3 + .../src/README.md | 14 + .../src/allowlist.txt | 6 + .../src/lib.rs | 263 ++++++++++++++++++ 14 files changed, 714 insertions(+) create mode 100644 dylints/ban_env_var_set_after_import/Cargo.toml create mode 100644 dylints/ban_env_var_set_after_import/README.md create mode 100644 dylints/ban_env_var_set_after_import/rust-toolchain.toml create mode 100644 dylints/ban_env_var_set_after_import/src/README.md create mode 100644 dylints/ban_env_var_set_after_import/src/allowlist.txt create mode 100644 dylints/ban_env_var_set_after_import/src/lib.rs create mode 100644 dylints/require_oncelock_install_before_use/Cargo.toml create mode 100644 dylints/require_oncelock_install_before_use/README.md create mode 100644 dylints/require_oncelock_install_before_use/rust-toolchain.toml create mode 100644 dylints/require_oncelock_install_before_use/src/README.md create mode 100644 dylints/require_oncelock_install_before_use/src/allowlist.txt create mode 100644 dylints/require_oncelock_install_before_use/src/lib.rs diff --git a/Cargo.toml b/Cargo.toml index 5ef5679b..14221ca8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,6 +44,11 @@ exclude = [ "dylints/ban_runtime_new_outside_main", "dylints/ban_poison_panic", "dylints/ban_print_in_production", + # FastLED/fbuild#840 initializer-order sweep — convention-based + # checks for env-var mutation and OnceLock installation outside + # binary entry points. MIR-based dataflow check is a follow-up. + "dylints/ban_env_var_set_after_import", + "dylints/require_oncelock_install_before_use", ] [workspace.metadata.dylint] diff --git a/ci/hooks/crate_guard.py b/ci/hooks/crate_guard.py index 48797cb7..be2d4881 100644 --- a/ci/hooks/crate_guard.py +++ b/ci/hooks/crate_guard.py @@ -84,6 +84,12 @@ "dylints/ban_runtime_new_outside_main", "dylints/ban_poison_panic", "dylints/ban_print_in_production", + # FastLED/fbuild#840 initializer-order sweep — convention-based + # checks for env-var mutation and OnceLock installation outside + # binary entry points. See dylints/ban_env_var_set_after_import + # and dylints/require_oncelock_install_before_use READMEs. + "dylints/ban_env_var_set_after_import", + "dylints/require_oncelock_install_before_use", } ) diff --git a/dylints/ban_env_var_set_after_import/Cargo.toml b/dylints/ban_env_var_set_after_import/Cargo.toml new file mode 100644 index 00000000..bfd8ec64 --- /dev/null +++ b/dylints/ban_env_var_set_after_import/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "ban_env_var_set_after_import" +version = "0.1.0" +description = "Ban std::env::set_var outside binary entry points (FastLED/fbuild#840)" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +dylint_linting = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[dev-dependencies] +dylint_testing = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/dylints/ban_env_var_set_after_import/README.md b/dylints/ban_env_var_set_after_import/README.md new file mode 100644 index 00000000..3decee51 --- /dev/null +++ b/dylints/ban_env_var_set_after_import/README.md @@ -0,0 +1,50 @@ +# `ban_env_var_set_after_import` + +Custom [dylint](https://github.com/trailofbits/dylint) for the +initializer-order sweep (FastLED/fbuild#840). + +## What + +Bans `std::env::set_var` in production code outside the following +allow-listed scopes: + +- `**/main.rs` (binary entry points) +- `**/src/bin/**` (alternate binary entry points) +- `**/tests/**` (integration tests) +- any module annotated `#[cfg(test)]` + +## Why + +Process-global env-var mutation has order-of-init hazards. The Python +fbuild hit exactly this bug: `paths.py` cached `FBUILD_DEV_MODE` at +module import time, but `cli.py` set the variable *after* that import +ran, so dev-mode paths silently resolved to prod (see the project +memory "Daemon paths.py import-time bug"). The Rust-side analog is a +module that reads `std::env::var` into a `static` / `OnceLock` before +another module ran `std::env::set_var`. + +The convention this lint enforces: only `main()` (and tests, which set +up their own isolated process state) may mutate env vars. Every other +site should accept the value as an argument, read it lazily through a +function (not a static), or — for path resolution — go through +`fbuild-paths`'s lazy helpers. + +## Limitation (convention-based, not dataflow) + +This is a *call-site* check. The lint catches every `std::env::set_var` +outside the allow-list; it does NOT prove the variable was actually +read by another module before this call ran. A proper "set after read" +check requires MIR-level dataflow across the whole crate graph +("**MIR for completeness later**" — tracked as a follow-up on #840). +The convention is the fastest-landing prevention available today. + +## Allowlist + +Empty by design. Scope exemptions live in `lib.rs` by file path, not +in `src/allowlist.txt`. + +## Toolchain + +Pinned to `nightly-2026-03-26` to match every other dylint in this +repo. See the top-level `dylints/README.md` for the full setup +instructions and the rationale for `build_dylint_driver.py`. diff --git a/dylints/ban_env_var_set_after_import/rust-toolchain.toml b/dylints/ban_env_var_set_after_import/rust-toolchain.toml new file mode 100644 index 00000000..3b6ccbe8 --- /dev/null +++ b/dylints/ban_env_var_set_after_import/rust-toolchain.toml @@ -0,0 +1,3 @@ +[toolchain] +channel = "nightly-2026-03-26" +components = ["llvm-tools-preview", "rust-src", "rustc-dev"] diff --git a/dylints/ban_env_var_set_after_import/src/README.md b/dylints/ban_env_var_set_after_import/src/README.md new file mode 100644 index 00000000..293044aa --- /dev/null +++ b/dylints/ban_env_var_set_after_import/src/README.md @@ -0,0 +1,14 @@ +# `ban_env_var_set_after_import` — sources + +See the top-level [`../README.md`](../README.md) for the lint contract, +allowlist policy, and the convention-vs-MIR limitation note. This +directory contains: + +- **`lib.rs`** — the late-pass `LintContext` visitor. +- **`allowlist.txt`** — empty by design for the #840 sweep. Scope + exemptions live in `lib.rs` by file path. + +Both files are loaded at lint-compile time via `include_str!` in +`lib.rs`. To regenerate after editing the allowlist bump the version +in this lint's `Cargo.toml` (the dylint .so cache is keyed off the +manifest, not the allowlist). diff --git a/dylints/ban_env_var_set_after_import/src/allowlist.txt b/dylints/ban_env_var_set_after_import/src/allowlist.txt new file mode 100644 index 00000000..295fb0cd --- /dev/null +++ b/dylints/ban_env_var_set_after_import/src/allowlist.txt @@ -0,0 +1,6 @@ +# FastLED/fbuild#840 ships this lint with ZERO allowlist entries. +# Scope exemptions (main.rs, src/bin/**, tests/**, #[cfg(test)]) live +# in `lib.rs` by file path, not here. +# +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). diff --git a/dylints/ban_env_var_set_after_import/src/lib.rs b/dylints/ban_env_var_set_after_import/src/lib.rs new file mode 100644 index 00000000..58ee6ad2 --- /dev/null +++ b/dylints/ban_env_var_set_after_import/src/lib.rs @@ -0,0 +1,252 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_middle; +extern crate rustc_span; + +use rustc_errors::DiagDecorator; +use rustc_hir::{def::Res, Expr, ExprKind, HirId}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::{symbol::Symbol, FileName, RemapPathScopeComponents}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Bans `std::env::set_var` in production code EXCEPT in: + /// + /// - `**/main.rs` (binary entry points) + /// - `**/src/bin/**` (alternate binary entry points) + /// - `**/tests/**` (integration tests) + /// - any module annotated `#[cfg(test)]` + /// + /// ### Why is this bad? + /// + /// Process-global env-var mutation has order-of-init hazards: a + /// module that reads the var at import time (e.g. caches a path + /// derived from `FBUILD_DEV_MODE` in a `OnceLock`/`static`) + /// silently captures the wrong value if another module ran + /// `set_var` *after* that import. The Python fbuild had exactly + /// this bug — `paths.py` cached `FBUILD_DEV_MODE` at module import + /// time but `cli.py` set the variable afterward, so dev-mode paths + /// resolved to prod. See the project memory note "Daemon paths.py + /// import-time bug" and FastLED/fbuild#840. + /// + /// The convention-based fix: only `main()` (and tests, which set + /// up their own isolated process state) may call `set_var`. Every + /// other site should accept the value as an argument, read it + /// lazily through a function (not a `static`), or — for the + /// daemon — go through `fbuild-paths`'s lazy `__getattr__`-style + /// helpers. + /// + /// ### Limitation + /// + /// This is a *call-site* check, not a dataflow check. The lint + /// catches every `std::env::set_var` outside the allow-list; it + /// does NOT prove the variable was actually read by another module + /// before this call ran. A proper "set after read" check requires + /// MIR-level dataflow across the whole crate graph and is tracked + /// as a follow-up on #840 ("MIR for completeness later"). The + /// convention is the fastest-landing prevention available today. + /// + /// ### Example + /// + /// ```rust,ignore + /// // crates/fbuild-something/src/foo.rs — banned + /// fn configure() { + /// std::env::set_var("FBUILD_DEV_MODE", "1"); + /// } + /// ``` + /// + /// Use instead: set the env var inside `main()` before any module + /// that reads it is touched, or accept the value as an argument + /// and pass it explicitly. + pub BAN_ENV_VAR_SET_AFTER_IMPORT, + Deny, + "ban std::env::set_var outside binary entry points (FastLED/fbuild#840)" +} + +const BANNED_PATHS: &[&[&str]] = &[ + &["std", "env", "set_var"], +]; + +const CRATES_PREFIX: &str = "crates/"; +const SRC_SEGMENT: &str = "/src/"; + +impl<'tcx> LateLintPass<'tcx> for BanEnvVarSetAfterImport { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + let filename = source_filename(cx, expr.span); + let normalized = normalize_slashes(&filename); + + if !in_production_scope(&normalized) { + return; + } + if is_entry_point(&normalized) || is_test_file(&normalized) { + return; + } + // `#[cfg(test)]` module walk: a `set_var` inside a unit-test + // module is fine even when the surrounding file is production. + if owned_by_cfg_test_module(cx, expr.hir_id) { + return; + } + + match expr.kind { + ExprKind::Path(ref qpath) => { + if let Res::Def(_, def_id) = cx.qpath_res(qpath, expr.hir_id) { + check_def_id(cx, expr.span, def_id); + } + } + ExprKind::Call(ref func, _) => { + if let ExprKind::Path(ref qpath) = func.kind { + if let Res::Def(_, def_id) = cx.qpath_res(qpath, func.hir_id) { + check_def_id(cx, expr.span, def_id); + } + } + } + _ => {} + } + } +} + +fn check_def_id(cx: &LateContext<'_>, span: rustc_span::Span, def_id: rustc_hir::def_id::DefId) { + for banned in BANNED_PATHS { + if def_path_equals(cx, def_id, banned) { + emit_lint(cx, span, banned); + return; + } + } +} + +fn emit_lint(cx: &LateContext<'_>, span: rustc_span::Span, banned: &[&str]) { + let joined = banned.join("::"); + cx.opt_span_lint( + BAN_ENV_VAR_SET_AFTER_IMPORT, + Some(span), + DiagDecorator(move |diag| { + diag.primary_message(format!( + "`{joined}` is only allowed in `main.rs`, `src/bin/*.rs`, \ + `#[cfg(test)]` modules, or `tests/**`. Process-global \ + env-var mutation has order-of-init hazards: any module \ + that already read the variable into a `static`/`OnceLock` \ + silently captures the pre-mutation value. Set the env var \ + in `main()` before any module reads it, or pass the value \ + explicitly as a function argument. See FastLED/fbuild#840." + )); + }), + ); +} + +fn owned_by_cfg_test_module(cx: &LateContext<'_>, hir_id: HirId) -> bool { + let mut current = cx.tcx.hir_get_parent_item(hir_id); + loop { + let attrs = cx.tcx.hir_attrs(current.into()); + for attr in attrs { + if attr_is_cfg_test(attr) { + return true; + } + } + let parent = cx.tcx.hir_get_parent_item(current.into()); + if parent == current { + return false; + } + current = parent; + } +} + +fn attr_is_cfg_test(attr: &rustc_hir::Attribute) -> bool { + let Some(meta) = attr.meta() else { + return false; + }; + let path = meta.path(); + if path.segments.len() != 1 || path.segments[0].ident.as_str() != "cfg" { + return false; + } + let Some(list) = meta.meta_item_list() else { + return false; + }; + list.iter().any(|nested| { + nested + .ident() + .map(|id| id.as_str() == "test") + .unwrap_or(false) + }) +} + +fn source_filename(cx: &LateContext<'_>, span: rustc_span::Span) -> String { + match cx.sess().source_map().span_to_filename(span) { + FileName::Real(real_filename) => real_filename + .local_path() + .map(|path| path.to_string_lossy().into_owned()) + .unwrap_or_else(|| { + real_filename + .path(RemapPathScopeComponents::DIAGNOSTICS) + .to_string_lossy() + .into_owned() + }), + filename => filename + .display(RemapPathScopeComponents::DIAGNOSTICS) + .to_string(), + } +} + +fn in_production_scope(normalized: &str) -> bool { + let Some(crates_at) = normalized.find(CRATES_PREFIX) else { + return false; + }; + let after_crates = &normalized[crates_at + CRATES_PREFIX.len()..]; + after_crates.contains(SRC_SEGMENT) +} + +fn is_entry_point(normalized: &str) -> bool { + // `**/main.rs` or `**/src/bin/**`. + normalized.ends_with("/main.rs") + || normalized.contains("/src/bin/") +} + +fn is_test_file(normalized: &str) -> bool { + if normalized.contains("/tests/") { + return true; + } + let Some(name) = normalized.rsplit('/').next() else { + return false; + }; + name.contains("tests") && name.ends_with(".rs") +} + +fn normalize_slashes(path: &str) -> String { + path.replace('\\', "/") +} + +fn def_path_equals( + cx: &LateContext<'_>, + def_id: rustc_hir::def_id::DefId, + expected: &[&str], +) -> bool { + let def_path = cx.get_def_path(def_id); + if def_path.len() != expected.len() { + return false; + } + def_path + .iter() + .zip(expected.iter()) + .all(|(actual, expected_segment)| *actual == Symbol::intern(expected_segment)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn entry_points_are_exempt() { + assert!(is_entry_point("crates/fbuild-cli/src/main.rs")); + assert!(is_entry_point("crates/fbuild-daemon/src/bin/containment_harness.rs")); + assert!(!is_entry_point("crates/fbuild-packages/src/library/library_manager.rs")); + } + + #[test] + fn test_files_are_exempt() { + assert!(is_test_file("crates/fbuild-cli/tests/integration.rs")); + assert!(is_test_file("crates/fbuild-core/src/subprocess_tests.rs")); + assert!(!is_test_file("crates/fbuild-cli/src/main.rs")); + } +} diff --git a/dylints/require_oncelock_install_before_use/Cargo.toml b/dylints/require_oncelock_install_before_use/Cargo.toml new file mode 100644 index 00000000..717ad814 --- /dev/null +++ b/dylints/require_oncelock_install_before_use/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "require_oncelock_install_before_use" +version = "0.1.0" +description = "Require OnceLock::set/set_blocking install sites to live in binary entry points (FastLED/fbuild#840)" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +dylint_linting = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[dev-dependencies] +dylint_testing = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/dylints/require_oncelock_install_before_use/README.md b/dylints/require_oncelock_install_before_use/README.md new file mode 100644 index 00000000..5969ea96 --- /dev/null +++ b/dylints/require_oncelock_install_before_use/README.md @@ -0,0 +1,56 @@ +# `require_oncelock_install_before_use` + +Custom [dylint](https://github.com/trailofbits/dylint) for the +initializer-order sweep (FastLED/fbuild#840). + +## What + +Bans `std::sync::OnceLock::set` and `OnceLock::set_blocking` in +production code outside the following allow-listed scopes: + +- `**/main.rs` (binary entry points) +- `**/src/bin/**` (alternate binary entry points) +- `**/tests/**` (integration tests) +- any module annotated `#[cfg(test)]` + +## Why + +`OnceLock` is process-global single-assignment state. The only safe +way to install a value is from a tightly-controlled call site that +runs *before* any reader gets a chance — in practice, `main()` (or +test setup). Any other site implies the order-of-init bug class: a +reader has already filled the lock via `get_or_init` and the late +`set` silently no-ops, so the wrong value sticks for the process +lifetime. + +This is the convention shortcut per #840's "Approaches to investigate +→ convention-based static check" recommendation. The motivating Python +analog was `paths.py` capturing `FBUILD_DEV_MODE` at module import +time, before `cli.py` mutated the variable. The Rust shape: someone +calls `MY_GLOBAL.set(...)` from a non-`main` module, but another +caller already populated the lock via `get_or_init`, so the late +`.set(...)` returns `Err` and is silently dropped. + +Use `get_or_init` (lazy, idempotent), or install the value inside +`main()` before any module reads it. + +## Limitation (convention-based, not dataflow) + +This lint catches the *attempt* at the call site. It does NOT prove +`get_or_init` ran first or that another reader has already populated +the lock. A proper "was `.get_or_init` already called?" check requires +MIR-level dataflow across the whole crate graph and is out of scope +here ("**MIR for completeness later**" — tracked as a follow-up on +#840). The convention is the fastest-landing prevention available +today. + +## Allowlist + +Empty by design. Scope exemptions live in `lib.rs` by file path, not +in `src/allowlist.txt`. + +## Toolchain + +Pinned to `nightly-2026-03-26` to match every other dylint in this +repo. See the top-level `dylints/README.md` for the full setup +instructions and the rationale for `build_dylint_driver.py`. diff --git a/dylints/require_oncelock_install_before_use/rust-toolchain.toml b/dylints/require_oncelock_install_before_use/rust-toolchain.toml new file mode 100644 index 00000000..3b6ccbe8 --- /dev/null +++ b/dylints/require_oncelock_install_before_use/rust-toolchain.toml @@ -0,0 +1,3 @@ +[toolchain] +channel = "nightly-2026-03-26" +components = ["llvm-tools-preview", "rust-src", "rustc-dev"] diff --git a/dylints/require_oncelock_install_before_use/src/README.md b/dylints/require_oncelock_install_before_use/src/README.md new file mode 100644 index 00000000..8bfd5cbc --- /dev/null +++ b/dylints/require_oncelock_install_before_use/src/README.md @@ -0,0 +1,14 @@ +# `require_oncelock_install_before_use` — sources + +See the top-level [`../README.md`](../README.md) for the lint contract, +allowlist policy, and the convention-vs-MIR limitation note. This +directory contains: + +- **`lib.rs`** — the late-pass `LintContext` visitor. +- **`allowlist.txt`** — empty by design for the #840 sweep. Scope + exemptions live in `lib.rs` by file path. + +Both files are loaded at lint-compile time via `include_str!` in +`lib.rs`. To regenerate after editing the allowlist bump the version +in this lint's `Cargo.toml` (the dylint .so cache is keyed off the +manifest, not the allowlist). diff --git a/dylints/require_oncelock_install_before_use/src/allowlist.txt b/dylints/require_oncelock_install_before_use/src/allowlist.txt new file mode 100644 index 00000000..295fb0cd --- /dev/null +++ b/dylints/require_oncelock_install_before_use/src/allowlist.txt @@ -0,0 +1,6 @@ +# FastLED/fbuild#840 ships this lint with ZERO allowlist entries. +# Scope exemptions (main.rs, src/bin/**, tests/**, #[cfg(test)]) live +# in `lib.rs` by file path, not here. +# +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). diff --git a/dylints/require_oncelock_install_before_use/src/lib.rs b/dylints/require_oncelock_install_before_use/src/lib.rs new file mode 100644 index 00000000..ec1bd3d5 --- /dev/null +++ b/dylints/require_oncelock_install_before_use/src/lib.rs @@ -0,0 +1,263 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_middle; +extern crate rustc_span; + +use rustc_errors::DiagDecorator; +use rustc_hir::{def::Res, Expr, ExprKind, HirId}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::{symbol::Symbol, FileName, RemapPathScopeComponents}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Bans `std::sync::OnceLock::set` and `OnceLock::set_blocking` in + /// production code EXCEPT in: + /// + /// - `**/main.rs` (binary entry points) + /// - `**/src/bin/**` (alternate binary entry points) + /// - `**/tests/**` (integration tests) + /// - any module annotated `#[cfg(test)]` + /// + /// ### Why is this bad? + /// + /// `OnceLock` is process-global single-assignment state. The only + /// safe way to install a value is from a tightly-controlled call + /// site that runs *before* any reader gets a chance — in practice, + /// `main()` (or test setup). Any other site implies the + /// order-of-init bug class: a reader has already filled the lock + /// via `get_or_init` and the late `set` silently no-ops. + /// + /// FastLED/fbuild#840. The motivating analog from the Python + /// fbuild was `paths.py`'s import-time `OnceLock`-equivalent + /// capture of `FBUILD_DEV_MODE` running before `cli.py` mutated + /// the variable. The Rust shape of the same bug is: someone calls + /// `MY_GLOBAL.set(...)` from a non-`main` module, but another + /// caller already populated the lock via `get_or_init` (or even + /// just `.get()` early-readers triggered a load through some other + /// path), so the late `.set(...)` returns `Err` and the wrong + /// value sticks. + /// + /// Use `get_or_init` (lazy, idempotent), or install the value + /// inside `main()` before any module reads it. + /// + /// ### Limitation (convention-based, not dataflow) + /// + /// This lint catches the *attempt* at the call site. It does NOT + /// prove `get_or_init` ran first — that requires MIR-level + /// dataflow across the whole crate graph and is tracked as a + /// follow-up on #840 ("MIR for completeness later"). The + /// convention is the fastest-landing prevention available today. + /// + /// ### Example + /// + /// ```rust,ignore + /// // crates/fbuild-something/src/foo.rs — banned + /// static GLOBAL: OnceLock = OnceLock::new(); + /// pub fn configure(cfg: Config) { + /// let _ = GLOBAL.set(cfg); // banned outside main() + /// } + /// ``` + /// + /// Use instead: install the value in `main()` before any module + /// touches `GLOBAL.get()`, or rewrite the API around + /// `get_or_init(|| ...)` so installation is lazy + idempotent. + pub REQUIRE_ONCELOCK_INSTALL_BEFORE_USE, + Deny, + "require std::sync::OnceLock::set/set_blocking install sites to live in binary entry points (FastLED/fbuild#840)" +} + +const BANNED_PATHS: &[&[&str]] = &[ + &["std", "sync", "OnceLock", "set"], + &["std", "sync", "OnceLock", "set_blocking"], +]; + +const CRATES_PREFIX: &str = "crates/"; +const SRC_SEGMENT: &str = "/src/"; + +impl<'tcx> LateLintPass<'tcx> for RequireOncelockInstallBeforeUse { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + let filename = source_filename(cx, expr.span); + let normalized = normalize_slashes(&filename); + + if !in_production_scope(&normalized) { + return; + } + if is_entry_point(&normalized) || is_test_file(&normalized) { + return; + } + // `#[cfg(test)]` module walk: a `.set(..)` inside a unit-test + // module is fine even when the surrounding file is production. + if owned_by_cfg_test_module(cx, expr.hir_id) { + return; + } + + match expr.kind { + ExprKind::Path(ref qpath) => { + if let Res::Def(_, def_id) = cx.qpath_res(qpath, expr.hir_id) { + check_def_id(cx, expr.span, def_id); + } + } + ExprKind::Call(ref func, _) => { + if let ExprKind::Path(ref qpath) = func.kind { + if let Res::Def(_, def_id) = cx.qpath_res(qpath, func.hir_id) { + check_def_id(cx, expr.span, def_id); + } + } + } + ExprKind::MethodCall(_, _receiver, _, _) => { + if let Some(def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) { + check_def_id(cx, expr.span, def_id); + } + } + _ => {} + } + } +} + +fn check_def_id(cx: &LateContext<'_>, span: rustc_span::Span, def_id: rustc_hir::def_id::DefId) { + for banned in BANNED_PATHS { + if def_path_equals(cx, def_id, banned) { + emit_lint(cx, span, banned); + return; + } + } +} + +fn emit_lint(cx: &LateContext<'_>, span: rustc_span::Span, banned: &[&str]) { + let joined = banned.join("::"); + cx.opt_span_lint( + REQUIRE_ONCELOCK_INSTALL_BEFORE_USE, + Some(span), + DiagDecorator(move |diag| { + diag.primary_message(format!( + "`{joined}` is only allowed in `main.rs`, `src/bin/*.rs`, \ + `#[cfg(test)]` modules, or `tests/**`. `OnceLock` is \ + process-global single-assignment state — installing it \ + from any other site is an order-of-init hazard (a reader \ + may have already populated the lock via `get_or_init` \ + and the late `.set(...)` silently no-ops). Either move \ + the install to `main()` before any reader runs, or \ + rewrite around `get_or_init(|| ...)` so installation is \ + lazy and idempotent. See FastLED/fbuild#840." + )); + }), + ); +} + +fn owned_by_cfg_test_module(cx: &LateContext<'_>, hir_id: HirId) -> bool { + let mut current = cx.tcx.hir_get_parent_item(hir_id); + loop { + let attrs = cx.tcx.hir_attrs(current.into()); + for attr in attrs { + if attr_is_cfg_test(attr) { + return true; + } + } + let parent = cx.tcx.hir_get_parent_item(current.into()); + if parent == current { + return false; + } + current = parent; + } +} + +fn attr_is_cfg_test(attr: &rustc_hir::Attribute) -> bool { + let Some(meta) = attr.meta() else { + return false; + }; + let path = meta.path(); + if path.segments.len() != 1 || path.segments[0].ident.as_str() != "cfg" { + return false; + } + let Some(list) = meta.meta_item_list() else { + return false; + }; + list.iter().any(|nested| { + nested + .ident() + .map(|id| id.as_str() == "test") + .unwrap_or(false) + }) +} + +fn source_filename(cx: &LateContext<'_>, span: rustc_span::Span) -> String { + match cx.sess().source_map().span_to_filename(span) { + FileName::Real(real_filename) => real_filename + .local_path() + .map(|path| path.to_string_lossy().into_owned()) + .unwrap_or_else(|| { + real_filename + .path(RemapPathScopeComponents::DIAGNOSTICS) + .to_string_lossy() + .into_owned() + }), + filename => filename + .display(RemapPathScopeComponents::DIAGNOSTICS) + .to_string(), + } +} + +fn in_production_scope(normalized: &str) -> bool { + let Some(crates_at) = normalized.find(CRATES_PREFIX) else { + return false; + }; + let after_crates = &normalized[crates_at + CRATES_PREFIX.len()..]; + after_crates.contains(SRC_SEGMENT) +} + +fn is_entry_point(normalized: &str) -> bool { + // `**/main.rs` or `**/src/bin/**`. + normalized.ends_with("/main.rs") + || normalized.contains("/src/bin/") +} + +fn is_test_file(normalized: &str) -> bool { + if normalized.contains("/tests/") { + return true; + } + let Some(name) = normalized.rsplit('/').next() else { + return false; + }; + name.contains("tests") && name.ends_with(".rs") +} + +fn normalize_slashes(path: &str) -> String { + path.replace('\\', "/") +} + +fn def_path_equals( + cx: &LateContext<'_>, + def_id: rustc_hir::def_id::DefId, + expected: &[&str], +) -> bool { + let def_path = cx.get_def_path(def_id); + if def_path.len() != expected.len() { + return false; + } + def_path + .iter() + .zip(expected.iter()) + .all(|(actual, expected_segment)| *actual == Symbol::intern(expected_segment)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn entry_points_are_exempt() { + assert!(is_entry_point("crates/fbuild-cli/src/main.rs")); + assert!(is_entry_point("crates/fbuild-daemon/src/bin/containment_harness.rs")); + assert!(!is_entry_point("crates/fbuild-packages/src/library/library_manager.rs")); + } + + #[test] + fn test_files_are_exempt() { + assert!(is_test_file("crates/fbuild-cli/tests/integration.rs")); + assert!(is_test_file("crates/fbuild-core/src/subprocess_tests.rs")); + assert!(!is_test_file("crates/fbuild-cli/src/main.rs")); + } +} From 1e2f36161d80d9f4e2f5eb00794ba6673248b422 Mon Sep 17 00:00:00 2001 From: zackees Date: Tue, 30 Jun 2026 11:53:04 -0700 Subject: [PATCH 2/2] fix(#840): allowlist 3 legitimate global-install sites for OnceLock lint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../src/allowlist.txt | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/dylints/require_oncelock_install_before_use/src/allowlist.txt b/dylints/require_oncelock_install_before_use/src/allowlist.txt index 295fb0cd..a4c5422f 100644 --- a/dylints/require_oncelock_install_before_use/src/allowlist.txt +++ b/dylints/require_oncelock_install_before_use/src/allowlist.txt @@ -1,6 +1,16 @@ -# FastLED/fbuild#840 ships this lint with ZERO allowlist entries. +# FastLED/fbuild#840 — legitimate one-shot global-install call sites. # Scope exemptions (main.rs, src/bin/**, tests/**, #[cfg(test)]) live # in `lib.rs` by file path, not here. # # Each non-empty, non-comment line is matched against the tail of each # source file path (slashes normalized to /). +# +# These files contain dedicated `install_global()` (or close-analog) +# functions that the binary entry points call exactly once at startup, +# *before* any subprocess spawns or any consumer reaches the OnceLock. +# The order-of-init bug class the lint targets is structurally +# impossible at these sites because the call IS the install path — +# any future addition of a non-install call to `OnceLock::set` outside +# these files will trip the lint as intended. +crates/fbuild-core/src/containment.rs +crates/fbuild-build/src/compile_backend.rs