fix(py): hold the batch slot for unreadable inputs - #1350
Merged
Conversation
`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
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
- 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
analyze_batch/analyze_pathsdocumented thatskip_generated=Falseguarantees one element per input, so callers could
zip(inputs, results). That promise was unsatisfiable:analyze_pathreturnsOk(None)from two places — theskip_generatedfilter, and the unconditionalread_file_with_eolgate (three bytes or fewer, a UTF-16 BOM, a non-UTF-8 leading window) —
and
push_one_resultdropped the slot for both.Verified against the 2.1.1 build before fixing:
No error, no
AnalysisFailure, nothing to observe — a silentdata-corruption class defect for any pipeline following the documented
pattern over a corpus containing one empty or binary-prefixed file.
The fix
With the filter off, the read gate is the only remaining
Ok(None)source — exactly the inputs single-file
analyzealready returnsNonefor. So that arm keeps its slot:
No signature change and no new parameter:
push_one_resultalreadyreceives
opts: AnalyzeOptions, which carriesskip_generated. Theskip_generated=Truedefault is untouched — a skipped file, generatedor unreadable, still yields no element.
analyze_pathsshares the funnel and so shares the behaviour. It has nopositional contract of its own (results are walk-ordered), so this was a
deliberate consistency call rather than a second bug fixed for free; the
docs say what the slots buy there (a discovered file that could not be
analysed stays visible instead of vanishing) and how to filter them.
Breaking-change note
Marked (breaking) in
CHANGELOG.md, in both halves:list[FuncSpaceDict | AnalysisFailure | None]. Amypy --strictcaller indexing a slot without a
Nonecheck newly fails totype-check — which is the point; the runtime values it would have
received were already wrong.
skip_generated=Falseloop that indexedevery slot used to run to completion (silently mis-paired) and now
raises
TypeError: 'NoneType' object is not subscriptable. Loud andlocal beats silent and downstream.
Widening a return union is not additive under
STABILITY.md. Itlanded in a minor rather than waiting for 3.0 because the alternative
was leaving silent data corruption in the released line, and because the
runtime contract it makes true is the one both entry points already
documented. The exception and its reasoning are recorded in
STABILITY.mdunder Python bindings → Typing.Two facts made
Nonethe low-risk shape rather than a new vocabulary:to_sarif's stub already acceptedIterable[FuncSpaceDict | AnalysisFailure | None]and called that "thenatural shape of
analyze_batch's return value", andNoneis whatsingle-file
analyzereturns for the same inputs.Tests
Rust patch coverage comes from
cargo llvm-cov nextest, which neverruns pytest — so the guard is pinned on both sides. Mutation-verified in
three directions, since a guard can be wrong by being absent,
unconditional, or inverted:
read_gate_skips_keep_their_slot_…every_skip_class_drops_its_slot_…Neither test alone covers both directions, which is why the default-path
companions exist. Fixture rows carry an
Expectso both tests deriveevery position and count from the table rather than hardcoded indices.
Review remediation (
016ee72b)An 8-angle review pass found 14 verified issues; 11 fixed here, 2 filed
(#1348, #1349), 1 by design. The two that mattered:
to_sariftest would have failed the Windows CI leg — itcompared a SARIF
artifactLocation.urito a rawstr(path), but thewriter emits an RFC 3986 reference (
file:///C:/…on Windows).lessons_learnedupdate misstated history.git show 3220e2a0shows the docs(stability): bring Python bindings & REST schema under the 2.0 stability contract #542 commit deleted the defensive fallbackguarding this arm. It did not "hold" — its removal is why fix(py): batch docs promise 1:1 results under skip_generated=False, but tiny/binary files still drop slots #1238 was
silent. The lesson now records the real corollary: a fallback for an
unreachable arm is at maximum risk exactly when the arm becomes
partially reachable, because the legitimising commit reads the whole
guard as obsolete.
Also: stale
AnalysisError(removed at 2.0) into_sarif's runtimeTypeError; three example surfaces labelling read-gate declines as"generated";
analyze_path's rustdoc claiming a UTF-16 BOM is strippedwhen it is rejected (#803); the over-broad "behaviour-preserving"
migration claim in four places;
analyze_pathspreallocation missingthe missing-seed elements.
Validation
make pre-commit→BCA_GATE: pass.mypy --strict+pyrightclean,367 pytest passed,
cargo llvm-covreports hit counts of 5 and 7 on thetwo new production lines.
Fixes #1238