From 0398d5c93f54fe7fee48215ae40d63f8c4b23820 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sat, 22 Aug 2026 23:16:46 -0700 Subject: [PATCH 1/4] fix(py): hold the batch slot for unreadable inputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `analyze_batch` and `analyze_paths` documented that `skip_generated=False` yields one element per input, so callers could `zip(inputs, results)`. `analyze_path` returns `Ok(None)` from two places, though — the `skip_generated` filter and the unconditional `read_file_with_eol` gate (three bytes or fewer, a UTF-16 BOM, a non-UTF-8 leading window) — and `push_one_result` dropped the slot for both. A batch containing one tiny or binary file therefore returned a shorter list, and the endorsed zip attributed every later result to the wrong path with no error and no `AnalysisFailure` to observe. With the filter off the read gate is the only remaining source, so that arm now pushes `None` — the same value single-file `analyze` returns for those files, and one `to_sarif` already skips. The `skip_generated=True` default is unchanged. The stub widens to `list[FuncSpaceDict | AnalysisFailure | None]`, which is what flagged the two documented examples that needed a `None` branch. Fixes #1238 --- CHANGELOG.md | 30 +++ STABILITY.md | 40 +++- big-code-analysis-book/src/python/batch.md | 40 +++- big-code-analysis-book/src/python/errors.md | 5 + .../src/python/flat-records.md | 4 +- big-code-analysis-book/src/python/sarif.md | 11 +- big-code-analysis-py/README.md | 35 +++- .../examples/batch_processing.py | 20 +- .../examples/jupyter_quickstart.ipynb | 2 +- big-code-analysis-py/examples/pipeline_db.py | 16 +- .../python/big_code_analysis/_native.pyi | 43 ++-- big-code-analysis-py/src/batch.rs | 197 ++++++++++++++++-- big-code-analysis-py/tests/test_batch.py | 129 +++++++++++- .../tests/test_book_examples.py | 31 ++- big-code-analysis-py/tests/test_discovery.py | 58 +++++- big-code-analysis-py/tests/test_types.py | 2 +- big-code-analysis-py/tests/test_vcs.py | 14 +- docs/development/lessons_learned.md | 20 +- 18 files changed, 605 insertions(+), 92 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4f94a44e..d24a04342 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -108,6 +108,36 @@ for historical reference. ### Fixed +- The Python bindings' `analyze_batch` / `analyze_paths` dropped a + result slot for any file the read gate declines to parse — three + bytes or fewer, a UTF-16 BOM, or a leading window that is not valid + UTF-8 — even under `skip_generated=False`, which both entry points + documented as guaranteeing one element per input. That gate is + unconditional, so a batch containing one such file returned a shorter + list and the `zip(inputs, results)` pattern the docstrings endorse + silently attributed every later result to the wrong path: a + data-corruption class defect with no error and no `AnalysisFailure` to + observe. With `skip_generated=False` such a file now holds its + position as a `None` element — the same value single-file `analyze` + returns for it, and one `to_sarif` already skips — so the documented + `zip` is finally safe (#1238). The `skip_generated=True` default is + unchanged: a skipped file, generated or unreadable, still yields no + element. The typed surface widens to match: `analyze_batch` and + `analyze_paths` are now annotated + `list[FuncSpaceDict | AnalysisFailure | None]` in `_native.pyi`, so a + `mypy --strict` consumer indexing a slot without a `None` check is + told to add one rather than discovering it at runtime. An **untyped** + `skip_generated=False` consumer sees the change at runtime instead: a + loop that indexed every slot used to run to completion on such a batch + (silently mis-paired) and now raises `TypeError: 'NoneType' object is + not subscriptable` at the placeholder, and `len(results)` grows. + That is the intended direction — loud and local beats silent and + downstream — but it is a behaviour break, and `analyze_paths` shares + it, so a directory walk under `skip_generated=False` gains one element + per discovered file the gate declined. Widening a return union is not + an additive change under [STABILITY.md](./STABILITY.md); the exception + and its reasoning are recorded there under *Python bindings → + Typing*. - Ruby regex literals and Perl bare match literals fabricated division operators: the delimiter tokens under the literal wrapper were classified through the generic `/` operator arm, so `x = /abc/` diff --git a/STABILITY.md b/STABILITY.md index afa3c3f29..82db3efc0 100644 --- a/STABILITY.md +++ b/STABILITY.md @@ -1035,6 +1035,31 @@ the existing values are frozen. ### Error mapping +A batch slot is not always a result or a failure. `analyze_batch` / +`analyze_paths` emit a `None` element, under `skip_generated=False` +only, for a file the shared read gate declines to parse — three bytes +or fewer, a UTF-16 BOM, or a leading window that is not valid UTF-8. +That gate is unconditional, so before #1238 those files produced no +element at all and the documented `zip(inputs, results)` mis-paired +every later entry; the placeholder is what makes `skip_generated=False` +genuinely one-element-per-input. `None` is the same value single-file +`analyze` returns for those files, and `to_sarif` skips it. The +`skip_generated=True` default is unchanged: a skipped file, generated +or unreadable, still yields no element. + +The placeholder is a **behaviour break for existing +`skip_generated=False` callers**, in both directions. A caller that +looped over the results untyped — `for r in results: r["metrics"]` — +previously ran to completion on a batch containing such a file (having +silently mis-paired every later entry) and now raises `TypeError: +'NoneType' object is not subscriptable` at the placeholder. A caller +that counted `len(results)` sees a larger number. Both are the intended +direction: the failure is now loud and local instead of silent and +downstream, and the count is now the truth. `analyze_paths` shares the +change, so a directory walk under `skip_generated=False` gains one +element per discovered file the gate declined — on a tree with binary +assets that can be a substantial fraction of the list. + Per-file failures from `analyze_batch` / `analyze_paths` are **returned**, not raised, as `AnalysisFailure` values (not `Exception` subclasses: the class is deliberately not raisable; it was renamed from @@ -1066,7 +1091,9 @@ as the library: additive in minor bumps, breaking only at a major. The analysis-result wire shape is also expressed as exported `TypedDict`s (#623): `analyze` / `analyze_source` return `FuncSpaceDict | None` / `FuncSpaceDict`, `analyze_batch` / -`analyze_paths` return `list[FuncSpaceDict | AnalysisFailure]`, and the +`analyze_paths` return `list[FuncSpaceDict | AnalysisFailure | None]` +(the `None` slot is the read-gate placeholder described under *Error +mapping* above, #1238), and the nested metric blocks (`CodeMetricsDict`, `LocDict`, `HalsteadDict`, `VcsDict`, …) are re-exported from the package. Like the enums, these are **generated** from the `big_code_analysis::wire` structs (`src/wire.rs`, @@ -1093,6 +1120,17 @@ structs moved into `big_code_analysis::wire` and the jit shapes are mirrored from `src/vcs/jit.rs`, so the former `dict[str, Any]` returns are gone. The same additive / major-only shape contract applies. +One documented exception to that contract, on the 2.x line: #1238 +widened the `analyze_batch` / `analyze_paths` return element to +`FuncSpaceDict | AnalysisFailure | None`. Widening a *return* union is +not additive — a `mypy --strict` consumer that indexed a slot without a +`None` check newly fails to type-check, which is the point: the runtime +values it would have received were already wrong. It landed in a minor +rather than waiting for the next major because the alternative was +leaving a silent data-corruption defect in the released line, and +because the runtime contract the widening makes true is the one both +entry points already documented. + [strenum]: https://docs.python.org/3/library/enum.html#enum.StrEnum [pep561]: https://peps.python.org/pep-0561/ diff --git a/big-code-analysis-book/src/python/batch.md b/big-code-analysis-book/src/python/batch.md index 4c361349d..4a372f6cd 100644 --- a/big-code-analysis-book/src/python/batch.md +++ b/big-code-analysis-book/src/python/batch.md @@ -2,16 +2,16 @@ `bca.analyze_batch(paths)` runs the same analysis as `bca.analyze` over every path in an iterable and **never raises on per-file -errors**: each result element is either an analysis `dict` or a -`bca.AnalysisFailure` describing the failure. Results preserve input -order, so `zip(inputs, results)` lines up by index **when no path -is skipped**. `analyze_batch` shares `analyze`'s keyword-only -options — `exclude_tests`, `allow_lossy_path`, `skip_generated` +errors**: each result element is an analysis `dict`, a +`bca.AnalysisFailure` describing the failure, or `None`. Results +preserve input order, so `zip(inputs, results)` lines up by index +**when no path is skipped**. `analyze_batch` shares `analyze`'s +keyword-only options — `exclude_tests`, `allow_lossy_path`, `skip_generated` (default `True`), and `metrics` — so the two entry points are behaviour-preserving. ```python -{{#include ../../../big-code-analysis-py/examples/batch_processing.py:18:44}} +{{#include ../../../big-code-analysis-py/examples/batch_processing.py:18:58}} ``` A few key contracts: @@ -30,6 +30,15 @@ A few key contracts: (the pre-2.0 default). This default flipped at 2.0 so that switching between `analyze` and `analyze_batch` no longer silently changes generated-file handling. +* A file that cannot be parsed at all still holds its slot under + `skip_generated=False`, as `None`. The read gate shared with the + CLI walker declines a file of three bytes or fewer, one carrying + a UTF-16 BOM, and one whose leading window is not valid UTF-8; + that gate is unconditional, so before #1238 those files shrank + the list even with the flag off and the `zip` above mis-paired + every later entry. `None` is the same value single-file + `analyze` returns for them, and `bca.to_sarif` skips it, so a + batch list can be passed straight through. ## Walking a directory: `analyze_paths` @@ -52,10 +61,19 @@ a file directly is always analysed regardless of `exclude` — an explicit request overrides ignore-style rules — while `include` still narrows it by basename. `respect_gitignore=False` opts into walking ignored files. The -result is the same `list[FuncSpaceDict | AnalysisFailure]` shape and -never-raise contract as `analyze_batch`, and it forwards the same -`exclude_tests` / `allow_lossy_path` / `skip_generated` / `metrics` / -`vcs` / `vcs_per_function` kwargs. +result is the same `list[FuncSpaceDict | AnalysisFailure | None]` +shape and never-raise contract as `analyze_batch`, and it forwards +the same `exclude_tests` / `allow_lossy_path` / `skip_generated` / +`metrics` / `vcs` / `vcs_per_function` kwargs. + +The `None` slots reach this entry point too, under +`skip_generated=False`, one per discovered file the read gate +declined. There is no caller-supplied ordering to pair against here — +results follow the walk — so they are not there for a `zip`. What +they buy is that a file the walk found but could not analyse stays +visible in the output instead of disappearing from it. On a tree with +many binary assets that is a lot of `None`s; filter them with +`[r for r in results if r is not None]` if you only want records. ## Attaching change-history metrics @@ -77,7 +95,7 @@ sequential sweep. For parallelism, fan the per-file `analyze` call out across a thread pool: ```python -{{#include ../../../big-code-analysis-py/examples/batch_processing.py:47:59}} +{{#include ../../../big-code-analysis-py/examples/batch_processing.py:61:73}} ``` PyO3's `Python::detach` releases the GIL across each file's read + diff --git a/big-code-analysis-book/src/python/errors.md b/big-code-analysis-book/src/python/errors.md index 640060109..0cfd07688 100644 --- a/big-code-analysis-book/src/python/errors.md +++ b/big-code-analysis-book/src/python/errors.md @@ -179,6 +179,11 @@ def report(paths: list[str]) -> None: log.warning( "skip %s (%s): %s", path, slot.error_kind, slot.error ) + elif slot is None: + # Nothing to parse: three bytes or fewer, a UTF-16 BOM, or a + # binary leading window. The slot is held open so the zip + # above stays aligned (#1238) — it is not a failure. + log.info("skip %s: empty or binary", path) else: log.info( "ok %s sloc=%s", path, diff --git a/big-code-analysis-book/src/python/flat-records.md b/big-code-analysis-book/src/python/flat-records.md index 9683475c4..09464c8b4 100644 --- a/big-code-analysis-book/src/python/flat-records.md +++ b/big-code-analysis-book/src/python/flat-records.md @@ -81,4 +81,6 @@ arrows) keep their `name == ""` marker verbatim — * `flatten_spaces` raises `TypeError` if the input is not a mapping; callers must filter `None` returns from `bca.analyze` (e.g. generated files with `skip_generated=True`) before - passing. + passing — and likewise the `None` slots `analyze_batch` emits + under `skip_generated=False`, which mark a file the read gate + declined to parse. diff --git a/big-code-analysis-book/src/python/sarif.md b/big-code-analysis-book/src/python/sarif.md index 40b68a6c8..f888a8570 100644 --- a/big-code-analysis-book/src/python/sarif.md +++ b/big-code-analysis-book/src/python/sarif.md @@ -22,10 +22,13 @@ the CLI binary. * A single `dict` returned by `bca.analyze` or `bca.analyze_source`. -* Any iterable yielding such dicts or `bca.AnalysisFailure` - instances (the natural shape of `bca.analyze_batch`'s return - value). `AnalysisFailure` entries are skipped silently — they - represent files that could not be analyzed, not findings. +* Any iterable yielding such dicts, `bca.AnalysisFailure` + instances, and/or `None` (the natural shape of + `bca.analyze_batch`'s return value). `AnalysisFailure` and `None` + entries are skipped silently — they represent files for which no + record was emitted, not findings. +* A scalar `None`, the documented return of `bca.analyze` for a + skipped file; it yields an empty SARIF run. ## Thresholds diff --git a/big-code-analysis-py/README.md b/big-code-analysis-py/README.md index 1989648fa..3d0b3984d 100644 --- a/big-code-analysis-py/README.md +++ b/big-code-analysis-py/README.md @@ -282,13 +282,13 @@ page for the full contract. `bca.analyze_batch(paths)` runs the same analysis as `bca.analyze` over every path in an iterable and **never raises on per-file -errors**: each result element is either an analysis ``dict`` or a -`bca.AnalysisFailure` describing the failure. Results preserve input -order, so `zip(inputs, results)` lines up by index **when no path is -skipped**. `analyze_batch` accepts the same keyword-only options as -`analyze` — `exclude_tests`, `allow_lossy_path`, `skip_generated` -(default `True`), `metrics` — so migrating between the two is -behavior-preserving. +errors**: each result element is an analysis `dict`, a +`bca.AnalysisFailure` describing the failure, or `None`. Results +preserve input order, so `zip(inputs, results)` lines up by index +**when no path is skipped**. `analyze_batch` accepts the same +keyword-only options as `analyze` — `exclude_tests`, +`allow_lossy_path`, `skip_generated` (default `True`), `metrics` — so +migrating between the two is behavior-preserving. With the default `skip_generated=True`, a generated file is *skipped* and produces no result element (matching single-file `analyze`, which @@ -297,6 +297,15 @@ This default flipped at 2.0 — the pre-2.0 `analyze_batch` always analyzed generated files (`skip_generated=False`); pass that explicitly to restore one-element-per-input. +`skip_generated=False` really is one element per input, including for +files that cannot be parsed at all. The read gate `analyze` shares with +the CLI walker declines a file of three bytes or fewer, one carrying a +UTF-16 BOM, and one whose leading window is not valid UTF-8; those hold +their slot as `None` — the same value single-file `analyze` returns for +them — rather than dropping out of the list. Before #1238 they dropped +regardless of the flag, so one binary file in a corpus silently +mis-attributed every later result to the wrong path. + ```python import big_code_analysis as bca @@ -324,6 +333,10 @@ results = bca.analyze_batch(paths) # now zip(paths, results) works ``` +A `None` slot means "no record emitted": the file exists and was read, +but there was nothing to parse. `bca.to_sarif` skips those entries, so +a batch list can be handed to it verbatim. + `bca.AnalysisFailure` is a frozen value type with `path: str`, `error: str`, and `error_kind: Literal["UnsupportedLanguage", "ParseError", "IoError"]`. It implements `__eq__`, `__hash__`, @@ -347,8 +360,8 @@ parallel single-file calls. With the 2.0 default `skip_generated=True`, `analyze_batch` applies the CLI's `is_generated` walker filter, so a generated file is skipped and contributes no result element — the result list can be shorter than the input. Pass `skip_generated=False` -to analyze every file and guarantee one `dict`-or-`AnalysisFailure` -element per input. +to analyze every file and guarantee one +`dict`-or-`AnalysisFailure`-or-`None` element per input. ## Flatten to records @@ -413,7 +426,9 @@ be observed in later records. `flatten_spaces` raises `TypeError` if the input is not a mapping; callers must filter `None` returns from `bca.analyze` (e.g. when -`skip_generated=True` matched a generated file) before passing. +`skip_generated=True` matched a generated file) before passing — and +likewise the `None` slots `analyze_batch` emits under +`skip_generated=False`. ## Errors diff --git a/big-code-analysis-py/examples/batch_processing.py b/big-code-analysis-py/examples/batch_processing.py index 8d68715e6..4bc9e3f07 100644 --- a/big-code-analysis-py/examples/batch_processing.py +++ b/big-code-analysis-py/examples/batch_processing.py @@ -18,8 +18,8 @@ def run(paths: Iterable[Path]) -> dict[str, int]: """Analyse ``paths`` as a batch and bucket successes vs failures. - Returns a small summary dict (`ok`, `errors`, `total`) so the - accompanying test can assert on it without re-parsing. + Returns a small summary dict (`ok`, `errors`, `skipped`, `total`) so + the accompanying test can assert on it without re-parsing. """ materialised = list(paths) # `skip_generated=False` guarantees one result element per input @@ -32,16 +32,30 @@ def run(paths: Iterable[Path]) -> dict[str, int]: ok = 0 errors = 0 + skipped = 0 for path, result in zip(materialised, results, strict=True): if isinstance(result, bca.AnalysisFailure): errors += 1 print(f" skip {path}: ({result.error_kind}) {result.error}") + elif result is None: + # A slot the read gate declined to parse — three bytes or + # fewer, a UTF-16 BOM, or a binary leading window (#1238). + # `skip_generated=False` keeps the position rather than + # dropping it, which is what makes the strict zip safe; the + # third branch is the price of that guarantee. + skipped += 1 + print(f" skip {path}: nothing to parse (empty or binary)") else: ok += 1 sloc = result["metrics"]["loc"]["sloc"] print(f" ok {path}: sloc = {sloc:.0f}") - return {"ok": ok, "errors": errors, "total": len(materialised)} + return { + "ok": ok, + "errors": errors, + "skipped": skipped, + "total": len(materialised), + } def run_parallel(paths: Iterable[Path], *, workers: int = 4) -> list[FuncSpaceDict | None]: diff --git a/big-code-analysis-py/examples/jupyter_quickstart.ipynb b/big-code-analysis-py/examples/jupyter_quickstart.ipynb index 028d0ab81..5059ebdd0 100644 --- a/big-code-analysis-py/examples/jupyter_quickstart.ipynb +++ b/big-code-analysis-py/examples/jupyter_quickstart.ipynb @@ -60,7 +60,7 @@ "cell_type": "markdown", "id": "bca-quickstart-batch-header", "metadata": {}, - "source": "## 4. Analyse + flatten into a `DataFrame`\n\nPer-file `bca.analyze(path)` is used here (not `bca.analyze_batch`) because the single-file entry point honours the CLI walker's `@generated` / `DO NOT EDIT` filter — `analyze_batch` hardcodes `skip_generated=False` and would otherwise pull generated code into the chart. `flatten_spaces` walks each result's `FuncSpace` tree and yields one row per function / class / namespace. The whole table fits into a `pandas.DataFrame` with a single `pd.DataFrame.from_records` call." + "source": "## 4. Analyse + flatten into a `DataFrame`\n\nPer-file `bca.analyze(path)` is used here (not `bca.analyze_batch`) because the single-file entry point returns `None` for a skipped file, which is the shape the loop below already handles. Since 2.0 (#542) `analyze_batch` applies the same `@generated` / `DO NOT EDIT` filter by default, so either entry point keeps generated code out of the chart. `flatten_spaces` walks each result's `FuncSpace` tree and yields one row per function / class / namespace. The whole table fits into a `pandas.DataFrame` with a single `pd.DataFrame.from_records` call." }, { "cell_type": "code", diff --git a/big-code-analysis-py/examples/pipeline_db.py b/big-code-analysis-py/examples/pipeline_db.py index ee8e04b1d..bec82a57a 100644 --- a/big-code-analysis-py/examples/pipeline_db.py +++ b/big-code-analysis-py/examples/pipeline_db.py @@ -94,7 +94,10 @@ def run( dashboard. Pass ``False`` to opt into the batch entry point with ``skip_generated=False`` explicitly, ingesting every file — generated or not — with one result element per input so the - ``zip(..., strict=True)`` against ``inputs`` holds. + ``zip(..., strict=True)`` against ``inputs`` holds. A file the read + gate declines to parse (three bytes or fewer, a UTF-16 BOM, a binary + leading window) holds its slot as ``None`` rather than vanishing + from the list (#1238), and is bucketed as ``skipped``. Returns a small summary dict (``analyzed``, ``errors``, ``skipped``, ``rows``, ``top_n``) so the caller can assert @@ -139,15 +142,18 @@ def run( errors += 1 print(f" skip {path}: ({batch_result.error_kind}) {batch_result.error}") continue + if batch_result is None: + # The read gate declined this file; the slot is held + # open so the strict zip above stays aligned (#1238). + skipped += 1 + print(f" skip {path}: nothing to parse (empty or binary)") + continue analyzed += 1 flat_rows.extend(dict(record) for record in bca.flatten_spaces(batch_result)) inserted = _persist(db_path, flat_rows) top = _top_n_cyclomatic(db_path, top_n) - print( - f"persisted {inserted} rows from {analyzed} files " - f"({errors} errors, {skipped} generated skipped)" - ) + print(f"persisted {inserted} rows from {analyzed} files ({errors} errors, {skipped} skipped)") # ASCII '-' separator (not U+2014 EM DASH) — non-UTF Windows # console code pages (cp1252 on default Windows) raise # UnicodeEncodeError on the em-dash and crash this happy-path diff --git a/big-code-analysis-py/python/big_code_analysis/_native.pyi b/big-code-analysis-py/python/big_code_analysis/_native.pyi index d1fadda32..70b348921 100644 --- a/big-code-analysis-py/python/big_code_analysis/_native.pyi +++ b/big-code-analysis-py/python/big_code_analysis/_native.pyi @@ -512,8 +512,9 @@ def analyze( matches the CLI walker's ``is_generated`` predicate — see ``skip_generated`` below. - A UTF-8 / UTF-16 BOM is stripped and CR/CRLF line endings are - normalised to LF before analysis, matching the CLI byte-for-byte. + A UTF-8 BOM is stripped and CR/CRLF line endings are normalised to + LF before analysis, matching the CLI byte-for-byte. A UTF-16 BOM is + not stripped — it is one of the binary signals above (#803). Callers must therefore handle the optional return: .. code-block:: python @@ -699,20 +700,30 @@ def analyze_batch( metrics: Sequence[str] | None = None, vcs: bool = False, vcs_per_function: bool = False, -) -> list[FuncSpaceDict | AnalysisFailure]: +) -> list[FuncSpaceDict | AnalysisFailure | None]: """Compute metrics for every path in ``paths``. Returns a list whose elements preserve the input order, so ``zip(paths, results)`` lines up by index **when no path is skipped**. Each element is either: - * a ``dict`` matching :func:`analyze`'s output shape, or - * an :class:`AnalysisFailure` describing the per-file failure. + * a ``dict`` matching :func:`analyze`'s output shape, + * an :class:`AnalysisFailure` describing the per-file failure, or + * ``None`` for a file the read gate declines to parse — three + bytes or fewer, a UTF-16 BOM, or a leading window that is not + valid UTF-8. This is the same ``None`` :func:`analyze` returns + for those files, and it appears only under + ``skip_generated=False``. A path that is skipped (``skip_generated=True`` and the file is - generated) produces **no** element, so with skipping enabled the - result list can be shorter than the input iterable. Pass - ``skip_generated=False`` to guarantee one element per input. + generated *or* declined by the read gate) produces **no** element, + so with skipping enabled the result list can be shorter than the + input iterable. Pass ``skip_generated=False`` to guarantee one + element per input: the generated-file filter is then off, and the + read gate — which is unconditional — holds its slot with ``None`` + instead of dropping it (#1238). Before that fix a tiny or binary + file silently shrank the list even with ``skip_generated=False``, + so the endorsed ``zip`` mis-paired every later entry. The function **never raises on per-file errors** — a missing file, an unknown extension, or a parser failure becomes an @@ -759,7 +770,8 @@ def analyze_batch( CLI walker and :func:`analyze`'s ``None`` return. This default flipped at 2.0 — the pre-2.0 ``analyze_batch`` hardcoded ``skip_generated=False`` (always one element per input). Pass - ``skip_generated=False`` to restore that behaviour. + ``skip_generated=False`` to restore that behaviour; a file the read + gate declines occupies its slot as ``None`` rather than a ``dict``. The GIL is released across each file's read + tree-sitter parse via PyO3's ``Python::detach``, so a multi-threaded @@ -795,7 +807,7 @@ def analyze_paths( metrics: Sequence[str] | None = None, vcs: bool = False, vcs_per_function: bool = False, -) -> list[FuncSpaceDict | AnalysisFailure]: +) -> list[FuncSpaceDict | AnalysisFailure | None]: """Walk one or more path seeds and analyse every discovered file (#658). Each positional ``path`` may be a file or a directory; directories @@ -826,8 +838,15 @@ def analyze_paths( Returns the :func:`analyze_batch` result shape with the same never-raise semantics: a per-file failure becomes an :class:`AnalysisFailure` element rather than a raise, and a generated - file (under ``skip_generated=True``) yields no element. The result - order follows the walk, not any caller-supplied ordering. + file (under ``skip_generated=True``) yields no element. Under + ``skip_generated=False`` a discovered file the read gate declines to + parse (tiny, UTF-16-BOM, or binary) contributes a ``None`` element + (#1238). There is no caller-supplied input position to pair it + against here — the result order follows the walk — so it is not + there for a ``zip``; it keeps a file the walk *found* but could not + analyse visible in the output instead of dropping it. On a tree with + many binary assets that is a lot of elements; filter with + ``[r for r in results if r is not None]`` if you only want records. A seed that does not exist (or whose symlink dangles) is surfaced as an :class:`AnalysisFailure` element (``error_kind="IoError"``, diff --git a/big-code-analysis-py/src/batch.rs b/big-code-analysis-py/src/batch.rs index 1377c12e2..4ef76be87 100644 --- a/big-code-analysis-py/src/batch.rs +++ b/big-code-analysis-py/src/batch.rs @@ -327,10 +327,17 @@ const _: fn() = || { /// `try_iter` (which calls Python's `iter()` builtin under the hood). /// With `skip_generated=false` the output list has the same length as /// the input iterable and preserves order one-to-one, so callers can -/// `zip(inputs, results)` without losing the pairing. Under the default -/// `skip_generated=true` a skipped file yields no slot (see below), so -/// the list can be shorter — `zip(inputs, results)` would then silently -/// mis-pair every entry after the first skip. +/// `zip(inputs, results)` without losing the pairing. Every slot is then +/// a `dict`, an `AnalysisFailure`, or `None` — the last for a file the +/// `read_file_with_eol` gate declines to parse (three bytes or fewer, a +/// UTF-16 BOM, a non-UTF-8 leading window), which is exactly what +/// single-file [`crate::analyze`] returns for the same input. That gate +/// is unconditional, so before #1238 those files yielded no slot even +/// with `skip_generated=false` and the documented `zip` mis-paired every +/// later entry. Under the default `skip_generated=true` a skipped file +/// yields no slot (see below), so the list can be shorter — +/// `zip(inputs, results)` would then silently mis-pair every entry after +/// the first skip. /// /// `metrics=` selects which metrics to compute (#268). `None` (the /// default) preserves the full suite; an empty list raises @@ -351,8 +358,8 @@ const _: fn() = || { /// yields no `dict`), so the result list can be **shorter** than the /// input iterable when `skip_generated=true`. Pass /// `skip_generated=false` to restore the legacy "one result per input, -/// always" behaviour (every position produces a `dict` or an -/// `AnalysisError`). +/// always" behaviour (every position produces a `dict`, an +/// `AnalysisFailure`, or `None`). #[pyfunction] #[pyo3(signature = (paths, /, *, exclude_tests = false, allow_lossy_path = false, skip_generated = true, metrics = None, vcs = false, vcs_per_function = false))] // `metrics: Option>` is taken by value to match the PyO3 @@ -418,9 +425,11 @@ pub(crate) fn analyze_batch<'py>( Ok(results) } -/// Analyse `path` and push its result (a dict or an `AnalysisFailure`) -/// onto `results`, attaching shared-index VCS blocks when requested. -/// Shared by [`analyze_batch`] and [`analyze_paths`]. +/// Analyse `path` and push its result (a dict, an `AnalysisFailure`, or — +/// under `skip_generated=false` — a `None` placeholder) onto `results`, +/// attaching shared-index VCS blocks when requested. Shared by +/// [`analyze_batch`] and [`analyze_paths`], so both entry points carry +/// the same slot vocabulary. fn push_one_result( py: Python<'_>, path: &Path, @@ -457,9 +466,17 @@ fn push_one_result( } } } - // `Ok(None)` means `analyze_path` skipped the file — with the #542 - // default `skip_generated=true` this is the generated-file case, - // omitted from the output entirely (matching `analyze`). + // `Ok(None)` means `analyze_path` emitted no record, and it has + // two sources: the `skip_generated` filter, and the + // unconditional `read_file_with_eol` gate (three bytes or fewer, + // a UTF-16 BOM, a non-UTF-8 leading window). Under the #542 + // default the slot is dropped for either, matching `analyze`. + // With `skip_generated=false` the filter is off, so the gate is + // the only source left — and the caller has asked for one slot + // per input, so emit `None` (what `analyze` returns for the same + // file) rather than shrinking the list and silently mis-pairing + // every later `zip` entry (#1238). + Ok(None) if !opts.skip_generated => results.push(py.None()), Ok(None) => {} Err(err) => { let py_err = PyAnalysisError::from_internal(err, path); @@ -606,12 +623,23 @@ fn attach_or_keep(json: String, inject: impl FnOnce(String) -> PyResult) /// see [`crate::walk`] for the glob and file-seed semantics (#726). /// The walk is the discovery step `analyze_batch` lacks; per-file analysis, /// the never-raise contract (failures become `AnalysisFailure` elements), -/// the generated-file filter, and language inference are identical to -/// `analyze_batch`. A seed that does not exist (or whose symlink dangles) -/// is surfaced as an `AnalysisFailure` element (`error_kind="IoError"`, -/// `error="path does not exist"`) rather than silently dropped (#858) — -/// keeping parity with the CLI's hard error on a missing `--paths` seed -/// (#596) while preserving the never-raise posture of the result vector. The kwarg surface mirrors `analyze` / `analyze_batch` +/// the generated-file filter, language inference, and the slot vocabulary +/// (`dict` / `AnalysisFailure` / — under `skip_generated=false` — `None` +/// for a file the read gate declines to parse, #1238) are identical to +/// `analyze_batch`. Element order follows the walk, so there is no input +/// position to pair against and the `None` slots are not there for a +/// `zip`: what they buy here is that a file the walk *found* but could +/// not analyse stays visible in the output rather than vanishing from +/// it. On a tree with many binary assets that is a lot of slots, which +/// is why it happens only under `skip_generated=false`. +/// +/// A seed that does not exist (or whose symlink dangles) is surfaced as +/// an `AnalysisFailure` element (`error_kind="IoError"`, `error="path +/// does not exist"`) rather than silently dropped (#858) — keeping +/// parity with the CLI's hard error on a missing `--paths` seed (#596) +/// while preserving the never-raise posture of the result vector. +/// +/// The kwarg surface mirrors `analyze` / `analyze_batch` /// (`exclude_tests` / `allow_lossy_path` / `skip_generated` / `metrics` / /// `vcs` / `vcs_per_function`), so a directory walk threads VCS attachment /// through the same shared-per-repo index (#670). @@ -893,4 +921,137 @@ mod tests { set.insert(a); assert!(set.contains(&b)); } + + // ── #1238: one slot per input under `skip_generated=false` ─────── + + /// Three bytes, so `read_file_with_eol` treats the file as empty + /// and `analyze_path` returns `Ok(None)` before any language + /// inference runs. + const TINY_SOURCE: &[u8] = b"ab\n"; + /// A UTF-16 BOM. `probe_decodable_prefix` returns on the BOM before + /// it validates anything, so this is its own branch — not the + /// invalid-UTF-8 one below, which the trailing bytes never reach. + const UTF16_BOM_SOURCE: &[u8] = b"\xff\xfe\x00\x01 fn main() {}\n"; + /// A leading window that is not valid UTF-8 and carries no BOM: the + /// third read-gate skip the docs enumerate, and the only one that + /// exercises the `str::from_utf8` arm. + const BINARY_SOURCE: &[u8] = b"\x80\x81\x82\x83\n"; + /// Carries the CLI walker's `is_generated` markers, so it is the + /// `skip_generated`-gated `Ok(None)` source — analysed normally + /// once the filter is off. + const GENERATED_SOURCE: &[u8] = b"// @generated DO NOT EDIT\npub fn x() {}\n"; + /// An ordinary parseable file, present so neither test asserts a + /// length against an all-skipped input. + const GOOD_SOURCE: &[u8] = b"fn main() {}\n"; + /// One input per `Ok(None)` source plus a parseable control, shared + /// by the pair of tests below so their only difference is the flag. + const MIXED_INPUTS: &[(&str, &[u8])] = &[ + ("tiny.rs", TINY_SOURCE), + ("bom.rs", UTF16_BOM_SOURCE), + ("binary.rs", BINARY_SOURCE), + ("gen.rs", GENERATED_SOURCE), + ("good.rs", GOOD_SOURCE), + ]; + + /// Materialise `files` under `dir` and run each through + /// [`push_one_result`], returning the slots it pushed. + fn pushed_slots( + py: Python<'_>, + dir: &Path, + skip_generated: bool, + files: &[(&str, &[u8])], + ) -> Vec> { + let opts = AnalyzeOptions { + skip_generated, + ..AnalyzeOptions::default() + }; + let mut vcs_repos = VcsRepoCache::new(false, false); + let mut results = Vec::new(); + for (name, bytes) in files { + let path = dir.join(name); + std::fs::write(&path, bytes).expect("write fixture file"); + push_one_result(py, &path, opts, &mut vcs_repos, &mut results) + .expect("push_one_result never fails on a readable temp file"); + } + results + } + + /// The `name` field of a result dict — the analysed file's path, so + /// an assertion on it catches a slot that shifted onto the wrong + /// input rather than merely checking the slot is a dict. + fn slot_name(py: Python<'_>, slot: &Py) -> String { + slot.bind(py) + .get_item("name") + .expect("a result slot is a dict carrying `name`") + .extract() + .expect("`name` is the analysed path as a string") + } + + /// `dir/name` as the string `analyze_path` records in `FuncSpace.name`. + fn path_str(dir: &Path, name: &str) -> String { + dir.join(name) + .to_str() + .expect("tempdir paths are UTF-8") + .to_owned() + } + + #[test] + fn read_gate_skips_keep_their_slot_when_skip_generated_is_false() { + // #1238: the `read_file_with_eol` gate is unconditional, so + // before the fix a file it declined produced no slot even + // with `skip_generated=false` — and the `zip(inputs, results)` + // the docs endorse then attributed `good.rs`'s metrics to + // `tiny.rs`. The generated file is in the fixture to pin the + // other half: with the filter off it is analysed, not + // placeheld, so the new guard must not intercept it. + let dir = tempfile::tempdir().expect("tempdir"); + Python::attach(|py| { + let slots = pushed_slots(py, dir.path(), false, MIXED_INPUTS); + assert_eq!( + slots.len(), + MIXED_INPUTS.len(), + "skip_generated=false must yield one slot per input so \ + `zip(inputs, results)` keeps its pairing", + ); + for (index, slot) in slots.iter().take(3).enumerate() { + assert!( + slot.is_none(py), + "slot {index}: a file the read gate declines to parse \ + must hold its slot as `None`, matching single-file \ + `analyze`", + ); + } + assert_eq!( + slot_name(py, &slots[3]), + path_str(dir.path(), "gen.rs"), + "with the filter off a generated file is analysed, not \ + replaced by a `None` placeholder", + ); + assert_eq!( + slot_name(py, &slots[4]), + path_str(dir.path(), "good.rs"), + "the parseable input must land in its own slot, not \ + shift onto an earlier one", + ); + }); + } + + #[test] + fn every_skip_class_drops_its_slot_under_the_default() { + // The default is unchanged by #1238: both `Ok(None)` sources + // still omit the slot entirely, so the list is shorter than the + // input. `good.rs` is present so the length assertion is `1` + // rather than the `0` an all-skipped fixture would produce + // whether or not the arm ran. + let dir = tempfile::tempdir().expect("tempdir"); + Python::attach(|py| { + let slots = pushed_slots(py, dir.path(), true, MIXED_INPUTS); + assert_eq!( + slots.len(), + 1, + "with skip_generated=true a skipped file yields no slot", + ); + assert_eq!(slot_name(py, &slots[0]), path_str(dir.path(), "good.rs")); + }); + } } diff --git a/big-code-analysis-py/tests/test_batch.py b/big-code-analysis-py/tests/test_batch.py index 6b67fee85..68e4a74a1 100644 --- a/big-code-analysis-py/tests/test_batch.py +++ b/big-code-analysis-py/tests/test_batch.py @@ -13,6 +13,7 @@ from __future__ import annotations +import json import pickle import sys from collections.abc import Iterator @@ -593,7 +594,13 @@ def test_generated_file_is_skipped_by_default(tmp_path: Path) -> None: def test_skip_generated_false_restores_one_result_per_input(tmp_path: Path) -> None: """``skip_generated=False`` is the explicit opt-out (the pre-2.0 - default) that guarantees one element per input position.""" + default) that guarantees one element per input position. + + It covers the *generated* skip source only; the read gate is the + other one, and it is unconditional — see + ``test_skip_generated_false_holds_the_slot_for_unreadable_inputs`` + for the case this test cannot reach (#1238). + """ generated = tmp_path / "gen.rs" generated.write_bytes(b"// @generated by some-tool. DO NOT EDIT.\npub fn x() {}\n") @@ -604,6 +611,126 @@ def test_skip_generated_false_restores_one_result_per_input(tmp_path: Path) -> N ) +# One input per branch of the walker's ``read_file_with_eol`` gate: +# three bytes (treated as empty), a UTF-16 BOM (rejected on the BOM, +# before any UTF-8 validation runs), and a leading window that is not +# valid UTF-8 and carries no BOM. All three skips are unconditional — +# no kwarg turns them off — which is what made them outlive +# ``skip_generated=False`` (#1238). +_TINY_SOURCE = b"ab\n" +_UTF16_BOM_SOURCE = b"\xff\xfe\x00\x01 fn main() {}\n" +_BINARY_SOURCE = b"\x80\x81\x82\x83\n" + + +def _unreadable_batch(tmp_path: Path) -> list[Path]: + """The issue #1238 reproducer: every read-gate skip ahead of a + perfectly ordinary file, so a dropped slot shows up as the good + file's metrics being attributed to ``tiny.rs``. + + Returns the inputs in order; the last is the only parseable file, + so callers can assert on ``results[-1]``. + """ + written: list[Path] = [] + for name, content in ( + ("tiny.rs", _TINY_SOURCE), + ("bom.rs", _UTF16_BOM_SOURCE), + ("binary.rs", _BINARY_SOURCE), + ("good.rs", b"fn main() {}\n"), + ): + path = tmp_path / name + path.write_bytes(content) + written.append(path) + return written + + +def test_skip_generated_false_holds_the_slot_for_unreadable_inputs( + tmp_path: Path, +) -> None: + """#1238: ``skip_generated=False`` must keep the list index-aligned + even for files the read gate declines to parse. + + Before the fix this returned a single element for three inputs, so + ``zip(inputs, results)`` — the pattern the docstrings endorse — + attributed ``good.rs``'s metrics to ``tiny.rs``, silently, with no + error and no ``AnalysisFailure``. + """ + inputs = _unreadable_batch(tmp_path) + good = inputs[-1] + + # Single-file `analyze` returns None for each declined file, which + # is the shape the batch placeholder mirrors. Asserting it here + # keeps the fixtures honest: if a gate change ever made one of these + # parseable, the test below would pass for the wrong reason. + for declined in inputs[:-1]: + assert bca.analyze(declined) is None, f"{declined.name} must be declined" + + results = bca.analyze_batch(inputs, skip_generated=False) + + assert len(results) == len(inputs), ( + "skip_generated=False must yield one element per input so " + "zip(inputs, results) keeps its pairing" + ) + for index, slot in enumerate(results[:-1]): + assert slot is None, ( + f"slot {index}: a file the read gate declines must hold its " + "slot as None, matching what single-file analyze() returns" + ) + surviving = results[-1] + assert isinstance(surviving, dict) + assert surviving["name"] == str(good), ( + "the parseable input must land in its own slot rather than shifting onto tiny.rs" + ) + + +def test_read_gate_skips_still_drop_their_slot_by_default(tmp_path: Path) -> None: + """#1238 changed nothing under the default: a file the read gate + declines yields no element at all, so the list stays shorter than + the input. + + ``good.rs`` is in the batch so the length assertion is ``1`` rather + than the ``0`` an all-skipped input would produce whether or not the + drop happened. + """ + inputs = _unreadable_batch(tmp_path) + + results = bca.analyze_batch(inputs) + + assert len(results) == 1, "with skip_generated=True an unreadable file yields no slot" + [only] = results + assert isinstance(only, dict) + assert only["name"] == str(inputs[-1]) + + +def test_to_sarif_accepts_the_placeholder_slots(tmp_path: Path) -> None: + """The #1238 placeholders must stay transparent to ``to_sarif``, + which documents ``None`` entries as "no record emitted" and skips + them — so a caller can hand it ``analyze_batch``'s list verbatim. + + This is a forward guard, not regression coverage for #1238: it + passes against the pre-fix code too, because a dropped slot and a + skipped slot render the same document. What it would catch is a + future ``to_sarif`` that raised on — or emitted a finding for — a + ``None`` entry. + """ + inputs = _unreadable_batch(tmp_path) + results = bca.analyze_batch(inputs, skip_generated=False) + + # A threshold every analysed file trips, so "no findings" cannot be + # mistaken for "the placeholders were skipped". + doc = json.loads(bca.to_sarif(results, thresholds={"loc.sloc": 0})) + + [run] = doc["runs"] + flagged = [ + location["physicalLocation"]["artifactLocation"]["uri"] + for result in run["results"] + for location in result["locations"] + ] + assert flagged == [str(inputs[-1])], ( + "only the parseable file may produce a finding; the None " + "placeholders are skipped, not rendered as findings or errors" + ) + + def test_exclude_tests_kwarg_is_effective(tmp_path: Path) -> None: """``exclude_tests=True`` mirrors ``analyze``: a ``#[test]`` fn is pruned from the batch result, so the two entry points agree.""" diff --git a/big-code-analysis-py/tests/test_book_examples.py b/big-code-analysis-py/tests/test_book_examples.py index d592751ac..74265a989 100644 --- a/big-code-analysis-py/tests/test_book_examples.py +++ b/big-code-analysis-py/tests/test_book_examples.py @@ -91,7 +91,7 @@ def test_quick_start() -> None: assert "cyclomatic" in result["metrics"] -def test_batch_processing() -> None: +def test_batch_processing(tmp_path: Path) -> None: """Regression (#882): the example's ``zip(..., strict=True)`` must hold when a generated file is in the batch. @@ -103,7 +103,9 @@ def test_batch_processing() -> None: the same bug #660 fixed in ``pipeline_db.py``. The fixtures below are deliberately mixed: a generated file (must be analysed here, not skipped), a normal source file, and a missing file (the - ``AnalysisFailure`` discriminator path). + ``AnalysisFailure`` discriminator path), and a three-byte file the + read gate declines to parse — the #1238 slot, which holds its + position as ``None`` and exercises the example's third branch. """ mod = _load("batch_processing") generated = FIXTURES_DIR / "generated.rs" @@ -111,20 +113,26 @@ def test_batch_processing() -> None: # batch's `skip_generated=False` must NOT — confirming the fixture is # genuinely generated keeps this test non-vacuous for the skip path. assert bca.analyze(generated) is None, "fixture must be detected as generated" + tiny = tmp_path / "tiny.rs" + tiny.write_bytes(b"ab\n") + assert bca.analyze(tiny) is None, "fixture must be declined by the read gate" summary = mod.run( [ generated, FIXTURES_DIR / "hello.rs", FIXTURES_DIR / "does_not_exist.rs", + tiny, ] ) # Reaching here proves the strict zip did not raise. The generated # file is analysed (not dropped), so it lands in the `ok` bucket - # alongside `hello.rs`; only the missing file errors. - assert summary["total"] == 3 + # alongside `hello.rs`; the missing file errors, and the tiny file + # occupies its slot as the `None` the example buckets as skipped. + assert summary["total"] == 4 assert summary["ok"] == 2 assert summary["errors"] == 1 + assert summary["skipped"] == 1 def test_batch_processing_parallel() -> None: @@ -539,7 +547,9 @@ def test_pipeline_db_batch_branch_analyses_generated_file(tmp_path: Path) -> Non Threads the ``generated.rs`` fixture (first line carries ``@generated`` / ``DO NOT EDIT``) through ``extra_paths`` so it is - among the inputs. + among the inputs, alongside a three-byte file — the #1238 slot the + read gate declines, which the branch buckets as ``skipped`` rather + than letting it shrink the list under the strict zip. """ mod = _load("pipeline_db") db_path = tmp_path / "metrics.db" @@ -548,20 +558,27 @@ def test_pipeline_db_batch_branch_analyses_generated_file(tmp_path: Path) -> Non # must not. (Confirms the fixture really is generated, so the test # is not vacuous.) assert bca.analyze(generated) is None, "fixture must be detected as generated" + tiny = tmp_path / "tiny.rs" + tiny.write_bytes(b"ab\n") + assert bca.analyze(tiny) is None, "fixture must be declined by the read gate" summary = mod.run( FIXTURES_DIR, db_path, - extra_paths=[generated], + extra_paths=[generated, tiny], top_n=3, skip_generated=False, ) # Reaching here at all proves the strict zip did not raise. The # generated file is analysed (not skipped), so it lands in the - # analyzed bucket and contributes rows. + # analyzed bucket and contributes rows; the three-byte file lands + # in `skipped`, which is the branch #1238 added. assert summary["analyzed"] > 0 assert summary["rows"] > 0 + assert summary["skipped"] == 1, ( + "the read-gate slot must be bucketed as skipped, not analysed or errored" + ) assert db_path.exists() diff --git a/big-code-analysis-py/tests/test_discovery.py b/big-code-analysis-py/tests/test_discovery.py index d09084c9a..c4daaa27d 100644 --- a/big-code-analysis-py/tests/test_discovery.py +++ b/big-code-analysis-py/tests/test_discovery.py @@ -83,22 +83,26 @@ def _write(root: Path, rel: str, content: str) -> Path: return path -def _names(results: list[FuncSpaceDict | bca.AnalysisFailure]) -> set[str]: +def _names(results: list[FuncSpaceDict | bca.AnalysisFailure | None]) -> set[str]: """Repo-relative-ish basenames of the analysed (dict) results. - Drops ``AnalysisFailure`` elements, so this set answers only "which - files were *analysed*". Negative assertions ("file X must not - appear") must additionally consult ``_failure_paths`` — a file that - degrades into the failure stream is invisible here (#921). + Drops ``AnalysisFailure`` and ``None`` elements, so this set answers + only "which files were *analysed*". Negative assertions ("file X + must not appear") must additionally consult ``_failure_paths`` — a + file that degrades into the failure stream is invisible here + (#921) — and, under ``skip_generated=False``, ``_placeholders``, + which counts the read-gate slots (#1238). """ return { Path(name).name for r in results - if not isinstance(r, bca.AnalysisFailure) and (name := r["name"]) is not None + if r is not None + and not isinstance(r, bca.AnalysisFailure) + and (name := r["name"]) is not None } -def _failure_paths(results: list[FuncSpaceDict | bca.AnalysisFailure]) -> set[str]: +def _failure_paths(results: list[FuncSpaceDict | bca.AnalysisFailure | None]) -> set[str]: """Basenames of every ``AnalysisFailure`` element. The complement of ``_names``: lets a negative assertion catch a file @@ -108,6 +112,13 @@ def _failure_paths(results: list[FuncSpaceDict | bca.AnalysisFailure]) -> set[st return {Path(r.path).name for r in results if isinstance(r, bca.AnalysisFailure)} +def _placeholders(results: list[FuncSpaceDict | bca.AnalysisFailure | None]) -> int: + """Count of ``None`` slots — files the read gate declined to parse + but which keep their position under ``skip_generated=False`` (#1238). + """ + return sum(1 for r in results if r is None) + + def test_analyze_paths_walks_a_directory(tmp_path: Path) -> None: """#658: pointing at a directory analyses every source file under it, returning the batch shape.""" @@ -209,6 +220,39 @@ def test_analyze_paths_skips_generated_by_default(tmp_path: Path) -> None: assert "gen.rs" not in _names(results) | _failure_paths(results) +def test_analyze_paths_placeholders_unreadable_files_when_not_skipping( + tmp_path: Path, +) -> None: + """#1238: the walker shares ``analyze_batch``'s slot vocabulary, so + ``skip_generated=False`` turns a read-gate skip into a ``None`` + element instead of dropping it. + + There is no caller-supplied ordering to pair against here — the + point is that the two entry points agree on what a slot can be. + """ + _write(tmp_path, "real.rs", "fn real() {}\n") + # Three bytes: the read gate treats the file as empty. Written as + # bytes because `_write` would append nothing and the size is the + # whole point. + (tmp_path / "tiny.rs").write_bytes(b"ab\n") + + kept = bca.analyze_paths(tmp_path, skip_generated=False) + assert _names(kept) == {"real.rs"} + assert _placeholders(kept) == 1, ( + "with skip_generated=False the unreadable file holds a None slot" + ) + assert "tiny.rs" not in _failure_paths(kept), ( + "an unparseable file is a skip, not an AnalysisFailure" + ) + + dropped = bca.analyze_paths(tmp_path) + assert _names(dropped) == {"real.rs"} + assert _placeholders(dropped) == 0, ( + "the default still omits the slot entirely — #1238 changed only " + "the skip_generated=False behaviour" + ) + + def test_analyze_paths_failure_is_a_failure_element_not_raise( tmp_path: Path, ) -> None: diff --git a/big-code-analysis-py/tests/test_types.py b/big-code-analysis-py/tests/test_types.py index 2a0174081..dbab73255 100644 --- a/big-code-analysis-py/tests/test_types.py +++ b/big-code-analysis-py/tests/test_types.py @@ -70,7 +70,7 @@ def test_analyze_batch_yields_funcspacedict_or_error(tmp_path: Path) -> None: entry = results[0] # The non-error branch narrows to FuncSpaceDict, so the metric table is # typed without a cast. - assert not isinstance(entry, bca.AnalysisFailure) + assert isinstance(entry, dict) assert_type(entry, FuncSpaceDict) assert "loc" in entry["metrics"] diff --git a/big-code-analysis-py/tests/test_vcs.py b/big-code-analysis-py/tests/test_vcs.py index 813908b3c..01818e548 100644 --- a/big-code-analysis-py/tests/test_vcs.py +++ b/big-code-analysis-py/tests/test_vcs.py @@ -788,7 +788,7 @@ def test_analyze_batch_vcs_matches_per_file_analyze(tmp_path: Path) -> None: repo = _build_repo(tmp_path) work = repo / "work.rs" [batch_result] = bca.analyze_batch([work], vcs=True) - assert not isinstance(batch_result, bca.AnalysisFailure) + assert isinstance(batch_result, dict) single = bca.analyze(work, vcs=True) assert single is not None assert batch_result["metrics"]["vcs"] == single["metrics"]["vcs"] @@ -799,7 +799,7 @@ def test_analyze_batch_without_vcs_has_no_block(tmp_path: Path) -> None: migrating a plain comprehension stays behaviour-preserving.""" repo = _build_repo(tmp_path) [result] = bca.analyze_batch([repo / "work.rs"]) - assert not isinstance(result, bca.AnalysisFailure) + assert isinstance(result, dict) assert "vcs" not in result["metrics"] @@ -810,7 +810,7 @@ def test_analyze_batch_vcs_per_function_attaches_nested_blocks( nested space across the batch, mirroring single-file ``analyze``.""" repo = _build_multifn_repo(tmp_path) [result] = bca.analyze_batch([repo / "work.rs"], vcs_per_function=True) - assert not isinstance(result, bca.AnalysisFailure) + assert isinstance(result, dict) spaces = _func_spaces(result) assert len(spaces) == 2 for space in spaces: @@ -823,7 +823,7 @@ def test_analyze_batch_vcs_file_outside_repo_degrades(tmp_path: Path) -> None: loose = tmp_path / "loose.rs" loose.write_text("fn solo() {}\n") [result] = bca.analyze_batch([loose], vcs=True) - assert not isinstance(result, bca.AnalysisFailure) + assert isinstance(result, dict) assert "vcs" not in result["metrics"] @@ -872,7 +872,7 @@ def test_analyze_batch_vcs_spans_two_subdirectories(tmp_path: Path) -> None: results = bca.analyze_batch([a, b], vcs=True) assert len(results) == 2 for result in results: - assert not isinstance(result, bca.AnalysisFailure) + assert isinstance(result, dict) vcs = result["metrics"]["vcs"] assert vcs["commits_long"] == 1 assert vcs["bug_fix_commits"] == 1 @@ -890,7 +890,7 @@ def test_analyze_batch_vcs_per_function_spans_two_subdirectories( results = bca.analyze_batch([a, b], vcs_per_function=True) assert len(results) == 2 for result in results: - assert not isinstance(result, bca.AnalysisFailure) + assert isinstance(result, dict) spaces = _func_spaces(result) assert spaces, "each file has at least one function space" assert all("vcs" in space["metrics"] for space in spaces) @@ -909,7 +909,7 @@ def test_analyze_batch_vcs_distinguishes_separate_repos(tmp_path: Path) -> None: results = bca.analyze_batch([work_one, work_two], vcs=True) assert len(results) == 2 for result in results: - assert not isinstance(result, bca.AnalysisFailure) + assert isinstance(result, dict) # Each file is tracked in exactly its own single-file repo, so the # block is present and reflects that repo's one bug-fix commit. vcs = result["metrics"]["vcs"] diff --git a/docs/development/lessons_learned.md b/docs/development/lessons_learned.md index c9a3bad7f..70a89cf89 100644 --- a/docs/development/lessons_learned.md +++ b/docs/development/lessons_learned.md @@ -1602,15 +1602,29 @@ The original `96fe3ab` shipped a defensive `PyAnalysisError` fallback; a review-remediation pass in `e670f8b` regressed it to `unreachable!()` with a comment claiming it would "fail loudly in development" — exactly the failure mode this lesson warns against — and `515e840` restored the -fallback. The single-file bridge returns `Ok(None)` only when -`skip_generated=true`, and `analyze_batch` hard-codes `false`, so the arm -is unreachable *today*. But the documented contract is "never raises on +fallback. At the time, the single-file bridge returned `Ok(None)` only +when `skip_generated=true` and `analyze_batch` hard-coded `false`, so +the arm was unreachable. But the documented contract is "never raises on per-file errors", which demands a structured `AnalysisError` in the result slot. The restored fallback names the invariant break and tells the operator to audit `analyze_path` for new skip surfaces, so the contract survives any future refactor adding a second one (a gitignore filter, a size cap). +**Both premises have since fallen, exactly as anticipated.** #706 routed +the bindings through the walker's `read_file_with_eol` gate — a second +and *unconditional* `Ok(None)` source (three bytes or fewer, a UTF-16 +BOM, a non-UTF-8 leading window) — and #542 flipped `skip_generated` to +default `true`. The arm is live on every call. That went unnoticed +until #1238, where the batch docstrings' "`skip_generated=False` +guarantees one element per input" turned out to be unsatisfiable and +the endorsed `zip(inputs, results)` was attributing metrics to the +wrong paths. The +fallback's job — carrying the contract across a refactor nobody +re-audited — is the part that held. What did not was any gate on the +arm's *documentation*, which went on describing one skip source for two +releases after the second was added. + --- ## 43. `to_string_lossy()` on a path field promoted into `Hash` / `PartialEq` keys silently collapses dedup From 4ce97f4d7b14b57d59f7d7938bef0fbdef4c58d1 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 23 Aug 2026 07:20:31 -0700 Subject: [PATCH 2/4] test(py): pin to_sarif placeholder precondition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The assertion "to_sarif skips the None entries" is vacuously true of a list that has none — which is exactly what pre-#1238 analyze_batch returned, so the test passed against the bug it sits downstream of. Assert the slot count first. Refs #1238 --- big-code-analysis-py/tests/test_batch.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/big-code-analysis-py/tests/test_batch.py b/big-code-analysis-py/tests/test_batch.py index 68e4a74a1..d0699298b 100644 --- a/big-code-analysis-py/tests/test_batch.py +++ b/big-code-analysis-py/tests/test_batch.py @@ -705,16 +705,16 @@ def test_to_sarif_accepts_the_placeholder_slots(tmp_path: Path) -> None: """The #1238 placeholders must stay transparent to ``to_sarif``, which documents ``None`` entries as "no record emitted" and skips them — so a caller can hand it ``analyze_batch``'s list verbatim. - - This is a forward guard, not regression coverage for #1238: it - passes against the pre-fix code too, because a dropped slot and a - skipped slot render the same document. What it would catch is a - future ``to_sarif`` that raised on — or emitted a finding for — a - ``None`` entry. """ inputs = _unreadable_batch(tmp_path) results = bca.analyze_batch(inputs, skip_generated=False) + # Pin the precondition. "to_sarif skips the None entries" is + # vacuously true of a list that has none — which is exactly what + # pre-#1238 `analyze_batch` returned, so without this line the test + # passes against the bug it is meant to sit downstream of. + assert results.count(None) == len(inputs) - 1 + # A threshold every analysed file trips, so "no findings" cannot be # mistaken for "the placeholders were skipped". doc = json.loads(bca.to_sarif(results, thresholds={"loc.sloc": 0})) From 71f7f93fd8f4e2dcfd46c0ba1331ff5f3915c7ae Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 23 Aug 2026 07:20:31 -0700 Subject: [PATCH 3/4] docs(rules): flag perturbation-parser failures A sweep's result parser is as much a part of the subject as the file under test. A uniform zero across perturbations describes the harness, not the coverage. Refs #1238 --- .claude/rules/testing.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.claude/rules/testing.md b/.claude/rules/testing.md index 76259f22e..6382ea52c 100644 --- a/.claude/rules/testing.md +++ b/.claude/rules/testing.md @@ -68,6 +68,17 @@ 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. +**The result *parser* is the other half of the subject.** During #1238 a +sweep drove three perturbations of one match arm and reported zero Rust +failures for all three, while the Python leg of the same sweep reported +the expected four, five and eight. Nothing was stale — the driver ran +`cargo test -q`, which prints dots rather than per-test lines, so a +parser scanning for `... FAILED` matched nothing and every mutation read +as "no test noticed". A uniform zero across perturbations is the same +tell as a uniform 34: it describes the harness, not the code. Cross-check +any parsed count against the process exit status and treat a +disagreement in *either* direction as a harness bug, not a result. + After restoring, `git status` / `git diff --stat` must show exactly the edits you intend — nothing extra, nothing missing. From 016ee72bfff8b159172e8f6de00a87e1c912c2c2 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 23 Aug 2026 07:26:02 -0700 Subject: [PATCH 4/4] fix(py): apply code-review findings on the #1238 fix - The new to_sarif test compared a SARIF artifactLocation.uri to a raw str(path); the writer emits an RFC 3986 reference (percent- encoded, file:///C:/... on Windows), so the Windows pytest leg would fail. Assert count + basename instead. - to_sarif's runtime TypeError and docstring still named AnalysisError, removed at 2.0 (#614). - Three example surfaces printed "generated" for files analyze() skips for read-gate reasons (empty/tiny, UTF-16 BOM, binary): pipeline_db's per-file branch, sarif_upload's summary line, and the Jupyter quick-start cell. sarif_upload's analysed counter also flips to isinstance(r, dict) so a future skip_generated=False caller cannot count None as analysed. - analyze_path's rustdoc claimed a UTF-16 BOM is stripped; it is rejected (#803), matching the correction already made in the stub. - analyze_paths preallocates for missing seeds too, which the 1:1 contract now makes the exact final length. - The Rust test table carries an Expect per row, so both tests derive every position and count from the table instead of hardcoded indices a new row would silently shift. - Docs: the CHANGELOG entry gains the **(breaking)** marker per the #1056 precedent; lesson 42's follow-up now records that the #542 commit deleted the defensive fallback (the read-gate silence was its absence, not its failure); "behaviour-preserving" migration claims are qualified with the list-shape difference; the read-gate enumeration gains the mid-read-shrink case in the two contract surfaces; flatten_spaces' docstring names the batch None slots; the README zip snippet demonstrates the safe three-branch form. Refs #1238 --- CHANGELOG.md | 2 +- STABILITY.md | 3 +- big-code-analysis-book/src/python/batch.md | 11 +- big-code-analysis-py/README.md | 11 +- .../examples/batch_processing.py | 7 +- .../examples/jupyter_quickstart.ipynb | 2 +- big-code-analysis-py/examples/pipeline_db.py | 5 +- big-code-analysis-py/examples/sarif_upload.py | 13 +- .../python/big_code_analysis/_flatten.py | 7 +- .../python/big_code_analysis/_native.pyi | 12 +- big-code-analysis-py/src/analysis.rs | 8 +- big-code-analysis-py/src/batch.rs | 124 +++++++++++------- big-code-analysis-py/src/sarif.rs | 8 +- big-code-analysis-py/tests/test_batch.py | 6 +- .../tests/test_book_examples.py | 24 +++- docs/development/lessons_learned.md | 34 +++-- 16 files changed, 172 insertions(+), 105 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d24a04342..fb8699054 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -108,7 +108,7 @@ for historical reference. ### Fixed -- The Python bindings' `analyze_batch` / `analyze_paths` dropped a +- **(breaking)** The Python bindings' `analyze_batch` / `analyze_paths` dropped a result slot for any file the read gate declines to parse — three bytes or fewer, a UTF-16 BOM, or a leading window that is not valid UTF-8 — even under `skip_generated=False`, which both entry points diff --git a/STABILITY.md b/STABILITY.md index 82db3efc0..3ce2da66b 100644 --- a/STABILITY.md +++ b/STABILITY.md @@ -1038,7 +1038,8 @@ the existing values are frozen. A batch slot is not always a result or a failure. `analyze_batch` / `analyze_paths` emit a `None` element, under `skip_generated=False` only, for a file the shared read gate declines to parse — three bytes -or fewer, a UTF-16 BOM, or a leading window that is not valid UTF-8. +or fewer, a UTF-16 BOM, a leading window that is not valid UTF-8, or +(rarely) a file that shrank between the size probe and the read. That gate is unconditional, so before #1238 those files produced no element at all and the documented `zip(inputs, results)` mis-paired every later entry; the placeholder is what makes `skip_generated=False` diff --git a/big-code-analysis-book/src/python/batch.md b/big-code-analysis-book/src/python/batch.md index 4a372f6cd..f499cb07c 100644 --- a/big-code-analysis-book/src/python/batch.md +++ b/big-code-analysis-book/src/python/batch.md @@ -7,11 +7,14 @@ errors**: each result element is an analysis `dict`, a preserve input order, so `zip(inputs, results)` lines up by index **when no path is skipped**. `analyze_batch` shares `analyze`'s keyword-only options — `exclude_tests`, `allow_lossy_path`, `skip_generated` -(default `True`), and `metrics` — so the two entry points are -behaviour-preserving. +(default `True`), and `metrics` — so a file is treated the same by +both entry points. (The list *shape* matches the +`[bca.analyze(p) for p in paths]` comprehension only under +`skip_generated=False`; with the default the comprehension keeps a +`None` per skipped file while the batch drops the slot.) ```python -{{#include ../../../big-code-analysis-py/examples/batch_processing.py:18:58}} +{{#include ../../../big-code-analysis-py/examples/batch_processing.py:18:55}} ``` A few key contracts: @@ -95,7 +98,7 @@ sequential sweep. For parallelism, fan the per-file `analyze` call out across a thread pool: ```python -{{#include ../../../big-code-analysis-py/examples/batch_processing.py:61:73}} +{{#include ../../../big-code-analysis-py/examples/batch_processing.py:58:70}} ``` PyO3's `Python::detach` releases the GIL across each file's read + diff --git a/big-code-analysis-py/README.md b/big-code-analysis-py/README.md index 3d0b3984d..15848a62a 100644 --- a/big-code-analysis-py/README.md +++ b/big-code-analysis-py/README.md @@ -291,8 +291,11 @@ keyword-only options as `analyze` — `exclude_tests`, migrating between the two is behavior-preserving. With the default `skip_generated=True`, a generated file is *skipped* -and produces no result element (matching single-file `analyze`, which -returns `None`), so the result list can be shorter than the input. +and produces no result element, so the result list can be shorter than +the input. Note the shape difference from the comprehension it +replaces: `[bca.analyze(p) for p in paths]` keeps a `None` per skipped +file (staying index-aligned), while the batch drops the slot — the +per-file *filter semantics* match, the list shape does not. This default flipped at 2.0 — the pre-2.0 `analyze_batch` always analyzed generated files (`skip_generated=False`); pass that explicitly to restore one-element-per-input. @@ -310,10 +313,12 @@ mis-attributed every later result to the wrong path. import big_code_analysis as bca paths = ["src/a.py", "src/missing.py", "src/b.rs"] -results = bca.analyze_batch(paths) +results = bca.analyze_batch(paths, skip_generated=False) for path, result in zip(paths, results): if isinstance(result, bca.AnalysisFailure): print(f"skipped {path}: ({result.error_kind}) {result.error}") + elif result is None: + print(f"skipped {path}: nothing to parse (empty or binary)") else: process(result) ``` diff --git a/big-code-analysis-py/examples/batch_processing.py b/big-code-analysis-py/examples/batch_processing.py index 4bc9e3f07..1ad9a5938 100644 --- a/big-code-analysis-py/examples/batch_processing.py +++ b/big-code-analysis-py/examples/batch_processing.py @@ -38,11 +38,8 @@ def run(paths: Iterable[Path]) -> dict[str, int]: errors += 1 print(f" skip {path}: ({result.error_kind}) {result.error}") elif result is None: - # A slot the read gate declined to parse — three bytes or - # fewer, a UTF-16 BOM, or a binary leading window (#1238). - # `skip_generated=False` keeps the position rather than - # dropping it, which is what makes the strict zip safe; the - # third branch is the price of that guarantee. + # The read gate declined this file; the slot is held open + # so the strict zip above stays aligned (#1238). skipped += 1 print(f" skip {path}: nothing to parse (empty or binary)") else: diff --git a/big-code-analysis-py/examples/jupyter_quickstart.ipynb b/big-code-analysis-py/examples/jupyter_quickstart.ipynb index 5059ebdd0..d68566d32 100644 --- a/big-code-analysis-py/examples/jupyter_quickstart.ipynb +++ b/big-code-analysis-py/examples/jupyter_quickstart.ipynb @@ -68,7 +68,7 @@ "id": "bca-quickstart-batch", "metadata": {}, "outputs": [], - "source": "rows: list[dict[str, object]] = []\nskipped: list[Path] = []\nerrors: list[tuple[Path, Exception]] = []\nfor path in sources:\n try:\n result = bca.analyze(path)\n except (OSError, ValueError) as exc:\n errors.append((path, exc))\n continue\n if result is None:\n skipped.append(path)\n continue\n # Anchor each flattened row to its source file so per-path\n # joins / group-bys work downstream.\n rows.extend({\"source\": path.name, **record} for record in bca.flatten_spaces(result))\n\nprint(\n f\"analysed {len(sources) - len(skipped) - len(errors)} files, \"\n f\"skipped {len(skipped)} generated, {len(errors)} errors\"\n)\ndf = pd.DataFrame.from_records(rows)\ndf.head()" + "source": "rows: list[dict[str, object]] = []\nskipped: list[Path] = []\nerrors: list[tuple[Path, Exception]] = []\nfor path in sources:\n try:\n result = bca.analyze(path)\n except (OSError, ValueError) as exc:\n errors.append((path, exc))\n continue\n if result is None:\n skipped.append(path)\n continue\n # Anchor each flattened row to its source file so per-path\n # joins / group-bys work downstream.\n rows.extend({\"source\": path.name, **record} for record in bca.flatten_spaces(result))\n\nprint(\n f\"analysed {len(sources) - len(skipped) - len(errors)} files, \"\n f\"skipped {len(skipped)} (generated, empty, or binary), {len(errors)} errors\"\n)\ndf = pd.DataFrame.from_records(rows)\ndf.head()" }, { "cell_type": "markdown", diff --git a/big-code-analysis-py/examples/pipeline_db.py b/big-code-analysis-py/examples/pipeline_db.py index bec82a57a..23f914d0e 100644 --- a/big-code-analysis-py/examples/pipeline_db.py +++ b/big-code-analysis-py/examples/pipeline_db.py @@ -127,8 +127,11 @@ def run( print(f" skip {path}: ({type(exc).__name__}) {exc}") continue if result is None: + # `analyze` returns None for a generated file AND for one + # the read gate declines (empty/tiny, UTF-16 BOM, binary) + # — the single-file API does not say which (#1238). skipped += 1 - print(f" skip {path}: looks generated") + print(f" skip {path}: generated, empty, or binary") continue analyzed += 1 flat_rows.extend(dict(record) for record in bca.flatten_spaces(result)) diff --git a/big-code-analysis-py/examples/sarif_upload.py b/big-code-analysis-py/examples/sarif_upload.py index 3b0f56134..5807e72a9 100644 --- a/big-code-analysis-py/examples/sarif_upload.py +++ b/big-code-analysis-py/examples/sarif_upload.py @@ -131,10 +131,11 @@ def run( zero independently — a regression that silently drops every finding would otherwise look identical to a healthy run. - ``bca.analyze_batch`` omits generated/skipped files from its - returned list entirely (with the #542 default ``skip_generated= - True``), so ``len(batch)`` is the analysed-or-failed count, not - the input count. We surface the gap as ``skipped`` (= + ``bca.analyze_batch`` omits skipped files — generated ones and + those the read gate declines (empty/tiny, UTF-16 BOM, binary) — + from its returned list entirely (with the #542 default + ``skip_generated=True``), so ``len(batch)`` is the + analysed-or-failed count, not the input count. We surface the gap as ``skipped`` (= ``len(materialised) - len(batch)``) so the totals reconcile: ``analyzed + errors + skipped == len(materialised)``. In a SARIF-upload pipeline a silently-skipped file is exactly the @@ -147,7 +148,7 @@ def run( raise SystemExit(msg) batch = bca.analyze_batch(materialised) - analyzed = sum(1 for r in batch if not isinstance(r, bca.AnalysisFailure)) + analyzed = sum(1 for r in batch if isinstance(r, dict)) errors = len(batch) - analyzed skipped = len(materialised) - len(batch) @@ -193,7 +194,7 @@ def run( print( f"wrote {output} ({analyzed} analysed, {errors} errors, " - f"{skipped} generated skipped, " + f"{skipped} skipped (generated or unreadable), " f"{len(results)} findings across {len(rules)} rules)" ) print( diff --git a/big-code-analysis-py/python/big_code_analysis/_flatten.py b/big-code-analysis-py/python/big_code_analysis/_flatten.py index c698fe459..4366513d8 100644 --- a/big-code-analysis-py/python/big_code_analysis/_flatten.py +++ b/big-code-analysis-py/python/big_code_analysis/_flatten.py @@ -114,8 +114,11 @@ def flatten_spaces( Raises: TypeError: if *result* is not a mapping. ``analyze`` can return ``None`` for filtered-out inputs (e.g. - ``skip_generated=True`` matched a generated file); - callers must filter ``None`` before flattening. + ``skip_generated=True`` matched a generated file), and + ``analyze_batch`` / ``analyze_paths`` emit ``None`` slots + under ``skip_generated=False`` for files the read gate + declines (#1238); callers must filter ``None`` before + flattening. """ # Annotation says ``Mapping[str, Any]``; the runtime check is # defensive (a caller can pass ``analyze()``'s ``None`` return diff --git a/big-code-analysis-py/python/big_code_analysis/_native.pyi b/big-code-analysis-py/python/big_code_analysis/_native.pyi index 70b348921..0d99510c8 100644 --- a/big-code-analysis-py/python/big_code_analysis/_native.pyi +++ b/big-code-analysis-py/python/big_code_analysis/_native.pyi @@ -710,9 +710,10 @@ def analyze_batch( * a ``dict`` matching :func:`analyze`'s output shape, * an :class:`AnalysisFailure` describing the per-file failure, or * ``None`` for a file the read gate declines to parse — three - bytes or fewer, a UTF-16 BOM, or a leading window that is not - valid UTF-8. This is the same ``None`` :func:`analyze` returns - for those files, and it appears only under + bytes or fewer, a UTF-16 BOM, a leading window that is not + valid UTF-8, or (rarely) a file that shrank between the size + probe and the read. This is the same ``None`` :func:`analyze` + returns for those files, and it appears only under ``skip_generated=False``. A path that is skipped (``skip_generated=True`` and the file is @@ -764,7 +765,10 @@ def analyze_batch( ``exclude_tests``, ``allow_lossy_path``, and ``skip_generated`` mirror the keyword-only kwargs on :func:`analyze` exactly (#542), so migrating ``[bca.analyze(p) for p in paths]`` to - ``bca.analyze_batch(paths)`` is behaviour-preserving. In + ``bca.analyze_batch(paths)`` preserves each file's treatment. The + list *shape* is preserved only under ``skip_generated=False``: with + the default the comprehension keeps a ``None`` per skipped file + (index-aligned) while the batch drops the slot. In particular ``skip_generated`` defaults to ``True`` here too: a generated file is *skipped* (it yields no element), matching the CLI walker and :func:`analyze`'s ``None`` return. This default diff --git a/big-code-analysis-py/src/analysis.rs b/big-code-analysis-py/src/analysis.rs index cd1defd0e..ea07b1790 100644 --- a/big-code-analysis-py/src/analysis.rs +++ b/big-code-analysis-py/src/analysis.rs @@ -286,8 +286,8 @@ impl From for AnalysisError { /// The read goes through the CLI walker's own /// [`big_code_analysis::read_file_with_eol`] helper (#706), so the /// binding applies the walker's pre-dispatch gating verbatim: files of -/// three bytes or fewer are treated as empty, a UTF-8 / UTF-16 BOM is -/// stripped, a non-UTF-8 leading window marks the file as binary, and +/// three bytes or fewer are treated as empty, a UTF-8 BOM is stripped, +/// a UTF-16 BOM or a non-UTF-8 leading window marks the file as binary, and /// CR/CRLF line endings are normalised to LF. Each of these cases /// returns `Ok(None)` — the same "no record emitted" signal the CLI /// walker uses when it skips a file (and the same the `skip_generated` @@ -351,8 +351,8 @@ pub(crate) fn analyze_path( // (`read_file_with_eol`) rather than a plain `std::fs::read`, so the // binding inherits the walker's pre-dispatch file gating verbatim // (issue #706, #640): files of three bytes or fewer are treated as - // empty, UTF-8 / UTF-16 BOMs are stripped, a non-UTF-8 leading window - // marks the file as binary, and CR/CRLF endings are normalised to LF + // empty, a UTF-8 BOM is stripped, a UTF-16 BOM or a non-UTF-8 leading + // window marks the file as binary, and CR/CRLF endings are normalised to LF // with a guaranteed trailing newline. Each skip case returns // `Ok(None)` — the same "no record emitted" signal the CLI walker // uses when it discards a file, and the same signal `skip_generated` diff --git a/big-code-analysis-py/src/batch.rs b/big-code-analysis-py/src/batch.rs index 4ef76be87..0f4e59d0d 100644 --- a/big-code-analysis-py/src/batch.rs +++ b/big-code-analysis-py/src/batch.rs @@ -352,14 +352,15 @@ const _: fn() = || { /// /// `exclude_tests`, `allow_lossy_path`, and `skip_generated` mirror /// the keyword-only kwargs on [`crate::analyze`] verbatim (#542), so -/// migrating a comprehension from `analyze` to `analyze_batch` is -/// behaviour-preserving. In particular `skip_generated` defaults to -/// `true` here too: a generated file is *skipped* (its input position -/// yields no `dict`), so the result list can be **shorter** than the -/// input iterable when `skip_generated=true`. Pass +/// migrating a comprehension from `analyze` to `analyze_batch` +/// preserves each file's treatment — though not the list shape under +/// the default, since the comprehension keeps a `None` per skipped +/// file while the batch drops the slot. In particular `skip_generated` +/// defaults to `true` here too: a generated file is *skipped* (its +/// input position yields no `dict`), so the result list can be +/// **shorter** than the input iterable when `skip_generated=true`. Pass /// `skip_generated=false` to restore the legacy "one result per input, -/// always" behaviour (every position produces a `dict`, an -/// `AnalysisFailure`, or `None`). +/// always" behaviour, with the slot vocabulary described above. #[pyfunction] #[pyo3(signature = (paths, /, *, exclude_tests = false, allow_lossy_path = false, skip_generated = true, metrics = None, vcs = false, vcs_per_function = false))] // `metrics: Option>` is taken by value to match the PyO3 @@ -685,7 +686,11 @@ pub(crate) fn analyze_paths<'py>( metrics: metric_set, }; let mut vcs_repos = VcsRepoCache::new(vcs, vcs_per_function); - let mut results: Vec> = Vec::with_capacity(walked.files.len()); + // Under `skip_generated=false` every discovered file yields exactly + // one slot, and each missing seed leads the list, so this is the + // exact final length (#1238 made the drop-a-slot shortfall go away). + let mut results: Vec> = + Vec::with_capacity(walked.files.len() + walked.missing_seeds.len()); // A nonexistent / dangling-symlink seed is surfaced as an // `AnalysisFailure` rather than silently dropped (#858). The CLI hard- // errors on a missing `--paths` seed (#596); the binding keeps that @@ -923,6 +928,24 @@ mod tests { } // ── #1238: one slot per input under `skip_generated=false` ─────── + // + // These duplicate the Python tests in `tests/test_batch.py` one + // layer down, on purpose: Rust patch coverage comes from + // `cargo llvm-cov nextest`, which never runs pytest, so without + // this pair the guarded arm reads as uncovered. Keep both pairs. + + /// What one fixture row must produce in a result slot. + #[derive(Clone, Copy)] + enum Expect { + /// The read gate declines the file: no slot under the default, + /// a `None` placeholder under `skip_generated=false` (#1238). + ReadGateSkip, + /// The `skip_generated` filter owns the file: no slot under the + /// default, analysed normally once the filter is off. + Generated, + /// Parseable regardless of the flag; always a `dict` slot. + Analysed, + } /// Three bytes, so `read_file_with_eol` treats the file as empty /// and `analyze_path` returns `Ok(None)` before any language @@ -937,37 +960,33 @@ mod tests { /// exercises the `str::from_utf8` arm. const BINARY_SOURCE: &[u8] = b"\x80\x81\x82\x83\n"; /// Carries the CLI walker's `is_generated` markers, so it is the - /// `skip_generated`-gated `Ok(None)` source — analysed normally - /// once the filter is off. + /// `skip_generated`-gated `Ok(None)` source. const GENERATED_SOURCE: &[u8] = b"// @generated DO NOT EDIT\npub fn x() {}\n"; /// An ordinary parseable file, present so neither test asserts a /// length against an all-skipped input. const GOOD_SOURCE: &[u8] = b"fn main() {}\n"; - /// One input per `Ok(None)` source plus a parseable control, shared - /// by the pair of tests below so their only difference is the flag. - const MIXED_INPUTS: &[(&str, &[u8])] = &[ - ("tiny.rs", TINY_SOURCE), - ("bom.rs", UTF16_BOM_SOURCE), - ("binary.rs", BINARY_SOURCE), - ("gen.rs", GENERATED_SOURCE), - ("good.rs", GOOD_SOURCE), + /// One input per `Ok(None)` source plus a parseable control, with + /// each row carrying its own expectation so the tests below derive + /// every position and count from the table instead of hardcoding + /// indices that a new row would silently shift. + const MIXED_INPUTS: &[(&str, &[u8], Expect)] = &[ + ("tiny.rs", TINY_SOURCE, Expect::ReadGateSkip), + ("bom.rs", UTF16_BOM_SOURCE, Expect::ReadGateSkip), + ("binary.rs", BINARY_SOURCE, Expect::ReadGateSkip), + ("gen.rs", GENERATED_SOURCE, Expect::Generated), + ("good.rs", GOOD_SOURCE, Expect::Analysed), ]; - /// Materialise `files` under `dir` and run each through + /// Materialise [`MIXED_INPUTS`] under `dir` and run each row through /// [`push_one_result`], returning the slots it pushed. - fn pushed_slots( - py: Python<'_>, - dir: &Path, - skip_generated: bool, - files: &[(&str, &[u8])], - ) -> Vec> { + fn pushed_slots(py: Python<'_>, dir: &Path, skip_generated: bool) -> Vec> { let opts = AnalyzeOptions { skip_generated, ..AnalyzeOptions::default() }; let mut vcs_repos = VcsRepoCache::new(false, false); let mut results = Vec::new(); - for (name, bytes) in files { + for (name, bytes, _) in MIXED_INPUTS { let path = dir.join(name); std::fs::write(&path, bytes).expect("write fixture file"); push_one_result(py, &path, opts, &mut vcs_repos, &mut results) @@ -1006,33 +1025,32 @@ mod tests { // placeheld, so the new guard must not intercept it. let dir = tempfile::tempdir().expect("tempdir"); Python::attach(|py| { - let slots = pushed_slots(py, dir.path(), false, MIXED_INPUTS); + let slots = pushed_slots(py, dir.path(), false); assert_eq!( slots.len(), MIXED_INPUTS.len(), "skip_generated=false must yield one slot per input so \ `zip(inputs, results)` keeps its pairing", ); - for (index, slot) in slots.iter().take(3).enumerate() { - assert!( - slot.is_none(py), - "slot {index}: a file the read gate declines to parse \ - must hold its slot as `None`, matching single-file \ - `analyze`", - ); + for ((name, _, expect), slot) in MIXED_INPUTS.iter().zip(&slots) { + match expect { + Expect::ReadGateSkip => assert!( + slot.is_none(py), + "{name}: a file the read gate declines to parse must \ + hold its slot as `None`, matching single-file \ + `analyze`", + ), + // With the filter off a generated file is analysed, + // not replaced by a placeholder — the guard must not + // intercept it. The name assertion also catches a + // slot that shifted onto the wrong input. + Expect::Generated | Expect::Analysed => assert_eq!( + slot_name(py, slot), + path_str(dir.path(), name), + "{name} must land in its own slot as a dict", + ), + } } - assert_eq!( - slot_name(py, &slots[3]), - path_str(dir.path(), "gen.rs"), - "with the filter off a generated file is analysed, not \ - replaced by a `None` placeholder", - ); - assert_eq!( - slot_name(py, &slots[4]), - path_str(dir.path(), "good.rs"), - "the parseable input must land in its own slot, not \ - shift onto an earlier one", - ); }); } @@ -1045,13 +1063,21 @@ mod tests { // whether or not the arm ran. let dir = tempfile::tempdir().expect("tempdir"); Python::attach(|py| { - let slots = pushed_slots(py, dir.path(), true, MIXED_INPUTS); + let slots = pushed_slots(py, dir.path(), true); + // Under the default only the `Analysed` rows keep a slot; + // both skip classes drop theirs entirely. + let analysed: Vec<_> = MIXED_INPUTS + .iter() + .filter(|(_, _, expect)| matches!(expect, Expect::Analysed)) + .collect(); assert_eq!( slots.len(), - 1, + analysed.len(), "with skip_generated=true a skipped file yields no slot", ); - assert_eq!(slot_name(py, &slots[0]), path_str(dir.path(), "good.rs")); + for ((name, _, _), slot) in analysed.iter().zip(&slots) { + assert_eq!(slot_name(py, slot), path_str(dir.path(), name)); + } }); } } diff --git a/big-code-analysis-py/src/sarif.rs b/big-code-analysis-py/src/sarif.rs index 37a34716a..6d48a346f 100644 --- a/big-code-analysis-py/src/sarif.rs +++ b/big-code-analysis-py/src/sarif.rs @@ -607,11 +607,11 @@ fn collect_offenders( /// output, or /// * a scalar ``None`` (the documented return of :func:`analyze` for /// generated files); produces a well-formed empty SARIF run, or -/// * any iterable yielding such dicts, :class:`AnalysisError` +/// * any iterable yielding such dicts, :class:`AnalysisFailure` /// instances, and/or ``None`` (e.g. the return of /// :func:`analyze_batch`, or a list comprehension over /// :func:`analyze` which returns ``None`` for generated files). -/// :class:`AnalysisError` and ``None`` entries are skipped silently +/// :class:`AnalysisFailure` and ``None`` entries are skipped silently /// — they represent files for which no record was emitted (either /// the pipeline could not analyse them, or they were classified as /// generated), not findings. @@ -704,7 +704,7 @@ fn collect_offenders_from_iter( ) -> PyResult<()> { // `try_iter()` errors if the value is not iterable — let that // propagate to the caller as a `TypeError`. Per the documented - // contract, `AnalysisError` entries are skipped silently (they + // contract, `AnalysisFailure` entries are skipped silently (they // represent files we couldn't analyse) and `None` entries are // likewise skipped (the documented return of :func:`analyze` for // generated files — issue #341); anything else that isn't a dict @@ -731,7 +731,7 @@ fn cast_iter_item_to_dict<'py>(item: &Bound<'py, PyAny>) -> PyResult".to_string(), |n| n.to_string()); PyTypeError::new_err(format!( - "to_sarif expected a result dict, AnalysisError, or None, got {type_name}" + "to_sarif expected a result dict, AnalysisFailure, or None, got {type_name}" )) }) } diff --git a/big-code-analysis-py/tests/test_batch.py b/big-code-analysis-py/tests/test_batch.py index d0699298b..1ec65327f 100644 --- a/big-code-analysis-py/tests/test_batch.py +++ b/big-code-analysis-py/tests/test_batch.py @@ -725,10 +725,14 @@ def test_to_sarif_accepts_the_placeholder_slots(tmp_path: Path) -> None: for result in run["results"] for location in result["locations"] ] - assert flagged == [str(inputs[-1])], ( + # The uri is an RFC 3986 reference (percent-encoded, and file:///C:/… + # on Windows), so compare by basename rather than raw str(path) — + # test_sarif.py owns the exact-encoding assertions. + assert len(flagged) == 1, ( "only the parseable file may produce a finding; the None " "placeholders are skipped, not rendered as findings or errors" ) + assert flagged[0].endswith(inputs[-1].name) def test_exclude_tests_kwarg_is_effective(tmp_path: Path) -> None: diff --git a/big-code-analysis-py/tests/test_book_examples.py b/big-code-analysis-py/tests/test_book_examples.py index 74265a989..3c3cb23fe 100644 --- a/big-code-analysis-py/tests/test_book_examples.py +++ b/big-code-analysis-py/tests/test_book_examples.py @@ -91,6 +91,18 @@ def test_quick_start() -> None: assert "cyclomatic" in result["metrics"] +def _declined_tiny_file(tmp_path: Path) -> Path: + """A three-byte file the read gate declines — the #1238 placeholder + input. The precondition assert keeps the fixture honest: if the + gate's threshold ever moved, the caller's test would otherwise pass + for the wrong reason. + """ + tiny = tmp_path / "tiny.rs" + tiny.write_bytes(b"ab\n") + assert bca.analyze(tiny) is None, "fixture must be declined by the read gate" + return tiny + + def test_batch_processing(tmp_path: Path) -> None: """Regression (#882): the example's ``zip(..., strict=True)`` must hold when a generated file is in the batch. @@ -113,9 +125,7 @@ def test_batch_processing(tmp_path: Path) -> None: # batch's `skip_generated=False` must NOT — confirming the fixture is # genuinely generated keeps this test non-vacuous for the skip path. assert bca.analyze(generated) is None, "fixture must be detected as generated" - tiny = tmp_path / "tiny.rs" - tiny.write_bytes(b"ab\n") - assert bca.analyze(tiny) is None, "fixture must be declined by the read gate" + tiny = _declined_tiny_file(tmp_path) summary = mod.run( [ @@ -558,9 +568,7 @@ def test_pipeline_db_batch_branch_analyses_generated_file(tmp_path: Path) -> Non # must not. (Confirms the fixture really is generated, so the test # is not vacuous.) assert bca.analyze(generated) is None, "fixture must be detected as generated" - tiny = tmp_path / "tiny.rs" - tiny.write_bytes(b"ab\n") - assert bca.analyze(tiny) is None, "fixture must be declined by the read gate" + tiny = _declined_tiny_file(tmp_path) summary = mod.run( FIXTURES_DIR, @@ -576,6 +584,10 @@ def test_pipeline_db_batch_branch_analyses_generated_file(tmp_path: Path) -> Non # in `skipped`, which is the branch #1238 added. assert summary["analyzed"] > 0 assert summary["rows"] > 0 + # Counts read-gate declines across the whole FIXTURES_DIR walk plus + # the spliced-in tiny.rs; it is exactly 1 only while no checked-in + # fixture is <= 3 bytes or binary. If this fails after adding a + # fixture, the count moved — not pipeline_db.py. assert summary["skipped"] == 1, ( "the read-gate slot must be bucketed as skipped, not analysed or errored" ) diff --git a/docs/development/lessons_learned.md b/docs/development/lessons_learned.md index 70a89cf89..9aa83e2ef 100644 --- a/docs/development/lessons_learned.md +++ b/docs/development/lessons_learned.md @@ -1611,19 +1611,27 @@ the operator to audit `analyze_path` for new skip surfaces, so the contract survives any future refactor adding a second one (a gitignore filter, a size cap). -**Both premises have since fallen, exactly as anticipated.** #706 routed -the bindings through the walker's `read_file_with_eol` gate — a second -and *unconditional* `Ok(None)` source (three bytes or fewer, a UTF-16 -BOM, a non-UTF-8 leading window) — and #542 flipped `skip_generated` to -default `true`. The arm is live on every call. That went unnoticed -until #1238, where the batch docstrings' "`skip_generated=False` -guarantees one element per input" turned out to be unsatisfiable and -the endorsed `zip(inputs, results)` was attributing metrics to the -wrong paths. The -fallback's job — carrying the contract across a refactor nobody -re-audited — is the part that held. What did not was any gate on the -arm's *documentation*, which went on describing one skip source for two -releases after the second was added. +**Both premises have since fallen, and the fallback did not survive to +catch it.** #706 routed the bindings through the walker's +`read_file_with_eol` gate — a second and *unconditional* `Ok(None)` +source (three bytes or fewer, a UTF-16 BOM, a non-UTF-8 leading +window) — and the #542 commit (`3220e2a0`), which made the arm +legitimately reachable by flipping `skip_generated` to default `true`, +**deleted the synthetic fallback** and replaced it with a bare +`Ok(None) => {}`. That deletion is precisely why #1238 was silent: the +fallback's message — "audit `analyze_path()` for new skip surfaces" — +was written for this exact event, and the event arrived with the guard +already gone. The batch docstrings' "`skip_generated=False` guarantees +one element per input" was unsatisfiable for two releases, and the +endorsed `zip(inputs, results)` attributed metrics to the wrong paths +with nothing to observe. The corollary this lesson missed the first +time: a defensive fallback for an unreachable arm is at maximum risk at +the moment the arm becomes *partially* reachable, because the commit +that legitimises one source reads the whole fallback as obsolete and +removes it — taking the guard against every *other* source with it. +Keep the fallback scoped to the still-invalid residue, or replace it +with something a refactor cannot silently drop (an exhaustive reason +enum from the callee). ---