diff --git a/Cargo.toml b/Cargo.toml index c6e7930a..05a54514 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,6 +27,11 @@ exclude = [ "dylints/ban_direct_serialport", "dylints/ban_file_based_locks", "dylints/ban_deploy_tool_direct_invocation", + "dylints/ban_process_exit_outside_main", + "dylints/ban_unwrap_in_daemon_handlers", + "dylints/cli_no_build_deploy_direct_use", + "dylints/require_multi_thread_flavor_when_spawning", + "dylints/ban_std_sync_mutex_in_async", ] [workspace.metadata.dylint] diff --git a/ci/hooks/crate_guard.py b/ci/hooks/crate_guard.py index 059b3f7f..e189548e 100644 --- a/ci/hooks/crate_guard.py +++ b/ci/hooks/crate_guard.py @@ -65,6 +65,11 @@ "dylints/ban_direct_serialport", "dylints/ban_file_based_locks", "dylints/ban_deploy_tool_direct_invocation", + "dylints/ban_process_exit_outside_main", + "dylints/ban_unwrap_in_daemon_handlers", + "dylints/cli_no_build_deploy_direct_use", + "dylints/require_multi_thread_flavor_when_spawning", + "dylints/ban_std_sync_mutex_in_async", } ) diff --git a/dylints/README.md b/dylints/README.md index 1ba0bfa1..f375f972 100644 --- a/dylints/README.md +++ b/dylints/README.md @@ -36,6 +36,33 @@ itself stays on stable 1.94.1). `Command::new("esptool" | "avrdude" | "picotool" | "dfu-util" | "pyocd")` invocations outside `crates/fbuild-deploy/`. All deploy- tool spawns must flow through `fbuild deploy`. See #826 / #694. +- **`ban_process_exit_outside_main/`** — bans `std::process::exit` + outside `crates/*/src/main.rs` and `crates/*/src/bin/*.rs`. Skipping + destructors leaks temp files, kills containment guards, and + truncates in-flight HTTP/WS responses. Legacy CLI subcommand + dispatchers exempted via `src/allowlist.txt`. See #826. +- **`ban_unwrap_in_daemon_handlers/`** — bans `.unwrap()` inside + `crates/fbuild-daemon/src/handlers/**/*.rs` production code (tests + exempt by filename and by `#[cfg(test)]` module walking). Locks in + PR #833's hardening; new violations would crash the daemon. See + #826. +- **`cli_no_build_deploy_direct_use/`** — bans `fbuild_build::*` / + `fbuild_deploy::*` references in `crates/fbuild-cli/src/` outside + the diagnostic-subcommand allowlist. Enforces the "thin HTTP + client" rule from `crates/CLAUDE.md` §"Dependency Graph". See + #826. +- **`require_multi_thread_flavor_when_spawning/`** — flags + `#[tokio::test]` (without `flavor = "multi_thread"`) on async fns + that call `tokio::spawn(...)` in their body. The default + `current_thread` flavor deadlocks any test that awaits cross-task + state. Legacy violators exempted via `src/allowlist.txt`. See + #826. +- **`ban_std_sync_mutex_in_async/`** — bans `std::sync::Mutex` / + `std::sync::RwLock` *type uses* inside `crates/fbuild-daemon/src/` + and `crates/fbuild-serial/src/` (tests exempt). Holding the guard + across `.await` starves the Tokio worker and a panic poisons the + lock. Existing synchronous uses exempted via `src/allowlist.txt`. + See #826. ## Running locally diff --git a/dylints/ban_process_exit_outside_main/Cargo.toml b/dylints/ban_process_exit_outside_main/Cargo.toml new file mode 100644 index 00000000..63b6b79a --- /dev/null +++ b/dylints/ban_process_exit_outside_main/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "ban_process_exit_outside_main" +version = "0.1.0" +description = "Ban std::process::exit outside main.rs / src/bin/*.rs" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +# Pinned to the same dylint_linting rev the other fbuild dylints use — +# same rustup channel in rust-toolchain.toml, same upstream commit. +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_process_exit_outside_main/README.md b/dylints/ban_process_exit_outside_main/README.md new file mode 100644 index 00000000..7d2acfc7 --- /dev/null +++ b/dylints/ban_process_exit_outside_main/README.md @@ -0,0 +1,59 @@ +# `ban_process_exit_outside_main` + +Custom [dylint](https://github.com/trailofbits/dylint) that forbids +calling `std::process::exit(_)` (and its `process::exit` re-imports) +outside the canonical entry-point files: `crates/*/src/main.rs` and +`crates/*/src/bin/*.rs`. + +## Why + +`std::process::exit` skips destructors. In fbuild that translates to: + +- Temp files in the cache root that were created via `NamedTempFile` / + `TempDir` are not unlinked → cache grows without bound. +- `running-process` containment guards do not run their drop handlers → + child processes (esptool / qemu / simavr) may keep running after the + parent exits. +- Tokio runtime is dropped abruptly, so in-flight HTTP/WS responses are + truncated and clients see EOF mid-response. + +The only places where `process::exit` is safe are the program entry +points (`main.rs` / `src/bin/*.rs`) where the runtime is being torn +down anyway. Everywhere else, return a `Result` (or `FbuildError`) and +let the entry point translate it into an exit code in one place. + +## Scope + +Only files whose path contains BOTH `crates/` and a subsequent `/src/` +segment are linted. Out of scope by design: + +- `crates/*/tests/`, `crates/*/examples/`, `crates/*/benches/` +- `ci/`, `dylints/`, build scripts +- Anything outside `crates/` + +Within scope, **two exemptions are unconditional** (no allowlist entry +needed): + +- `crates/*/src/main.rs` — every crate's library/binary entry point. +- `crates/*/src/bin/*.rs` — any extra binary in `src/bin/`. + +## Allowlist + +Files in scope that legitimately need `process::exit` (for example, +subcommand dispatchers that have already printed an error and want to +short-circuit `main`'s return path) are listed in `src/allowlist.txt`. +Each entry needs an inline comment explaining why. + +## Toolchain + +Pinned to the same `nightly-2026-03-26` channel and the same +`trailofbits/dylint` git rev (`4bd91ce…`) the other fbuild dylints use. + +## Running locally + +See `dylints/README.md` for the full local-run recipe. CI runs all +dylints on every push/PR via `.github/workflows/dylint.yml`. + +## See also + +- Issue #826 — this lint's tracking issue (gotcha sweep) diff --git a/dylints/ban_process_exit_outside_main/rust-toolchain.toml b/dylints/ban_process_exit_outside_main/rust-toolchain.toml new file mode 100644 index 00000000..3b6ccbe8 --- /dev/null +++ b/dylints/ban_process_exit_outside_main/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_process_exit_outside_main/src/allowlist.txt b/dylints/ban_process_exit_outside_main/src/allowlist.txt new file mode 100644 index 00000000..ebe477af --- /dev/null +++ b/dylints/ban_process_exit_outside_main/src/allowlist.txt @@ -0,0 +1,34 @@ +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). Use forward-slash paths +# only — the lint normalizes \ to / before matching. +# +# `crates/*/src/main.rs` and `crates/*/src/bin/*.rs` are UNCONDITIONALLY +# exempt in lib.rs (they are the canonical program entry points). They +# do NOT need entries here. +# +# Rollout strategy: +# - The lint is ON and denies new `std::process::exit` calls in +# production code outside `main.rs` / `src/bin/*.rs`. +# - This file lists the *legacy* call sites that existed when the +# lint was first enabled (FastLED/fbuild#826). They are exempted so +# the lint can land without a repo-wide migration to `Result`-based +# control flow. +# - New files MUST NOT be added here. The target state is zero +# entries. + +# CLI subcommand dispatchers that translate daemon HTTP responses +# into shell exit codes. The canonical pattern is "print the message +# the daemon already wrote, then exit with the daemon's exit_code". +# These are effectively a thin layer between the daemon and the +# shell — migrating them to bubble Result back into main() requires +# threading exit codes through fbuild_core::Result, which is a +# separate refactor (#826 follow-up). +crates/fbuild-cli/src/cli/build.rs +crates/fbuild-cli/src/cli/deploy.rs +crates/fbuild-cli/src/cli/dispatch.rs +crates/fbuild-cli/src/cli/lnk.rs +crates/fbuild-cli/src/cli/pio.rs +crates/fbuild-cli/src/cli/purge.rs +crates/fbuild-cli/src/cli/reset.rs +crates/fbuild-cli/src/cli/serial_probe.rs +crates/fbuild-cli/src/cli/show.rs diff --git a/dylints/ban_process_exit_outside_main/src/lib.rs b/dylints/ban_process_exit_outside_main/src/lib.rs new file mode 100644 index 00000000..874b5901 --- /dev/null +++ b/dylints/ban_process_exit_outside_main/src/lib.rs @@ -0,0 +1,200 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_span; + +use rustc_errors::DiagDecorator; +use rustc_hir::{def::Res, Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::{symbol::Symbol, FileName, RemapPathScopeComponents}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Bans `std::process::exit(_)` calls in fbuild production code + /// (`crates/*/src/`) outside the canonical program entry-point + /// files (`crates/*/src/main.rs` and `crates/*/src/bin/*.rs`). + /// + /// ### Why is this bad? + /// + /// `std::process::exit` skips destructors. In fbuild that + /// translates to: temp files / `TempDir`s under the cache root are + /// not cleaned up; `running-process` containment guards do not run + /// their drop handlers (child processes keep running after the + /// parent dies); the tokio runtime is dropped abruptly so + /// in-flight HTTP/WS responses are truncated. + /// + /// The only safe places to call `process::exit` are the program + /// entry points where the runtime is being torn down anyway. + /// Everywhere else, return a `Result` (or `FbuildError`) and let + /// the entry point translate it into an exit code in one place. + /// + /// ### Known problems + /// + /// `std::process::abort` and `libc::_exit` are out of scope by + /// design — they're explicit "I know what I'm doing" calls and + /// already used inside post-fork containment paths. + /// + /// ### Example + /// + /// ```rust,ignore + /// // Banned outside `main.rs` / `src/bin/*.rs`: + /// if config.broken { + /// eprintln!("error: bad config"); + /// std::process::exit(1); + /// } + /// ``` + /// + /// Use instead: + /// + /// ```rust,ignore + /// if config.broken { + /// return Err(FbuildError::msg("bad config")); + /// } + /// ``` + pub BAN_PROCESS_EXIT_OUTSIDE_MAIN, + Deny, + "ban std::process::exit outside main.rs / src/bin/*.rs" +} + +/// Banned path. Matched exactly against the resolved DefId's def-path. +const BANNED_PATH: &[&str] = &["std", "process", "exit"]; + +const ALLOWLIST: &str = include_str!("allowlist.txt"); + +/// Production-code scope. Only files whose path contains BOTH +/// `crates/` and a subsequent `/src/` segment are linted. +const CRATES_PREFIX: &str = "crates/"; +const SRC_SEGMENT: &str = "/src/"; + +impl<'tcx> LateLintPass<'tcx> for BanProcessExitOutsideMain { + 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) + || is_entry_point(&normalized) + || is_allowlisted(&normalized) + { + return; + } + + if let ExprKind::Path(ref qpath) = expr.kind { + if let Res::Def(_, def_id) = cx.qpath_res(qpath, expr.hir_id) { + if def_path_equals(cx, def_id, BANNED_PATH) { + emit_lint(cx, expr.span); + } + } + } + } +} + +fn emit_lint(cx: &LateContext<'_>, span: rustc_span::Span) { + cx.opt_span_lint( + BAN_PROCESS_EXIT_OUTSIDE_MAIN, + Some(span), + DiagDecorator(|diag| { + diag.primary_message( + "`std::process::exit` outside `main.rs` / `src/bin/*.rs` skips destructors \ + — temp files leak, containment guards don't run, the tokio runtime is \ + dropped abruptly, and in-flight HTTP/WS responses get truncated. Return a \ + `Result` / `FbuildError` and let `main` translate it into an exit code in \ + one place. If `process::exit` is truly justified here, allowlist the file \ + in `dylints/ban_process_exit_outside_main/src/allowlist.txt` with a \ + one-line reason.", + ); + }), + ); +} + +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) +} + +/// Unconditional exemptions for canonical program entry points: +/// * `crates/*/src/main.rs` +/// * `crates/*/src/bin/*.rs` +fn is_entry_point(normalized: &str) -> bool { + normalized.ends_with("/src/main.rs") + || normalized.contains("/src/bin/") && normalized.ends_with(".rs") +} + +fn is_allowlisted(normalized: &str) -> bool { + ALLOWLIST + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .any(|allowed| normalized.ends_with(allowed)) +} + +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 in_production_scope_matches_src_files() { + assert!(in_production_scope("crates/fbuild-cli/src/cli/build.rs")); + assert!(in_production_scope("crates/fbuild-daemon/src/main.rs")); + } + + #[test] + fn in_production_scope_rejects_non_src() { + assert!(!in_production_scope("crates/fbuild-cli/tests/foo.rs")); + assert!(!in_production_scope("build.rs")); + } + + #[test] + fn entry_points_are_recognised() { + assert!(is_entry_point("crates/fbuild-cli/src/main.rs")); + assert!(is_entry_point("crates/fbuild-daemon/src/main.rs")); + assert!(is_entry_point( + "crates/fbuild-config/src/bin/enrich_boards.rs" + )); + assert!(is_entry_point( + "crates/fbuild-daemon/src/bin/containment_harness.rs" + )); + assert!(!is_entry_point("crates/fbuild-cli/src/cli/build.rs")); + // `lib.rs` is NOT an entry point — the function `main()` isn't there. + assert!(!is_entry_point("crates/fbuild-cli/src/lib.rs")); + } +} diff --git a/dylints/ban_std_sync_mutex_in_async/Cargo.toml b/dylints/ban_std_sync_mutex_in_async/Cargo.toml new file mode 100644 index 00000000..90f9a41f --- /dev/null +++ b/dylints/ban_std_sync_mutex_in_async/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "ban_std_sync_mutex_in_async" +version = "0.1.0" +description = "Ban std::sync::Mutex / std::sync::RwLock in async-reachable fbuild code (daemon, serial)" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +# Pinned to the same dylint_linting rev the other fbuild dylints use — +# same rustup channel in rust-toolchain.toml, same upstream commit. +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_std_sync_mutex_in_async/README.md b/dylints/ban_std_sync_mutex_in_async/README.md new file mode 100644 index 00000000..e311b04c --- /dev/null +++ b/dylints/ban_std_sync_mutex_in_async/README.md @@ -0,0 +1,69 @@ +# `ban_std_sync_mutex_in_async` + +Custom [dylint](https://github.com/trailofbits/dylint) that forbids +`std::sync::Mutex` and `std::sync::RwLock` *type uses* inside +files under `crates/fbuild-daemon/src/` and +`crates/fbuild-serial/src/`. + +## Why + +Holding a `std::sync::Mutex` guard across an `.await` point is two +bugs at once: + +1. **Scheduler starvation.** While the guard is held, the executor + cannot park the task — the mutex doesn't know about Tokio's + futures. Other tasks on the same worker thread stall. +2. **Poison panic.** A panic inside an async section that's holding + the guard poisons the mutex; every subsequent `.lock()` call + panics. In a long-running daemon, one panic per type makes the + whole subsystem unusable. + +The fix is to use `tokio::sync::Mutex` / `tokio::sync::RwLock` (or +`parking_lot::Mutex` for hot non-async paths that never cross an +await), or to restructure so the std mutex is released *before* any +await. + +## Scope + +Only files whose path matches one of these prefixes are linted: + +``` +crates/fbuild-daemon/src/**/*.rs +crates/fbuild-serial/src/**/*.rs +``` + +Test files inside scope (`crates/fbuild-daemon/src/**/*tests*.rs`, +`crates/fbuild-serial/src/**/*tests*.rs`) are exempt — synchronous +test plumbing legitimately uses `std::sync::Mutex` to share state +between background threads. + +Everything else (other crates, integration tests, examples) is out of +scope. + +## Allowlist + +Files in scope that legitimately use a `std::sync::Mutex` / +`std::sync::RwLock` (typically because the lock is genuinely held by +a synchronous code path — e.g. `device_manager.rs`, +`status_manager.rs`, the per-session output buffer in +`fbuild-serial/src/manager.rs`) are listed in `src/allowlist.txt`. + +The goal is to lock in current state and prevent NEW violations from +spreading; the existing entries are not aspirational targets for +migration. Each entry has an inline comment explaining the +synchronous discipline that justifies it. + +## Toolchain + +Pinned to the same `nightly-2026-03-26` channel and the same +`trailofbits/dylint` git rev (`4bd91ce…`) the other fbuild dylints +use. + +## Running locally + +See `dylints/README.md` for the full local-run recipe. CI runs all +dylints on every push/PR via `.github/workflows/dylint.yml`. + +## See also + +- Issue #826 — this lint's tracking issue diff --git a/dylints/ban_std_sync_mutex_in_async/rust-toolchain.toml b/dylints/ban_std_sync_mutex_in_async/rust-toolchain.toml new file mode 100644 index 00000000..3b6ccbe8 --- /dev/null +++ b/dylints/ban_std_sync_mutex_in_async/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_std_sync_mutex_in_async/src/allowlist.txt b/dylints/ban_std_sync_mutex_in_async/src/allowlist.txt new file mode 100644 index 00000000..11668414 --- /dev/null +++ b/dylints/ban_std_sync_mutex_in_async/src/allowlist.txt @@ -0,0 +1,49 @@ +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). Use forward-slash paths +# only — the lint normalizes \ to / before matching. +# +# Test files (path tail matches `*tests*.rs`) and code inside a +# `#[cfg(test)] mod` block are UNCONDITIONALLY exempt in lib.rs. They +# do NOT need entries here. +# +# Rollout strategy: +# - The lint is ON and denies new `std::sync::{Mutex,RwLock}` uses +# in daemon / serial code. +# - This file lists the *existing* production call sites at the +# time the lint was first enabled (FastLED/fbuild#826). Each is +# justified inline. +# - New files MUST NOT be added here without a one-line reason and +# a PR review. + +# `DaemonState`, `current_operation`, `dependency_install`, and +# `last_activity` are read under `std::sync::RwLock` / `Mutex` from +# request handlers but the critical sections are tiny (single struct +# field updates) and never cross an await — the daemon's policy is +# "snapshot under the lock, drop the guard, then await". Migrating +# to `tokio::sync` would force every probe / observer to be async +# without changing the actual concurrency surface. +crates/fbuild-daemon/src/context.rs + +# Daemon device manager: per-port state machine kept under a +# `std::sync::Mutex`. The lock is held only across in-memory DashMap +# updates and never across `.await`. +crates/fbuild-daemon/src/device_manager.rs + +# Daemon status manager: append-only ring of recent build/deploy +# outcomes under a `std::sync::Mutex`. Same discipline as the +# device manager — short critical sections, no await held. +crates/fbuild-daemon/src/status_manager.rs + +# Handlers/operations/common.rs reads the Arc> +# and Arc>> defined in context.rs. The lock +# types flow through here only as parameter / field types — no new +# `std::sync` lock is constructed. +crates/fbuild-daemon/src/handlers/operations/common.rs + +# fbuild-serial's per-session output buffer is a small VecDeque +# guarded by `std::sync::Mutex` to serialize pushes from the +# background read task and pops from `read_lines()` consumers. The +# mutex is never held across `.await` — pushes hand off via a +# `tokio::sync::broadcast` channel; this lock only protects the +# in-memory snapshot used for late-joiner replay. +crates/fbuild-serial/src/manager.rs diff --git a/dylints/ban_std_sync_mutex_in_async/src/lib.rs b/dylints/ban_std_sync_mutex_in_async/src/lib.rs new file mode 100644 index 00000000..ffae07ab --- /dev/null +++ b/dylints/ban_std_sync_mutex_in_async/src/lib.rs @@ -0,0 +1,282 @@ +#![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, AmbigArg, Expr, ExprKind, HirId, Ty, TyKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::{symbol::Symbol, FileName, RemapPathScopeComponents}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Bans `std::sync::Mutex` and `std::sync::RwLock` type + /// uses inside files under `crates/fbuild-daemon/src/**` and + /// `crates/fbuild-serial/src/**`. Test files (`*tests*.rs`) and + /// files explicitly listed in `src/allowlist.txt` are exempt. + /// + /// ### Why is this bad? + /// + /// Holding an `std::sync::Mutex` guard across an `.await` point + /// is two bugs at once: + /// + /// 1. **Scheduler starvation.** The mutex doesn't know about + /// Tokio's futures; while the guard is held the executor + /// can't park the task. Other tasks on the same worker + /// thread stall. + /// 2. **Poison panic.** A panic inside an async section that's + /// holding the guard poisons the mutex; every subsequent + /// `.lock()` call panics. One panic per type makes the whole + /// subsystem unusable in a long-running daemon. + /// + /// The fix is `tokio::sync::Mutex` / `tokio::sync::RwLock` (or + /// `parking_lot::Mutex` for hot non-async paths that never cross + /// an await), or restructuring so the std lock is released + /// *before* any await. + /// + /// ### Known problems + /// + /// - The lint matches the *type position*, not the actual + /// `.lock()` / `.read()` / `.write()` call. A file that + /// declares a struct field of type `std::sync::Mutex` will + /// trip the lint even if the struct is never accessed from an + /// async context. That's the right *direction* though — if a + /// type lives in the daemon's tree it can flow into async code + /// later, and the lint is loud at type-introduction time so + /// the conversation happens early. + /// - Path aliases / re-exports that resolve back to + /// `std::sync::Mutex` are caught by `qpath_res` / + /// `get_def_path`. + /// + /// ### Example + /// + /// ```rust,ignore + /// // crates/fbuild-daemon/src/some_async_path.rs + /// use std::sync::Mutex; // ← caught + /// struct State { inner: std::sync::Mutex } // ← caught + /// ``` + /// + /// Use instead: + /// + /// ```rust,ignore + /// use tokio::sync::Mutex; + /// struct State { inner: tokio::sync::Mutex } + /// ``` + pub BAN_STD_SYNC_MUTEX_IN_ASYNC, + Deny, + "ban std::sync::Mutex / std::sync::RwLock in daemon / serial code" +} + +/// Banned canonical def-paths. Matched exactly against the resolved +/// `DefId`'s def-path. Listed both with and without the rustc-internal +/// `poison` submodule because both shapes can appear in the +/// `get_def_path` output across rustc nightlies. +const BANNED_PATHS: &[&[&str]] = &[ + &["std", "sync", "Mutex"], + &["std", "sync", "RwLock"], + &["std", "sync", "poison", "Mutex"], + &["std", "sync", "poison", "RwLock"], +]; + +const ALLOWLIST: &str = include_str!("allowlist.txt"); + +/// Scope: any file path matching one of these directory prefixes. +const SCOPES: &[&str] = &[ + "crates/fbuild-daemon/src/", + "crates/fbuild-serial/src/", +]; + +impl<'tcx> LateLintPass<'tcx> for BanStdSyncMutexInAsync { + fn check_ty(&mut self, cx: &LateContext<'tcx>, ty: &'tcx Ty<'tcx, AmbigArg>) { + let filename = source_filename(cx, ty.span); + let normalized = normalize_slashes(&filename); + if !in_scope(&normalized) || is_test_file(&normalized) || is_allowlisted(&normalized) { + return; + } + if owned_by_cfg_test_module(cx, ty.hir_id) { + return; + } + if let TyKind::Path(qpath) = ty.kind { + let res = cx.qpath_res(&qpath, ty.hir_id); + if let Some(banned) = res_banned(cx, res) { + emit_lint(cx, ty.span, banned); + } + } + } + + 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_scope(&normalized) || is_test_file(&normalized) || is_allowlisted(&normalized) { + return; + } + if owned_by_cfg_test_module(cx, expr.hir_id) { + return; + } + // Catches `std::sync::Mutex::new(...)` constructor calls — the + // type-position match handles field/variable declarations, this + // arm catches the path-as-callee form. + if let ExprKind::Path(ref qpath) = expr.kind { + let res = cx.qpath_res(qpath, expr.hir_id); + if let Some(banned) = res_banned(cx, res) { + emit_lint(cx, expr.span, banned); + } + } + } +} + +/// Walk up the HIR owner chain looking for a module annotated with +/// `#[cfg(test)]`. Mirrors `ban_unwrap_in_daemon_handlers`. +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 emit_lint(cx: &LateContext<'_>, span: rustc_span::Span, banned: &[&str]) { + let joined = banned.join("::"); + cx.opt_span_lint( + BAN_STD_SYNC_MUTEX_IN_ASYNC, + Some(span), + DiagDecorator(move |diag| { + diag.primary_message(format!( + "`{joined}` in an async-reachable fbuild crate (daemon / serial) — \ + holding a `std::sync::Mutex` / `RwLock` guard across `.await` starves \ + the Tokio worker and a panic poisons the lock for every subsequent \ + caller. Use `tokio::sync::Mutex` / `tokio::sync::RwLock` (or \ + `parking_lot::Mutex` for hot non-async paths that never cross an await), \ + or restructure so the lock is released before any `.await`. If this \ + file is a genuinely-synchronous hot path that should keep using \ + `std::sync`, allowlist it in \ + `dylints/ban_std_sync_mutex_in_async/src/allowlist.txt` with a one-line \ + reason." + )); + }), + ); +} + +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_scope(normalized: &str) -> bool { + SCOPES.iter().any(|s| normalized.contains(s)) +} + +fn is_test_file(normalized: &str) -> bool { + let Some(name) = normalized.rsplit('/').next() else { + return false; + }; + name.contains("tests") && name.ends_with(".rs") +} + +fn is_allowlisted(normalized: &str) -> bool { + ALLOWLIST + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .any(|allowed| normalized.ends_with(allowed)) +} + +fn normalize_slashes(path: &str) -> String { + path.replace('\\', "/") +} + +fn res_banned(cx: &LateContext<'_>, res: Res) -> Option<&'static [&'static str]> { + let Res::Def(_, def_id) = res else { + return None; + }; + let def_path = cx.get_def_path(def_id); + for banned in BANNED_PATHS { + if def_path_equals_path(&def_path, banned) { + return Some(banned); + } + } + None +} + +fn def_path_equals_path(def_path: &[Symbol], expected: &[&str]) -> bool { + 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 daemon_and_serial_src_are_in_scope() { + assert!(in_scope("crates/fbuild-daemon/src/context.rs")); + assert!(in_scope( + "crates/fbuild-daemon/src/handlers/operations/build.rs" + )); + assert!(in_scope("crates/fbuild-serial/src/manager.rs")); + } + + #[test] + fn other_crates_are_out_of_scope() { + assert!(!in_scope("crates/fbuild-cli/src/cli/build.rs")); + assert!(!in_scope("crates/fbuild-core/src/lib.rs")); + assert!(!in_scope("crates/fbuild-daemon/tests/foo.rs")); + } + + #[test] + fn test_files_in_scope_are_exempt() { + assert!(is_test_file( + "crates/fbuild-daemon/src/handlers/websockets_tests.rs" + )); + assert!(is_test_file("crates/fbuild-serial/src/manager/tests.rs")); + assert!(!is_test_file("crates/fbuild-serial/src/manager.rs")); + } +} diff --git a/dylints/ban_unwrap_in_daemon_handlers/Cargo.toml b/dylints/ban_unwrap_in_daemon_handlers/Cargo.toml new file mode 100644 index 00000000..853ceae5 --- /dev/null +++ b/dylints/ban_unwrap_in_daemon_handlers/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "ban_unwrap_in_daemon_handlers" +version = "0.1.0" +description = "Ban .unwrap() inside fbuild-daemon HTTP/WS handler code" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +# Pinned to the same dylint_linting rev the other fbuild dylints use — +# same rustup channel in rust-toolchain.toml, same upstream commit. +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_unwrap_in_daemon_handlers/README.md b/dylints/ban_unwrap_in_daemon_handlers/README.md new file mode 100644 index 00000000..2445cc81 --- /dev/null +++ b/dylints/ban_unwrap_in_daemon_handlers/README.md @@ -0,0 +1,64 @@ +# `ban_unwrap_in_daemon_handlers` + +Custom [dylint](https://github.com/trailofbits/dylint) that forbids +`.unwrap()` method calls inside files under +`crates/fbuild-daemon/src/handlers/`. Test code (`#[cfg(test)] mod +tests { ... }`, `tests.rs`, `*_tests.rs` files) is exempt — the lint +fires only on production handler code. + +## Why + +Panics inside HTTP/WebSocket handler paths crash the daemon, which +disconnects every connected client (CLI build streams, monitor +WebSockets, FastLED's `SerialMonitor`, the FastAPI bridge). Production +handlers must convert errors into structured HTTP responses or WS +error frames instead of unwrapping. + +PR #833 already fixed 10 known `.unwrap()` violations in daemon +handlers (see the websocket hardening section of that PR). This lint +locks in that state and prevents new violations from sneaking in. + +## Scope + +The lint runs only on files matching: + +``` +crates/fbuild-daemon/src/handlers/**/*.rs +``` + +Within scope, the following are exempt: + +- Files whose name matches `*tests*.rs` (e.g. `websockets_tests.rs`, + `tests_npm_cache.rs`, `tests_process.rs`, + `tests_select_runner.rs`). +- `.unwrap()` calls inside a module under a `#[cfg(test)]` attribute + (the lint walks up the owning module chain). + +Everything else (`crates/fbuild-daemon/src/{context,broker,...}.rs`, +the rest of the daemon, all other crates) is out of scope. + +## Allowlist + +This lint has no per-call allowlist file. Either the call site is in +test code (exempt automatically) or it is a real handler bug that must +be fixed. If you genuinely need an unwrap in a handler (e.g. +serializing a known-good struct that cannot fail), prefer +`.unwrap_or_default()`, `.expect("…")`, or a structured error response +— and document the invariant inline. + +## Toolchain + +Pinned to the same `nightly-2026-03-26` channel and the same +`trailofbits/dylint` git rev (`4bd91ce…`) the other fbuild dylints +use. + +## Running locally + +See `dylints/README.md` for the full local-run recipe. CI runs all +dylints on every push/PR via `.github/workflows/dylint.yml`. + +## See also + +- Issue #826 — this lint's tracking issue +- PR #833 — original hardening that fixed the existing 10 unwrap + violations diff --git a/dylints/ban_unwrap_in_daemon_handlers/rust-toolchain.toml b/dylints/ban_unwrap_in_daemon_handlers/rust-toolchain.toml new file mode 100644 index 00000000..3b6ccbe8 --- /dev/null +++ b/dylints/ban_unwrap_in_daemon_handlers/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_unwrap_in_daemon_handlers/src/allowlist.txt b/dylints/ban_unwrap_in_daemon_handlers/src/allowlist.txt new file mode 100644 index 00000000..e3509d58 --- /dev/null +++ b/dylints/ban_unwrap_in_daemon_handlers/src/allowlist.txt @@ -0,0 +1,41 @@ +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). Use forward-slash paths +# only — the lint normalizes \ to / before matching. +# +# Test files inside the handlers tree (any file whose base name +# matches `*tests*.rs`, e.g. `tests.rs`, `tests_npm_cache.rs`, +# `websockets_tests.rs`) are UNCONDITIONALLY exempt in lib.rs. They +# do NOT need entries here. +# +# `.unwrap()` calls inside a module annotated with `#[cfg(test)]` are +# also UNCONDITIONALLY exempt in lib.rs (the lint walks the owning +# module chain). +# +# Rollout strategy: +# - The lint is ON and denies new `.unwrap()` calls in production +# handler code. +# - PR #833 already fixed the 10 known pre-existing violations, but +# a fresh audit found a small number of `.unwrap()` calls that +# survived (they were in async closures inside `tokio::spawn(...)` +# blocks that aren't tagged `#[cfg(test)]`). They are exempted at +# file level so the lint can land green; each one is tracked for +# follow-up. +# - New files MUST NOT be added here. The target state is zero +# entries. + +# WebSocket serial stream: `serde_json::to_string(&serialMessage).unwrap()` +# inside the spawned send-loop. The input is a `SerialServerMessage` +# struct whose `Serialize` impl cannot fail in practice, but +# `.unwrap_or_default()` is a one-line follow-up. See #826. +crates/fbuild-daemon/src/handlers/websockets.rs + +# Streaming build response: `Response::builder()...body(body).unwrap()` +# for the NDJSON axum response. The builder only fails on header +# encoding errors that don't apply here. See #826. +crates/fbuild-daemon/src/handlers/operations/build.rs + +# AVR deploy path: `fbuild_build::avr::mcu_config::get_avr_config().unwrap()` +# — the embedded MCU table is statically populated and the lookup +# can't fail. Migrating to `expect("avr mcu table missing")` is a +# one-line follow-up. See #826. +crates/fbuild-daemon/src/handlers/operations/deploy.rs diff --git a/dylints/ban_unwrap_in_daemon_handlers/src/lib.rs b/dylints/ban_unwrap_in_daemon_handlers/src/lib.rs new file mode 100644 index 00000000..5fd21b65 --- /dev/null +++ b/dylints/ban_unwrap_in_daemon_handlers/src/lib.rs @@ -0,0 +1,264 @@ +#![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::{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 `.unwrap()` method calls inside files under + /// `crates/fbuild-daemon/src/handlers/` (any depth), with two + /// exemptions: + /// * test files (path tail matches `*tests*.rs`), and + /// * `.unwrap()` calls inside a `#[cfg(test)] mod` block (the + /// lint walks the owning module chain looking for the + /// `cfg(test)` attribute). + /// + /// ### Why is this bad? + /// + /// Panics in HTTP/WebSocket handler paths crash the daemon + /// process, which disconnects every connected client (CLI build + /// streams, monitor WebSockets, FastLED's `SerialMonitor`, the + /// FastAPI bridge). Handlers must convert errors into structured + /// HTTP responses or WS error frames, not panic. + /// + /// PR #833 already fixed the 10 known pre-existing `.unwrap()` + /// violations in daemon handlers; this lint locks in that state + /// so new violations can't sneak in. + /// + /// ### Known problems + /// + /// - The lint walks owning modules looking for `#[cfg(test)]`. + /// It does NOT detect `#[cfg(test)]` on individual *items* + /// (e.g. `#[cfg(test)] fn helper() {}` in a non-test module). + /// Such items are rare in handler code and should be moved + /// under a proper `#[cfg(test)] mod tests` instead. + /// - The lint resolves the *method* DefId to `Option::unwrap` / + /// `Result::unwrap`. Custom types that define their own + /// `unwrap()` method are not affected. + /// + /// ### Example + /// + /// ```rust,ignore + /// // crates/fbuild-daemon/src/handlers/websockets.rs + /// async fn handler(req: Request) -> Response { + /// let body = serde_json::to_string(&payload).unwrap(); // banned + /// ... + /// } + /// ``` + /// + /// Use instead: `.unwrap_or_default()`, `.expect("…")` with a + /// post-mortem-friendly message, or convert the error into a + /// structured response. + pub BAN_UNWRAP_IN_DAEMON_HANDLERS, + Deny, + "ban .unwrap() inside fbuild-daemon HTTP/WS handler code" +} + +/// Methods we ban. Matched on the canonical DefId path returned by +/// `type_dependent_def_id` — so `.unwrap()` on any `Option` / +/// `Result` resolves correctly regardless of how the type was named +/// in source. +const BANNED_METHOD_PATHS: &[&[&str]] = &[ + &["core", "option", "Option", "unwrap"], + &["std", "option", "Option", "unwrap"], + &["core", "result", "Result", "unwrap"], + &["std", "result", "Result", "unwrap"], +]; + +/// Scope: any file path matching this prefix is in scope. +const HANDLERS_DIR: &str = "crates/fbuild-daemon/src/handlers/"; + +impl<'tcx> LateLintPass<'tcx> for BanUnwrapInDaemonHandlers { + 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_scope(&normalized) || is_test_file(&normalized) { + return; + } + + let ExprKind::MethodCall(_, _, _, _) = expr.kind else { + return; + }; + + let Some(def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) else { + return; + }; + + let mut is_banned = false; + for banned in BANNED_METHOD_PATHS { + if def_path_equals(cx, def_id, banned) { + is_banned = true; + break; + } + } + if !is_banned { + return; + } + + if owned_by_cfg_test_module(cx, expr.hir_id) { + return; + } + + emit_lint(cx, expr.span); + } +} + +/// Walk up the HIR owner chain looking for a module annotated with +/// `#[cfg(test)]`. We deliberately only look at module items — +/// `#[cfg(test)]` on a function is the right wrong-shape to nudge +/// the developer into `mod tests { ... }`. +fn owned_by_cfg_test_module(cx: &LateContext<'_>, hir_id: HirId) -> bool { + let mut current = cx.tcx.hir_get_parent_item(hir_id); + loop { + // `is_in_test_module` via attribute scan: + 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 emit_lint(cx: &LateContext<'_>, span: rustc_span::Span) { + cx.opt_span_lint( + BAN_UNWRAP_IN_DAEMON_HANDLERS, + Some(span), + DiagDecorator(|diag| { + diag.primary_message( + "`.unwrap()` inside an fbuild-daemon handler can crash the daemon and \ + disconnect every connected client. Convert the error into a structured \ + response (HTTP 500 / WS error frame) or use `.unwrap_or_default()` / \ + `.expect(\"\")`. Tests are exempt — either \ + move the call into a `#[cfg(test)] mod tests { ... }` block or rename \ + the file to `*tests*.rs`.", + ); + }), + ); +} + +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_scope(normalized: &str) -> bool { + normalized.contains(HANDLERS_DIR) +} + +fn is_test_file(normalized: &str) -> bool { + let Some(name) = normalized.rsplit('/').next() else { + return false; + }; + // Match files whose base name contains "tests" (e.g. + // `websockets_tests.rs`, `tests_npm_cache.rs`, + // `tests_process.rs`, `tests_select_runner.rs`, `tests_outcome.rs`, + // `tests.rs`). A plain `tests` substring is enough — the handler + // tree doesn't use `tests` as a prefix for non-test code. + 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 handler_paths_are_in_scope() { + assert!(in_scope( + "crates/fbuild-daemon/src/handlers/operations/build.rs" + )); + assert!(in_scope("crates/fbuild-daemon/src/handlers/websockets.rs")); + assert!(in_scope( + "/anywhere/crates/fbuild-daemon/src/handlers/emulator/qemu_deploy.rs" + )); + } + + #[test] + fn non_handler_paths_are_out_of_scope() { + assert!(!in_scope("crates/fbuild-daemon/src/context.rs")); + assert!(!in_scope("crates/fbuild-daemon/src/main.rs")); + assert!(!in_scope("crates/fbuild-cli/src/cli/build.rs")); + } + + #[test] + fn test_files_are_exempt_by_name() { + assert!(is_test_file( + "crates/fbuild-daemon/src/handlers/websockets_tests.rs" + )); + assert!(is_test_file( + "crates/fbuild-daemon/src/handlers/operations/tests.rs" + )); + assert!(is_test_file( + "crates/fbuild-daemon/src/handlers/emulator/tests_npm_cache.rs" + )); + assert!(!is_test_file( + "crates/fbuild-daemon/src/handlers/operations/build.rs" + )); + assert!(!is_test_file( + "crates/fbuild-daemon/src/handlers/websockets.rs" + )); + } +} diff --git a/dylints/cli_no_build_deploy_direct_use/Cargo.toml b/dylints/cli_no_build_deploy_direct_use/Cargo.toml new file mode 100644 index 00000000..2b206e2e --- /dev/null +++ b/dylints/cli_no_build_deploy_direct_use/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "cli_no_build_deploy_direct_use" +version = "0.1.0" +description = "Forbid fbuild-cli from importing fbuild-build / fbuild-deploy outside the diagnostic-subcommand allowlist" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +# Pinned to the same dylint_linting rev the other fbuild dylints use — +# same rustup channel in rust-toolchain.toml, same upstream commit. +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/cli_no_build_deploy_direct_use/README.md b/dylints/cli_no_build_deploy_direct_use/README.md new file mode 100644 index 00000000..5c6fccb7 --- /dev/null +++ b/dylints/cli_no_build_deploy_direct_use/README.md @@ -0,0 +1,54 @@ +# `cli_no_build_deploy_direct_use` + +Custom [dylint](https://github.com/trailofbits/dylint) that forbids +files under `crates/fbuild-cli/src/` from naming items in the +`fbuild_build::*` or `fbuild_deploy::*` paths, except in a small +allowlist of diagnostic subcommands. + +## Why + +The "thin HTTP client" rule (see `crates/CLAUDE.md` §"Dependency +Graph" and §"Diagnostic subcommand exception"): `fbuild-cli` is meant +to be a tiny CLI that sends JSON over HTTP to the daemon. The daemon +owns all build orchestration (`fbuild-build`) and all firmware upload +(`fbuild-deploy`). Importing those crates into the CLI silently +bypasses the daemon, defeats build streaming, breaks +deploy-preemption semantics, and produces inconsistent error +surfaces. + +The exception is "diagnostic subcommands" — read-only in-process +inspectors like `clang-tidy`, `lib-select`, `symbols`, `bloat`, +`graph`, `lnk`, that don't need the build pipeline and would only add +latency by going through HTTP. Those subcommands are explicitly +allowlisted by file path. + +## Scope + +The lint runs only on `.rs` files under `crates/fbuild-cli/src/`. Any +file outside that scope is unconditionally exempt. + +## Allowlist + +Files allowed to import `fbuild_build::*` or `fbuild_deploy::*` are +listed in `src/allowlist.txt`. Each entry needs an inline comment +explaining what diagnostic the file implements. + +Current entries reflect the state at the time #826 was implemented; +the target is to drive the list down as diagnostic subcommands grow +their own crate (or as runtime-only paths move into the daemon). + +## Toolchain + +Pinned to the same `nightly-2026-03-26` channel and the same +`trailofbits/dylint` git rev (`4bd91ce…`) the other fbuild dylints +use. + +## Running locally + +See `dylints/README.md` for the full local-run recipe. CI runs all +dylints on every push/PR via `.github/workflows/dylint.yml`. + +## See also + +- Issue #826 — this lint's tracking issue +- `crates/CLAUDE.md` — "Diagnostic subcommand exception" diff --git a/dylints/cli_no_build_deploy_direct_use/rust-toolchain.toml b/dylints/cli_no_build_deploy_direct_use/rust-toolchain.toml new file mode 100644 index 00000000..3b6ccbe8 --- /dev/null +++ b/dylints/cli_no_build_deploy_direct_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/cli_no_build_deploy_direct_use/src/allowlist.txt b/dylints/cli_no_build_deploy_direct_use/src/allowlist.txt new file mode 100644 index 00000000..4e370516 --- /dev/null +++ b/dylints/cli_no_build_deploy_direct_use/src/allowlist.txt @@ -0,0 +1,32 @@ +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). +# +# Files listed here are "diagnostic subcommands" — read-only, +# in-process inspectors that don't need the build pipeline and would +# only add latency by going through HTTP. See `crates/CLAUDE.md` +# §"Diagnostic subcommand exception". +# +# Adding a new entry needs a one-line justification comment and a PR +# review — the diff to this file is the audit trail. + +# `args.rs` declares clap-derived enums (e.g. `CliShrinkMode`) that +# mirror `fbuild_build::shrink::ShrinkMode`. The CLI must own the +# clap layer; importing the corresponding `ShrinkMode` enum is a +# presentation type, not a build-pipeline call. +crates/fbuild-cli/src/cli/args.rs + +# `bloat-lookup`, `graph`, `symbols`, `compile-many` diagnostic +# subcommands run the build-info / symbol-analyzer code paths +# in-process — they're read-only inspectors over an already-built +# project's artifacts, no daemon round-trip needed. +crates/fbuild-cli/src/cli/bloat_lookup.rs +crates/fbuild-cli/src/cli/graph_cmd.rs +crates/fbuild-cli/src/cli/symbols_cmd.rs +crates/fbuild-cli/src/cli/compile_many.rs + +# `fbuild reset` calls into `fbuild_deploy::reset` directly because +# resetting a device is a single, fast, no-build-state operation — +# wrapping it in a daemon HTTP round-trip adds latency for no +# benefit. This may migrate to the daemon if/when reset gains state +# (lock holding, port multiplexing). +crates/fbuild-cli/src/cli/reset.rs diff --git a/dylints/cli_no_build_deploy_direct_use/src/lib.rs b/dylints/cli_no_build_deploy_direct_use/src/lib.rs new file mode 100644 index 00000000..f9a957ac --- /dev/null +++ b/dylints/cli_no_build_deploy_direct_use/src/lib.rs @@ -0,0 +1,178 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_span; + +use rustc_errors::DiagDecorator; +use rustc_hir::{def::Res, AmbigArg, Expr, ExprKind, Ty, TyKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::{symbol::Symbol, FileName, RemapPathScopeComponents}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Bans direct references to items from the `fbuild_build` and + /// `fbuild_deploy` crates in `crates/fbuild-cli/src/`, except in + /// the files explicitly allowlisted as "diagnostic subcommands". + /// + /// ### Why is this bad? + /// + /// `fbuild-cli` is supposed to be a thin HTTP client to the + /// daemon (see `crates/CLAUDE.md` §"Dependency Graph"). All build + /// orchestration lives in `fbuild-build` and all firmware-upload + /// logic lives in `fbuild-deploy`, both consumed by the daemon — + /// not by the CLI directly. Calling those crates from the CLI + /// silently bypasses the daemon, breaks build streaming, defeats + /// deploy-preemption, and produces inconsistent error surfaces. + /// + /// The one exception is "diagnostic subcommands" — read-only, + /// in-process inspectors like `lib-select`, `symbols`, `bloat`, + /// `graph`, `lnk` — that don't need the build pipeline and would + /// only add latency by going through HTTP. Those files are listed + /// in `src/allowlist.txt`. + /// + /// ### Known problems + /// + /// Allowlisting is file-level. A new diagnostic subcommand must + /// add itself to `src/allowlist.txt` with a one-line reason — the + /// PR review is the audit trail. + /// + /// ### Example + /// + /// ```rust,ignore + /// // crates/fbuild-cli/src/cli/my_new_cmd.rs + /// use fbuild_build::orchestrator::run_build; // banned + /// use fbuild_deploy::reset::reset_device; // banned + /// ``` + /// + /// Use instead: POST to the daemon's HTTP endpoint and stream the + /// response. + pub CLI_NO_BUILD_DEPLOY_DIRECT_USE, + Deny, + "ban fbuild-cli from importing fbuild-build / fbuild-deploy outside diagnostic subcommands" +} + +const BANNED_CRATES: &[&str] = &["fbuild_build", "fbuild_deploy"]; + +const ALLOWLIST: &str = include_str!("allowlist.txt"); + +/// Scope: only `crates/fbuild-cli/src/` Rust files. +const CLI_SRC_DIR: &str = "crates/fbuild-cli/src/"; + +impl<'tcx> LateLintPass<'tcx> for CliNoBuildDeployDirectUse { + fn check_ty(&mut self, cx: &LateContext<'tcx>, ty: &'tcx Ty<'tcx, AmbigArg>) { + let filename = source_filename(cx, ty.span); + let normalized = normalize_slashes(&filename); + if !in_scope(&normalized) || is_allowlisted(&normalized) { + return; + } + if let TyKind::Path(qpath) = ty.kind { + let res = cx.qpath_res(&qpath, ty.hir_id); + if let Some(banned) = res_banned_crate(cx, res) { + emit_lint(cx, ty.span, banned); + } + } + } + + 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_scope(&normalized) || is_allowlisted(&normalized) { + return; + } + if let ExprKind::Path(ref qpath) = expr.kind { + let res = cx.qpath_res(qpath, expr.hir_id); + if let Some(banned) = res_banned_crate(cx, res) { + emit_lint(cx, expr.span, banned); + } + } + } +} + +fn emit_lint(cx: &LateContext<'_>, span: rustc_span::Span, banned: &str) { + let banned = banned.to_owned(); + cx.opt_span_lint( + CLI_NO_BUILD_DEPLOY_DIRECT_USE, + Some(span), + DiagDecorator(move |diag| { + diag.primary_message(format!( + "`fbuild-cli` references `{banned}::*` — the CLI is supposed to be a thin \ + HTTP client to the daemon (see `crates/CLAUDE.md` §\"Dependency Graph\"). \ + Build orchestration belongs in the daemon. If this is a read-only \ + diagnostic subcommand that legitimately bypasses the daemon, allowlist \ + the file in `dylints/cli_no_build_deploy_direct_use/src/allowlist.txt` \ + with a one-line reason." + )); + }), + ); +} + +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_scope(normalized: &str) -> bool { + normalized.contains(CLI_SRC_DIR) +} + +fn is_allowlisted(normalized: &str) -> bool { + ALLOWLIST + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .any(|allowed| normalized.ends_with(allowed)) +} + +fn normalize_slashes(path: &str) -> String { + path.replace('\\', "/") +} + +fn res_banned_crate(cx: &LateContext<'_>, res: Res) -> Option<&'static str> { + let Res::Def(_, def_id) = res else { + return None; + }; + let def_path = cx.get_def_path(def_id); + if def_path.is_empty() { + return None; + } + for banned in BANNED_CRATES { + if def_path[0] == Symbol::intern(banned) { + return Some(banned); + } + } + None +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn cli_src_files_in_scope() { + assert!(in_scope("crates/fbuild-cli/src/cli/build.rs")); + assert!(in_scope( + "/home/runner/work/fbuild/crates/fbuild-cli/src/lib_select.rs" + )); + } + + #[test] + fn non_cli_files_out_of_scope() { + assert!(!in_scope("crates/fbuild-daemon/src/main.rs")); + assert!(!in_scope("crates/fbuild-build/src/lib.rs")); + assert!(!in_scope("crates/fbuild-cli/tests/integration.rs")); + } +} diff --git a/dylints/require_multi_thread_flavor_when_spawning/Cargo.toml b/dylints/require_multi_thread_flavor_when_spawning/Cargo.toml new file mode 100644 index 00000000..d91325f7 --- /dev/null +++ b/dylints/require_multi_thread_flavor_when_spawning/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "require_multi_thread_flavor_when_spawning" +version = "0.1.0" +description = "Require tokio::test(flavor = \"multi_thread\") when the test body calls tokio::spawn" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +# Pinned to the same dylint_linting rev the other fbuild dylints use — +# same rustup channel in rust-toolchain.toml, same upstream commit. +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_multi_thread_flavor_when_spawning/README.md b/dylints/require_multi_thread_flavor_when_spawning/README.md new file mode 100644 index 00000000..2e940fb2 --- /dev/null +++ b/dylints/require_multi_thread_flavor_when_spawning/README.md @@ -0,0 +1,64 @@ +# `require_multi_thread_flavor_when_spawning` + +Custom [dylint](https://github.com/trailofbits/dylint) that flags +`#[tokio::test]` (or `#[tokio::test(...)]` *without* `flavor = +"multi_thread"`) on an `async fn` whose body calls `tokio::spawn(...)` +anywhere. + +## Why + +The default `tokio::test` flavor is `current_thread`, which runs all +spawned tasks on the same executor thread as the test itself. If a +spawned task and the test body both need to be polled to make +progress (e.g. the test awaits a channel the spawned task writes +to), the test deadlocks under load and runs serially otherwise — the +exact wrong shape for "I'm testing concurrent code". + +The fix is one line: + +```rust +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn my_concurrent_test() { ... } +``` + +This lint catches the bug class at lint time instead of waiting for a +flaky CI run. + +## Scope + +The lint runs on all Rust files in the workspace. There is no +file-path scoping — the contract is "any tokio test that spawns must +use multi_thread", regardless of where the test lives. + +## Allowlist + +Spans flagged by this lint are listed in `src/allowlist.txt` by source +file path (the lint matches on the tail of the file path). The current +entries are the legacy violators that existed when the lint was first +enabled (#826) — they need to migrate to `flavor = "multi_thread"` and +their allowlist entries should be removed. + +## Known limitations + +- The lint only inspects the function body lexically — it does not + follow calls into helper functions that themselves call + `tokio::spawn`. If you wrap `tokio::spawn` in a helper, the test + itself will not be flagged. +- The lint trusts the attribute's literal `flavor` string — it does + not resolve constants or macros that could produce the attribute. + In practice attributes are written as literals. + +## Toolchain + +Pinned to the same `nightly-2026-03-26` channel and the same +`trailofbits/dylint` git rev (`4bd91ce…`) the other fbuild dylints +use. + +## Running locally + +See `dylints/README.md` for the full local-run recipe. CI runs all +dylints on every push/PR via `.github/workflows/dylint.yml`. + +## See also + +- Issue #826 — this lint's tracking issue diff --git a/dylints/require_multi_thread_flavor_when_spawning/rust-toolchain.toml b/dylints/require_multi_thread_flavor_when_spawning/rust-toolchain.toml new file mode 100644 index 00000000..3b6ccbe8 --- /dev/null +++ b/dylints/require_multi_thread_flavor_when_spawning/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_multi_thread_flavor_when_spawning/src/allowlist.txt b/dylints/require_multi_thread_flavor_when_spawning/src/allowlist.txt new file mode 100644 index 00000000..d6f5eeed --- /dev/null +++ b/dylints/require_multi_thread_flavor_when_spawning/src/allowlist.txt @@ -0,0 +1,28 @@ +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). Use forward-slash paths +# only — the lint normalizes \ to / before matching. +# +# Rollout strategy: +# - The lint is ON and denies new `#[tokio::test]` annotations on +# async fns that call `tokio::spawn` in their body without +# `flavor = "multi_thread"`. +# - This file lists the *legacy* call sites that existed when the +# lint was first enabled (FastLED/fbuild#826). They are exempted +# so the lint can land without an upfront audit of every test — +# each entry must be migrated and removed in follow-up PRs. +# - New files MUST NOT be added here. + +# Daemon WebSocket reader-control tests: spawn a "toy reader" inside +# the test body, then synchronously poke a control channel. Under +# `current_thread` this happens to work because the reader's +# `select!` polls broadcast-then-control with biased ordering and +# the broadcast events are pre-queued before the test awaits. The +# test is racy under `multi_thread` until reworked. Tracked. +crates/fbuild-daemon/src/handlers/websockets_tests.rs + +# Packages install-lock tests: spawn a waiter to verify the +# acquire/release contract. Single-threaded scheduling happens to +# serialize the spawn relative to the parent's `.await`, which is +# the timing the test asserts. Reworking for `multi_thread` +# requires explicit synchronization. Tracked. +crates/fbuild-packages/src/install_lock.rs diff --git a/dylints/require_multi_thread_flavor_when_spawning/src/lib.rs b/dylints/require_multi_thread_flavor_when_spawning/src/lib.rs new file mode 100644 index 00000000..d756f73b --- /dev/null +++ b/dylints/require_multi_thread_flavor_when_spawning/src/lib.rs @@ -0,0 +1,283 @@ +#![feature(rustc_private)] + +extern crate rustc_ast; +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_middle; +extern crate rustc_span; + +use rustc_ast::ast::LitKind; +use rustc_errors::DiagDecorator; +use rustc_hir::{ + def::Res, + def_id::LocalDefId, + intravisit::{walk_expr, Visitor}, + Attribute, BodyId, Expr, ExprKind, FnDecl, +}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::hir::nested_filter; +use rustc_span::{symbol::Symbol, FileName, RemapPathScopeComponents, Span}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Flags `async fn` items annotated with `#[tokio::test]` (or + /// `#[tokio::test(...)]` *without* `flavor = "multi_thread"`) + /// whose body calls `tokio::spawn(...)` anywhere. + /// + /// ### Why is this bad? + /// + /// The default `tokio::test` flavor is `current_thread`, which + /// runs every spawned task on the same executor thread as the + /// test itself. If the test awaits something the spawned task + /// produces (e.g. a channel send), the test deadlocks under load + /// — and runs serially otherwise. That's the exact wrong shape + /// for testing concurrent code. + /// + /// The fix is one line: + /// + /// ```rust,ignore + /// #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + /// async fn my_test() { + /// tokio::spawn(async { ... }); + /// // ... + /// } + /// ``` + /// + /// ### Known problems + /// + /// - Only the function body is inspected lexically — the lint + /// does not follow calls into helper functions that themselves + /// call `tokio::spawn`. If you wrap spawn in a helper, the test + /// itself will not be flagged. + /// - The lint trusts the attribute's literal `flavor` string — + /// it does not resolve constants or macros. + /// + /// ### Example + /// + /// ```rust,ignore + /// #[tokio::test] // ← flagged: missing flavor + /// async fn test_streaming() { + /// let (tx, mut rx) = tokio::sync::mpsc::channel(4); + /// tokio::spawn(async move { tx.send(()).await.unwrap(); }); + /// rx.recv().await; + /// } + /// ``` + pub REQUIRE_MULTI_THREAD_FLAVOR_WHEN_SPAWNING, + Deny, + "require tokio::test(flavor = \"multi_thread\") when the test body calls tokio::spawn" +} + +const ALLOWLIST: &str = include_str!("allowlist.txt"); + +impl<'tcx> LateLintPass<'tcx> for RequireMultiThreadFlavorWhenSpawning { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + kind: rustc_hir::intravisit::FnKind<'tcx>, + _decl: &'tcx FnDecl<'tcx>, + body_id: BodyId, + span: Span, + def_id: LocalDefId, + ) { + // Only async fns are candidates — `#[tokio::test]` only goes + // on async fns. + if !is_async_fn(&kind) { + return; + } + + let filename = source_filename(cx, span); + let normalized = normalize_slashes(&filename); + if is_allowlisted(&normalized) { + return; + } + + let hir_id = cx.tcx.local_def_id_to_hir_id(def_id); + let attrs = cx.tcx.hir_attrs(hir_id); + + let Some(test_attr) = find_tokio_test_attr(attrs) else { + return; + }; + + if attr_has_multi_thread_flavor(test_attr) { + return; + } + + // Walk the body looking for `tokio::spawn(...)` calls. + let body = cx.tcx.hir_body(body_id); + let mut visitor = SpawnFinder { + cx, + found_at: None, + }; + visitor.visit_expr(body.value); + + if let Some(spawn_span) = visitor.found_at { + emit_lint(cx, span, spawn_span); + } + } +} + +fn is_async_fn(kind: &rustc_hir::intravisit::FnKind<'_>) -> bool { + use rustc_hir::intravisit::FnKind; + let header = match kind { + FnKind::ItemFn(_, _, header) => header, + FnKind::Method(_, sig) => &sig.header, + FnKind::Closure => return false, + }; + matches!( + header.asyncness, + rustc_hir::IsAsync::Async(_) + ) +} + +/// Locate an attribute whose path ends in `test` and whose preceding +/// segments contain `tokio`. We accept `#[tokio::test]`, +/// `#[::tokio::test]`, and `#[tokio::test(...)]`. +fn find_tokio_test_attr(attrs: &[Attribute]) -> Option<&Attribute> { + for attr in attrs { + let meta = attr.meta()?; + let path = meta.path(); + let segments: Vec<&str> = path + .segments + .iter() + .map(|s| s.ident.as_str()) + .collect::>(); + let last = segments.last().copied().unwrap_or(""); + if last != "test" { + continue; + } + if segments.iter().any(|s| *s == "tokio") { + return Some(attr); + } + } + None +} + +fn attr_has_multi_thread_flavor(attr: &Attribute) -> bool { + let Some(meta) = attr.meta() else { + return false; + }; + let Some(list) = meta.meta_item_list() else { + return false; + }; + for nested in list { + let Some(item) = nested.meta_item() else { + continue; + }; + let name = item + .path + .segments + .last() + .map(|s| s.ident.as_str()) + .unwrap_or(""); + if name != "flavor" { + continue; + } + // Read the literal value. + let Some(value) = item.name_value_literal() else { + continue; + }; + if let LitKind::Str(sym, _) = value.kind { + if sym.as_str() == "multi_thread" { + return true; + } + } + } + false +} + +struct SpawnFinder<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + found_at: Option, +} + +impl<'tcx> Visitor<'tcx> for SpawnFinder<'_, 'tcx> { + type NestedFilter = nested_filter::OnlyBodies; + + fn maybe_tcx(&mut self) -> Self::MaybeTyCtxt { + self.cx.tcx + } + + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + if self.found_at.is_some() { + return; + } + if let ExprKind::Call(callee, _) = expr.kind { + if let ExprKind::Path(ref qpath) = callee.kind { + if let Res::Def(_, def_id) = self.cx.qpath_res(qpath, callee.hir_id) { + if def_path_equals(self.cx, def_id, &["tokio", "task", "spawn"]) + || def_path_equals(self.cx, def_id, &["tokio", "spawn"]) + { + self.found_at = Some(expr.span); + return; + } + } + } + } + walk_expr(self, expr); + } +} + +fn emit_lint(cx: &LateContext<'_>, fn_span: Span, spawn_span: Span) { + cx.opt_span_lint( + REQUIRE_MULTI_THREAD_FLAVOR_WHEN_SPAWNING, + Some(fn_span), + DiagDecorator(move |diag| { + diag.primary_message( + "`#[tokio::test]` on an async fn that calls `tokio::spawn` — the default \ + `current_thread` flavor runs spawned tasks on the same thread as the \ + test, so any cross-task await deadlocks. Add \ + `#[tokio::test(flavor = \"multi_thread\", worker_threads = 2)]` (or move \ + the spawn out of the test body). If this test is genuinely fine on \ + `current_thread`, allowlist the file in \ + `dylints/require_multi_thread_flavor_when_spawning/src/allowlist.txt` \ + with a one-line reason.", + ); + diag.span_help(spawn_span, "the `tokio::spawn` call lives here"); + }), + ); +} + +fn source_filename(cx: &LateContext<'_>, 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 is_allowlisted(normalized: &str) -> bool { + ALLOWLIST + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .any(|allowed| normalized.ends_with(allowed)) +} + +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)) +}