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..49de6aaf3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,64 @@ for historical reference. ## [Unreleased] +### Fixed + +- The CLI and web crates no longer terminate on a library parse error. + 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 + 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). +- `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. 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. 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 A feature and correctness release on the `2.x` line, and the first to 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..39c0cb030 100644 --- a/big-code-analysis-bench/src/lib.rs +++ b/big-code-analysis-bench/src/lib.rs @@ -20,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/dispatch.rs b/big-code-analysis-cli/src/dispatch.rs index 0f1d476ea..eaa61fd50 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(()); @@ -203,9 +239,9 @@ 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. - let ast = parse_ast(language, source, &path, pr).expect(FEATURES_PINNED); + // `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 // 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..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; @@ -109,18 +113,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-cli/src/main.rs b/big-code-analysis-cli/src/main.rs index cfcb51dba..e1d809a5a 100644 --- a/big-code-analysis-cli/src/main.rs +++ b/big-code-analysis-cli/src/main.rs @@ -2,6 +2,11 @@ //! [`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))] + fn main() { big_code_analysis_cli::run(); } 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/big-code-analysis-web/src/web/server.rs b/big-code-analysis-web/src/web/server.rs index 4f88e5f06..1a792e01b 100644 --- a/big-code-analysis-web/src/web/server.rs +++ b/big-code-analysis-web/src/web/server.rs @@ -47,13 +47,7 @@ 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\"]"; +use big_code_analysis::MetricsError; struct ParseConfig { /// `None` means no timeout (`parse_timeout_secs = 0`). @@ -80,6 +74,24 @@ pub const DEFAULT_PARSE_TIMEOUT_SECS: u64 = 30; /// the same `413` JSON body (#639). const MAX_BODY_SIZE: usize = 1_024 * 1_024 * 4; +/// 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 +/// 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)) +} + /// Runs `f` on the blocking pool under the timeout / orphan-pool policy. /// /// `payload_id` is the client-supplied request id from the JSON payload diff --git a/big-code-analysis-web/src/web/server/errors.rs b/big-code-analysis-web/src/web/server/errors.rs index 2c096e7ad..9c966d63d 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,30 @@ 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. + /// + /// 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 + /// 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..cfbb9b37f 100644 --- a/big-code-analysis-web/src/web/server/handlers.rs +++ b/big-code-analysis-web/src/web/server/handlers.rs @@ -138,10 +138,8 @@ pub(crate) async fn ast_parser( comment: payload.comment, span: payload.span, }; - let result = run_parse(&config, &payload_id, move || { - Ast::parse(Source::from_bytes(language, buf)) - .expect(FEATURES_PINNED) - .dump(cfg) + let result = run_parse_fallible(&config, &payload_id, move || { + Ast::parse(Source::from_bytes(language, buf)).map(|ast| ast.dump(cfg)) }) .await?; // `root == None` previously surfaced as a `200` carrying @@ -188,8 +186,8 @@ 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 || { - strip_comments(language, buf, cfg).expect(FEATURES_PINNED) + let result = run_parse_fallible(&config, &payload_id, move || { + strip_comments(language, buf, cfg) }) .await?; // The JSON variant returns `code` as a string (#629). The request @@ -233,10 +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).expect(FEATURES_PINNED) - }) - .await?; + 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 @@ -271,8 +267,8 @@ 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 || { - compute_metrics(language, buf, cfg).expect(FEATURES_PINNED) + let response = run_parse_fallible(&config, &payload_id, move || { + compute_metrics(language, buf, cfg) }) .await?; // `None` means metric computation failed: answer with an explicit @@ -383,10 +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).expect(FEATURES_PINNED) - }) - .await?; + 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 { @@ -420,8 +414,8 @@ pub(crate) async fn function_json( id: payload.id, language: language.name().to_string(), }; - let result = run_parse(&config, &payload_id, move || { - function_spans(language, buf, cfg).expect(FEATURES_PINNED) + let result = run_parse_fallible(&config, &payload_id, move || { + function_spans(language, buf, cfg) }) .await?; // `function_spans` returns a `serde_json::Value`, so the echoed @@ -447,10 +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).expect(FEATURES_PINNED) - }) - .await?; + 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())) 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..3a782a5e8 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 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 +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/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..a04421bb7 100644 --- a/enums/src/go.rs +++ b/enums/src/go.rs @@ -24,7 +24,14 @@ 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. 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(..) .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..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; @@ -28,7 +27,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 +70,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 +79,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 +106,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)] 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/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/src/metrics/loc.rs b/src/metrics/loc.rs index b107d1e08..151ed5656 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,21 @@ 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. 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 { - 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 +153,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 +197,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 +419,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] + 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 +520,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 +550,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); @@ -10276,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 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..b61202ed5 100644 --- a/src/metrics/loc/shared.rs +++ b/src/metrics/loc/shared.rs @@ -87,9 +87,18 @@ 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. +// +// 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 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 - 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 +107,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 +191,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); 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,