fix: Objc block nargs, diagnostic-prefix lexer, and 2.1.0 delta-review polish - #1221
Merged
Conversation
The `Objc::BlockLiteral` arm counted parameters with a positive
`matches!(ParameterDeclaration | VariadicParameter)`, bypassing the
shared `count_args` helper and therefore `Checker::is_empty_param_marker`.
The objc grammar reuses C's `parameter_list` rule, so `^(void){ … }`
emits a real `parameter_declaration` for the `void` and reported one
parameter where zero belongs. The function channel beside it was already
correct, routing through `compute_args` -> `count_args`.
Routing the block arm through `count_args` closes the whole class rather
than this one member: the block channel now inherits the comment and
punctuation exclusions too. It inherits the shared rule's inclusions as
well -- an `ERROR` or `compound_statement` child of an invalid parameter
list now counts -- which makes the arm agree with every other
`count_args` caller instead of being the one that answered differently.
Swept the siblings per grammar-dispatch.md: `is_empty_param_marker` is
implemented only by C, C++, Mozcpp and Objc, and the first three reach
both their channels through `compute_args`. Objc's `MethodDefinition`
arm keeps its positive match -- a method's `(void)` is a return type,
never a parameter. `ParameterList2` is verified unreachable from
`block_literal` and is now marked as excluded rather than omitted.
Fixtures cover all four block spellings. Only the `^(void)` one can fail
against the old arm; the comment guard becomes load-bearing through
`count_args`' negative filtering and is perturbed there instead, which
its doc comment says so it does not read as coverage it is not.
Also pins the iRules half of a parity claim `tcl_has_no_comment_inside_
a_parameter_list` and the #1201 changelog entry both made while only Tcl
was exercised. Confirmed by dump before asserting: iRules emits four
`argument` nodes and no comment node, as Tcl does.
Fixes #1218
`check-diagnostic-prefix.py` decided per line whether it sat inside a
multi-line raw string, using a regex for raw-string opens. Three inputs
made it read an open where there was none, after which every line to the
next quote was skipped and any severity literal in between was a false
clean -- the one outcome a gate whose docstring names that failure must
not have:
let p = "dir/r"; a closing quote after `r`
let x = 1; // r"foo an unterminated open in a trailing comment
let x = 1; /* r"foo */ the same in a block comment, which the
line scanner never saw in any position
Only the first two are in #1219. The third is the same class and falls
out of the same cure.
Neither of the cheap fixes works. No lookbehind can express the first:
`/` is a legitimate raw-open context, and what distinguishes the case is
that the `"` *closes* a literal, which is state. Stripping comments by
regex cannot fix the others, because `"http://x"` would be truncated
mid-literal and open a phantom span itself -- finding where a comment
starts already requires knowing whether you are inside a string.
So the scan now runs off one left-to-right lexer ported from
`scan_ignore_spans` in `check-snapshot-anchors.py`, which needed the
identical machine and is gated by its own self-tests (#1192). Ported
rather than shared: every gate under `utils/` is a standalone
hyphen-named script resolving the repo root from its own location, with
a `_load_module()` shim per self-test, so a shared import target grows
that concern on four sides. Both sides now name the other.
Matching moves from the quote to the literal's *content start*, which
the lexer knows exactly, so the escaped-quote rule stops needing a
lookbehind and follows from `"he said \"Error: no\""` being one span.
One deliberate widening: a severity quoted inside any comment is now
skipped, where before only a whole-line comment was. A comment is never
a diagnostic, and this removes a false-positive class that would
otherwise have needed a `diag-prefix-ok` marker.
Latent -- no live count changed. Verified by diffing both scanners over
all 559 tracked Rust files: identical output, zero hits, and the gate
still reports the same file count.
Self-tests go 26 -> 43, both directions per the #1192 precedent. Five
fail against the old scanner; the rest pin the ported machinery, and
each says which it is. The lifetime fixture uses three lifetimes rather
than two on purpose -- with an even number a greedy variant pairs them
off and the test passes against the bug it names, a trap the donor's
own suite records.
Fixes #1219
Gate `the_1184_constructs_open_quiet_function_spaces` on its fixtures' own features, as its two siblings were in 9d71de3. Every row of its case list carries a `cfg`, so a feature set enabling none of the six left an empty list and tripped `assert_fixtures_present` -- a failure that reads as a defect in whatever was being changed. `--no-default-features --features rust` is one such set; the canonical minimal-langs configuration `rust,typescript` is not, since `typescript` supplies two of the seven rows. Scope the "three object-oriented blocks" rule in the book and in STABILITY.md. It held for npm/npa and was wrong for wmc in two language-level ways, both measured rather than inferred: Go emits no `wmc` block on any space including the unit root, while its npa/npm do appear there; and a C++ namespace carries npm/npa but no wmc, because its member functions are free functions rather than methods of a class. STABILITY.md's "no grammar deviates from that rule in either direction" was the clause most in need of it -- npm/npa are gated centrally by space kind, wmc is decided per language and never had such a gate. The issue scoped this to Go. The namespace half is the same sentence being wrong a second time, and STABILITY.md is a contract document being read during release prep, so both are corrected together. Nothing pinned wmc's emission scope -- `has_wmc` did not exist and no test reads either document -- which is how one rule came to describe three blocks. `Emitted` now tracks it, and both narrowings are asserted. Verified by perturbation: letting `Namespace` through `class_interface_compute` fails the namespace test alone, and giving Go a real `Wmc` impl fails the Go test alone. Record the ERROR-tree divergence in `declarator_name` rather than undoing it. Where a `function_declarator` lacks its `declarator` field the pre-99180326 loop returned `None` and the `successors` chain yields the whole declarator span. No caller observes it: each getter gates the result on its own identifier kinds and `function_declarator` is in none of those lists, so `get_func_space_name` returns `None` either way. Restoring the `None` would buy nothing observable while reintroducing the two uncoverable arms 9918032 removed, so the comment now names the getter gates as the dependency that keeps it unobservable. Fixes #1220
Records #1219 and #1220, and corrects two claims the #1218 fix made false. The `(void)` entry said "Fixed for C, C++, Mozcpp and Objective-C" while only Objective-C's function channel was; the #1201 entry listed Objective-C blocks among the languages "already correct and unchanged", which held for comments but not for the marker, and was untested either way. All three entries are inside `## [Unreleased]`, so this amends text that has not shipped rather than rewriting release history.
Review of the batch found the #1220 doc fix wrong in one direction and incomplete in the other, which is the same failure #1220 set out to correct. The book said "a C++ or Objective-C `namespace` space". Objective-C has no namespace construct -- `getter/objc.rs` has no `NamespaceDefinition` arm -- and Ruby's `module`, which does map to `SpaceKind::Namespace`, was omitted. Exactly three constructs reach that kind: C++ and Mozcpp `namespace`, and Ruby `module`. Measured: a Ruby `module M` carries npm/npa and no wmc, and the class inside it carries all three. So the narrowing is space-kind-level rather than a per-language quirk, and the book and CHANGELOG now say that instead of naming one spelling. Naming only C++ reads as a language deviation and sends the next reader looking for a `CppCode` special case that does not exist. Pins Ruby alongside the C++ case in `container_scope_tests.rs`. It was unasserted: perturbing `RubyCode::Wmc::compute` to remap `Namespace -> Class` failed zero tests before, and fails only the new one after. Also corrects `comments_in_parameter_lists`' module doc, which still listed Objective-C blocks among the languages that "filter positively for their parameter kind". The preceding commit moved them onto `count_args`' negative filtering; the CHANGELOG's copy of that claim was corrected in this batch and the in-source one was not.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1221 +/- ##
=======================================
Coverage 98.33% 98.33%
=======================================
Files 278 278
Lines 71979 72043 +64
Branches 71549 71613 +64
=======================================
+ Hits 70779 70845 +66
+ Misses 790 788 -2
Partials 410 410
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The `if !lang.is_enabled() { continue; }` guard was the one uncovered
line in this branch's diff: the coverage job runs `--all-features`, so
both languages are enabled and the `continue` is unreachable by
construction there.
Filtering at iteration is what the module's sibling sweeps already do
(`LANG::into_enum_iter().filter(LANG::is_enabled)`), so this is the
local idiom rather than a shape chosen to move a number. Behaviour is
unchanged -- verified under `--all-features`, `tcl`, `irules` and
`rust`, and the `(0, 4)` row still fails when perturbed, so both
languages are still asserted.
Lesson 88 gains the argument its `**Lesson:**` paragraph rested on implicitly: no lookbehind separates a quote that opens a literal from one that closes it, because that is state and a regex sees only context. `"dir/r"` is the shape that settles it -- `/` is a legitimate raw-open position, so the preceding character carries no information. Two sub-examples: the third gate in the family (#1219), where the same regex-and-skip hid every severity literal behind three shapes; and the port that dropped its donor's even/odd lifetime arithmetic, leaving a fixture that passed against the bug it named. Lesson 89 is new rather than a fourth sub-example under 59. Its mechanism is not "the rule was duplicated" but "a positive enumeration and a negative filter disagree on every kind neither names", and 59 was already the file's longest entry at 72 lines. It carries a bare `cf.` to 59 and 65 rather than a paragraph explaining the difference. Every measurement in both entries was re-run for this commit rather than quoted from the issues: the three scanner shapes against both implementations, and the four Objective-C shifts (`^(void)` 1 -> 0, `^(int a,,)` 1 -> 2, `^({ int x; })` 0 -> 1) against the pre- and post-fix trees, with `int f(int a,,)` -> 2 and `int g({ int x; })` -> 1 confirming the function channel already answered that way.
Two standing obligations from the #1218-#1220 batch that are rules rather than lessons -- you follow them without re-reading the evidence. A multi-perturbation sweep needs a scripted in-memory backup. The existing inverse-edit advice does not scale past one or two, and the loop is exactly where `git checkout -- <file>` gets reached for; it destroyed an uncommitted rewrite a third time during #1219. The companion trap is that the resulting harness reports a plausible failure count rather than an error: three perturbations returning an identical 34 meant the gate had been reverted to its `main` version and the new suite was running against the old implementation, not that the code was well covered. A feature-gated fixture table needs both halves. Ungated, a feature set enabling none of its rows leaves an empty list and a test asserting nothing, which is what `assert_fixtures_present` exists to make loud; loud alone turns that build into a spurious failure instead. The `#[cfg(any(...))]` makes the test absent, and the non-vacuity assertion covers the residual runtime case. Both #1220 item 1 and the #1218 fix tripped one half each.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes three issues from the 2.1.0 pre-release delta review. Each issue
carried a resolution plan; I verified the claims against the code rather
than taking them at their word, and three did not hold as written.
Fixes #1218
Fixes #1219
Fixes #1220
#1218 — Objective-C block literals count the
(void)markerObjcCode'sBlockLiteralarm counted parameters with a positivematches!(ParameterDeclaration | VariadicParameter), bypassingcount_argsand thereforeChecker::is_empty_param_marker. Measured:the four-block fixture reported
closure_args = 5where 4 belongs. Thefunction channel beside it was already correct.
The arm now routes through
count_args, which closes the class ratherthan this one member. Sweep confirmed complete —
is_empty_param_markeris implemented by C, C++, Mozcpp and Objc, andthe first three reach both channels through
compute_args. Objc'sMethodDefinitionarm keeps its positive match: a method's(void)isa return type, never a parameter.
Three things beyond the plan:
ParameterList2(369) is unreachable fromblock_literal. Theguard matches only 368, which looked like a
grammar-dispatch.md§1alias hole. Probed across nine constructs including K&R definitions —
never emitted. Now marked as excluded rather than silently omitted.
count_args' inclusions too. On invalid source^(int a,,)→ 2 and^({ int x; })→ 1. Both measured. That is thepoint rather than a regression: those are the numbers
int f(int a,,)already reported, so the block arm stopped being theone caller that answered differently.
is_enabledcheck — see the note below.#1219 — phantom raw-string windows in the diagnostic-prefix gate
scan_textdecided raw-string state per line, so anything resembling anopen was read as one and hid every following line to the next quote.
Three windows of that class, not the two in the issue:
let p = "dir/r";let x = 1; // e.g. r"foolet x = 1; /* r"foo */Neither cheap fix works. No lookbehind can express the first —
/is alegitimate raw-open context, and what distinguishes the case is that the
"closes a literal. Stripping comments by regex cannot fix theothers, because
"http://x"would be truncated mid-literal and open aphantom span of its own.
So the scan runs off one lexer ported from
scan_ignore_spansincheck-snapshot-anchors.py, which needed the identical machine and isgated by its own self-tests (#1192). Ported rather than shared: every
gate under
utils/is a standalone hyphen-named script resolving therepo root from its own location. 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 tracked Rust files: byte-identical, zero hits either way.
#1220 — three polish items, one of them wrong twice
Items 1 and 3 as planned. Item 2 was not.
The issue scopes the WMC over-claim to Go and its plan says "no broader
rewrite is needed".
class_interface_computerecords onlyUnit | Class | Interface | Function, so anamespacespace carriesnpm/npaand nowmc— makingmetrics.md's container list wrongindependently of Go. Measured on C++ and Ruby.
The first pass then got the correction wrong in the other direction:
it said "C++ or Objective-C namespace", but Objective-C has no namespace
construct and Ruby's
modulewas omitted. Exactly three constructs mapto
SpaceKind::Namespace— C++ and Mozcppnamespace, Rubymodule—so the rule is space-kind-level, and the docs now say that instead of
naming one spelling.
Nothing pinned WMC's emission scope:
has_wmcdid not exist and no testreads either document, which is how one rule came to describe three
blocks.
Emittednow tracks it, with Go, C++ and Ruby all asserted.Item 3 takes the comment option.
function_declaratorappears in noneof the four getters' identifier-kind gates, so
get_func_space_namereturns
Noneeither way and the ERROR-tree divergence is unobservable.Restoring the
Nonewould buy nothing while reintroducing the twouncoverable arms 9918032 removed, so the comment names the getter gates
as the dependency that keeps it unobservable.
Verification
make pre-commit→BCA_GATE: passis a
continuefor a feature-subset path unreachable under--all-features. Project covered counts rose in every dimension(+65 lines, +55 regions, +7 functions). Note
utils/*.pyis outsideevery coverage report in this repo — its 43 self-tests all execute.
big-code-analysis-outputsubmodule bump.^(void)appears in twocorpus files, both
.mm, which route to C++.Every new test was verified by perturbing the exact production line it
names, each isolating one failure. Two were wrong on the first pass and
are worth flagging:
lexer pairs them off and it passed against the bug it named. The
donor's suite records that trap ("Three lifetimes, not two").
ran > 0assertionintroduced a false failure under
--no-default-features --features rust— precisely the defect chore: polish items from the 2.1.0 delta review #1220 item 1 exists to fix. Both nowcarry the cfg gate, verified across four feature subsets.
CHANGELOG
Two
[Unreleased]claims were made false by #1218 and are corrected:the
(void)entry said "Fixed for C, C++, Mozcpp and Objective-C" whileonly the function channel was, and the #1201 entry listed Objective-C
blocks among languages "already correct and unchanged".