From aea30b11fba5a3aaf3fba21ce6ec1417e1f8441b Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Fri, 7 Aug 2026 09:24:50 -0700 Subject: [PATCH 01/10] chore(lints): gate span arithmetic and lexer indexing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adopt two clippy lints on the paths that handle attacker-controlled input, each validated by replaying it against the tree that carried the bug it claims to catch, and drop the sixteen `expect(FEATURES_PINNED)` call sites the CLI and web crates used to discharge a widened `Result`. `clippy::arithmetic_side_effects` on the `loc` metric module. Replayed at 92680fa8^ it flags both of #1051's reported panic sites — the `end - 1` in `loc/rust.rs` and the `end - start` in `add_cloc_lines`. Scoped to `loc` rather than `src/metrics/`: the other metrics contribute 244 hits, all `+=` on their own counters, which is not this bug class and would bury it. The 69 in-scope hits resolve to zero `#[allow]`s in the span code — 36 `logical_lines += 1` collapse into `Lloc::count_logical_line`, and the rest become saturating — plus one file-level carve-out in `line_set.rs`, where the arithmetic is on word indices and saturating would read the wrong row rather than prevent anything. `clippy::indexing_slicing` on `src/c_macro.rs`. The issue rated this unproven; it is not. #126 was `&DOLLARS[..(i - start)]` panicking on a 2049-byte macro identifier, and replayed at f59c5c06^ the lint flags both slices. Nine per-function carve-outs, never file-wide, so a new function is covered by default. `step_raw_string`'s delimiter comparison is hardened with `get` instead of allowed: its bound is established in `enter_raw_string` and carried through `LexState`, which is the #126 shape and the one a reader cannot check locally. `expect(FEATURES_PINNED)` was sound only while `MetricsError` had no variant it did not handle, and that enum is `#[non_exhaustive]` with a doc reserving the right to add variants in a minor release — a panic scheduled against a routine dependency bump. Both crates already had an error channel: the CLI's helpers return `io::Result`, whose runner prints a per-file line and continues, and the web handlers have an existing sanitized 500. Neither needed a new failure mode, only for the existing one to be used. No reachable behaviour change today, so both error paths are pinned by unit tests on the mapping rather than end-to-end. Metric values are unchanged: a saturating operation equals the plain one unless it would have overflowed, and none does. `dispatch_find` and `dispatch_exemptions` gain one `nexits` each — the `?` that replaced the `expect` — tripping the soft tier at 4.75. Both are baselined; the hard limit of 5 is unchanged and still met. Fixes #1152 --- .bca-baseline.toml | 14 ++- CHANGELOG.md | 38 ++++++++ big-code-analysis-cli/src/dispatch.rs | 97 +++++++++++++++---- big-code-analysis-cli/src/lib.rs | 12 --- big-code-analysis-web/src/web/server.rs | 8 -- .../src/web/server/errors.rs | 24 +++++ .../src/web/server/handlers.rs | 43 ++++---- big-code-analysis-web/src/web/server_tests.rs | 45 +++++++++ docs/development/lessons_learned.md | 25 ++++- src/c_macro.rs | 62 +++++++++++- src/metrics/loc.rs | 64 ++++++++++-- src/metrics/loc/bash.rs | 2 +- src/metrics/loc/c.rs | 6 +- src/metrics/loc/cpp.rs | 6 +- src/metrics/loc/csharp.rs | 4 +- src/metrics/loc/elixir.rs | 2 +- src/metrics/loc/go.rs | 4 +- src/metrics/loc/groovy.rs | 4 +- src/metrics/loc/irules.rs | 4 +- src/metrics/loc/java.rs | 4 +- src/metrics/loc/javascript.rs | 2 +- src/metrics/loc/kotlin.rs | 4 +- src/metrics/loc/line_set.rs | 18 ++++ src/metrics/loc/lua.rs | 4 +- src/metrics/loc/mozcpp.rs | 6 +- src/metrics/loc/mozjs.rs | 2 +- src/metrics/loc/objc.rs | 6 +- src/metrics/loc/perl.rs | 4 +- src/metrics/loc/php.rs | 2 +- src/metrics/loc/python.rs | 4 +- src/metrics/loc/ruby.rs | 2 +- src/metrics/loc/rust.rs | 8 +- src/metrics/loc/shared.rs | 11 ++- src/metrics/loc/tcl.rs | 4 +- src/metrics/loc/tsx.rs | 2 +- src/metrics/loc/typescript.rs | 2 +- 36 files changed, 425 insertions(+), 124 deletions(-) diff --git a/.bca-baseline.toml b/.bca-baseline.toml index f4c7d112a..6946beeee 100644 --- a/.bca-baseline.toml +++ b/.bca-baseline.toml @@ -167,12 +167,24 @@ qualified = "dispatch_exemptions" metric = "nargs" value = 5.0 +[[entry]] +path = "big-code-analysis-cli/src/dispatch.rs" +qualified = "dispatch_exemptions" +metric = "nexits" +value = 5.0 + [[entry]] path = "big-code-analysis-cli/src/dispatch.rs" qualified = "dispatch_find" metric = "nargs" value = 6.0 +[[entry]] +path = "big-code-analysis-cli/src/dispatch.rs" +qualified = "dispatch_find" +metric = "nexits" +value = 5.0 + [[entry]] path = "big-code-analysis-cli/src/dispatch.rs" qualified = "dispatch_functions" @@ -1005,7 +1017,7 @@ value = 5.0 path = "src/metrics/loc/perl.rs" qualified = "PerlCode::compute" metric = "halstead.effort" -value = 55025.91689557041 +value = 52134.594881912846 [[entry]] path = "src/metrics/loc/shared.rs" diff --git a/CHANGELOG.md b/CHANGELOG.md index e7011124d..3f1ac8a49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,44 @@ for historical reference. ## [Unreleased] +### Fixed + +- The CLI and web crates no longer terminate on a library parse error. + All sixteen `.expect(FEATURES_PINNED)` call sites — eight in `bca`'s + dispatch helpers, seven in `bca-web`'s handlers, plus the constant + itself in each crate — now propagate onto the error channel each + caller already had: an `io::Error` of kind `InvalidData` that the + concurrent runner reports per file and continues past, and the + existing sanitized `500` that logs the cause server-side. The pinned + `all-languages` feature does make `MetricsError::LanguageDisabled` + unreachable, but `MetricsError` is `#[non_exhaustive]` and documents + that variants may be added in a *minor* release, so the `expect` was a + panic scheduled against a routine dependency bump rather than an + invariant. No behaviour changes today: the only reachable outcome is + still success (#1152). + +### Changed + +- `clippy::arithmetic_side_effects` is enforced on the `loc` metric + module, and the span arithmetic there is now explicitly saturating. + `Loc` is the one metric computing on tree-sitter row coordinates, and + #1051 was a `usize` underflow of exactly that shape — a Rust doc + comment at EOF drove `end - 1` below zero from an input as small as + `/// x`, panicking in debug and wrapping to `usize::MAX` in release. + Validated by replaying the lint against the pre-#1051 tree, where it + flags both reported panic sites. Metric values are unchanged: a + saturating operation is identical to the plain one unless it would + have overflowed, and none does (#1152). +- `clippy::indexing_slicing` is enforced on `src/c_macro.rs`, the C/C++ + macro-masking byte lexer, with nine per-function carve-outs each + naming the bound that makes its indexing safe. Validated by replaying + it against the pre-#126 tree, where it flags the `&DOLLARS[..]` slices + that panicked on macro identifiers longer than 2048 bytes. The one + slice whose bound is established in a *different* function — + `step_raw_string`'s delimiter comparison, carried through + `LexState::RawString` — is hardened with `get` rather than allowed + (#1152). + ## [2.1.0] - 2026-08-06 A feature and correctness release on the `2.x` line, and the first to diff --git a/big-code-analysis-cli/src/dispatch.rs b/big-code-analysis-cli/src/dispatch.rs index 0f1d476ea..663ce0f3f 100644 --- a/big-code-analysis-cli/src/dispatch.rs +++ b/big-code-analysis-cli/src/dispatch.rs @@ -29,7 +29,7 @@ use big_code_analysis::{ use crate::exemptions::FileMarkers; use crate::formats::{MetricsDispatch, MetricsFormat, dump_csv}; use crate::markdown_report::extract_summaries; -use crate::{Action, Config, FEATURES_PINNED, note, warn}; +use crate::{Action, Config, note, warn}; /// Analyze one already-read file via the explicit-name [`Source`] seam. /// @@ -66,6 +66,42 @@ fn parse_ast( ) } +/// [`parse_ast`], with the library error mapped onto the `io::Result` +/// channel every dispatch helper already returns. +/// +/// This replaces `.expect(FEATURES_PINNED)` at all eight dispatch call +/// sites (#1152). The feature pin does make [`MetricsError`]'s only +/// reachable variant, `LanguageDisabled`, unreachable here — but +/// `MetricsError` is `#[non_exhaustive]` and its own documentation +/// reserves the right to add variants in a *minor* release, so the +/// `expect` was a panic scheduled against a routine dependency bump +/// rather than an invariant. `ErrorKind::InvalidData` is the honest +/// classification: whatever a future variant turns out to mean, it +/// means this file's bytes did not yield a tree. +/// +/// The failure is per-file. `act_on_file` returns this to the +/// concurrent runner, which prints a per-file error line and carries +/// on — so an unparseable file costs that file, where the `expect` +/// unwound a worker mid-walk and took the rest of the run with it. +fn parse_ast_io( + language: LANG, + source: Vec, + path: &Path, + pr: Option>, +) -> std::io::Result { + parse_ast(language, source, path, pr).map_err(parse_error_to_io) +} + +/// Lifts a [`MetricsError`] into the `io::Error` channel. +/// +/// Split out of [`parse_ast_io`] so the mapping is reachable from a +/// test: the CLI's feature pin makes every current `MetricsError` +/// variant unreachable through `parse_ast_io` itself, so the branch has +/// no end-to-end trigger and would otherwise ship uncovered. +fn parse_error_to_io(err: MetricsError) -> std::io::Error { + std::io::Error::new(std::io::ErrorKind::InvalidData, err) +} + pub(crate) fn act_on_file(path: PathBuf, cfg: &Config) -> std::io::Result<()> { let Some((path, source, language)) = validate_and_resolve_file(path, cfg)? else { return Ok(()); @@ -205,7 +241,7 @@ fn dispatch_dump( // The CLI pins the library's `all-languages` feature, so // `LanguageDisabled` from `Ast::parse` is unreachable; the `expect` // documents that invariant. - let ast = parse_ast(language, source, &path, pr).expect(FEATURES_PINNED); + let ast = parse_ast_io(language, source, &path, pr)?; // Per-file banner so a multi-file dump is attributable: the parallel // walk interleaves trees by worker scheduling, and without a header // which tree belongs to which file is unrecoverable (#690). @@ -281,10 +317,7 @@ fn dispatch_metrics( // Human-readable metric dump: parse once, then render the tree. // A walker error degrades to no output (matching the prior // `Metrics` callback), never an `Err`. - match parse_ast(language, source, &path, pr) - .expect(FEATURES_PINNED) - .metrics(cfg.metrics_options()) - { + match parse_ast_io(language, source, &path, pr)?.metrics(cfg.metrics_options()) { Ok(space) => dump_root_with_color(&space, cfg.color), Err(_) => Ok(()), } @@ -323,10 +356,7 @@ fn dispatch_ops( } else { // Human-readable ops dump: a walker error degrades to no output // (matching the prior `OpsCode` callback), never an `Err`. - match parse_ast(language, source, &path, pr) - .expect(FEATURES_PINNED) - .ops() - { + match parse_ast_io(language, source, &path, pr)?.ops() { Ok(ops) => dump_ops_with_color(&ops, cfg.color), Err(_) => Ok(()), } @@ -349,7 +379,7 @@ fn dispatch_strip_comments( } else { language }; - let ast = parse_ast(lang, source, &path, pr).expect(FEATURES_PINNED); + let ast = parse_ast_io(lang, source, &path, pr)?; if let Some(new_source) = ast.strip_comments() { if in_place { write_file(&path, &new_source)?; @@ -390,7 +420,7 @@ fn dispatch_functions( pr: Option>, cfg: &Config, ) -> std::io::Result<()> { - let ast = parse_ast(language, source, &path, pr).expect(FEATURES_PINNED); + let ast = parse_ast_io(language, source, &path, pr)?; dump_function_spans_with_color(ast.functions(), &path, cfg.color) } @@ -402,7 +432,7 @@ fn dispatch_find( cfg: &Config, filters: &Arc<[String]>, ) -> std::io::Result<()> { - let ast = parse_ast(language, source, &path, pr).expect(FEATURES_PINNED); + let ast = parse_ast_io(language, source, &path, pr)?; // A walker error degrades to no output, matching `dispatch_metrics` // / `dispatch_ops`. `Ast::find` is infallible today, but its `Result` // is contracted to become fallible under a future strict-parsing mode @@ -449,9 +479,7 @@ fn dispatch_count( .count_lock .clone() .expect("Count handler initializes count_lock before dispatch"); - let (good, total) = parse_ast(language, source, &path, pr) - .expect(FEATURES_PINNED) - .count(&filters[..]); + let (good, total) = parse_ast_io(language, source, &path, pr)?.count(&filters[..]); stats.add(good, total); Ok(()) } @@ -606,9 +634,7 @@ fn dispatch_exemptions( } return Ok(()); }; - let markers = parse_ast(language, source, &path, pr) - .expect(FEATURES_PINNED) - .suppressions(); + let markers = parse_ast_io(language, source, &path, pr)?.suppressions(); // Empty files are the dominant case (most source carries no // markers); skip the channel send and the per-file allocation when // there is nothing to report. @@ -655,6 +681,39 @@ mod tests { use std::sync::Mutex; use std::sync::atomic::AtomicUsize; + /// The dispatch helpers propagate a library parse failure instead of + /// panicking through `expect(FEATURES_PINNED)` (#1152). + /// + /// Unreachable end-to-end by construction: the CLI pins + /// `all-languages`, so `LanguageDisabled` cannot be produced here, + /// and `MetricsError` is `#[non_exhaustive]` precisely so that a + /// *future* variant can be. That is the whole reason the `expect` + /// was wrong, and it is why this asserts on the mapping directly + /// rather than through a `bca` invocation. + /// + /// `InvalidData` is load-bearing: `act_on_file`'s caller reports the + /// per-file line and continues, and `BrokenPipe` is the one kind it + /// treats specially, so a mapping that reached for that would + /// silently swallow the failure. + #[test] + fn a_library_parse_error_becomes_an_invalid_data_io_error() { + let err = parse_error_to_io(MetricsError::LanguageDisabled(LANG::Rust)); + + assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); + // The cause survives the lift rather than being flattened to a + // generic string, so the per-file line names the language. + assert!( + err.to_string().contains("rust"), + "the io::Error must carry the library message, got {err}" + ); + assert!( + err.get_ref() + .and_then(|inner| inner.downcast_ref::()) + .is_some(), + "the MetricsError must be retrievable, not stringified" + ); + } + // Minimal `Config` for exercising `dispatch_preproc` in isolation. // Only `preproc_lock` and `warning` are load-bearing here; every // other field is defaulted to the inert value used elsewhere. diff --git a/big-code-analysis-cli/src/lib.rs b/big-code-analysis-cli/src/lib.rs index bd90f0f98..c28425267 100644 --- a/big-code-analysis-cli/src/lib.rs +++ b/big-code-analysis-cli/src/lib.rs @@ -109,18 +109,6 @@ use big_code_analysis::{ }; use big_code_analysis::{FuncSpace, Ops, get_from_ext, get_language_for_file, read_file}; -/// `expect` message used at every `action::<_>` call site inside the -/// extracted `dispatch` module. Kept in `lib.rs` so any module that -/// terminates with `expect(FEATURES_PINNED)` can import the same -/// string and the invariant lives in one place. -/// -/// The CLI pins `big-code-analysis` with `features = ["all-languages"]`, -/// so a `LANG` value that reached this point must be enabled at compile -/// time. Any future caller that loosens the feature pin must change -/// this invariant explicitly. -pub(crate) const FEATURES_PINNED: &str = - "CLI pins big-code-analysis features = [\"all-languages\"]"; - /// Process exit code for tool errors — bad flags/values, unreadable /// input, I/O failures. Distinct from [`EXIT_GATE_BREACH`] so CI can /// tell a broken invocation from a failed metric gate (#594); the full diff --git a/big-code-analysis-web/src/web/server.rs b/big-code-analysis-web/src/web/server.rs index 4f88e5f06..8252058a9 100644 --- a/big-code-analysis-web/src/web/server.rs +++ b/big-code-analysis-web/src/web/server.rs @@ -47,14 +47,6 @@ use handlers::*; #[allow(clippy::wildcard_imports)] use routing::*; -/// `expect` message used at every `action::<_>` call site below. -/// -/// The web crate pins `big-code-analysis` with `features = -/// ["all-languages"]`, so a `LANG` value that reached this point must -/// be enabled at compile time. Any future caller that loosens the -/// feature pin must change this invariant explicitly. -const FEATURES_PINNED: &str = "web crate pins big-code-analysis features = [\"all-languages\"]"; - struct ParseConfig { /// `None` means no timeout (`parse_timeout_secs = 0`). timeout: Option, diff --git a/big-code-analysis-web/src/web/server/errors.rs b/big-code-analysis-web/src/web/server/errors.rs index 2c096e7ad..85cb3a27d 100644 --- a/big-code-analysis-web/src/web/server/errors.rs +++ b/big-code-analysis-web/src/web/server/errors.rs @@ -5,6 +5,7 @@ #![allow(clippy::wildcard_imports)] use super::*; +use big_code_analysis::MetricsError; /// `error` message returned when the submitted `file_name` (and content /// sniffing) cannot be mapped to a supported language. @@ -80,6 +81,29 @@ pub(crate) enum ParseError { } impl ParseError { + /// Maps a library [`MetricsError`] returned out of a `run_parse` + /// closure onto the existing `500` path, logging the cause + /// server-side and leaking nothing to the client. + /// + /// Replaces `.expect(FEATURES_PINNED)` at all seven handler call + /// sites (#1152). The crate pins `all-languages`, so + /// `LanguageDisabled` is unreachable today — but `MetricsError` is + /// `#[non_exhaustive]` and documents that variants may be added in a + /// *minor* release, which made the `expect` a panic scheduled + /// against a routine dependency bump. + /// + /// The client-visible status does not change: unwinding inside + /// `spawn_blocking` already arrived here as [`ParseError::Internal`] + /// via the `JoinError` arm. What changes is that the log names the + /// real cause instead of "task panicked", and no worker thread + /// unwinds to produce it. + pub(crate) fn from_metrics(payload_id: &str, err: MetricsError) -> Self { + tracing::error!(payload_id = %payload_id, error = %err, "Parse failed"); + ParseError::Internal { + id: payload_id.to_owned(), + } + } + fn id(&self) -> &str { match self { ParseError::Saturated { id } diff --git a/big-code-analysis-web/src/web/server/handlers.rs b/big-code-analysis-web/src/web/server/handlers.rs index 15ddb71ef..73ef71a08 100644 --- a/big-code-analysis-web/src/web/server/handlers.rs +++ b/big-code-analysis-web/src/web/server/handlers.rs @@ -139,11 +139,10 @@ pub(crate) async fn ast_parser( span: payload.span, }; let result = run_parse(&config, &payload_id, move || { - Ast::parse(Source::from_bytes(language, buf)) - .expect(FEATURES_PINNED) - .dump(cfg) + Ast::parse(Source::from_bytes(language, buf)).map(|ast| ast.dump(cfg)) }) - .await?; + .await? + .map_err(|err| ParseError::from_metrics(&payload_id, err))?; // `root == None` previously surfaced as a `200` carrying // `root: null` (an error signalled inside a success body); map it // to an explicit `500` with an error body instead (issue #517). @@ -189,9 +188,10 @@ pub(crate) async fn comment_removal_json( }; let language = comment_language(language); let result = run_parse(&config, &payload_id, move || { - strip_comments(language, buf, cfg).expect(FEATURES_PINNED) + strip_comments(language, buf, cfg) }) - .await?; + .await? + .map_err(|err| ParseError::from_metrics(&payload_id, err))?; // The JSON variant returns `code` as a string (#629). The request // `code` arrived as a JSON string and comment removal only deletes // byte ranges, so the stripped source stays valid UTF-8; a decode @@ -233,10 +233,9 @@ pub(crate) async fn comment_removal_plain( let language = comment_language(language); // The octet-stream variants carry no request id in the body, so log // correlation falls back to the `TracingLogger` request span. - let res = run_parse(&config, "", move || { - strip_comments(language, buf, cfg).expect(FEATURES_PINNED) - }) - .await?; + let res = run_parse(&config, "", move || strip_comments(language, buf, cfg)) + .await? + .map_err(|err| ParseError::from_metrics("", err))?; // The "no comments to strip" outcome is the empty byte // sequence; both content types report it as `200` with an empty // payload rather than the JSON variant `200` diverging from a @@ -272,9 +271,10 @@ pub(crate) async fn metrics_json( let payload_id = payload.id.clone(); let cfg = WebMetricsCfg::new(payload.id, path, payload.scope, name.to_string()); let response = run_parse(&config, &payload_id, move || { - compute_metrics(language, buf, cfg).expect(FEATURES_PINNED) + compute_metrics(language, buf, cfg) }) - .await?; + .await? + .map_err(|err| ParseError::from_metrics(&payload_id, err))?; // `None` means metric computation failed: answer with an explicit // `500` instead of the former `200`-with-`spaces: null` (issue #517). match response { @@ -383,10 +383,9 @@ pub(crate) async fn metrics_plain( if let Some(language) = language { // Same `exclude_tests` rationale as the JSON variant above. let cfg = WebMetricsCfg::new(String::new(), path, scope, name.to_string()); - let response = run_parse(&config, "", move || { - compute_metrics(language, buf, cfg).expect(FEATURES_PINNED) - }) - .await?; + let response = run_parse(&config, "", move || compute_metrics(language, buf, cfg)) + .await? + .map_err(|err| ParseError::from_metrics("", err))?; // Same error mapping as the JSON variant (issue #517); errors use // the uniform JSON body even on the octet-stream endpoint (#541). match response { @@ -421,9 +420,10 @@ pub(crate) async fn function_json( language: language.name().to_string(), }; let result = run_parse(&config, &payload_id, move || { - function_spans(language, buf, cfg).expect(FEATURES_PINNED) + function_spans(language, buf, cfg) }) - .await?; + .await? + .map_err(|err| ParseError::from_metrics(&payload_id, err))?; // `function_spans` returns a `serde_json::Value`, so the echoed // correlation id comes from the request, not the response body. Ok(negotiated_ok(&req, &result, payload_id)) @@ -447,10 +447,9 @@ pub(crate) async fn function_plain( id: String::new(), language: language.name().to_string(), }; - let result = run_parse(&config, "", move || { - function_spans(language, buf, cfg).expect(FEATURES_PINNED) - }) - .await?; + let result = run_parse(&config, "", move || function_spans(language, buf, cfg)) + .await? + .map_err(|err| ParseError::from_metrics("", err))?; Ok(negotiated_ok(&req, &result, String::new())) } else { Ok(unsupported_language(String::new())) diff --git a/big-code-analysis-web/src/web/server_tests.rs b/big-code-analysis-web/src/web/server_tests.rs index 00ddcb736..5a956db8b 100644 --- a/big-code-analysis-web/src/web/server_tests.rs +++ b/big-code-analysis-web/src/web/server_tests.rs @@ -12,6 +12,7 @@ use serde_json::value::Value; use tracing_test::traced_test; use super::*; +use big_code_analysis::MetricsError; /// Generous body limit for tests that are not exercising the 413 path. const TEST_MAX_BODY_SIZE: usize = 1_024 * 1_024 * 4; @@ -4349,3 +4350,47 @@ async fn test_cors_allow_list_same_origin_request_gets_no_headers() { "a same-origin allow-list response must still carry Vary: Origin" ); } + +/// A library parse failure escaping a `run_parse` closure becomes the +/// existing sanitized `500` rather than an `expect(FEATURES_PINNED)` +/// panic (#1152). +/// +/// Unreachable end-to-end by construction — the web crate pins +/// `all-languages`, so `LanguageDisabled` cannot be produced by a +/// request, and `MetricsError` is `#[non_exhaustive]` exactly so that a +/// future variant can be. That is why the `expect` was wrong and why +/// this asserts on the mapping directly. +/// +/// The correlation id must survive (ops correlate the 500 to the +/// request) while the cause must not (it is logged, never returned) — +/// the same split `assert_error_sanitized` pins for the other variants. +#[traced_test] +#[actix_web::test] +async fn a_library_metrics_error_maps_to_a_sanitized_internal_error() { + let err = ParseError::from_metrics("id-1152", MetricsError::LanguageDisabled(LANG::Rust)); + + assert!( + matches!(&err, ParseError::Internal { id } if id == "id-1152"), + "the correlation id must survive the mapping, got {err:?}", + ); + + let response = err.error_response(); + assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR); + + let body = actix_web::body::to_bytes(response.into_body()) + .await + .unwrap(); + let json: Value = serde_json::from_slice(&body).unwrap(); + assert_eq!(json["id"], json!("id-1152")); + assert_eq!(json["error_kind"], json!("internal_error")); + assert_eq!( + json["error"], + json!(INTERNAL_SERVER_ERROR), + "the client body must stay generic and never name the language", + ); + + // The cause is logged server-side instead, so ops can tell this 500 + // apart from a panicked task without it reaching the client. + assert!(logs_contain("Parse failed")); + assert!(logs_contain("id-1152")); +} diff --git a/docs/development/lessons_learned.md b/docs/development/lessons_learned.md index 58d40d1c4..f1bb33451 100644 --- a/docs/development/lessons_learned.md +++ b/docs/development/lessons_learned.md @@ -1065,10 +1065,9 @@ making it look deliberate. cannot be feature-gated without widening the return to a `Result` (or another error-carrying shape). Plan the widening into the same change as the feature flag — splitting into separate PRs costs an unbuildable -intermediate state. Always-pinned downstream callers can carry the -invariant with a single `const FEATURES_PINNED: &str` plus -`.expect(FEATURES_PINNED)` at every call site; defining the invariant -once is more honest than scattering identical panic literals. +intermediate state. Do **not** discharge the widened `Result` at the +call site with an `expect`, however well the invariant is documented: +propagate it onto whatever error channel the caller already has. When per-language features remove `LANG` variants from the build, the dispatch macro must still match every variant of the always-defined @@ -1086,6 +1085,24 @@ crates, where every call site became `.expect(FEATURES_PINNED)` because both pin `features = ["all-languages"]` and the disabled arm is provably unreachable. Recorded in `CHANGELOG.md` and `STABILITY.md`. +**The `expect` half of that was wrong, and #1152 removed all sixteen +call sites.** The invariant it named was real — the feature pin does +make `LanguageDisabled` unreachable — but it was the wrong invariant to +rest a panic on. `MetricsError` is `#[non_exhaustive]`, and its own +documentation reserves the right to add variants in a *minor* release, +so "provably unreachable" held only for the variants that existed on the +day it was written. A future variant would have turned a routine +dependency bump into a panic, in a library whose input is +attacker-controlled source. Naming an invariant once does not make it +load-bearing; what makes it safe is that violating it cannot panic. + +Both crates already had an error channel to propagate onto, which is the +tell that the `expect` was never necessary: the CLI's dispatch helpers +return `std::io::Result<()>` and its runner prints a per-file line and +continues, and the web handlers return `Result` +with an existing sanitized `500`. Neither needed a new failure mode — +only for the existing one to be used. + --- ## 27. Share a private walker across deprecation shims to keep them thin diff --git a/src/c_macro.rs b/src/c_macro.rs index 2d06c8e01..0f8f64dcc 100644 --- a/src/c_macro.rs +++ b/src/c_macro.rs @@ -1,3 +1,19 @@ +// This file is a hand-rolled byte lexer over attacker-controlled input: +// `code` is whatever bytes a caller handed `bca` or `bca-web`, and every +// index into it is computed rather than iterated. #126 was exactly that +// shape — `&DOLLARS[..(i - start)]` sliced a fixed 2048-byte array with +// an identifier length, so a 2049-byte macro name panicked the library. +// Enabling the lint here was validated by replaying it against the +// pre-#126 tree, where it flags both `DOLLARS` slices (#1152). +// +// The carve-outs below are per *function*, never file-wide, so a newly +// added function is covered by default — that, rather than the existing +// sites, is what the lint is here to guard. Each names the invariant +// that makes its indexing safe; a site whose bound is established in a +// *different* function is hardened with `get` instead of allowed, since +// that is the #126 shape and the one a reader cannot check locally. +#![warn(clippy::indexing_slicing)] + use std::borrow::Borrow; use std::collections::HashSet; use std::hash::Hash; @@ -41,6 +57,9 @@ fn is_identifier_starter(c: u8) -> bool { /// numeric runs, so `in_number` is false and the literal still opens /// Char correctly. #[inline] +// `i > 0` and `i + 1 < code.len()` are the two conjuncts immediately +// left of each index, and `&&` short-circuits. +#[allow(clippy::indexing_slicing)] fn is_digit_separator(code: &[u8], i: usize, in_number: bool) -> bool { in_number && i > 0 @@ -59,6 +78,9 @@ fn is_digit_separator(code: &[u8], i: usize, in_number: bool) -> bool { /// for the separator keeps the separator `'` from reaching the /// char-literal opener path. #[inline] +// `i < code.len()` for every call: `replace`'s driver loop is the sole +// caller of `step_normal`, which is the sole caller of this. +#[allow(clippy::indexing_slicing)] fn track_numeric_run(code: &[u8], i: usize, k_start: usize, in_number: &mut bool) -> bool { let c = code[i]; if c == b'\'' && is_digit_separator(code, i, *in_number) { @@ -116,6 +138,11 @@ struct MaskState { new_code: Vec, } +// `i < code.len()` from `replace`'s driver loop, its only caller. The +// two lookaheads are each guarded by `i + 1 < code.len()`, and the +// `code[start..i]` slices run from `k_start - 1`, a cursor this +// function set from an earlier iteration of that same loop. +#[allow(clippy::indexing_slicing)] fn step_normal( code: &[u8], i: usize, @@ -196,6 +223,10 @@ fn step_normal( /// delimiter (bytes between `"` and `(`) and transition to /// `RawString`. Returns the number of bytes consumed up to and /// including the `(`. +// `i` is the driver loop's cursor. `delim_end` is only dereferenced +// inside `delim_end < code.len()`, and the `>= code.len()` bail below +// covers the post-loop read. +#[allow(clippy::indexing_slicing)] fn enter_raw_string(code: &[u8], i: usize, state: &mut LexState) -> usize { debug_assert_eq!(code[i], b'"'); let delim_start = i + 1; @@ -231,6 +262,9 @@ fn is_raw_string_prefix(ident: &[u8]) -> bool { /// Step inside a `"..."` string or `'.'` char literal. `quote` is the /// terminating byte; backslash-escapes (including line continuations) /// are consumed in one step so `\"` / `\'` do not exit the literal. +// `i < code.len()` from the driver loop; the lookahead is guarded by +// `i + 1 < code.len()`. +#[allow(clippy::indexing_slicing)] fn step_quoted(code: &[u8], i: usize, quote: u8, state: &mut LexState) -> usize { let c = code[i]; if c == b'\\' && i + 1 < code.len() { @@ -242,6 +276,9 @@ fn step_quoted(code: &[u8], i: usize, quote: u8, state: &mut LexState) -> usize 1 } +// `i < code.len()` from the driver loop; the lookahead is guarded by +// `i + 1 < code.len()`. +#[allow(clippy::indexing_slicing)] fn step_line_comment(code: &[u8], i: usize, state: &mut LexState) -> usize { let c = code[i]; if c == b'\\' && i + 1 < code.len() && code[i + 1] == b'\n' { @@ -255,6 +292,9 @@ fn step_line_comment(code: &[u8], i: usize, state: &mut LexState) -> usize { 1 } +// `i < code.len()` from the driver loop; the lookahead is guarded by +// `i + 1 < code.len()`. +#[allow(clippy::indexing_slicing)] fn step_block_comment(code: &[u8], i: usize, state: &mut LexState) -> usize { if code[i] == b'*' && i + 1 < code.len() && code[i + 1] == b'/' { *state = LexState::Normal; @@ -263,6 +303,10 @@ fn step_block_comment(code: &[u8], i: usize, state: &mut LexState) -> usize { 1 } +// `i < code.len()` from the driver loop. The delimiter comparison is +// deliberately *not* covered by this allow — see the `get` calls in the +// body and the comment on them. +#[allow(clippy::indexing_slicing)] fn step_raw_string( code: &[u8], i: usize, @@ -274,9 +318,15 @@ fn step_raw_string( // happens inside, so this is a literal match. if code[i] == b')' { let close_quote = i + 1 + delim_len; - if close_quote < code.len() - && code[close_quote] == b'"' - && code[i + 1..close_quote] == code[delim_start..delim_start + delim_len] + // `delim_start` / `delim_len` were computed by `enter_raw_string` + // and carried here through `LexState::RawString`, so unlike every + // other bound in this file theirs cannot be checked by reading + // this function — the #126 shape. `get` rather than a slice index + // keeps a drifted delimiter a missed close rather than a panic. + // The `Some(&b'"')` test also proves the `i + 1..close_quote` + // lookup is in range, so the two `None`s can never compare equal. + if code.get(close_quote) == Some(&b'"') + && code.get(i + 1..close_quote) == code.get(delim_start..delim_start + delim_len) { *state = LexState::Normal; return close_quote - i + 1; @@ -285,6 +335,12 @@ fn step_raw_string( 1 } +// The trailing-identifier block slices `start..end` where `end` is +// `code.len()` and `start` is `k_start - 1`, a cursor `step_normal` +// only ever sets to an index it has already read. `code_start` is +// likewise a position reached by the loop, so `code[code_start..]` is +// in range, and empty rather than out of bounds at `code.len()`. +#[allow(clippy::indexing_slicing)] pub(crate) fn replace( code: &[u8], macros: &HashSet, diff --git a/src/metrics/loc.rs b/src/metrics/loc.rs index b107d1e08..b9177f455 100644 --- a/src/metrics/loc.rs +++ b/src/metrics/loc.rs @@ -21,6 +21,20 @@ clippy::cast_possible_truncation, clippy::cast_sign_loss )] +// `Loc` is the one metric that computes on tree-sitter *span +// coordinates* rather than on its own accumulators, and a row index +// arriving from a parse is attacker-controlled through the source +// layout. #1051 was a `usize` underflow of exactly this shape — a Rust +// doc comment at EOF drove `end - 1` below zero, panicking in debug and +// wrapping to `usize::MAX` in release, from an input as small as +// `/// x`. Warning here forces every span adjustment to be explicitly +// saturating, checked, or annotated, rather than relying on a bound +// that holds only until a grammar changes shape. +// +// Deliberately scoped to `loc` rather than to `src/metrics/`: the other +// metrics contribute 244 hits, all of them `+=` on their own counters, +// which is not this bug class and would bury it (#1152). +#![warn(clippy::arithmetic_side_effects)] use crate::checker::Checker; use crate::metrics::npa::python_is_block; @@ -55,10 +69,20 @@ fn min_or_zero(v: usize) -> u64 { /// does (false for Perl, whose last `sub` gained one). /// /// Requires `end_line >= start_row`, which tree-sitter guarantees for a -/// single node's own span. +/// single node's own span. The `debug_assert` pins that in tests; the +/// `saturating_sub` decides what release does if it is ever violated +/// anyway. Zero is the right answer there — an inverted span covers no +/// rows — and it is emphatically better than wrapping, because the only +/// consumer is `Sloc::excluded_lines`, which is later *subtracted* from +/// a row count: a wrapped `usize::MAX` would propagate into SLOC and MI +/// as garbage far from its cause, which is precisely how #1051 surfaced. #[inline] fn span_rows(start_row: usize, end_line: usize) -> usize { - end_line - start_row + debug_assert!( + end_line >= start_row, + "span_rows: end_line {end_line} < start_row {start_row}" + ); + end_line.saturating_sub(start_row) } mod line_set; @@ -128,7 +152,9 @@ impl Sloc { /// without an interval merge (issue #722). #[inline] pub(crate) fn exclude_span(&mut self, start_row: usize, end_line: usize) { - self.excluded_lines += span_rows(start_row, end_line); + self.excluded_lines = self + .excluded_lines + .saturating_add(span_rows(start_row, end_line)); } /// The `Sloc` metric minimum value. See `min_or_zero` for the @@ -170,7 +196,7 @@ impl Sloc { // double-count: pruned subtrees never descend, so a nested pruned // item is recorded on a single space and folded up one altitude at // a time. - self.excluded_lines += other.excluded_lines; + self.excluded_lines = self.excluded_lines.saturating_add(other.excluded_lines); } #[inline] @@ -392,11 +418,25 @@ impl Lloc { self.lloc_max as u64 } + /// Records one logical statement. + /// + /// Exists so the 23 per-language `Loc` impls name the operation + /// instead of each reaching into a private field, which also keeps + /// the module's `arithmetic_side_effects` carve-out to this one + /// line rather than 36 of them (#1152). Saturating is unreachable — + /// the count is bounded by the AST's node count — and is the right + /// answer if it ever were: a pinned `usize::MAX` is a visibly broken + /// LLOC, where a wrap to 0 reads as a legitimately empty space. + #[inline] + pub(crate) fn count_logical_line(&mut self) { + self.logical_lines = self.logical_lines.saturating_add(1); + } + /// Folds `other` into `self`, summing statement counts and updating min/max. #[inline] pub fn merge(&mut self, other: &Lloc) { // Merge lloc lines - self.logical_lines += other.logical_lines; + self.logical_lines = self.logical_lines.saturating_add(other.logical_lines); // Fold the child's own min/max so nested spaces propagate (#437). self.lloc_min = self.lloc_min.min(other.lloc_min); self.lloc_max = self.lloc_max.max(other.lloc_max); @@ -479,20 +519,24 @@ impl Stats { stats.sloc.start = 0; // `end_row + 1`: the synthetic span models a real one ending // mid-line, so the final row counts and `sloc == sloc_end_row + 1`. - stats.sloc.end_line = sloc_end_row + 1; + stats.sloc.end_line = sloc_end_row.saturating_add(1); // Inject `code_comment_lines` distinct synthetic code-comment // rows. An offset past `sloc_end_row` keeps them disjoint from // any real span row, so `cloc()` (the set's cardinality) equals // the requested count without colliding with sloc attribution. if code_comment_lines > 0 { - let synthetic_base = sloc_end_row + 1; + let synthetic_base = sloc_end_row.saturating_add(1); // Explicit rather than leaning on `insert_range`'s inverted-span // guard: that guard exists to survive a bug, not to serve as a - // caller's empty case. + // caller's empty case. The `- 1` is exact under the `> 0` test + // above, which is also what makes the inclusive end well-formed. + let synthetic_end = synthetic_base + .saturating_add(code_comment_lines) + .saturating_sub(1); stats .cloc .code_comment_line_starts - .insert_range(synthetic_base, synthetic_base + code_comment_lines - 1); + .insert_range(synthetic_base, synthetic_end); } stats } @@ -505,7 +549,7 @@ impl Stats { self.lloc.merge(&other.lloc); // Count spaces - self.space_count += other.space_count; + self.space_count = self.space_count.saturating_add(other.space_count); // Fold the child's own min/max so nested spaces propagate (#437). self.blank_min = self.blank_min.min(other.blank_min); diff --git a/src/metrics/loc/bash.rs b/src/metrics/loc/bash.rs index a43f4c4ec..2e07fc37a 100644 --- a/src/metrics/loc/bash.rs +++ b/src/metrics/loc/bash.rs @@ -30,7 +30,7 @@ impl Loc for BashCode { Command | VariableAssignment | DeclarationCommand | UnsetCommand | IfStatement | ForStatement | CStyleForStatement | WhileStatement | CaseStatement | FunctionDefinition => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } _ => { if node.child_count() == 0 { diff --git a/src/metrics/loc/c.rs b/src/metrics/loc/c.rs index 199b0ea2f..ddb27630e 100644 --- a/src/metrics/loc/c.rs +++ b/src/metrics/loc/c.rs @@ -35,7 +35,7 @@ impl Loc for CCode { | ReturnStatement | BreakStatement | ContinueStatement | GotoStatement | ExpressionStatement | ExpressionStatement2 | LabeledStatement | StatementIdentifier => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } Declaration => { if node.count_specific_ancestors::( @@ -49,7 +49,7 @@ impl Loc for CCode { |node| node.kind_id() == CompoundStatement, ) == 0 { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } } _ => { @@ -60,7 +60,7 @@ impl Loc for CCode { // `tree-sitter-cpp` doesn't expand macros, providing a single `PreprocArg` node for the entire macro argument. // Therefore, all lines from `start_row` to `end_row` must be added to PLOC to account for the unexpanded macro content if let PreprocArg = node.kind_id().into() { - (node.start_row() + 1..=node.end_row()).for_each(|line| { + (node.start_row().saturating_add(1)..=node.end_row()).for_each(|line| { stats.ploc.lines.insert(line); }); } diff --git a/src/metrics/loc/cpp.rs b/src/metrics/loc/cpp.rs index 2866d656b..cc1c5b526 100644 --- a/src/metrics/loc/cpp.rs +++ b/src/metrics/loc/cpp.rs @@ -34,7 +34,7 @@ impl Loc for CppCode { | ReturnStatement | BreakStatement | ContinueStatement | GotoStatement | ThrowStatement | TryStatement | TryStatement2 | ExpressionStatement | ExpressionStatement2 | LabeledStatement | StatementIdentifier => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } Declaration => { if node.count_specific_ancestors::( @@ -48,7 +48,7 @@ impl Loc for CppCode { |node| node.kind_id() == CompoundStatement, ) == 0 { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } } _ => { @@ -59,7 +59,7 @@ impl Loc for CppCode { // `tree-sitter-cpp` doesn't expand macros, providing a single `PreprocArg` node for the entire macro argument. // Therefore, all lines from `start_row` to `end_row` must be added to PLOC to account for the unexpanded macro content if let PreprocArg = node.kind_id().into() { - (node.start_row() + 1..=node.end_row()).for_each(|line| { + (node.start_row().saturating_add(1)..=node.end_row()).for_each(|line| { stats.ploc.lines.insert(line); }); } diff --git a/src/metrics/loc/csharp.rs b/src/metrics/loc/csharp.rs index f040d8aad..5921101dd 100644 --- a/src/metrics/loc/csharp.rs +++ b/src/metrics/loc/csharp.rs @@ -35,7 +35,7 @@ impl Loc for CsharpCode { | GotoStatement | IfStatement | LabeledStatement | LockStatement | ReturnStatement | SwitchStatement | ThrowStatement | TryStatement | UnsafeStatement | UsingStatement | WhileStatement | YieldStatement => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } LocalDeclarationStatement => { // Variable declarations inside a `for_statement` init/condition/update @@ -47,7 +47,7 @@ impl Loc for CsharpCode { |n| n.kind_id() == Block, ) == 0 { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } } _ => { diff --git a/src/metrics/loc/elixir.rs b/src/metrics/loc/elixir.rs index 6dd6b2015..ddc4cb9e4 100644 --- a/src/metrics/loc/elixir.rs +++ b/src/metrics/loc/elixir.rs @@ -62,7 +62,7 @@ impl Loc for ElixirCode { ) }) { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } if node.child_count() == 0 { check_comment_ends_on_code_line(stats, start); diff --git a/src/metrics/loc/go.rs b/src/metrics/loc/go.rs index 0492b8977..b25e1f5bf 100644 --- a/src/metrics/loc/go.rs +++ b/src/metrics/loc/go.rs @@ -45,7 +45,7 @@ impl Loc for GoCode { | G::TypeSwitchStatement | G::SelectStatement | G::LabeledStatement => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } G::ExpressionStatement | G::SendStatement @@ -65,7 +65,7 @@ impl Loc for GoCode { |n| n.kind_id() == G::Block, ) == 0 { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } } _ => { diff --git a/src/metrics/loc/groovy.rs b/src/metrics/loc/groovy.rs index 8e187ee69..50d653981 100644 --- a/src/metrics/loc/groovy.rs +++ b/src/metrics/loc/groovy.rs @@ -59,7 +59,7 @@ impl Loc for GroovyCode { | DoWhileStatement | ExpressionStatement | ForInStatement | ForStatement | IfStatement | PipelineStatement | ReturnStatement | SwitchExpression | ThrowStatement | TryStatement | WhileStatement | YieldStatement => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } LocalVariableDeclaration => { if node.count_specific_ancestors::( @@ -70,7 +70,7 @@ impl Loc for GroovyCode { { // Skip the initializer slot of a classic `for` loop — // same reason as Java's impl. - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } } _ => { diff --git a/src/metrics/loc/irules.rs b/src/metrics/loc/irules.rs index 8a29c1c64..27bcb34cd 100644 --- a/src/metrics/loc/irules.rs +++ b/src/metrics/loc/irules.rs @@ -55,7 +55,7 @@ impl Loc for IrulesCode { | Irules::Try | Irules::Catch | Irules::Regexp => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } // `expr` and a bare command are logical lines at statement @@ -70,7 +70,7 @@ impl Loc for IrulesCode { .parent(node) .is_none_or(|p| p.kind_id() != Irules::CommandSubstitution) => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } _ => { diff --git a/src/metrics/loc/java.rs b/src/metrics/loc/java.rs index 679219baf..c00474825 100644 --- a/src/metrics/loc/java.rs +++ b/src/metrics/loc/java.rs @@ -36,7 +36,7 @@ impl Loc for JavaCode { | EnhancedForStatement | ExpressionStatement | ForStatement | IfStatement | ReturnStatement | SwitchExpression | ThrowStatement | TryStatement | WhileStatement => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } LocalVariableDeclaration => { if node.count_specific_ancestors::( @@ -48,7 +48,7 @@ impl Loc for JavaCode { // The initializer, condition, and increment in a for loop are expressions. // Don't count the variable declaration if in a ForStatement. // https://docs.oracle.com/javase/tutorial/java/nutsandbolts/for.html - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } } _ => { diff --git a/src/metrics/loc/javascript.rs b/src/metrics/loc/javascript.rs index 600b11405..4dc9e8960 100644 --- a/src/metrics/loc/javascript.rs +++ b/src/metrics/loc/javascript.rs @@ -37,7 +37,7 @@ impl Loc for JavascriptCode { | TryStatement | WithStatement | BreakStatement | ContinueStatement | DebuggerStatement | ReturnStatement | ThrowStatement | EmptyStatement | StatementIdentifier => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } _ => { check_comment_ends_on_code_line(stats, start); diff --git a/src/metrics/loc/kotlin.rs b/src/metrics/loc/kotlin.rs index a776bd952..ad3d81e45 100644 --- a/src/metrics/loc/kotlin.rs +++ b/src/metrics/loc/kotlin.rs @@ -33,7 +33,7 @@ impl Loc for KotlinCode { ForStatement | WhileStatement | DoWhileStatement | IfExpression | WhenExpression | TryExpression | ThrowExpression | ReturnExpression | Assignment | PropertyDeclaration => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } // Bare expression statements (e.g. `println(x)`) have no // ExpressionStatement wrapper in tree-sitter-kotlin-ng. Count @@ -47,7 +47,7 @@ impl Loc for KotlinCode { Block | FunctionBody | SourceFile | CatchBlock | FinallyBlock ) { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } else { check_comment_ends_on_code_line(stats, start); stats.ploc.lines.insert(start); diff --git a/src/metrics/loc/line_set.rs b/src/metrics/loc/line_set.rs index 6f2add73a..4b11158b3 100644 --- a/src/metrics/loc/line_set.rs +++ b/src/metrics/loc/line_set.rs @@ -49,6 +49,24 @@ //! place of a probe per row per nesting level) is paid for by every //! space. +// The enclosing module warns on `arithmetic_side_effects` because its +// arithmetic is on tree-sitter span coordinates (#1051, #1152). This +// file is the deliberate carve-out: its arithmetic is on *word indices +// into `words`*, and every site is bounds-established at the point of +// use rather than by a property of the input — `reserve` runs before +// each subtraction in `insert`/`insert_range`/`union_with`, `slot` and +// `word` already use `checked_sub`, and `insert_range` returns early on +// an inverted span. +// +// Making these saturating would be actively worse than leaving them +// checked. `self.words[word - self.first_word]` saturating to index 0 +// reads or writes *the wrong row's word* and silently miscounts the +// metric; the current form panics on a corrupt offset, which is how +// `intersection_len`'s comment says it is meant to fail. Prevention and +// masking point in opposite directions here, so the lint is off rather +// than satisfied. +#![allow(clippy::arithmetic_side_effects)] + use std::fmt; /// Bits per element of [`LineSet::words`]. diff --git a/src/metrics/loc/lua.rs b/src/metrics/loc/lua.rs index 21b97adbb..8a88a3f3c 100644 --- a/src/metrics/loc/lua.rs +++ b/src/metrics/loc/lua.rs @@ -57,7 +57,7 @@ impl Loc for LuaCode { ) }) => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } Lua::IfStatement @@ -75,7 +75,7 @@ impl Loc for LuaCode { | Lua::FunctionDeclaration | Lua::FunctionDeclaration2 | Lua::FunctionDeclaration3 => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } _ => { diff --git a/src/metrics/loc/mozcpp.rs b/src/metrics/loc/mozcpp.rs index be4c7cae7..bb056feeb 100644 --- a/src/metrics/loc/mozcpp.rs +++ b/src/metrics/loc/mozcpp.rs @@ -34,7 +34,7 @@ impl Loc for MozcppCode { | ReturnStatement | BreakStatement | ContinueStatement | GotoStatement | ThrowStatement | TryStatement | TryStatement2 | ExpressionStatement | ExpressionStatement2 | LabeledStatement | StatementIdentifier => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } Declaration => { if node.count_specific_ancestors::( @@ -48,7 +48,7 @@ impl Loc for MozcppCode { |node| node.kind_id() == CompoundStatement, ) == 0 { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } } _ => { @@ -59,7 +59,7 @@ impl Loc for MozcppCode { // `tree-sitter-cpp` doesn't expand macros, providing a single `PreprocArg` node for the entire macro argument. // Therefore, all lines from `start_row` to `end_row` must be added to PLOC to account for the unexpanded macro content if let PreprocArg = node.kind_id().into() { - (node.start_row() + 1..=node.end_row()).for_each(|line| { + (node.start_row().saturating_add(1)..=node.end_row()).for_each(|line| { stats.ploc.lines.insert(line); }); } diff --git a/src/metrics/loc/mozjs.rs b/src/metrics/loc/mozjs.rs index 8fa5d0c6d..bce0c4db5 100644 --- a/src/metrics/loc/mozjs.rs +++ b/src/metrics/loc/mozjs.rs @@ -43,7 +43,7 @@ impl Loc for MozjsCode { | TryStatement | WithStatement | BreakStatement | ContinueStatement | DebuggerStatement | ReturnStatement | ThrowStatement | EmptyStatement | StatementIdentifier => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } _ => { check_comment_ends_on_code_line(stats, start); diff --git a/src/metrics/loc/objc.rs b/src/metrics/loc/objc.rs index 15ab4992d..5772d6664 100644 --- a/src/metrics/loc/objc.rs +++ b/src/metrics/loc/objc.rs @@ -55,7 +55,7 @@ impl Loc for ObjcCode { | ExpressionStatement2 | LabeledStatement | StatementIdentifier => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } Declaration => { // A declaration in a `for`/`while`/`if` *header* (not its @@ -73,7 +73,7 @@ impl Loc for ObjcCode { |node| node.kind_id() == CompoundStatement, ) == 0 { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } } _ => { @@ -84,7 +84,7 @@ impl Loc for ObjcCode { // macro handling: a single `PreprocArg` node spans the // whole macro argument, so every line it covers is PLOC. if let PreprocArg = node.kind_id().into() { - (node.start_row() + 1..=node.end_row()).for_each(|line| { + (node.start_row().saturating_add(1)..=node.end_row()).for_each(|line| { stats.ploc.lines.insert(line); }); } diff --git a/src/metrics/loc/perl.rs b/src/metrics/loc/perl.rs index e70386699..84c582278 100644 --- a/src/metrics/loc/perl.rs +++ b/src/metrics/loc/perl.rs @@ -73,7 +73,7 @@ impl Loc for PerlCode { | P::UseParentStatement | P::UseNoVersion | P::EllipsisStatement => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } P::SEMI => { // A `;` at top of `source_file` / a function `block` ends a @@ -84,7 +84,7 @@ impl Loc for PerlCode { if let Some(parent) = ancestors.parent(node) && matches!(parent.kind_id().into(), P::SourceFile | P::Block) { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } check_comment_ends_on_code_line(stats, start); stats.ploc.lines.insert(start); diff --git a/src/metrics/loc/php.rs b/src/metrics/loc/php.rs index ba1c220b2..79bfe3ab4 100644 --- a/src/metrics/loc/php.rs +++ b/src/metrics/loc/php.rs @@ -56,7 +56,7 @@ impl Loc for PhpCode { | ConstDeclaration2 | PropertyDeclaration | NamedLabelStatement => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } _ => { check_comment_ends_on_code_line(stats, start); diff --git a/src/metrics/loc/python.rs b/src/metrics/loc/python.rs index 7c94d5c3f..c6343034c 100644 --- a/src/metrics/loc/python.rs +++ b/src/metrics/loc/python.rs @@ -54,7 +54,7 @@ impl Loc for PythonCode { check_comment_ends_on_code_line(stats, start); stats.ploc.lines.insert(start); } - (start + 1..=end).for_each(|line| { + (start.saturating_add(1)..=end).for_each(|line| { stats.ploc.lines.insert(line); }); } @@ -93,7 +93,7 @@ impl Loc for PythonCode { | NonlocalStatement | ExecStatement | ExpressionStatement => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } _ => { check_comment_ends_on_code_line(stats, start); diff --git a/src/metrics/loc/ruby.rs b/src/metrics/loc/ruby.rs index 921dbb70b..3838eb220 100644 --- a/src/metrics/loc/ruby.rs +++ b/src/metrics/loc/ruby.rs @@ -73,7 +73,7 @@ impl Loc for RubyCode { | R::Undef | R::Alias | R::EmptyStatement => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } _ => { check_comment_ends_on_code_line(stats, start); diff --git a/src/metrics/loc/rust.rs b/src/metrics/loc/rust.rs index 0417a0946..6b0dd2563 100644 --- a/src/metrics/loc/rust.rs +++ b/src/metrics/loc/rust.rs @@ -51,8 +51,12 @@ impl Loc for RustCode { // // Cheap operand first: `end > start` is false for every plain // line comment, short-circuiting `is_child`'s child walk. + // + // `saturating_sub` is exact under that guard — `end > start` + // and `start >= 0` give `end >= 1` — and is belt-and-braces + // on the one line in this crate known to have underflowed. let end = if end > start && node.is_child(DocComment as u16) { - end - 1 + end.saturating_sub(1) } else { end }; @@ -64,7 +68,7 @@ impl Loc for RustCode { | LetDeclaration | AssignmentExpression | CompoundAssignmentExpr => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } _ => { check_comment_ends_on_code_line(stats, start); diff --git a/src/metrics/loc/shared.rs b/src/metrics/loc/shared.rs index a82dce0c2..0d23146c7 100644 --- a/src/metrics/loc/shared.rs +++ b/src/metrics/loc/shared.rs @@ -87,9 +87,14 @@ pub(crate) fn init(node: &Node, stats: &mut Stats, is_func_space: bool) -> (usiz // whose own arithmetic already wrapped, because `(0, usize::MAX)` // satisfies `end >= start`. The real guard against that is refusing to // underflow in the first place, at the site that adjusts the span. +// +// What the assert does not cover, the `saturating_sub` below does: +// an inverted span yields a zero-row comment rather than wrapping to +// `usize::MAX` and reaching `add_only_comment_lines` as a span that +// `LineSet::insert_range` would then have to reject (#1152). pub(crate) fn add_cloc_lines(stats: &mut Stats, start: usize, end: usize) { debug_assert!(end >= start, "add_cloc_lines: end {end} < start {start}"); - let comment_diff = end - start; + let comment_diff = end.saturating_sub(start); let is_comment_after_code_line = stats.ploc.lines.contains(start); if is_comment_after_code_line && comment_diff == 0 { // A comment is *entirely* next to a code line @@ -98,7 +103,7 @@ pub(crate) fn add_cloc_lines(stats: &mut Stats, start: usize, end: usize) { // A block comment that starts next to a code line and ends on // independent lines. add_code_comment_line(stats, start); - add_only_comment_lines(stats, start + 1, end); + add_only_comment_lines(stats, start.saturating_add(1), end); } else { // A comment on an independent line AND // a block comment on independent lines OR @@ -182,7 +187,7 @@ pub(crate) fn add_multiline_string_ploc( check_comment_ends_on_code_line(stats, start); stats.ploc.lines.insert(start); } - (start + 1..=end).for_each(|line| { + (start.saturating_add(1)..=end).for_each(|line| { stats.ploc.lines.insert(line); }); } diff --git a/src/metrics/loc/tcl.rs b/src/metrics/loc/tcl.rs index 4a86b3b25..1c5c8a41c 100644 --- a/src/metrics/loc/tcl.rs +++ b/src/metrics/loc/tcl.rs @@ -42,7 +42,7 @@ impl Loc for TclCode { | Tcl::Try | Tcl::Catch | Tcl::Regexp => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } // `expr` and a bare command are logical lines at statement @@ -57,7 +57,7 @@ impl Loc for TclCode { .parent(node) .is_none_or(|p| p.kind_id() != Tcl::CommandSubstitution) => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } _ => { diff --git a/src/metrics/loc/tsx.rs b/src/metrics/loc/tsx.rs index a33b9c436..2a4a2c548 100644 --- a/src/metrics/loc/tsx.rs +++ b/src/metrics/loc/tsx.rs @@ -37,7 +37,7 @@ impl Loc for TsxCode { | TryStatement | WithStatement | BreakStatement | ContinueStatement | DebuggerStatement | ReturnStatement | ThrowStatement | EmptyStatement | StatementIdentifier => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } _ => { check_comment_ends_on_code_line(stats, start); diff --git a/src/metrics/loc/typescript.rs b/src/metrics/loc/typescript.rs index 3379cfcbc..6a7762016 100644 --- a/src/metrics/loc/typescript.rs +++ b/src/metrics/loc/typescript.rs @@ -37,7 +37,7 @@ impl Loc for TypescriptCode { | TryStatement | WithStatement | BreakStatement | ContinueStatement | DebuggerStatement | ReturnStatement | ThrowStatement | EmptyStatement | StatementIdentifier => { - stats.lloc.logical_lines += 1; + stats.lloc.count_logical_line(); } _ => { check_comment_ends_on_code_line(stats, start); From af5d36900d8621bac21568d79fa36cd962a52283 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Fri, 7 Aug 2026 09:27:34 -0700 Subject: [PATCH 02/10] refactor(web): fold the parse-error mapping into run_parse_fallible The seven handlers each repeated `.await?.map_err(ParseError::from_metrics)?` after dropping their `expect(FEATURES_PINNED)`. That is seven chances to reach for `expect` again instead of propagating, which is the thing #1152 was cleaning up. One wrapper leaves each handler with a single `?`. The three VCS handlers keep plain `run_parse`: their closures do not return `MetricsError`. --- big-code-analysis-web/src/web/server.rs | 20 +++++++++++ .../src/web/server/errors.rs | 5 +-- .../src/web/server/handlers.rs | 35 ++++++++----------- 3 files changed, 37 insertions(+), 23 deletions(-) diff --git a/big-code-analysis-web/src/web/server.rs b/big-code-analysis-web/src/web/server.rs index 8252058a9..d6eea26fb 100644 --- a/big-code-analysis-web/src/web/server.rs +++ b/big-code-analysis-web/src/web/server.rs @@ -47,6 +47,8 @@ use handlers::*; #[allow(clippy::wildcard_imports)] use routing::*; +use big_code_analysis::MetricsError; + struct ParseConfig { /// `None` means no timeout (`parse_timeout_secs = 0`). timeout: Option, @@ -80,6 +82,24 @@ const MAX_BODY_SIZE: usize = 1_024 * 1_024 * 4; /// with `tracing-actix-web`'s own span-level `request_id` (a per-request /// UUID), and it is echoed back in the `{error, id}` body of the typed /// [`ParseError`] returned on every failure (#639). +/// [`run_parse`] for a closure that can itself fail. +/// +/// The library entry points the handlers call all return +/// `Result<_, MetricsError>`, so without this every call site repeats +/// the same `.await?.map_err(ParseError::from_metrics)?` pair — seven +/// chances to reach for `expect` instead, which is what #1152 was +/// cleaning up. Folding it here leaves each handler with a single `?` +/// and makes propagation the path of least resistance. +async fn run_parse_fallible( + config: &web::Data, + payload_id: &str, + f: impl FnOnce() -> Result + Send + 'static, +) -> Result { + run_parse(config, payload_id, f) + .await? + .map_err(|err| ParseError::from_metrics(payload_id, err)) +} + async fn run_parse( config: &web::Data, payload_id: &str, diff --git a/big-code-analysis-web/src/web/server/errors.rs b/big-code-analysis-web/src/web/server/errors.rs index 85cb3a27d..9c966d63d 100644 --- a/big-code-analysis-web/src/web/server/errors.rs +++ b/big-code-analysis-web/src/web/server/errors.rs @@ -85,8 +85,9 @@ impl ParseError { /// closure onto the existing `500` path, logging the cause /// server-side and leaking nothing to the client. /// - /// Replaces `.expect(FEATURES_PINNED)` at all seven handler call - /// sites (#1152). The crate pins `all-languages`, so + /// Reached through [`run_parse_fallible`], which replaced + /// `.expect(FEATURES_PINNED)` at all seven handler call sites + /// (#1152). The crate pins `all-languages`, so /// `LanguageDisabled` is unreachable today — but `MetricsError` is /// `#[non_exhaustive]` and documents that variants may be added in a /// *minor* release, which made the `expect` a panic scheduled diff --git a/big-code-analysis-web/src/web/server/handlers.rs b/big-code-analysis-web/src/web/server/handlers.rs index 73ef71a08..cfbb9b37f 100644 --- a/big-code-analysis-web/src/web/server/handlers.rs +++ b/big-code-analysis-web/src/web/server/handlers.rs @@ -138,11 +138,10 @@ pub(crate) async fn ast_parser( comment: payload.comment, span: payload.span, }; - let result = run_parse(&config, &payload_id, move || { + let result = run_parse_fallible(&config, &payload_id, move || { Ast::parse(Source::from_bytes(language, buf)).map(|ast| ast.dump(cfg)) }) - .await? - .map_err(|err| ParseError::from_metrics(&payload_id, err))?; + .await?; // `root == None` previously surfaced as a `200` carrying // `root: null` (an error signalled inside a success body); map it // to an explicit `500` with an error body instead (issue #517). @@ -187,11 +186,10 @@ pub(crate) async fn comment_removal_json( language: language.name().to_string(), }; let language = comment_language(language); - let result = run_parse(&config, &payload_id, move || { + let result = run_parse_fallible(&config, &payload_id, move || { strip_comments(language, buf, cfg) }) - .await? - .map_err(|err| ParseError::from_metrics(&payload_id, err))?; + .await?; // The JSON variant returns `code` as a string (#629). The request // `code` arrived as a JSON string and comment removal only deletes // byte ranges, so the stripped source stays valid UTF-8; a decode @@ -233,9 +231,8 @@ pub(crate) async fn comment_removal_plain( let language = comment_language(language); // The octet-stream variants carry no request id in the body, so log // correlation falls back to the `TracingLogger` request span. - let res = run_parse(&config, "", move || strip_comments(language, buf, cfg)) - .await? - .map_err(|err| ParseError::from_metrics("", err))?; + let res = + run_parse_fallible(&config, "", move || strip_comments(language, buf, cfg)).await?; // The "no comments to strip" outcome is the empty byte // sequence; both content types report it as `200` with an empty // payload rather than the JSON variant `200` diverging from a @@ -270,11 +267,10 @@ pub(crate) async fn metrics_json( // request payload and chain `.with_exclude_tests(...)` here. let payload_id = payload.id.clone(); let cfg = WebMetricsCfg::new(payload.id, path, payload.scope, name.to_string()); - let response = run_parse(&config, &payload_id, move || { + let response = run_parse_fallible(&config, &payload_id, move || { compute_metrics(language, buf, cfg) }) - .await? - .map_err(|err| ParseError::from_metrics(&payload_id, err))?; + .await?; // `None` means metric computation failed: answer with an explicit // `500` instead of the former `200`-with-`spaces: null` (issue #517). match response { @@ -383,9 +379,8 @@ pub(crate) async fn metrics_plain( if let Some(language) = language { // Same `exclude_tests` rationale as the JSON variant above. let cfg = WebMetricsCfg::new(String::new(), path, scope, name.to_string()); - let response = run_parse(&config, "", move || compute_metrics(language, buf, cfg)) - .await? - .map_err(|err| ParseError::from_metrics("", err))?; + let response = + run_parse_fallible(&config, "", move || compute_metrics(language, buf, cfg)).await?; // Same error mapping as the JSON variant (issue #517); errors use // the uniform JSON body even on the octet-stream endpoint (#541). match response { @@ -419,11 +414,10 @@ pub(crate) async fn function_json( id: payload.id, language: language.name().to_string(), }; - let result = run_parse(&config, &payload_id, move || { + let result = run_parse_fallible(&config, &payload_id, move || { function_spans(language, buf, cfg) }) - .await? - .map_err(|err| ParseError::from_metrics(&payload_id, err))?; + .await?; // `function_spans` returns a `serde_json::Value`, so the echoed // correlation id comes from the request, not the response body. Ok(negotiated_ok(&req, &result, payload_id)) @@ -447,9 +441,8 @@ pub(crate) async fn function_plain( id: String::new(), language: language.name().to_string(), }; - let result = run_parse(&config, "", move || function_spans(language, buf, cfg)) - .await? - .map_err(|err| ParseError::from_metrics("", err))?; + let result = + run_parse_fallible(&config, "", move || function_spans(language, buf, cfg)).await?; Ok(negotiated_ok(&req, &result, String::new())) } else { Ok(unsupported_language(String::new())) From dba1a479e8a0e0ee733f85f32cbc416245abde9b Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Fri, 7 Aug 2026 09:33:10 -0700 Subject: [PATCH 03/10] refactor(metrics/loc): narrow count_logical_line to the module subtree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Its callers are the per-language `loc/*.rs` modules, which are descendants of `loc` and so can already see a private item — the same reason they could write to the private `logical_lines` field before. `pub(crate)` claimed a wider surface than the helper has. --- src/metrics/loc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/metrics/loc.rs b/src/metrics/loc.rs index b9177f455..09845df21 100644 --- a/src/metrics/loc.rs +++ b/src/metrics/loc.rs @@ -428,7 +428,7 @@ impl Lloc { /// answer if it ever were: a pinned `usize::MAX` is a visibly broken /// LLOC, where a wrap to 0 reads as a legitimately empty space. #[inline] - pub(crate) fn count_logical_line(&mut self) { + fn count_logical_line(&mut self) { self.logical_lines = self.logical_lines.saturating_add(1); } From a0925a7fcbdfd1a0c9bc65400f6c9f38e0686102 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Fri, 7 Aug 2026 09:38:56 -0700 Subject: [PATCH 04/10] docs(metrics/loc): correct two overstated saturating-guard comments Both claimed more than the change buys, found in review. `add_cloc_lines`: `comment_diff` feeds only the two branch tests, never a row index, so both branches already hand `add_only_comment_lines` the raw span and `insert_range` already rejects an inversion. The `saturating_sub` changes which branch runs, not whether the rejection happens. `span_rows`: `sloc()` already clamps with `saturating_sub`, so a wrapped `excluded_lines` could never have escaped as `usize::MAX`. It would have escaped as `sloc: 0` for a non-empty file. Same class as the follow-up the #1051 fix itself needed on this function. --- src/metrics/loc.rs | 9 +++++---- src/metrics/loc/shared.rs | 12 ++++++++---- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/metrics/loc.rs b/src/metrics/loc.rs index 09845df21..8eabf889b 100644 --- a/src/metrics/loc.rs +++ b/src/metrics/loc.rs @@ -72,10 +72,11 @@ fn min_or_zero(v: usize) -> u64 { /// single node's own span. The `debug_assert` pins that in tests; the /// `saturating_sub` decides what release does if it is ever violated /// anyway. Zero is the right answer there — an inverted span covers no -/// rows — and it is emphatically better than wrapping, because the only -/// consumer is `Sloc::excluded_lines`, which is later *subtracted* from -/// a row count: a wrapped `usize::MAX` would propagate into SLOC and MI -/// as garbage far from its cause, which is precisely how #1051 surfaced. +/// rows. Note what this does *not* buy: `sloc()` already clamps with +/// `saturating_sub`, so a wrapped value could not have escaped as +/// `usize::MAX` either. It would have escaped as `sloc: 0` for a +/// non-empty file, and on into MI's SLOC term — a wrong number surfacing +/// far from its cause, which is how #1051 was reported. #[inline] fn span_rows(start_row: usize, end_line: usize) -> usize { debug_assert!( diff --git a/src/metrics/loc/shared.rs b/src/metrics/loc/shared.rs index 0d23146c7..f4d6a1ad8 100644 --- a/src/metrics/loc/shared.rs +++ b/src/metrics/loc/shared.rs @@ -88,10 +88,14 @@ pub(crate) fn init(node: &Node, stats: &mut Stats, is_func_space: bool) -> (usiz // satisfies `end >= start`. The real guard against that is refusing to // underflow in the first place, at the site that adjusts the span. // -// What the assert does not cover, the `saturating_sub` below does: -// an inverted span yields a zero-row comment rather than wrapping to -// `usize::MAX` and reaching `add_only_comment_lines` as a span that -// `LineSet::insert_range` would then have to reject (#1152). +// The `saturating_sub` below narrows what release does with an +// inverted span, and the effect is smaller than it looks: `comment_diff` +// feeds only the two branch tests, never a row index, so both branches +// already hand `add_only_comment_lines` the raw `start`/`end` and +// `LineSet::insert_range` already rejects the inversion. What changes is +// which branch runs — `0` instead of a wrapped `usize::MAX` keeps a +// comment sharing a code line in the `== 0` arm, where it belongs, +// rather than the block-comment arm it wrapped into (#1152). pub(crate) fn add_cloc_lines(stats: &mut Stats, start: usize, end: usize) { debug_assert!(end >= start, "add_cloc_lines: end {end} < start {start}"); let comment_diff = end.saturating_sub(start); From 8cf09bc4e9426c0068dcf2c2b0d5ede2f125580b Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Fri, 7 Aug 2026 10:06:03 -0700 Subject: [PATCH 05/10] chore(lints): gate production unwrap, decline expect_used MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Triage of the 37 production `unwrap`/`expect` sites #1227 asked for, plus the one gate that triage justifies. The count is the finding: **0 production `unwrap()`, 37 production `expect()`**, measured with the lints enabled over lib and bin targets so `#[cfg(test)]` modules are excluded by construction rather than by a grep heuristic. `unwrap_used` therefore costs nothing to adopt today and fails CI on the first one added. It is set per crate root as `#![cfg_attr(not(test), warn(...))]`, not in `[workspace.lints]`, because a Cargo lint applies to every target of its package: the same measurement across `--all-targets` reports 1_023 `unwrap()` and 2_157 `expect()`, all of them legitimate test code. `cfg(test)` is set for integration-test crates as well as for the unit-test target, so the per-root form needs no per-file carve-out and carries zero `#[allow]`s. Verified in both directions with a cache-busted probe: a production `unwrap` is flagged, the same `unwrap` in a `#[cfg(test)]` module is not. `expect_used` is declined. All 37 sites already name their invariant in the message, which is the form `AGENTS.md` sanctions, so gating it buys 37 annotations that each restate the line above them — the "drowned in allows, reads as coverage" outcome #1152 was avoiding. The distinction that decides it is not `expect`-vs-`unwrap` but what can invalidate the invariant. The `FEATURES_PINNED` sites #1152 removed rested on a `#[non_exhaustive]` enum a dependency bump could change underneath them. These 37 rest on facts local to this repository — a constant regex compiles, a walker pushed a root before descending, a `strip_prefix` follows a `starts_with` three lines up — which review and tests already cover. No test accompanies the gate: the only shape available would grep this repository's own source for the attribute, which `.claude/rules/testing.md` bans as vacuous. The lint firing in CI is the test. Fixes #1227 --- CHANGELOG.md | 12 ++++++++++++ Cargo.toml | 19 +++++++++++++++++++ big-code-analysis-bench/src/lib.rs | 5 +++++ big-code-analysis-cli/src/lib.rs | 4 ++++ big-code-analysis-cli/src/main.rs | 5 +++++ big-code-analysis-py/src/lib.rs | 4 ++++ big-code-analysis-web/src/bin/bca-web.rs | 4 ++++ big-code-analysis-web/src/lib.rs | 4 ++++ src/lib.rs | 4 ++++ xtask/src/main.rs | 4 ++++ 10 files changed, 65 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f1ac8a49..eefaebd7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,18 @@ for historical reference. `step_raw_string`'s delimiter comparison, carried through `LexState::RawString` — is hardened with `get` rather than allowed (#1152). +- `clippy::unwrap_used` is enforced on production code across every + crate, as `#![cfg_attr(not(test), warn(...))]` at each of the eight + lib/bin roots rather than a `[workspace.lints]` entry: a Cargo lint + applies to every target of its package, and the ban is a production + rule — this workspace has **0** production `unwrap()` calls against + 1,023 legitimate ones in test targets. `cfg(test)` is set for + integration-test crates as well as the unit-test target, so the gate + needs no per-file carve-out and carries zero `#[allow]`s. Adopting it + costs nothing today and fails CI on the first production `unwrap()` + added. `clippy::expect_used` is deliberately **not** enabled — all 37 + production `expect` sites already name their invariant in the message, + the form `AGENTS.md` sanctions (#1227). ## [2.1.0] - 2026-08-06 diff --git a/Cargo.toml b/Cargo.toml index d587014b8..87920b960 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -118,6 +118,25 @@ unexpected_cfgs = { level = "warn", check-cfg = ['cfg(chain_audit)'] } pedantic = { level = "warn", priority = -1 } module_name_repetitions = "allow" +# `clippy::unwrap_used` is deliberately NOT declared here. It is set as +# `#![cfg_attr(not(test), warn(clippy::unwrap_used))]` at each production +# crate root instead (the eight lib/bin roots), because a Cargo lint +# applies to every target of its package and the `unwrap` ban is a +# production rule: this workspace's test targets hold 1_023 legitimate +# `unwrap()` calls, against 0 in production. `cfg(test)` is set for +# integration-test crates as well as for the unit-test target, so the +# per-root form needs no per-file carve-out and carries zero `#[allow]`s. +# +# `clippy::expect_used` is not enabled at all. All 37 production `expect` +# sites already name their invariant in the message, which is the form +# `AGENTS.md` sanctions, so gating it would buy 37 annotations that each +# restate the line above them. The distinction that matters is not +# `expect`-vs-`unwrap` but *what can invalidate the invariant*: the +# `FEATURES_PINNED` sites removed in #1152 rested on a `#[non_exhaustive]` +# enum a dependency bump could change underneath them, whereas these 37 +# rest on facts local to this repository, which review and tests cover. +# See #1227 for the full triage. + [package] name = "big-code-analysis" version.workspace = true diff --git a/big-code-analysis-bench/src/lib.rs b/big-code-analysis-bench/src/lib.rs index db2020c5b..a6979ffb1 100644 --- a/big-code-analysis-bench/src/lib.rs +++ b/big-code-analysis-bench/src/lib.rs @@ -1,4 +1,9 @@ //! Benchmark harness for the `big-code-analysis` metric walk (#1068). + +// Production-only `unwrap()` ban. See `[workspace.lints.clippy]` in the +// root `Cargo.toml` for why this is a per-root attribute and not a +// Cargo lint (#1227). +#![cfg_attr(not(test), warn(clippy::unwrap_used))] //! //! The crate is split three ways: //! diff --git a/big-code-analysis-cli/src/lib.rs b/big-code-analysis-cli/src/lib.rs index c28425267..2ee2635b8 100644 --- a/big-code-analysis-cli/src/lib.rs +++ b/big-code-analysis-cli/src/lib.rs @@ -35,6 +35,10 @@ // section on the entry point adds noise without adding signal. clippy::missing_panics_doc )] +// Production-only `unwrap()` ban. See `[workspace.lints.clippy]` in the +// root `Cargo.toml` for why this is a per-root attribute and not a +// Cargo lint (#1227). +#![cfg_attr(not(test), warn(clippy::unwrap_used))] mod baseline; mod baseline_diff; mod check_flags; diff --git a/big-code-analysis-cli/src/main.rs b/big-code-analysis-cli/src/main.rs index cfcb51dba..e59ab0984 100644 --- a/big-code-analysis-cli/src/main.rs +++ b/big-code-analysis-cli/src/main.rs @@ -1,4 +1,9 @@ //! `bca` binary entry point. All logic lives in the + +// Production-only `unwrap()` ban. See `[workspace.lints.clippy]` in the +// root `Cargo.toml` for why this is a per-root attribute and not a +// Cargo lint (#1227). +#![cfg_attr(not(test), warn(clippy::unwrap_used))] //! [`big_code_analysis_cli`] library so the workspace `xtask` crate can //! reuse the same `clap` definition to render man pages. diff --git a/big-code-analysis-py/src/lib.rs b/big-code-analysis-py/src/lib.rs index adc7c277e..1dad18dd7 100644 --- a/big-code-analysis-py/src/lib.rs +++ b/big-code-analysis-py/src/lib.rs @@ -7,6 +7,10 @@ //! without spinning up a Python interpreter. #![allow(unsafe_op_in_unsafe_fn)] +// Production-only `unwrap()` ban. See `[workspace.lints.clippy]` in the +// root `Cargo.toml` for why this is a per-root attribute and not a +// Cargo lint (#1227). +#![cfg_attr(not(test), warn(clippy::unwrap_used))] // The `#[pymodule]` macro expands to an `extern "C"` init function // that PyO3 marks `#[unsafe(no_mangle)]`. The expansion contains // unsafe FFI shims that the macro itself wraps in `unsafe { ... }`; diff --git a/big-code-analysis-web/src/bin/bca-web.rs b/big-code-analysis-web/src/bin/bca-web.rs index 7c2f526a8..cbc120edd 100644 --- a/big-code-analysis-web/src/bin/bca-web.rs +++ b/big-code-analysis-web/src/bin/bca-web.rs @@ -1,4 +1,8 @@ #![allow(missing_docs)] +// Production-only `unwrap()` ban. See `[workspace.lints.clippy]` in the +// root `Cargo.toml` for why this is a per-root attribute and not a +// Cargo lint (#1227). +#![cfg_attr(not(test), warn(clippy::unwrap_used))] use std::process::ExitCode; use clap::Parser; diff --git a/big-code-analysis-web/src/lib.rs b/big-code-analysis-web/src/lib.rs index 105aa06b6..f52c913f1 100644 --- a/big-code-analysis-web/src/lib.rs +++ b/big-code-analysis-web/src/lib.rs @@ -3,6 +3,10 @@ // The deeply nested `json!` literals in server.rs tests exceed the default // recursion limit (128) during `json_internal!` macro expansion. #![recursion_limit = "256"] +// Production-only `unwrap()` ban. See `[workspace.lints.clippy]` in the +// root `Cargo.toml` for why this is a per-root attribute and not a +// Cargo lint (#1227). +#![cfg_attr(not(test), warn(clippy::unwrap_used))] /// HTTP endpoints and request handlers. pub mod web; diff --git a/src/lib.rs b/src/lib.rs index 258977fbd..8d3fa0880 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -119,6 +119,10 @@ //! in a class. #![allow(clippy::upper_case_acronyms)] +// Production-only `unwrap()` ban. See `[workspace.lints.clippy]` in the +// root `Cargo.toml` for why this is a per-root attribute and not a +// Cargo lint (#1227). +#![cfg_attr(not(test), warn(clippy::unwrap_used))] // Internal-only modules. Nothing is re-exported from these. mod c_declarator; diff --git a/xtask/src/main.rs b/xtask/src/main.rs index dde60b11d..69667d39d 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -7,6 +7,10 @@ //! re-running `cargo xtask` fails the manpage job. #![allow(missing_docs)] #![allow(clippy::pedantic)] +// Production-only `unwrap()` ban. See `[workspace.lints.clippy]` in the +// root `Cargo.toml` for why this is a per-root attribute and not a +// Cargo lint (#1227). +#![cfg_attr(not(test), warn(clippy::unwrap_used))] use std::{ env, From 7ba15b155b31bb29bd27d42a7c3f1ce60fd5ce61 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Fri, 7 Aug 2026 10:21:28 -0700 Subject: [PATCH 06/10] fix(enums): propagate codegen unwraps, extend the unwrap gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1227's gate stopped at the cargo workspace, and `enums` is not in it. The crate is excluded from `[workspace] members` for build reasons but is still first-party Rust, still CI-linted (`make enums-check` runs clippy with `-D warnings` and its tests), and held 7 production `unwrap()` calls that `cargo clippy --workspace` never saw. So the "0 production `unwrap()`" figure was true of the workspace and not of the repository, which is the reading the gate invites. One of the 7 was a live latent panic rather than a style issue: the Go generator's `names.iter().map(|x| x.0.len()).max().unwrap()` panics on a grammar that contributes no token names. It becomes `unwrap_or(0)`, which is the identity here — with no names the `map` below yields nothing, so the padding width is never read. The rest propagate onto the `io::Result<()>` every generator already returned, via a shared `render_error` lift for the `askama` failures. `env::var("CARGO_MANIFEST_DIR").unwrap()` becomes `env!(...)`: the compile-time macro is both infallible and more correct here, since it resolves the enums crate's own source tree rather than whatever environment the binary is invoked from. Both `enums` roots (lib and bin) now carry the same `#![cfg_attr(not(test), warn(clippy::unwrap_used))]` as the eight workspace roots, so the gate's claim now matches its scope. Verified by the existing codegen-drift gate rather than by assertion: `utils/check-enums-codegen-drift.sh` regenerates in both `rust` and `c_macros` modes and diffs against the checked-in output. `c_macros` is the mode that goes through the `CARGO_MANIFEST_DIR` path, and the output is byte-identical. The new `render_error` lift carries a unit test; the empty-`names` case has no reachable input through any real grammar, so `unwrap_or(0)` is correct-by-construction rather than a fixed live bug with a regression test. Follow-up to 8cf09bc4 (#1227). --- CHANGELOG.md | 8 +++++++- enums/src/common.rs | 34 ++++++++++++++++++++++++++++++++++ enums/src/go.rs | 7 +++++-- enums/src/json.rs | 2 +- enums/src/lib.rs | 6 ++++++ enums/src/main.rs | 6 ++++++ enums/src/rust.rs | 11 +++++++---- 7 files changed, 66 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eefaebd7a..451519574 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -72,7 +72,13 @@ for historical reference. costs nothing today and fails CI on the first production `unwrap()` added. `clippy::expect_used` is deliberately **not** enabled — all 37 production `expect` sites already name their invariant in the message, - the form `AGENTS.md` sanctions (#1227). + the form `AGENTS.md` sanctions. The workspace-excluded `enums` codegen + crate carries the same gate: it is CI-linted by `make enums-check` but + invisible to `cargo clippy --workspace`, so its 7 production + `unwrap()` calls were outside the original count. One was a live + latent panic — `names.iter().map(..).max().unwrap()` in the Go + generator panics on a token-less grammar — and the rest now propagate + onto the `io::Result` each generator already returned (#1227). ## [2.1.0] - 2026-08-06 diff --git a/enums/src/common.rs b/enums/src/common.rs index 920662cb7..baab2cbbb 100644 --- a/enums/src/common.rs +++ b/enums/src/common.rs @@ -2,6 +2,17 @@ use std::collections::BTreeMap; use std::collections::hash_map::{Entry, HashMap}; use tree_sitter::Language; +/// Lifts an `askama` render failure onto the `io::Result` channel every +/// generator already returns. +/// +/// The templates are compile-time checked, so a render failure means a +/// formatting error rather than a malformed template — but "unlikely" +/// is not "impossible", and each caller is three lines from an `Err` +/// it can return (#1227). +pub fn render_error(err: askama::Error) -> std::io::Error { + std::io::Error::other(err) +} + pub fn sanitize_identifier(name: &str) -> String { // Match both the canonical U+FEFF (a UTF-8-decoded BOM token, the // shape tree-sitter actually produces from `node_kind_for_id`) and @@ -182,6 +193,29 @@ pub fn get_token_names(language: &Language, escape: bool) -> Vec<(String, bool, mod tests { use super::*; + /// A render failure reaches the caller as an `io::Error` that still + /// carries the `askama` cause, rather than panicking (#1227). + /// + /// Unreachable through the generators — the templates are + /// compile-time checked, so `render` only fails on a formatting + /// error — which is exactly why this asserts on the lift directly + /// instead of through `generate_rust`. + #[test] + fn a_render_failure_becomes_an_io_error_carrying_its_cause() { + let err = render_error(askama::Error::Fmt); + + // `other` rather than a more specific kind: a template that + // failed to format is not bad *input*, and the generators have + // no kind of their own to claim. + assert_eq!(err.kind(), std::io::ErrorKind::Other); + assert!( + err.get_ref() + .and_then(|inner| inner.downcast_ref::()) + .is_some(), + "the askama::Error must be retrievable, not stringified" + ); + } + // Issue #345: the previous `""` literal was the three-codepoint // mojibake form (U+00EF U+00BB U+00BF) — the three UTF-8 BOM bytes // reinterpreted as Latin-1 chars. A tree-sitter grammar that diff --git a/enums/src/go.rs b/enums/src/go.rs index a4c406239..c74848f0e 100644 --- a/enums/src/go.rs +++ b/enums/src/go.rs @@ -24,7 +24,10 @@ pub fn generate_go(output: &Path, file_template: &str) -> std::io::Result<()> { let mut file = File::create(path)?; let mut names = get_token_names(&language, false); - let max_len = names.iter().map(|x| x.0.len()).max().unwrap(); + // `unwrap_or(0)` is the identity, not a swallowed error: with no + // names the `map` below yields nothing, so the padding width is + // never read. `unwrap()` here panicked on a token-less grammar. + let max_len = names.iter().map(|x| x.0.len()).max().unwrap_or(0); let names: Vec<_> = names .drain(..) .map(move |(n, d, t)| (n.clone(), d, t, format!("{: std::io::Result<()> { let args = GoTemplate { c_name, names }; - file.write_all(args.render().unwrap().as_bytes())?; + file.write_all(args.render().map_err(render_error)?.as_bytes())?; } Ok(()) diff --git a/enums/src/json.rs b/enums/src/json.rs index 7b302c161..b6ff454c4 100644 --- a/enums/src/json.rs +++ b/enums/src/json.rs @@ -36,7 +36,7 @@ pub fn generate_json(output: &Path, file_template: &str) -> std::io::Result<()> let args = JsonTemplate { names }; - file.write_all(args.render().unwrap().as_bytes())?; + file.write_all(args.render().map_err(render_error)?.as_bytes())?; } Ok(()) diff --git a/enums/src/lib.rs b/enums/src/lib.rs index c6ffdd4ed..83ee18294 100644 --- a/enums/src/lib.rs +++ b/enums/src/lib.rs @@ -1,3 +1,9 @@ +// Production-only `unwrap()` ban. See `[workspace.lints.clippy]` in the +// root `Cargo.toml` for why this is a per-root attribute and not a +// Cargo lint (#1227). `enums` is excluded from that workspace, so it +// carries the attribute for the same reason and gets it separately. +#![cfg_attr(not(test), warn(clippy::unwrap_used))] + #[macro_use] mod macros; diff --git a/enums/src/main.rs b/enums/src/main.rs index 4ed5d7413..2f00cb57a 100644 --- a/enums/src/main.rs +++ b/enums/src/main.rs @@ -1,3 +1,9 @@ +// Production-only `unwrap()` ban. See `[workspace.lints.clippy]` in the +// root `Cargo.toml` for why this is a per-root attribute and not a +// Cargo lint (#1227). `enums` is excluded from that workspace, so it +// carries the attribute for the same reason and gets it separately. +#![cfg_attr(not(test), warn(clippy::unwrap_used))] + use std::path::PathBuf; use clap::{Parser, ValueEnum}; diff --git a/enums/src/rust.rs b/enums/src/rust.rs index 22217f17c..4a0afc663 100644 --- a/enums/src/rust.rs +++ b/enums/src/rust.rs @@ -28,7 +28,8 @@ pub fn generate_rust(output: &Path, file_template: &str) -> std::io::Result<()> let path = output.join(file_name); let mut file = File::create(path)?; - file.write_all(build_rust_template(&lang).render().unwrap().as_bytes())?; + let rendered = build_rust_template(&lang).render().map_err(render_error)?; + file.write_all(rendered.as_bytes())?; } Ok(()) @@ -70,7 +71,7 @@ pub fn generate_macros(output: &Path) -> std::io::Result<()> { fn create_macros_file(output: &Path, filename: &str, u_name: &str) -> std::io::Result<()> { let mut macro_file = File::open(Path::new(&format!( "{}/{}/{}.txt", - env::var("CARGO_MANIFEST_DIR").unwrap(), + env!("CARGO_MANIFEST_DIR"), MACROS_DEFINITION_DIR, filename )))?; @@ -79,7 +80,9 @@ fn create_macros_file(output: &Path, filename: &str, u_name: &str) -> std::io::R let mut names = Vec::new(); for tok in data.split(|c| *c == b'\n') { - let tok = std::str::from_utf8(tok).unwrap().trim(); + let tok = std::str::from_utf8(tok) + .map_err(|err| std::io::Error::new(std::io::ErrorKind::InvalidData, err))? + .trim(); if !tok.is_empty() { names.push(tok.to_owned()); } @@ -104,7 +107,7 @@ fn create_macros_file(output: &Path, filename: &str, u_name: &str) -> std::io::R names, }; - file.write_all(args.render().unwrap().as_bytes()) + file.write_all(args.render().map_err(render_error)?.as_bytes()) } #[cfg(test)] From 7f8efe53ce4ab0829c44d80cfc5347df24eff3ca Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Fri, 7 Aug 2026 10:57:05 -0700 Subject: [PATCH 07/10] refactor(lints): move gate attributes out of the crate docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scripted insertion in 7ba15b15 placed the attribute between the first `//!` line and the rest of the crate doc in two files. In `big-code-analysis-cli/src/main.rs` that split a sentence across it. rustdoc concatenates the blocks so the rendered output was unaffected, and rustfmt does not move attributes, so neither gate could see it. Also drops `use std::env;`, left dead by the `env::var` -> `env!` change. rustc does not warn: the `env!` macro path marks the name used, which is why `-D warnings` stayed green. Verified by compiling without the import. Both moved attributes are still in effect — a probe `unwrap()` in `main.rs` is flagged. --- big-code-analysis-bench/src/lib.rs | 10 +++++----- big-code-analysis-cli/src/main.rs | 4 ++-- enums/src/rust.rs | 1 - 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/big-code-analysis-bench/src/lib.rs b/big-code-analysis-bench/src/lib.rs index a6979ffb1..39c0cb030 100644 --- a/big-code-analysis-bench/src/lib.rs +++ b/big-code-analysis-bench/src/lib.rs @@ -1,9 +1,4 @@ //! Benchmark harness for the `big-code-analysis` metric walk (#1068). - -// Production-only `unwrap()` ban. See `[workspace.lints.clippy]` in the -// root `Cargo.toml` for why this is a per-root attribute and not a -// Cargo lint (#1227). -#![cfg_attr(not(test), warn(clippy::unwrap_used))] //! //! The crate is split three ways: //! @@ -25,6 +20,11 @@ //! See `docs/development/benchmarking.md` for invocation and for the //! measurement traps this harness exists to prevent. +// Production-only `unwrap()` ban. See `[workspace.lints.clippy]` in the +// root `Cargo.toml` for why this is a per-root attribute and not a +// Cargo lint (#1227). +#![cfg_attr(not(test), warn(clippy::unwrap_used))] + pub mod cli; pub mod corpus; pub mod scaling; diff --git a/big-code-analysis-cli/src/main.rs b/big-code-analysis-cli/src/main.rs index e59ab0984..e1d809a5a 100644 --- a/big-code-analysis-cli/src/main.rs +++ b/big-code-analysis-cli/src/main.rs @@ -1,11 +1,11 @@ //! `bca` binary entry point. All logic lives in the +//! [`big_code_analysis_cli`] library so the workspace `xtask` crate can +//! reuse the same `clap` definition to render man pages. // Production-only `unwrap()` ban. See `[workspace.lints.clippy]` in the // root `Cargo.toml` for why this is a per-root attribute and not a // Cargo lint (#1227). #![cfg_attr(not(test), warn(clippy::unwrap_used))] -//! [`big_code_analysis_cli`] library so the workspace `xtask` crate can -//! reuse the same `clap` definition to render man pages. fn main() { big_code_analysis_cli::run(); diff --git a/enums/src/rust.rs b/enums/src/rust.rs index 4a0afc663..58bfd2a51 100644 --- a/enums/src/rust.rs +++ b/enums/src/rust.rs @@ -1,5 +1,4 @@ use askama::Template; -use std::env; use std::fs::File; use std::io::{Read, Write}; use std::path::Path; From 828355346a4f6e779fe28b65de217c81eb770119 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Fri, 7 Aug 2026 11:02:23 -0700 Subject: [PATCH 08/10] docs(enums): correct the max().unwrap() severity claim Found in review. The changelog and the go.rs comment described the `max().unwrap()` conversion as fixing a live latent panic. It is not reachable: `get_token_names` walks `0..node_kind_count()` and every real tree-sitter grammar has at least the ERROR sentinel, so `names` is never empty and `.max()` is never `None`. The conversion is still right, for the reason #1227 rests on rather than the one claimed: an `unwrap()` states no invariant, which is what separates it from the 37 `expect` sites left alone. Both now say that instead. Third time this session that a guard comment claimed more than the change buys. --- CHANGELOG.md | 10 ++++++---- enums/src/go.rs | 6 +++++- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 451519574..1ac11897f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,10 +75,12 @@ for historical reference. the form `AGENTS.md` sanctions. The workspace-excluded `enums` codegen crate carries the same gate: it is CI-linted by `make enums-check` but invisible to `cargo clippy --workspace`, so its 7 production - `unwrap()` calls were outside the original count. One was a live - latent panic — `names.iter().map(..).max().unwrap()` in the Go - generator panics on a token-less grammar — and the rest now propagate - onto the `io::Result` each generator already returned (#1227). + `unwrap()` calls were outside the original count. They now propagate + onto the `io::Result` each generator already returned, except the Go + generator's `max()` width, which becomes `unwrap_or(0)`. None was a + reachable crash: every one rests on an invariant as solid as the 37 + `expect` sites left alone. The difference is that an `unwrap()` states + no invariant, which is the whole basis for gating it (#1227). ## [2.1.0] - 2026-08-06 diff --git a/enums/src/go.rs b/enums/src/go.rs index c74848f0e..a04421bb7 100644 --- a/enums/src/go.rs +++ b/enums/src/go.rs @@ -26,7 +26,11 @@ pub fn generate_go(output: &Path, file_template: &str) -> std::io::Result<()> { let mut names = get_token_names(&language, false); // `unwrap_or(0)` is the identity, not a swallowed error: with no // names the `map` below yields nothing, so the padding width is - // never read. `unwrap()` here panicked on a token-less grammar. + // never read. The empty case is unreachable — `get_token_names` + // walks `0..node_kind_count()` and every real grammar has at + // least the ERROR sentinel — so this is correct-by-construction + // hardening, not a fixed crash. It is converted rather than left + // alone because an `unwrap()` states no invariant at all (#1227). let max_len = names.iter().map(|x| x.0.len()).max().unwrap_or(0); let names: Vec<_> = names .drain(..) From 5ee330b4ba1a3358c01ae5d615789ae9a1c3b00e Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Fri, 7 Aug 2026 13:18:15 -0700 Subject: [PATCH 09/10] docs(lints): correct four claims found in code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All comment and prose only; no behaviour change. `run_parse_fallible` was inserted between `run_parse`'s doc block and `run_parse` itself, so the block documented the wrong function and left `run_parse` undocumented. This is the third time in this branch that an insertion-based edit landed between a doc comment and its item — the other two were the crate-root attributes fixed in 7f8efe53. Neither rustfmt nor clippy can see this class, and `missing_docs` does not fire because `run_parse` is private. "Sixteen `.expect(FEATURES_PINNED)` call sites" is fifteen: 8 in the CLI dispatch helpers and 7 in the web handlers, plus the two constants, which the original wording folded into the site count. Corrected in CHANGELOG.md and lessons-learned; the commit messages in this branch still say sixteen and are left as history. `dispatch_dump` still carried "the `expect` documents that invariant" above a line that now propagates with `?`. The `add_cloc_lines` rationale, itself already corrected once in a0925a7f, was still imprecise: the `== 0` arm does not call `add_only_comment_lines` at all, and the `> 0` arm passes `start + 1` rather than the raw span. It now says what the `saturating_sub` actually buys — a truthful branch classification, not an observable metric change. --- CHANGELOG.md | 2 +- big-code-analysis-cli/src/dispatch.rs | 4 ++-- big-code-analysis-web/src/web/server.rs | 18 +++++++++--------- docs/development/lessons_learned.md | 2 +- src/metrics/loc/shared.rs | 12 ++++++------ 5 files changed, 19 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ac11897f..49de6aaf3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,7 @@ for historical reference. ### Fixed - The CLI and web crates no longer terminate on a library parse error. - All sixteen `.expect(FEATURES_PINNED)` call sites — eight in `bca`'s + All fifteen `.expect(FEATURES_PINNED)` call sites — eight in `bca`'s dispatch helpers, seven in `bca-web`'s handlers, plus the constant itself in each crate — now propagate onto the error channel each caller already had: an `io::Error` of kind `InvalidData` that the diff --git a/big-code-analysis-cli/src/dispatch.rs b/big-code-analysis-cli/src/dispatch.rs index 663ce0f3f..eaa61fd50 100644 --- a/big-code-analysis-cli/src/dispatch.rs +++ b/big-code-analysis-cli/src/dispatch.rs @@ -239,8 +239,8 @@ fn dispatch_dump( cfg: &Config, ) -> std::io::Result<()> { // The CLI pins the library's `all-languages` feature, so - // `LanguageDisabled` from `Ast::parse` is unreachable; the `expect` - // documents that invariant. + // `LanguageDisabled` from `Ast::parse` is unreachable here; a future + // variant surfaces as a per-file `io::Error` instead (#1152). let ast = parse_ast_io(language, source, &path, pr)?; // Per-file banner so a multi-file dump is attributable: the parallel // walk interleaves trees by worker scheduling, and without a header diff --git a/big-code-analysis-web/src/web/server.rs b/big-code-analysis-web/src/web/server.rs index d6eea26fb..1a792e01b 100644 --- a/big-code-analysis-web/src/web/server.rs +++ b/big-code-analysis-web/src/web/server.rs @@ -74,15 +74,7 @@ pub const DEFAULT_PARSE_TIMEOUT_SECS: u64 = 30; /// the same `413` JSON body (#639). const MAX_BODY_SIZE: usize = 1_024 * 1_024 * 4; -/// Runs `f` on the blocking pool under the timeout / orphan-pool policy. -/// -/// `payload_id` is the client-supplied request id from the JSON payload -/// (empty for the octet-stream endpoints, which carry none). It is logged -/// on the failure path under a distinct field name so it does not collide -/// with `tracing-actix-web`'s own span-level `request_id` (a per-request -/// UUID), and it is echoed back in the `{error, id}` body of the typed -/// [`ParseError`] returned on every failure (#639). -/// [`run_parse`] for a closure that can itself fail. +/// Like [`run_parse`], but for a closure that can itself fail. /// /// The library entry points the handlers call all return /// `Result<_, MetricsError>`, so without this every call site repeats @@ -100,6 +92,14 @@ async fn run_parse_fallible( .map_err(|err| ParseError::from_metrics(payload_id, err)) } +/// Runs `f` on the blocking pool under the timeout / orphan-pool policy. +/// +/// `payload_id` is the client-supplied request id from the JSON payload +/// (empty for the octet-stream endpoints, which carry none). It is logged +/// on the failure path under a distinct field name so it does not collide +/// with `tracing-actix-web`'s own span-level `request_id` (a per-request +/// UUID), and it is echoed back in the `{error, id}` body of the typed +/// [`ParseError`] returned on every failure (#639). async fn run_parse( config: &web::Data, payload_id: &str, diff --git a/docs/development/lessons_learned.md b/docs/development/lessons_learned.md index f1bb33451..3a782a5e8 100644 --- a/docs/development/lessons_learned.md +++ b/docs/development/lessons_learned.md @@ -1085,7 +1085,7 @@ crates, where every call site became `.expect(FEATURES_PINNED)` because both pin `features = ["all-languages"]` and the disabled arm is provably unreachable. Recorded in `CHANGELOG.md` and `STABILITY.md`. -**The `expect` half of that was wrong, and #1152 removed all sixteen +**The `expect` half of that was wrong, and #1152 removed all fifteen call sites.** The invariant it named was real — the feature pin does make `LanguageDisabled` unreachable — but it was the wrong invariant to rest a panic on. `MetricsError` is `#[non_exhaustive]`, and its own diff --git a/src/metrics/loc/shared.rs b/src/metrics/loc/shared.rs index f4d6a1ad8..b61202ed5 100644 --- a/src/metrics/loc/shared.rs +++ b/src/metrics/loc/shared.rs @@ -90,12 +90,12 @@ pub(crate) fn init(node: &Node, stats: &mut Stats, is_func_space: bool) -> (usiz // // The `saturating_sub` below narrows what release does with an // inverted span, and the effect is smaller than it looks: `comment_diff` -// feeds only the two branch tests, never a row index, so both branches -// already hand `add_only_comment_lines` the raw `start`/`end` and -// `LineSet::insert_range` already rejects the inversion. What changes is -// which branch runs — `0` instead of a wrapped `usize::MAX` keeps a -// comment sharing a code line in the `== 0` arm, where it belongs, -// rather than the block-comment arm it wrapped into (#1152). +// feeds only the branch tests, never a row index. `0` routes to the +// `== 0` arm rather than the block-comment arm a wrapped `usize::MAX` +// selected, but the two differ only by an `add_only_comment_lines(start +// + 1, end)` that `LineSet::insert_range` already rejects as inverted — +// so this buys a truthful classification, not an observable metric +// change (#1152). pub(crate) fn add_cloc_lines(stats: &mut Stats, start: usize, end: usize) { debug_assert!(end >= start, "add_cloc_lines: end {end} < start {start}"); let comment_diff = end.saturating_sub(start); From debd15385d4bbc9130aada1774ae09ee22272fec Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Fri, 7 Aug 2026 13:44:23 -0700 Subject: [PATCH 10/10] test(metrics/loc): cover the C-family continued-macro arm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codecov reported 95.52% patch coverage against 98.33% project, with three missing lines: the `PreprocArg` range-insert in `loc/c.rs`, `loc/mozcpp.rs` and `loc/objc.rs`. C++'s identical copy was already covered, which is the per-language blind spot `AGENTS.md` warns about — these four modules are deliberate clones, so one passing language said nothing about the other three. A backslash-continued `#define` body parses as one `PreprocArg` node spanning every continuation row, and each of those rows is PLOC because `tree-sitter-cpp` does not expand macros. The fixture is one macro across three rows plus a `main`, swept over all four languages through `metrics_verbatim`, which takes a `LANG` — the only way to reach `Mozcpp`, which owns no file extension. Values are measured, not guessed, and the test is confirmed discriminating per language: deleting the arm from each module in turn fails it, each time compiling first so the failure is the assertion and not the build. `ploc` is 4 with the arm and 3 without. This closes the patch-coverage gap; the three lines were the only ones in the PR that any test could reach. My earlier "zero coverable lines" claim was scoped to the lint-gate commits alone, not the whole PR. --- src/metrics/loc.rs | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/metrics/loc.rs b/src/metrics/loc.rs index 8eabf889b..151ed5656 100644 --- a/src/metrics/loc.rs +++ b/src/metrics/loc.rs @@ -10321,6 +10321,47 @@ class A { } } + /// A backslash-continued `#define` body is one `PreprocArg` node + /// spanning every continuation row, so each of those rows is PLOC. + /// + /// The four C-family `Loc` impls carry an identical arm for this + /// (`tree-sitter-cpp` does not expand macros — see the comment at + /// each site), and until #1229 only C++'s copy was exercised: the + /// other three were the sole uncovered lines in that PR. They are + /// deliberate clones, so a fixture for one is a fixture for all + /// four, and `Mozcpp` in particular owns no file extension and can + /// only be reached by naming the language. + /// + /// Measured, and confirmed discriminating by deleting the arm from + /// all four modules: `ploc` is 4 with it and 3 without, the lost row + /// being the macro's last continuation line. `sloc` is 5 (three + /// macro rows, one blank, one `main`) and `lloc` is 1 — the single + /// `return` statement — since a `#define` declares no statement. + #[test] + fn a_continued_macro_body_counts_every_row_it_spans() { + // Rows: 0-2 are the macro, 3 is blank, 4 is `main`. + const CONTINUED_MACRO: &[u8] = + b"#define SUM(a, b) \\\n ((a) + \\\n (b))\n\nint main(void) { return SUM(1, 2); }\n"; + + for lang in [ + crate::LANG::C, + crate::LANG::Cpp, + crate::LANG::Mozcpp, + crate::LANG::Objc, + ] { + let loc = metrics_verbatim(lang, CONTINUED_MACRO, MetricsOptions::default()).loc; + assert_eq!( + loc.ploc(), + 4, + "{lang:?}: every continuation row of the macro body is code" + ); + assert_eq!(loc.sloc(), 5, "{lang:?} sloc"); + assert_eq!(loc.lloc(), 1, "{lang:?} lloc"); + assert_eq!(loc.cloc(), 0, "{lang:?} cloc"); + assert_eq!(loc.blank(), 1, "{lang:?} blank"); + } + } + /// Whether the last line ends in a newline is a formatting detail, not /// a property of the code — no LOC sub-metric, and therefore no MI /// value, may depend on it. This is the invariant #1067 violated, and