Batch fix: collapse lang_to_name (#500) + honor suppress markers in reports (#501)#502
Merged
Conversation
lang_to_name was a 22-arm flat match (cyclomatic 22) carrying a suppress(cyclomatic) marker, yet 19 of its arms returned exactly what the upstream LANG::get_name() already returns. Delegate the common case to other.get_name() and keep only the three arms that genuinely deviate: - Cpp: get_name() -> "c/c++", not a usable lookup token -> "cpp" - Csharp: get_name() -> "c#" -> "csharp" - Tsx: get_name() -> "typescript", collides with Typescript -> "tsx" Behaviour is identical for all 22 variants (verified arm-by-arm against src/langs.rs display names). Cyclomatic drops 22 -> 4 as a side effect, so the suppress(cyclomatic) marker is removed and the file-header note about it dropped; the suppress-file(halstead, nargs) marker stays. The doc comment is rewritten to state the real rule (CLI display name plus three identifier/collision overrides) instead of the inaccurate "lowercased variant name" framing. Adds tests: pin Cpp -> "cpp" and Csharp -> "csharp" (the two overrides no other test fixed; test-via-revert-able), plus a parity test asserting lang_to_name(lang) == lang.get_name() for every public variant outside the three overrides, making the delegation contract explicit and surfacing any future upstream display rename. Fixes #500
Post-review remediation for #500. The get_name() parity loop asserted nothing if public_languages() ever returned only the overrides (or empty); add a checked-count guard so an over-filtered language set fails loudly instead of passing green. Also reword the Tsx doc bullet to state the override is what makes Tsx/Typescript distinct, rather than implying they were already distinct after delegation.
`bca report markdown|html` listed raw metric values and ignored `bca: suppress` / `bca: suppress-file` markers, so the published hotspots report re-surfaced every silenced function — contradicting `bca check` and the SARIF emitter. The report now HONORS markers BY DEFAULT, per metric: a function is omitted from a metric's hotspot table when that metric is suppressed for it. `extract_summaries` folds the top-level Unit's `suppress-file` scope into each function's own scope (file ∪ function), mirroring `ThresholdSet::evaluate_with_policy`; each hotspot table filters on its `MetricKind` via `SuppressionScope::covers`, including the nexits→exit alias. `--no-suppress` (and `[report] no_suppress = true` in the manifest) opts back into the raw audit view. Advisory roll-ups (Actionable Summary, CC-stats note) intentionally keep counting raw measurements; only the per-metric hotspot tables filter. Widens `SuppressionScope::merge` from `pub(crate)` to `pub` (additive) so report consumers can fold scopes. Tests: markdown + HTML cover per-metric function-scope drop, file-scope suppress-all drop, --no-suppress inclusion, nexits→exit aliasing, and the extract_summaries file∪function merge; manifest tests cover the [report] known-key and merge_report OR semantics. Fixes #501
The #500 regression tests pushed big-code-analysis-py/src/language.rs's file-level nom to 30 (hard limit 30, headroom limit 28.5), tripping the soft self-scan tier. File-level nom here is a many-function aggregation artifact — tiny lookup helpers plus their tests — exactly like the halstead/nargs already suppressed on this file. Extend the existing suppress-file marker rather than baseline a brittle exact value.
Post-review remediation for #501. Pair the NEXITS-table exit-suppression drop assertion with a SuppressionPolicy::Ignore positive control so the test is self-contained: it now proves the table is empty *because of* the exit-alias filter, not because the function failed to qualify as a hotspot. Also document why write_actionable_summary intentionally takes no SuppressionPolicy (advisory roll-ups count raw measurements).
Hoist the NEXITS_HEADER const above the test's statements (items_after_statements) and apply cargo fmt to the manifest [report] test. Pure lint/format cleanup, no behavior change.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
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.
Batch fix for two related findings surfaced by the published metrics report's
lang_to_namecyclomatic hotspot.#500 —
refactor(py): collapse lang_to_name onto LANG::get_namelang_to_namewas a 22-arm flatmatchwhose 19 non-override arms duplicated the upstreamLANG::get_name()display names. It now delegates toget_name()and keeps only the three genuine lookup-token overrides:Cpp→"cpp"(upstream"c/c++"isn't a usable lookup token)Csharp→"csharp"(upstream"c#")Tsx→"tsx"(upstream"typescript"collides withTypescript)Output is byte-identical for all 22 variants (verified arm-by-arm against
src/langs.rs); the change removes the hand-maintained duplication and the drift risk against the documented CLI-parity contract. Added an override pin test (Cpp/Csharp) and aget_name()parity test over the non-override public variants (with a vacuity guard).#501 —
feat(report): honor bca: suppress markers in aggregated reportsbca report markdown|htmllisted raw metric values and ignoredbca: suppress/bca: suppress-filemarkers, so the published hotspots report re-surfaced every silenced function — contradictingbca checkand the SARIF emitter.The report now honors markers by default, per metric: a function is omitted from a metric's hotspot table when that metric is suppressed for it.
extract_summariesfolds the top-level Unit'ssuppress-filescope into each function's own scope (file ∪ function), mirroringThresholdSet::evaluate_with_policy; each table filters on itsMetricKindviaSuppressionScope::covers, including thenexits → exitalias.bca report --no-suppress(or[report] no_suppress = trueinbca.toml) opts into the raw audit view.SuppressionScope::mergewidenedpub(crate)→pub(additive;SuppressionScopeis already re-exported fromlib.rs).Quality / review
Each issue went through fix → simplify → review → remediation. After each wave the simplify-rust / rust-optimize / audit-tests / code-review / review skills were run; remediations applied:
--no-suppresspositive control on the exit-table drop testsuppress-file(nom)onlanguage.rs(the refactor(py): collapse lang_to_name onto LANG::get_name #500 regression tests tipped file-levelnompast the headroom gate — an aggregation artifact, like the already-suppressedhalstead/nargs)The 8 new #501 tests were verified by test-via-revert (the filter-dependent ones fail when
is_hidden_foris neutered).Validation
make pre-commitgreen: fmt, clippy-D warnings(both feature flavors), workspace tests, doc, udeps, markdown/TOML/shell/Makefile/Actions lints, man-page drift, both self-scan tiers, and the Python suite (192 passed, 1 xfailed).Closes #500
Closes #501