diff --git a/.claude/rules/testing.md b/.claude/rules/testing.md index cea93412..76259f22 100644 --- a/.claude/rules/testing.md +++ b/.claude/rules/testing.md @@ -46,6 +46,28 @@ work, do one of: - when a partial revert would not build, patch the production line to a no-op in place instead (the approach used in the #615 fix session). +**A sweep of several perturbations needs a script, not a shell loop.** +The inverse-edit advice above does not scale past one or two: a loop +that perturbs, tests, and restores is where `git checkout -- ` +gets reached for, and it destroyed an entire uncommitted rewrite a third +time during #1219. Read the file once into a variable, write each +perturbation from that string, and restore from it in a `finally` — the +backup then lives in memory and cannot be defeated by the working tree +changing underneath. Put the driver in a file rather than nesting quotes +through zsh into `python3 -c`, which is how those edits came to be +applied inconsistently in the first place. + +**A broken harness reports a plausible failure count, not an error.** +In that same sweep the restore silently reverted the gate to its `main` +version, so every subsequent perturbation ran the *new* test suite +against the *old* implementation and reported an identical 34 — six +failures and twenty-eight errors, reproducible today by checking out +either side alone. Three perturbations in a row returning the same +number reads as "well covered"; it meant the file under test was no +longer the file being edited. Before believing any perturbation result, +confirm the subject still contains the change: `rg -c ` on +the file, or a `git diff --stat` that shows what you expect. + After restoring, `git status` / `git diff --stat` must show exactly the edits you intend — nothing extra, nothing missing. @@ -291,3 +313,40 @@ executable name, a path separator, a line ending — in one OS's spelling. `.exe` on Windows: green on Linux and macOS, failing only on the Windows leg. Mirror the production code's platform logic in the fixture (`bca{EXE}`) rather than hardcoding one OS's form. + +## Gate a feature-gated fixture table on the union of its rows + +A test whose case list is built from `#[cfg(feature = …)]` rows has two +failure modes, and fixing one reintroduces the other. Left ungated, a +feature set that enables none of the rows leaves an empty list, a loop +of zero iterations, and a test that passes having asserted nothing — +which is why `assert_fixtures_present` (`src/test_support.rs`) exists to +make that state loud. But loudness alone turns the same build into a +spurious *failure* that reads as a defect in whatever was being changed. + +Both halves are required: + +- `#[cfg(any(feature = "a", feature = "b", …))]` on the **`fn`**, naming + the union of the features its rows use, so the test is *absent* rather + than failing when none is enabled. +- The non-vacuity assertion (`assert_fixtures_present`, or a `ran > 0` + counter for a hand-rolled loop) inside it, covering the residual case + where a runtime `is_enabled()` check stops agreeing with the feature + it compiled under. + +This bit twice in one batch (#1220, PR #1221). +`the_1184_constructs_open_quiet_function_spaces` had the assertion and +no gate, so `--no-default-features --features rust` failed with "at +least one language feature must be enabled for this test to mean +anything"; its two siblings had been gated in an earlier fix and it was +missed. Then the fix for #1218 +rewrote a Tcl/iRules parity test as a loop, added a `ran > 0` guard, and +reproduced the identical false failure in a second file. + +Verify against a subset that enables **none** of the named features. +`rust,typescript` — the canonical minimal-langs configuration — is not +such a subset for any table containing a TypeScript or TSX row, so a +single non-listed language (`--features go`) is the reproducer. + +Note the union is over *features*, not languages: `LANG::Tsx` rides +`feature = "typescript"`, so a seven-row table can need only six. diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c7f1756..3a592612 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -897,14 +897,21 @@ for historical reference. - C's `(void)` marker is no longer counted as a parameter. `int f(void)` declares nothing, but the grammar emits a real `parameter_declaration` for the `void` and every `nargs` filter counted it, so `f(void)` and - `f(int)` both reported 1. Fixed for C, C++, Mozcpp and Objective-C. + `f(int)` both reported 1. Fixed for C, C++, Mozcpp and Objective-C, + in both the function and the closure channel — an Objective-C block + literal `^(void){ … }` counted the marker too, because its arm + matched parameter kinds positively instead of routing through the + shared `count_args` helper, and so never consulted the hook (#1218). The distinction needs the source bytes rather than the tree — an unnamed parameter is the same shape and really is one argument — so `Checker` gained an `is_empty_param_marker` hook that defaults to `false` and reads them. **Metric drift.** 24 recorded values fall to 0 in the `DeepSpeech` - corpus, all in `pywrapfst.cc`, its only `(void)` definitions. + corpus, all in `pywrapfst.cc`, its only `(void)` definitions. The + block-literal half moves nothing recorded: `^(void)` appears in two + corpus files, both `.mm`, which route to C++ — where blocks are not a + construct. - A comment written inside a parameter list is no longer counted as a parameter (#1201). tree-sitter attaches such a comment as a direct @@ -916,8 +923,12 @@ for historical reference. TypeScript, TSX, Python, Rust, Java, C#, PHP, Ruby, Groovy, Elixir and Kotlin lambdas in one shared predicate. Go, Lua, Tcl, iRules, Objective-C methods and blocks, Kotlin functions and Groovy closures - were already correct and are unchanged; Perl had carried the exclusion - privately since its signature support landed. + already reported the right count here and needed no fix; Perl had + carried the exclusion privately since its signature support landed. + Objective-C blocks were correct only incidentally — their arm listed + the parameter kinds it wanted rather than excluding comments, and + nothing asserted it — so #1218 routed them through the shared + predicate and added the fixture. **Metric drift.** Serialized `nargs` falls wherever a signature carries a comment — across the `DeepSpeech` corpus, 854 recorded @@ -1062,6 +1073,36 @@ for historical reference. the 126 files under the per-language subdirectories were never checked. Latent — no live count changed. +- `utils/check-diagnostic-prefix.py` decides string and comment state + with a lexer rather than a per-line regex (#1219). Three inputs made + it read a raw-string open where there was none — a plain string whose + closing quote follows `r` (`"dir/r"`), and an unterminated `r"` in a + trailing `//` or a `/* … */` comment — after which every line to the + next quote was skipped and any severity literal in between was a + false clean. Neither cheap fix works: no lookbehind can express the + first, since what distinguishes it is that the quote *closes* a + literal, and stripping comments by regex would truncate `"http://x"` + mid-literal and open a phantom span of its own. The walk is ported + from `check-snapshot-anchors.py`, which needed the identical machine; + both sides now name the other. One deliberate widening: a severity + quoted inside any comment is skipped, where before only a whole-line + comment was. Latent — no live count changed, verified by diffing both + scanners over all 559 tracked Rust files. + +- The book and [`STABILITY.md`](./STABILITY.md) scope the + object-oriented emission rule to `npm` and `npa` (#1220). Both said + all three blocks follow the space's kind with no grammar deviating in + either direction; that holds for `npm` / `npa`, which are gated + centrally by kind, and not for `wmc`, which is decided per language. + Go emits no `wmc` block on any space including the file root while its + `npa` / `npm` do appear there, and a `namespace` space — a C++ or + Mozcpp `namespace`, or a Ruby `module` — carries `npm` / `npa` but no + `wmc` because its member functions are free functions rather than + methods of a class. Both narrowings are now asserted in + `container_scope_tests.rs`, which previously tracked only `npm` and + `npa` — nothing pinned `wmc`'s scope, which is how one rule came to + describe three blocks. + - **Metric values move.** A Java record's compact constructor (`record R(int a) { R { … } }`) now opens its own function space instead of charging its body to the enclosing class (#1160). diff --git a/STABILITY.md b/STABILITY.md index 53a10ea4..9f605057 100644 --- a/STABILITY.md +++ b/STABILITY.md @@ -1076,9 +1076,12 @@ dicts, byte-identical to the CLI output), so it only narrows the static type. Every metric block is `NotRequired` because a `metrics=` selection can elide blocks, and because the object-oriented blocks are scope-gated: `wmc`, `npm` and `npa` are emitted on container spaces and -on the file root, and never on a function space (#1197, #1203). Since -the latter, the space's own kind is the sole input for every language, -so no grammar deviates from that rule in either direction. A language +on the file root, and never on a function space (#1197, #1203). For +`npm` and `npa` the space's own kind has been the sole input for every +language since the latter, with no grammar deviating in either +direction. `wmc` is narrower in two language-level ways: Go emits no +`wmc` block on any space, including the file root, and a `namespace` +space carries `npm` and `npa` but no `wmc`. A language with no class-like construct emits them nowhere. Which spaces carry which block is not part of the shape contract — the `wire` struct definitions are. The VCS *report* dicts are now single-sourced and typed too diff --git a/big-code-analysis-book/src/metrics.md b/big-code-analysis-book/src/metrics.md index 8fe873b5..3f5b5d31 100644 --- a/big-code-analysis-book/src/metrics.md +++ b/big-code-analysis-book/src/metrics.md @@ -950,10 +950,12 @@ counts as "attribute" rather than "method". ### Which spaces carry NPA, NPM and WMC {#oop-emission-scope} -The three object-oriented blocks are emitted on **container spaces** — -`class`, `struct`, `trait`, `impl`, `namespace`, `interface` — and on -the whole-file `unit` root, which carries the roll-up across every -container in the file. A **function space does not carry them**: a +NPA and NPM are emitted on **container spaces** — `class`, `struct`, +`trait`, `impl`, `namespace`, `interface` — and on the whole-file `unit` +root, which carries the roll-up across every container in the file. WMC +follows the same rule wherever it is computed at all, which is narrower +in two ways set out below. A **function space does not carry any of +them**: a method owns no methods or attributes of its own, so the block would be all zeros. Before big-code-analysis 2.1.0, NPA and NPM did emit that all-zero block on function spaces in C#, JavaScript, MozJS, TypeScript, @@ -971,12 +973,28 @@ roll up through every enclosing space regardless. So a type declared inside a function body is reported by the nearest enclosing container, or by the file root when there is none. -Two things read differently. A Go file's NPA and NPM live on the `unit` +Four things read differently. A Go file's NPA and NPM live on the `unit` root and nowhere else, because Go is the one language that emits them without having a container kind in its space tree — `type … struct` and `type … interface` open no space of their own. (Bash, C, Lua, Perl, Tcl and iRules have no container kind either, but they emit neither -block anywhere, so the question does not arise.) And the CSV projection +block anywhere, so the question does not arise.) + +Neither WMC narrowing moves NPA or NPM, which is why the rule above +still holds as stated for those two. The first is language-level: **Go +emits no `wmc` block at all**, on any space including the `unit` root, +because its flat space model cannot attribute a method to a receiver +class — so a Go file carries `npa` and `npm` at the root and `wmc` +nowhere. The second is space-kind-level: a **`namespace` space carries +`npa` and `npm` but no `wmc`**, because a namespace's member functions +are free functions rather than methods of a class, so there is no +per-class complexity to weight. That covers every construct mapping to +`SpaceKind::Namespace` — a C++ or Mozcpp `namespace`, and a Ruby +`module` — not just the C++ spelling. Objective-C has no namespace +construct of its own, so the case does not arise there. The class +*inside* the namespace carries all three, and so does the file root. + +Finally, the CSV projection is a fixed-column format: it writes the `npa.*` / `npm.*` columns on **every** row regardless of space kind, carrying the real accessor values rather than eliding them. diff --git a/docs/development/lessons_learned.md b/docs/development/lessons_learned.md index 6b1c936f..58d40d1c 100644 --- a/docs/development/lessons_learned.md +++ b/docs/development/lessons_learned.md @@ -132,6 +132,7 @@ number and the higher number stays as a redirect. | [86](#86-a-test-helper-that-normalizes-the-value-under-test-blinds-every-caller-at-once) | A helper normalising the observation blinds every caller | | [87](#87-an-assertion-can-be-correct-and-still-be-about-the-wrong-rows) | An assertion can be about the wrong rows | | [88](#88-a-text-scan-that-does-not-lex-the-language-measures-noise) | A text scan that does not lex the language measures noise | +| [89](#89-a-positive-enumeration-and-a-negative-filter-differ-on-what-neither-names) | A positive enumeration and a negative filter differ on what neither names | --- @@ -3202,7 +3203,10 @@ model the language's lexical structure — strings, raw strings, comments, not report a wrong number occasionally; it reports a confident, uniform, plausible one every run, which is then quoted as fact. Give the scanner its own tests, and pin **both** directions: the under-lex that misses -constructs and the over-lex that swallows them. +constructs and the over-lex that swallows them. **Do not try to buy the +distinction back with a sharper pattern.** Whether a quote opens or +closes a literal is *state*, and a regex sees only the characters around +it, so no lookbehind can separate the two cases. The remedy for a wrong claim is normally "run the measurement" (lesson 84). That does not help here, because the measurement *was* run. When @@ -3238,4 +3242,68 @@ because no file under `src/metrics/` spells one. Lifting the fix's `'a` has no closing quote and treating it as a literal swallows the file the other way — closes both. +**The third gate in the family, and the proof no pattern would have +done** (#1219, PR #1221). `utils/check-diagnostic-prefix.py` matched +raw-string opens with a regex and skipped to the next quote, so three +shapes opened a span that hid every severity literal until it closed: +`let p = "dir/r";`, where the closing quote of an ordinary string +follows an `r`; and an unterminated `r"` inside a trailing `//` or a +`/* … */` comment, neither of which a scanner that only skips +*whole-line* comments can see. The first is the one that settles the +approach — `/` is a legitimate raw-open context, so the character before +`r` carries no information; what differs is that the `"` **closes** a +literal. Stripping comments first fails the same way, because +`"http://x"` truncates mid-literal and opens a phantom span of its own. +Finding where a comment starts already requires knowing whether you are +inside a string, which is the lexer. All three were latent: replaying +both scanners over all 559 tracked Rust files gave identical output. + +**Porting a lexer without porting the shape that makes its tests +discriminate** (#1219). The port's lifetime fixture used an *even* number +of lifetimes, so a greedy `char_literal_end` pairs them off, every bogus +span closes before the offender, and the test passes against the exact +bug it names. The donor's suite had recorded that trap — "Three +lifetimes, not two, and a real char literal after the call" — and the +copy dropped it. When you lift a scanner, lift its fixtures' arithmetic, +not just its assertions. + +--- + +## 89. A positive enumeration and a negative filter differ on what neither names + +**Lesson:** Replacing a bespoke `matches!(A | B)` with a shared +`!is_x && !is_y` predicate does not just move the rule — it changes the +set. The two agree on every kind either one names and disagree on +everything else, so the inputs that move are exactly the ones no one +enumerated and no test covers. Before consolidating, enumerate the node +kinds the container can actually hold, and decide the leftovers +deliberately: adopting the shared answer is usually right, but it is a +decision, not a refactor. (cf. lesson 59 for why you are consolidating, +and lesson 65 for the structural inverse.) + +The direction of the change is what hides it. A positive filter is +closed — a grammar that starts emitting a new kind silently scores zero, +which is lesson 19. A negative filter is open, so the same grammar +change silently scores *one*, and neither shows up as a diff in the +snapshot suite unless a fixture happens to hold the construct. Both +forms read as "the obvious thing" at the call site. + +**The Objective-C block arm inherited three exclusions and two +inclusions** (#1218, PR #1221). The `Objc::BlockLiteral` `nargs` arm +counted `matches!(ParameterDeclaration | VariadicParameter)`, which +bypassed the shared `count_args` and therefore +`Checker::is_empty_param_marker`, so `^(void){ … }` reported one +parameter where zero belongs. Routing it through `count_args` fixed +that and put the comment and punctuation rules on the shared footing +too — those had *happened* to be right, since a `comment` child is not +a `ParameterDeclaration` either, but for a reason no test asserted. It +also changed two shapes nobody had considered: on invalid source +`^(int a,,)` went 1 → 2 as an `ERROR` child began counting, and +`^({ int x; })` went 0 → 1 for a `compound_statement` child. Both were +kept, because both are what the function channel already reported +through the same helper — `int f(int a,,)` gives 2 and +`int g({ int x; })` gives 1 — so the block arm had been the one caller +answering differently. Recording that in the arm's comment is what +stops the next reader from filing it as a regression. + --- diff --git a/src/c_declarator.rs b/src/c_declarator.rs index 29f653e7..2e4607b9 100644 --- a/src/c_declarator.rs +++ b/src/c_declarator.rs @@ -234,6 +234,24 @@ pub(crate) fn declarator_name<'tree, T: Checker>(node: &Node<'tree>) -> Option(node)?.child_by_field_name("declarator"), |link| { diff --git a/src/metrics/container_scope_tests.rs b/src/metrics/container_scope_tests.rs index 96754ad7..7de085d9 100644 --- a/src/metrics/container_scope_tests.rs +++ b/src/metrics/container_scope_tests.rs @@ -50,6 +50,11 @@ struct Emitted { name: Option, has_npm: bool, has_npa: bool, + /// Tracked alongside the other two because `wmc`'s emission is + /// *narrower* than theirs in two language-level ways, and nothing + /// pinned that until #1220 — which is how the book and STABILITY.md + /// came to describe all three blocks with one rule. + has_wmc: bool, } /// Analyses `source` and flattens every space in the serialized tree, @@ -81,6 +86,7 @@ fn flatten(value: &Value, out: &mut Vec) { name: value["name"].as_str().map(str::to_owned), has_npm: metrics.contains_key("npm"), has_npa: metrics.contains_key("npa"), + has_wmc: metrics.contains_key("wmc"), }); for child in value["spaces"].as_array().into_iter().flatten() { flatten(child, out); @@ -652,7 +658,24 @@ fn no_space_is_emitted_with_an_unknown_kind() { /// [`function_spaces_emit_neither`] would still pass if a /// grammar stopped opening these spaces at all; naming them pins that /// they exist *and* stay quiet. +// Gated on the fixtures' own features for the reason its two siblings +// below already are (9d71de34): the case list is six languages wide and +// every row carries its own `cfg`, so a feature set enabling none of +// them leaves an empty list and trips `assert_fixtures_present` — a +// failure that reads as a defect in whatever was being changed rather +// than as an unrelated build configuration. `--no-default-features +// --features rust` is one such set; the canonical minimal-langs +// configuration `rust,typescript` is not, since `typescript` supplies +// two rows (#1220). #[test] +#[cfg(any( + feature = "kotlin", + feature = "java", + feature = "groovy", + feature = "javascript", + feature = "mozjs", + feature = "typescript" +))] fn the_1184_constructs_open_quiet_function_spaces() { let cases: &[(LANG, &[&str])] = &[ #[cfg(feature = "kotlin")] @@ -814,6 +837,89 @@ fn a_cpp_namespace_is_a_member_scope() { namespace.has_npm, namespace.has_npa ); + // The narrowing #1220 found: `wmc` does *not* follow npm/npa here. + // A namespace's member functions are free functions rather than + // methods of a class, so `CppCode::compute` drops `SpaceKind:: + // Namespace` and the space records no kind, leaving the block + // unserialized. The class inside the namespace still carries all + // three — asserted below so this reads as a scope rule rather than + // as wmc being absent from the file. + assert!( + !namespace.has_wmc, + "a namespace weights no per-class complexity and must carry no wmc" + ); + let class = only_space(LANG::Cpp, &spaces, "C"); + assert!( + class.has_wmc && class.has_npm && class.has_npa, + "the class inside the namespace carries all three \ + (wmc={}, npm={}, npa={})", + class.has_wmc, + class.has_npm, + class.has_npa + ); +} + +/// Ruby's `module` is the second construct reaching the `namespace` +/// narrowing, so the rule is space-kind-level rather than a C++ quirk. +/// +/// `Getter::get_space_kind` maps `Module` to `SpaceKind::Namespace` for +/// Ruby exactly as it maps `NamespaceDefinition` for C++ and Mozcpp, and +/// `class_interface_compute` drops that kind for every language, so all +/// three spell the same rule. Pinned beside the C++ case because a doc +/// that names only C++ here reads as a language deviation and sends the +/// next reader looking for a `CppCode` special case that does not exist. +#[test] +#[cfg(feature = "ruby")] +fn a_ruby_module_is_a_namespace_without_wmc() { + let spaces = emitted_spaces(LANG::Ruby, fixture_source(LANG::Ruby)); + let module = only_space(LANG::Ruby, &spaces, "M"); + assert_eq!(module.kind, SpaceKind::Namespace); + assert!( + module.has_npm && module.has_npa && !module.has_wmc, + "a Ruby module carries npm/npa and no wmc \ + (npm={}, npa={}, wmc={})", + module.has_npm, + module.has_npa, + module.has_wmc + ); + let class = only_space(LANG::Ruby, &spaces, "C"); + assert!( + class.has_wmc && class.has_npm && class.has_npa, + "the class inside the module carries all three \ + (wmc={}, npm={}, npa={})", + class.has_wmc, + class.has_npm, + class.has_npa + ); +} + +/// Go emits no `wmc` block on any space, including the file root, while +/// its `npa` and `npm` do appear there (#1220). +/// +/// `GoCode` sits in the `Wmc` no-op list rather than deviating on space +/// kind: Go's flat space model cannot attribute a method to a receiver +/// class, so there is no per-class complexity to weight. It is the one +/// language where the `Wmc` and `Npa` no-op sets differ, which is why +/// the book and STABILITY.md now scope the three-blocks rule to npm/npa +/// and describe wmc's two narrowings separately. Asserting the npa/npm +/// half in the same test is what keeps this from passing for a Go file +/// that had simply stopped emitting anything. +#[test] +#[cfg(feature = "go")] +fn go_emits_npa_and_npm_but_never_wmc() { + let spaces = emitted_spaces(LANG::Go, fixture_source(LANG::Go)); + let root = &spaces[0]; + assert_eq!(root.kind, SpaceKind::Unit); + assert!( + root.has_npm && root.has_npa, + "Go's root carries npa/npm (npm={}, npa={})", + root.has_npm, + root.has_npa + ); + assert!( + !spaces.iter().any(|space| space.has_wmc), + "no Go space may carry a wmc block, including the unit root" + ); } /// A language with no class-shaped construct emits no block at all. diff --git a/src/metrics/nargs.rs b/src/metrics/nargs.rs index e5855f74..c7480d25 100644 --- a/src/metrics/nargs.rs +++ b/src/metrics/nargs.rs @@ -383,15 +383,31 @@ impl NArgs for ObjcCode { }); } Objc::BlockLiteral => { + // Through `count_args`, so the block channel gets the same + // three exclusions `compute_args` gives the function + // channel. Counting `ParameterDeclaration | + // VariadicParameter` positively could not consult + // `Checker::is_empty_param_marker`, so `^(void){ … }` — + // whose `parameter_list` holds a real + // `parameter_declaration` for the `void`, exactly as + // `int f(void)` does — reported one parameter (#1218). + // + // It inherits the shared rule's *inclusions* too, which the + // narrower positive match had excluded by construction: on + // invalid source an `ERROR` child (`^(int a,,)`) or a + // `compound_statement` one (`^({ int x; })`) now counts. + // That is the point rather than a regression — those are + // the numbers `int f(int a,,)` already reported through + // `count_args`, so the block arm stopped being the one + // caller that answered differently. + // + // `ParameterList2` is deliberately not matched: it is the + // alias for the hidden `_old_style_parameter_list`, and + // `block_literal` cannot produce it — even a K&R function + // definition emits `ParameterList`. Marked rather than + // silently omitted per `grammar-dispatch.md` §1/§2. if let Some(params) = node.first_child(|id| Objc::ParameterList == id) { - params.act_on_child(&mut |n| { - if matches!( - n.kind_id().into(), - Objc::ParameterDeclaration | Objc::VariadicParameter - ) { - stats.closure_nargs += 1; - } - }); + stats.closure_nargs += count_args::(¶ms, code); } } _ => {} @@ -4027,6 +4043,122 @@ when HTTP_REQUEST { log local0. \"hit\" } ); } + /// A block's `(void)` marker declares nothing, so `^(void){ … }` is a + /// closure of zero parameters (#1218). + /// + /// The objc grammar reuses C's `parameter_list` rule, so `^(void)` + /// emits a real `parameter_declaration` for the `void` — the same + /// shape `int f(void)` produces, and the reason + /// `Checker::is_empty_param_marker` reads the source bytes rather + /// than the tree. The block arm counted it until it began routing + /// through `count_args`, while the function channel beside it was + /// already correct: `host` below reports 0 either way, which is what + /// makes this a test of the block channel specifically. + #[test] + fn objc_block_void_marker_is_not_a_parameter() { + check_metrics::( + "void host(void) { + void (^empty)(void) = ^(void){ }; + empty(); +} +", + "foo.m", + |metric| { + assert_eq!(metric.nargs.closure_args_sum(), 0); + assert_eq!(metric.nargs.function_args_sum(), 0); + insta::assert_json_snapshot!(metric.nargs, @r#" + { + "function_args": 0, + "closure_args": 0, + "function_args_average": 0.0, + "closure_args_average": 0.0, + "total": 0, + "average": 0.0, + "function_args_min": 0, + "function_args_max": 0, + "closure_args_min": 0, + "closure_args_max": 0 + } + "#); + }, + ); + } + + /// A comment inside a block's parameter list is not a parameter, so + /// `^(int a /* c */, int b){ … }` is 2 (#1201, #1218). + /// + /// **This fixture cannot fail by reverting the block arm.** The + /// positive `matches!(ParameterDeclaration | VariadicParameter)` the + /// arm used before #1218 already ignored a `comment` child, so the + /// count was correct for the wrong reason — nothing asserted it, and + /// the #1201 changelog claimed Objective-C blocks were swept when + /// only the method fixture existed. It became load-bearing when the + /// arm switched to `count_args`, whose *negative* filtering is what + /// now makes `Checker::is_comment` live on this path. Perturb it by + /// dropping `is_comment` from `count_args`, not by reverting the arm. + #[test] + fn objc_block_comment_is_not_a_parameter() { + check_metrics::( + "void host(void) { + void (^two)(int, int) = ^(int a /* c */, int b){ }; + two(1, 2); +} +", + "foo.m", + |metric| { + assert_eq!(metric.nargs.closure_args_sum(), 2); + assert_eq!(metric.nargs.function_args_sum(), 0); + }, + ); + } + + /// A variadic block keeps its `...` counted: `^(int a, ...){ … }` is 2. + /// + /// The guard against #1218's fix, not against #1218. Swapping the + /// arm's positive `matches!` for `count_args`' negative filters is + /// what could silently drop `variadic_parameter` — it is named in the + /// old match and in none of the new filters, so only a fixture says + /// whether it survived. `ObjcCode::is_non_arg` covers the list's + /// punctuation (`(`, `,`, `)`) and nothing else, so it does. + #[test] + fn objc_block_variadic_parameter_still_counts() { + check_metrics::( + "void host(void) { + void (^var)(int, ...) = ^(int a, ...){ }; + var(1); +} +", + "foo.m", + |metric| { + assert_eq!(metric.nargs.closure_args_sum(), 2); + assert_eq!(metric.nargs.function_args_sum(), 0); + }, + ); + } + + /// A block written without a parameter list at all is 0. + /// + /// `^{ }` has no `parameter_list` child, so the arm's + /// `first_child(ParameterList)` guard short-circuits before + /// `count_args` is reached. Pinned beside the `^(void)` case because + /// the two spellings mean the same thing and only one of them ever + /// went through the counting path. + #[test] + fn objc_block_without_a_parameter_list_is_zero() { + check_metrics::( + "void host(void) { + void (^none)(void) = ^{ }; + none(); +} +", + "foo.m", + |metric| { + assert_eq!(metric.nargs.closure_args_sum(), 0); + assert_eq!(metric.nargs.function_args_sum(), 0); + }, + ); + } + /// Regression for #782: the textual `Display` headline must report /// the cross-space *sum* (`function_args_sum`/`closure_args_sum`), /// matching the JSON/YAML/TOML/CBOR serializers, not the per-space @@ -4203,11 +4335,19 @@ mod lambda_parenthesisation_parity { /// which already excluded comments and is here as a no-change guard on /// its collapse onto the shared `count_args`. /// -/// Go, Lua, Objective-C methods and blocks, Kotlin *functions* and -/// Groovy *closures* were already correct — each filters positively for -/// its parameter kind — and are swept anyway, because "this one is a +/// Go, Lua, Objective-C *methods*, Kotlin *functions* and Groovy +/// *closures* were already correct — each filters positively for its +/// parameter kind — and are swept anyway, because "this one is a /// positive filter" is the reasoning that has to hold for a grammar /// bump, not just for today. +/// +/// Objective-C *blocks* were on that list until #1218 and are not any +/// more: their arm now routes through the shared `count_args`, so what +/// keeps a comment out of a block's count is the same negative filter +/// the repaired languages rely on, not a positive parameter-kind match. +/// Their fixture is `objc_block_comment_is_not_a_parameter`, which sits +/// in the module above beside the `^(void)` case that motivated the +/// move rather than in the table below. #[cfg(test)] mod comments_in_parameter_lists { use crate::test_support::metrics_verbatim; @@ -4436,19 +4576,53 @@ mod comments_in_parameter_lists { /// `compute_tcl_args` never needed the exclusion the other /// languages did. Issue #1201 cited Tcl as the language that had /// already solved this; it had not — it has no problem to solve. - #[test] + /// + /// **Both rows assert parity over a non-problem, not a defence.** + /// Neither language has an exclusion here that a regression could + /// remove; what these pin is that the *shape* stays the one described + /// above, so a grammar bump that started emitting a comment node + /// would surface as a count change rather than silently. The iRules + /// row is the second half of a claim this doc and the #1201 changelog + /// entry both made while only Tcl was exercised (#1218). It is a + /// separate dialect grammar, and dialect grammars do diverge on leaf + /// naming — its `argument` is kind 137 against Tcl's 93 — so it was + /// dumped rather than assumed: `proc h {a\n# c\n b}` yields four + /// `argument` nodes and no comment node under both. + // Gated on the fixtures' own features for the reason #1220 names: the + // case list is two languages wide and the loop below asserts it ran, so + // a feature set enabling neither — `--no-default-features --features + // rust` — would fail here and read as a defect in whatever was being + // changed. The gate makes the test absent rather than vacuous; the + // `ran > 0` assertion then covers the narrower case where `is_enabled` + // stops agreeing with the feature it is compiled under. + #[test] + #[cfg(any(feature = "tcl", feature = "irules"))] fn tcl_has_no_comment_inside_a_parameter_list() { - if !LANG::Tcl.is_enabled() { - return; + // Guarded per language rather than once: the two features are + // independent, so a build with only one enabled must still run + // that one's row. + let mut ran = 0; + for lang in [LANG::Tcl, LANG::Irules] + .into_iter() + .filter(LANG::is_enabled) + { + ran += 1; + assert_eq!( + args(lang, "proc h {a\n # c\n b} { return $a }"), + (0, 4), + "{lang:?}: a `#` in an argument list is an argument named `#`, not a comment" + ); + // The uncommented control, so a regression that zeroed the + // whole count would not read as this rule holding. + assert_eq!(args(lang, "proc h {a b} { return $a }"), (0, 2), "{lang:?}"); } - assert_eq!( - args(LANG::Tcl, "proc h {a\n # c\n b} { return $a }"), - (0, 4), - "a `#` in a Tcl argument list is an argument named `#`, not a comment" + // A feature set enabling neither leaves a loop of zero iterations + // and a test that passes having asserted nothing — the shape + // `assert_fixtures_present` exists to make loud (#1220). + assert!( + ran > 0, + "neither tcl nor irules is enabled; this test asserted nothing" ); - // The uncommented control, so a regression that zeroed the whole - // count would not read as this rule holding. - assert_eq!(args(LANG::Tcl, "proc h {a b} { return $a }"), (0, 2)); } /// The one path `count_args` never runs on: `Checker::is_bare_param` diff --git a/utils/check-diagnostic-prefix-test.py b/utils/check-diagnostic-prefix-test.py index df8c8c82..f33ff158 100644 --- a/utils/check-diagnostic-prefix-test.py +++ b/utils/check-diagnostic-prefix-test.py @@ -159,6 +159,106 @@ def test_a_string_holding_only_r_does_not_open_a_raw_string(self) -> None: text = 'Ruby::R => "r",\neprintln!("Error: x");\n' self.assertEqual([(2, "Error")], [(n, w) for n, w, _ in GATE.scan_text(text)]) + def test_a_string_ending_in_r_does_not_open_a_raw_string(self) -> None: + # `"dir/r"` closes an ordinary string, but its final `r"` is a + # textbook raw-string opener and `/` is a legitimate context for + # one, so the old regex read it as opening a multi-line raw + # string and skipped everything to the next quote (#1219). No + # lookbehind can express the difference — what distinguishes this + # case is that the `"` *closes* a literal, which is state. + text = 'let p = "dir/r";\neprintln!("Warning: hidden");\n' + self.assertEqual([(2, "Warning")], [(n, w) for n, w, _ in GATE.scan_text(text)]) + + def test_a_string_ending_in_a_plain_letter_is_the_control(self) -> None: + # The near miss that discriminates. It passes against the old + # scanner too — that is the point: it is what stops the case + # above from being satisfied by a gate that had simply given up + # on raw strings altogether, which would "fix" #1219 by removing + # the skip that the multi-line-fixture rule depends on. + text = 'let p = "dir/x";\neprintln!("Warning: hidden");\n' + self.assertEqual([(2, "Warning")], [(n, w) for n, w, _ in GATE.scan_text(text)]) + + def test_a_trailing_line_comment_cannot_open_a_raw_string(self) -> None: + # The old scanner skipped a line only when the *whole* line was a + # comment, so an unterminated `r"` in a trailing one opened a + # phantom span (#1219). Stripping comments by regex first is not + # the fix — see the `http://` case below. + text = 'let x = 1; // e.g. r"foo\neprintln!("Warning: hidden");\n' + self.assertEqual([(2, "Warning")], [(n, w) for n, w, _ in GATE.scan_text(text)]) + + def test_a_block_comment_cannot_open_a_raw_string(self) -> None: + # The third window of the same class, which the issue did not + # name: block comments were invisible to the line-oriented + # scanner in either position, whole-line or trailing. + text = 'let x = 1; /* r"foo */\neprintln!("Warning: hidden");\n' + self.assertEqual([(2, "Warning")], [(n, w) for n, w, _ in GATE.scan_text(text)]) + + def test_a_url_in_a_literal_is_not_read_as_a_comment(self) -> None: + # Passes against the old scanner as well; it guards the *fix* + # rather than the bug. The cheap route to #1219's window 2 is to + # strip trailing comments with a regex, and that route cuts + # `"http://x"` mid-literal, leaving an unbalanced quote that + # opens a phantom span of its own — one false-clean window traded + # for another. This fails the moment anyone reaches for it. + text = 'let u = "http://x";\neprintln!("Error: y");\n' + self.assertEqual([(2, "Error")], [(n, w) for n, w, _ in GATE.scan_text(text)]) + + def test_a_char_literal_holding_a_quote_does_not_open_a_string(self) -> None: + # Inherited from the ported lexer rather than fixed here: the + # old scanner passed this too, having no notion of a char literal + # but also no string-span tracking for one to corrupt. Pinned + # because the port *introduced* the machinery that can get it + # wrong — `b'"'` holds an unpaired double quote, and #1192 is the + # sibling gate shipping exactly that bug. + text = "let q = b'\"';\neprintln!(\"Error: z\");\n" + self.assertEqual([(2, "Error")], [(n, w) for n, w, _ in GATE.scan_text(text)]) + + def test_a_lifetime_does_not_open_a_char_span(self) -> None: + # The other half of #1192's char-literal rule, and likewise a + # pin on the new machinery rather than a #1219 regression guard: + # a lifetime has the opening quote and no terminator, so reading + # it as a literal swallows the rest of the file the other way. + # + # Three lifetimes, not two, and a real char literal after the + # offender — the quote count before it is the whole discriminator. + # A greedy variant that scans from one `'` to the next pairs them + # off, so with an *even* number ahead of the offender every bogus + # span closes before reaching it and the test passes against the + # bug it names. With three, the last pairs with the `'z'` below + # and the span swallows the offender. Copied deliberately from + # `check-snapshot-anchors-test.py`, whose version of this test + # records the same trap. + text = ( + "fn f<'a>(x: &'a str, y: &'a str) {\n" + ' eprintln!("Error: w");\n' + " let c = 'z';\n" + "}\n" + ) + self.assertEqual([(2, "Error")], [(n, w) for n, w, _ in GATE.scan_text(text)]) + + def test_an_escaped_quote_does_not_hide_a_later_literal(self) -> None: + # `regular_string_end` consumes `\"` as one escaped character, so + # `"a\"b"` is a single span. Without that branch the span closes + # at the inner quote and the *next* one reopens it, swallowing + # everything to the following quote — a false clean of the same + # class as #1219's windows. The obvious candidate for this + # coverage, `test_escaped_inner_quote_is_prose_not_a_prefix`, + # does not provide it: under the perturbation its input merely + # splits into two spans that also yield no hit. + text = 'let s = "a\\"b";\neprintln!("Error: real");\n' + self.assertEqual([(2, "Error")], [(n, w) for n, w, _ in GATE.scan_text(text)]) + + def test_severity_inside_a_trailing_comment_is_not_a_prefix(self) -> None: + # A widening the lexer brings with it: before, only a *whole-line* + # comment was skipped, so a severity quoted in a trailing one was + # reported and needed a `diag-prefix-ok` marker. A comment is + # never a diagnostic, so this is the same direction the whole-line + # rule already took. + self.assertEqual(GATE.scan_text('let x = 1; // never write "Warning: x"\n'), []) + + def test_severity_inside_a_block_comment_is_not_a_prefix(self) -> None: + self.assertEqual(GATE.scan_text('/* never write "Warning: x" */\n'), []) + def test_hash_count_must_match_to_close_a_raw_string(self) -> None: # A bare `"#` inside an `r##"…"##` fixture does not terminate it, # so the lines after it are still fixture data. @@ -198,6 +298,79 @@ def test_blank_line_breaks_the_comment_block(self) -> None: self.assertEqual(len(GATE.scan_text(text)), 1) +class PortedLexerHelpers(unittest.TestCase): + """Direct tests on the three helpers ported from check-snapshot-anchors. + + ``_scan_literals``' docstring tells the next reader to fix a lexing + bug in one gate and check the other. That instruction needs something + behind it: end-to-end ``scan_text`` cases do not fail when block-comment + nesting, the unterminated-raw rule, or the lifetime rejection is + perturbed in *this* copy, because no current input distinguishes them. + These mirror the donor's ``CharLiteralEndTest`` / ``RawStringEndTest`` + so a divergence introduced here is caught here. + """ + + def test_char_literal_end_accepts_real_literals(self) -> None: + for source, expected in ( + ("'a'", 3), + ("'\"'", 3), + ("'\\''", 4), + ("'\\\\'", 4), + ("'\\n'", 4), + ("'\\x41'", 6), + ("'\\u{1F600}'", 11), + ): + with self.subTest(source=source): + self.assertEqual(GATE.char_literal_end(source, 0), expected) + + def test_char_literal_end_rejects_lifetimes_and_labels(self) -> None: + # Returning None is what stops a lifetime opening a span that + # swallows the rest of the file (#1192). + for source in ("'a>", "'_,", "'outer:", "'static ", "'"): + with self.subTest(source=source): + self.assertIsNone(GATE.char_literal_end(source, 0)) + + def test_raw_string_end_covers_every_spelling(self) -> None: + for source, expected in ( + ('r"x"', 4), + ('r#"x"#', 6), + ('r##"x"##', 8), + ('br"x"', 5), + ('br##"x"##', 9), + # A `"##` inside an `r###"…"###` does not close it. + ('r###"a "## b"###', 16), + ): + with self.subTest(source=source): + self.assertEqual(GATE.raw_string_end(source, 0), expected) + + def test_raw_string_end_rejects_non_raw_openers(self) -> None: + for source in ('b"x"', "rust", "bar", "r", "r#"): + with self.subTest(source=source): + self.assertIsNone(GATE.raw_string_end(source, 0)) + + def test_raw_string_end_runs_to_eof_when_unterminated(self) -> None: + # The deliberate "skip to the close" behaviour the line-oriented + # scanner had, now reachable only from real code position. + source = 'r#"never closed' + self.assertEqual(GATE.raw_string_end(source, 0), len(source)) + + def test_regular_string_end_consumes_escapes(self) -> None: + for source, expected in ( + ('"x"', 3), + ('"a\\"b"', 6), + ('"a\\\\"', 5), + ('"\\n"', 4), + ): + with self.subTest(source=source): + self.assertEqual(GATE.regular_string_end(source, 0), expected) + + def test_block_comments_nest(self) -> None: + # Rust allows nesting; stopping at the first `*/` would leave the + # trailing ` */` as code and a stray quote in it could open a span. + text = '/* outer /* inner */ still comment "Error: x" */\neprintln!("Error: y");\n' + self.assertEqual([(2, "Error")], [(n, w) for n, w, _ in GATE.scan_text(text)]) + + class MainOverSyntheticRoot(unittest.TestCase): """``main()`` end-to-end over a temporary tree.""" diff --git a/utils/check-diagnostic-prefix.py b/utils/check-diagnostic-prefix.py index c87d313c..7e089f86 100644 --- a/utils/check-diagnostic-prefix.py +++ b/utils/check-diagnostic-prefix.py @@ -31,23 +31,34 @@ * The match is anchored at the *start* of a string literal, so prose that merely contains the word — in a doc comment, or as an escaped inner quotation (``"he said \\"Error: no\\""``) — is not a hit. -* Whole-line comments are skipped, so a ``//`` explaining this rule — - including the examples above — does not trip it. -* The interior of a *multi-line* raw string is skipped. That is where - this workspace's embedded source fixtures live, real foreign code - says ``std::cerr << "Warning: …"``, and neither opt-out position is - reachable: both sit inside the fixture, so writing a marker would - change the text the metric test measures. +* Comments are skipped — ``//`` and ``/* … */``, whole-line or + trailing — so a comment explaining this rule, including the examples + above, does not trip it. A comment is never a diagnostic. +* The interior of a string literal is skipped — only what a literal + *starts with* is inspected. That matters most for the multi-line raw + strings this workspace's embedded source fixtures live in, where real + foreign code says ``std::cerr << "Warning: …"`` and neither opt-out + position is reachable: both sit inside the fixture, so writing a + marker would change the text the metric test measures. * Anything else that legitimately contains such a literal opts out with ``// diag-prefix-ok`` on the same line or in the comment block above it. One site uses it today: the #609 regression test that asserts the capitalised spelling is *absent* from `bca check` stderr. +Which of those a given position falls under is decided by a single +left-to-right lexer (``_scan_literals``) rather than by matching each +line in isolation. That is not an implementation detail: three +false-clean windows came from deciding it per line, because something +that merely *looks* like a raw-string open — a plain string ending in +``r``, an unterminated ``r"`` inside a trailing or block comment — was +read as one, and every line to the next quote was then skipped (#1219). + See AGENTS.md "Validation gates" for the policy this enforces. """ from __future__ import annotations +import bisect import pathlib import re import subprocess @@ -59,27 +70,11 @@ SKIP_DIRS = {".git", "target", "node_modules", ".venv", "__pycache__"} -# A string literal — plain, raw, or hash-delimited raw — whose first -# characters are a capitalised severity word followed by a colon. -# -# The `(?Warning|WARNING|Error|ERROR|Note|NOTE):' -) - -# The opening delimiter of a raw (or raw byte) string literal, capturing -# its hash count so the matching terminator can be searched for. -# -# The lookbehind excludes `"` and `\` as well as word characters, because -# without them an ordinary string is read as opening a raw one and every -# line until the next quote is skipped. Both shapes are live in this -# tree: `line.strip_suffix(b"\r")` ends in `\r"`, and `Ruby::R => "r",` -# ends in `"r"`. Measured across 559 files, dropping the two characters -# turns 99 candidate multi-line opens into 27. -RAW_STRING_OPEN = re.compile(r'(?#*)"') +# The severity word a literal's *content* may begin with, anchored: the +# gate is about what a literal starts with, not about the word appearing +# in one. `_scan_literals` supplies the content start, so the anchor is +# structural here rather than a lookbehind on the quote. +SEVERITY_PREFIX = re.compile(r"(?PWarning|WARNING|Error|ERROR|Note|NOTE):") # Opt-out marker for a literal that is data rather than a diagnostic. ALLOW_MARKER = "diag-prefix-ok" @@ -127,20 +122,182 @@ def _is_allowed(lines: list[str], index: int) -> bool: return False -def _unterminated_raw_open(segment: str) -> int | None: - """Hash count of a raw string ``segment`` opens and does not close. +def char_literal_end(source: str, i: int) -> int | None: + """End index (exclusive) of the char literal at ``i``, else ``None``. - ``None`` when every raw string opened here also closes here, which is - the ordinary case — a one-line ``r"…"`` stays in scope for the scan. + Rust spells lifetimes (``'a``), anonymous lifetimes (``'_``) and loop + labels (``'outer:``) with the same leading quote and no terminator, + so the two are told apart by looking for the closing ``'``. Returning + ``None`` for a lifetime is what keeps it from opening a span that + swallows the rest of the file. + """ + n = len(source) + j = i + 1 + if j >= n: + return None + if source[j] == "\\": + j += 1 + if j >= n: + return None + if source[j] == "u": + # '\u{1F600}' — braced, variable length. + close = source.find("}", j) + if close == -1: + return None + j = close + 1 + elif source[j] == "x": + # '\x41' — always exactly two hex digits. + j += 3 + else: + j += 1 + else: + j += 1 + return j + 1 if j < n and source[j] == "'" else None + + +def raw_string_end(source: str, i: int) -> int | None: + """End index (exclusive) of the raw string at ``i``, else ``None``. + + Covers ``r"…"``, ``r#"…"#`` and the byte spellings ``br"…"`` / + ``br##"…"##``. A plain ``b"…"`` needs no special case: the ``b`` is + consumed as an ordinary character and the ``"`` after it opens a + regular literal, which escapes identically. An unterminated open + returns the end of the source, so the rest of the file is one span — + the same "skip to the close" behaviour the line-oriented scanner had, + now reachable only from real code position. + """ + n = len(source) + j = i + if source[j] == "b" and j + 1 < n and source[j + 1] == "r": + j += 1 + if j >= n or source[j] != "r": + return None + j += 1 + hashes = 0 + while j < n and source[j] == "#": + hashes += 1 + j += 1 + if j >= n or source[j] != '"': + return None + close = '"' + ("#" * hashes) + end = source.find(close, j + 1) + return n if end == -1 else end + len(close) + + +def regular_string_end(source: str, i: int) -> int: + """End index (exclusive) of the ``"``-delimited literal at ``i``.""" + n = len(source) + j = i + 1 + while j < n: + if source[j] == "\\" and j + 1 < n: + j += 2 + continue + if source[j] == '"': + break + j += 1 + return j + 1 + + +def _record(hits: list[tuple[int, str]], source: str, content_start: int) -> None: + """Append a hit when the literal content at ``content_start`` offends.""" + match = SEVERITY_PREFIX.match(source, content_start) + if match is not None: + hits.append((content_start, match.group("word"))) + + +def _scan_literals(source: str) -> list[tuple[int, str]]: + """``(content start index, severity word)`` for every offending literal. + + One left-to-right walk that knows, at each position, whether it is in + code, a comment, or a literal — ported from ``scan_ignore_spans`` in + ``check-snapshot-anchors.py``, which needed the identical machine and + is gated by its own self-tests (#1192). The two are siblings; fix a + lexing bug in one and check the other. They are copies rather than a + shared import because every gate under ``utils/`` is a standalone + hyphen-named script that resolves the repository root from its own + location, and a shared module would grow that concern on four sides. + + Deciding this from *state* rather than from a regex over each line is + what closes three false-clean windows (#1219). All three are the same + class — something that merely looks like a raw-string open is read as + one, and every line until the next quote is then skipped, hiding any + offender in between: + + * ``let p = "dir/r";`` — the closing quote of an ordinary string sits + after ``r``, which is a legitimate raw-open context. No lookbehind + can express the difference, because what distinguishes this case is + that the ``"`` *closes* a literal rather than opening one. + * ``let x = 1; // e.g. r"foo`` — a *trailing* comment. The previous + scanner skipped a line only when the whole line was a comment. + * ``let x = 1; /* r"foo */`` — a block comment, which it never saw at + all. + + Stripping comments with a regex first cannot fix the latter two: + ``"http://x"`` would be truncated mid-literal and could open a + phantom span itself. Finding where a comment starts already requires + knowing whether you are inside a string. + + A consequence worth stating: a severity literal quoted inside *any* + comment is now skipped, where before only a whole-line comment was. + That is the same direction the whole-line rule already took — a + comment is never a diagnostic — and it removes a false-positive class + that would otherwise have needed a ``diag-prefix-ok`` marker. """ - pos = 0 - while (opening := RAW_STRING_OPEN.search(segment, pos)) is not None: - terminator = '"' + "#" * len(opening.group("hashes")) - close = segment.find(terminator, opening.end()) - if close < 0: - return len(opening.group("hashes")) - pos = close + len(terminator) - return None + hits: list[tuple[int, str]] = [] + i = 0 + n = len(source) + while i < n: + ch = source[i] + # Line comment: consume to (not including) the newline. + if ch == "/" and i + 1 < n and source[i + 1] == "/": + nl = source.find("\n", i) + i = n if nl == -1 else nl + continue + # Block comment, which Rust allows to nest. + if ch == "/" and i + 1 < n and source[i + 1] == "*": + depth = 1 + i += 2 + while i < n and depth > 0: + if source[i] == "/" and i + 1 < n and source[i + 1] == "*": + depth += 1 + i += 2 + continue + if source[i] == "*" and i + 1 < n and source[i + 1] == "/": + depth -= 1 + i += 2 + continue + i += 1 + continue + # Raw / byte-raw string. Only its opening delimiter is inspected: + # the interior of a multi-line one holds this workspace's embedded + # source fixtures, where neither opt-out position is reachable. + if ch in "rb": + stop = raw_string_end(source, i) + if stop is not None: + # Safe: between `i` and here `raw_string_end` consumed only + # `b`, `r` and `#`, none of which is a quote, and it has + # already verified the quote it stopped on. + quote = source.index('"', i) + _record(hits, source, quote + 1) + i = stop + continue + # Char literal — or a lifetime, which `char_literal_end` rejects + # so it cannot open a span that swallows the rest of the file. + if ch == "'": + stop = char_literal_end(source, i) + i = stop if stop is not None else i + 1 + continue + # Regular string literal. Escapes are consumed by + # `regular_string_end`, so `"he said \"Error: no\""` is one span + # whose content begins at `he`, and the inner quote — which never + # starts a literal — cannot register as a prefix. + if ch == '"': + stop = regular_string_end(source, i) + _record(hits, source, i + 1) + i = stop + continue + i += 1 + return hits def scan_text(text: str) -> list[tuple[int, str, str]]: @@ -148,44 +305,54 @@ def scan_text(text: str) -> list[tuple[int, str, str]]: Line numbers are 1-based so they paste straight into an editor. - The interior of a *multi-line* raw string is skipped: those hold the - embedded source fixtures this workspace tests against, and real + Only what a string literal *starts with* is inspected, so the + interior of every literal is skipped — including the multi-line raw + strings this workspace's embedded source fixtures live in, where real foreign code says ``std::cerr << "Warning: …"``. Neither opt-out - position works in there — the marker would have to sit on the + position works in there: the marker would have to sit on the offending line or in a ``//`` block above it, and both are inside the fixture, so writing one changes the very text the metric test - measures. A raw string that opens and closes on one line is still - scanned; a diagnostic is never emitted from inside a fixture, so the + measures. A diagnostic is never emitted from inside a fixture, so the skip costs no coverage. """ - offenders: list[tuple[int, str, str]] = [] # `split("\n")`, not `splitlines()`: the latter also breaks on U+2028 # and the vertical-tab family, which rustc does not treat as line # terminators, so one inside a string literal would shift every - # reported line number past it. + # reported line number past it. `_line_of` counts the same `\n`, so + # the two agree by construction. lines = text.split("\n") - open_hashes: int | None = None - for index, line in enumerate(lines): - if open_hashes is not None: - terminator = '"' + "#" * open_hashes - close = line.find(terminator) - if close < 0: - continue - segment = line[close + len(terminator) :] - open_hashes = None - else: - segment = line - # A `//` line cannot open a raw string either, so this returns - # before the open-tracking below. - if segment.lstrip().startswith("//"): - continue - if not _is_allowed(lines, index): - offenders.extend( - (index + 1, match.group("word"), line.lstrip()) - for match in SEVERITY_LITERAL.finditer(segment) - ) - open_hashes = _unterminated_raw_open(segment) - return offenders + starts = _line_starts(text) + return [ + (index + 1, word, lines[index].lstrip()) + for offset, word in _scan_literals(text) + for index in (_line_of(starts, offset),) + if not _is_allowed(lines, index) + ] + + +def _line_starts(text: str) -> list[int]: + """Start offset of each line, indexed as ``text.split("\\n")`` is.""" + starts = [0] + pos = text.find("\n") + while pos != -1: + starts.append(pos + 1) + pos = text.find("\n", pos + 1) + return starts + + +def _line_of(starts: list[int], offset: int) -> int: + """0-based index of the line containing ``offset``. + + ``bisect_left`` here would differ only for an ``offset`` that is + itself a line start, and no recorded offset can be one: every offset + ``_record`` receives is ``quote + 1`` for a position holding ``"``, + so the character before it is never a newline. The two spellings are + therefore indistinguishable by any input this gate can produce — a + perturbation swapping them fails nothing, and that is a property of + the call site rather than a gap in the tests. ``bisect_right`` is + still the correct spelling of the intent. + """ + return bisect.bisect_right(starts, offset) - 1 def _report(offenders: list[tuple[pathlib.Path, int, str, str]]) -> None: diff --git a/utils/check-snapshot-anchors.py b/utils/check-snapshot-anchors.py index 379ef5a7..eef1e213 100755 --- a/utils/check-snapshot-anchors.py +++ b/utils/check-snapshot-anchors.py @@ -246,6 +246,15 @@ def scan_ignore_spans(source: str) -> IgnoreSpans: report zero for a file that really does carry a bare snapshot — the exact "reads as clean" failure it exists to prevent, which must not be its own failure mode. + + ``check-diagnostic-prefix.py`` carries a port of this walk, for the + same reason and against the same failure (#1219). The two are + siblings rather than one shared module because every gate under + ``utils/`` is a standalone hyphen-named script resolving the + repository root from its own location, and each self-test loads its + subject through a ``_load_module()`` shim — a shared import target + grows that concern on four sides. Fix a lexing bug here and check + there. """ spans = IgnoreSpans() i = 0