From 8bf856a7cb40bb7328437927a7e6e68678247986 Mon Sep 17 00:00:00 2001 From: Cameron Brooks Date: Sat, 5 Sep 2026 21:59:26 -0400 Subject: [PATCH] fix(parse): report draw operations that use an undefined aperture A flash or stroke whose D-code selects an aperture that was never defined -- or that draws before any Dnn selection at all -- carried an aperture index with no definition behind it. Every consumer read that as nothing to draw: both geometry emit helpers returned early on `ap is None` and the raster renderer drew nothing. No diagnostic was emitted at any severity. diff and geomdiff therefore reported 0 changes at exit 0, and the JSON report was byte-identical to a comparison of two identical boards, so a real fabrication change could pass a --fail-on-diff gate invisibly. Check at the point of use rather than at selection. That is exactly coextensive with the harm: it also catches drawing before any selection (index 0), and it does not fail a file that selects a stray D-code but never draws with it, which renders identically. Offending codes are reported once each, so one bad aperture used by thousands of flashes yields one diagnostic. D02 moves and G36/G37 region contours consume no aperture and are skipped. The existing promotion path turns the Error into exit 2 for parse, render, diff and geomdiff alike, so no downstream change is needed. test_parse_minimal was itself an instance of this: it defined D10 but never selected it, so its stroke expanded to zero geometry ops while the test asserted no errors. Its D10* selection is now load-bearing. Closes #17 --- CHANGELOG.md | 15 +++++++ gerberdiff/parse/gerber_state.py | 29 ++++++++++++- tests/test_cli_diff.py | 21 ++++++++++ tests/test_cli_geomdiff.py | 27 ++++++++++++ tests/test_gerber_state.py | 70 +++++++++++++++++++++++++++++++- 5 files changed, 158 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bb7527..dd69292 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Draw operations using an undefined aperture are no longer dropped in + silence** ([#17]) -- selecting a D-code that was never defined (or drawing + before any `Dnn` selection at all) left the flash or stroke carrying an + aperture index with no definition behind it. Every consumer treated that + as nothing to draw: both geometry emit helpers returned early and the + raster renderer drew nothing, with no diagnostic at any severity. `diff` + and `geomdiff` then reported `0 changes` at exit `0`, and the JSON report + was byte-identical to a comparison of two genuinely identical boards -- so + a real fabrication change could pass a `--fail-on-diff` gate invisibly. + The parser now emits an `Error` diagnostic at the offending draw + operation, which the existing promotion path turns into exit `2` for + `parse`, `render`, `diff` and `geomdiff` alike. D02 moves and G36/G37 + region contours consume no aperture and are unaffected. + - **Degenerate region contours no longer leak line geometry** -- a G36/G37 contour whose points are collinear produced a zero-area `MultiLineString` from `make_valid` that flowed into the geometry engine; region expansion @@ -671,6 +685,7 @@ merge_tolerance) -> SingleLayerDiff`. - mypy `strict=true`, `warn_unused_ignores=true`, `cairocffi.*` override for missing stubs. - 2 smoke tests in `tests/test_scaffold.py`. +[#17]: https://github.com/heibench/gerberdiff/issues/17 [Unreleased]: https://github.com/heibench/gerberdiff/compare/v0.29.1...HEAD [0.29.1]: https://github.com/heibench/gerberdiff/compare/v0.29.0...v0.29.1 [0.29.0]: https://github.com/heibench/gerberdiff/compare/9ffd4c8f...v0.29.0 diff --git a/gerberdiff/parse/gerber_state.py b/gerberdiff/parse/gerber_state.py index f7b422d..d0e5195 100644 --- a/gerberdiff/parse/gerber_state.py +++ b/gerberdiff/parse/gerber_state.py @@ -163,6 +163,10 @@ def __init__(self, source_path: Path | None) -> None: self._net_attrs: dict[str, str] = {} self._aperture_attrs: dict[str, str] = {} + # Offending D-codes already reported, so one bad aperture used by + # thousands of flashes yields one diagnostic rather than thousands. + self._undefined_apertures: set[int] = set() + # ---- block aperture stack ---- # Each frame saves state so that when %AB*% closes the block the # parent drawing context is fully restored. @@ -241,7 +245,28 @@ def _reset_block(self) -> None: # Net emission # ------------------------------------------------------------------ - def _emit_net(self) -> None: + def _check_aperture_defined(self, line: int) -> None: + """Error when the op about to be emitted would draw with no aperture. + + Region contours are filled from their outline and D02 only moves the + cursor, so neither consumes an aperture. Every other op does, and a + missing definition means the geometry is dropped -- silently, before + this check existed. + """ + if self._in_region_fill or self._aperture_state == ApertureState.Off: + return + code = self._current_aperture + if code in self._apertures or code in self._undefined_apertures: + return + self._undefined_apertures.add(code) + if code == 0: + self._error("Draw operation before any aperture was selected", line) + else: + self._error(f"Draw operation uses undefined aperture D{code}", line) + + def _emit_net(self, line: int) -> None: + self._check_aperture_defined(line) + fmt = self._fmt # Resolve stop position (use prev if coordinate not updated this block) @@ -736,7 +761,7 @@ def parse(self, content: str) -> ParsedImage: elif tt == TokenType.END_OF_BLOCK: if self._coord_changed: - self._emit_net() + self._emit_net(line) self._reset_block() elif tt == TokenType.EXTENDED: diff --git a/tests/test_cli_diff.py b/tests/test_cli_diff.py index 233aa51..c35253d 100644 --- a/tests/test_cli_diff.py +++ b/tests/test_cli_diff.py @@ -260,3 +260,24 @@ def test_diff_removed_layer_reports_correctly(tmp_path: Path) -> None: ) result = _run("diff", str(before), str(after), "--width", "64", "--height", "64") assert result.exit_code == 0, result.output + + +# --------------------------------------------------------------------------- +# Undefined apertures (issue #17) +# --------------------------------------------------------------------------- + + +def test_diff_undefined_aperture_fails_the_gate(tmp_path: Path) -> None: + """The raster engine drops the flash too -- it must not report 0 changes.""" + before, after = tmp_path / "b", tmp_path / "a" + header = "%FSLAX26Y26*%\n%MOIN*%\n" + good = header + "%ADD10C,0.1*%\nD10*\nX0Y0D03*\n" + "M02*\n" + before.mkdir() + after.mkdir() + (before / "board-F.Cu.gbr").write_text(good) + (after / "board-F.Cu.gbr").write_text( + header + "%ADD10C,0.1*%\nD10*\nX0Y0D03*\nD11*\nX500000Y500000D03*\n" + "M02*\n" + ) + result = _run("diff", str(before), str(after), "--fail-on-diff") + assert result.exit_code == 2, result.output + assert "undefined aperture D11" in result.output diff --git a/tests/test_cli_geomdiff.py b/tests/test_cli_geomdiff.py index 09d48a8..01cf27d 100644 --- a/tests/test_cli_geomdiff.py +++ b/tests/test_cli_geomdiff.py @@ -180,3 +180,30 @@ def test_geomdiff_parse_error_exit_2(tmp_path: Path) -> None: result = _run("geomdiff", str(before), str(after)) assert result.exit_code == 2 assert "error" in result.output.lower() + + +# --------------------------------------------------------------------------- +# Undefined apertures (issue #17) +# --------------------------------------------------------------------------- + + +def _write_board_with_undefined_aperture(directory: Path) -> None: + """The same pad, plus a second flash on a D-code that is never defined.""" + directory.mkdir(parents=True, exist_ok=True) + src = _HEADER + "%ADD10C,0.1*%\nD10*\nX0Y0D03*\n" + "D11*\nX500000Y500000D03*\n" + _FOOTER + (directory / "board-F.Cu.gbr").write_text(src) + + +def test_geomdiff_undefined_aperture_fails_the_gate(tmp_path: Path) -> None: + """An added pad on an undefined aperture must not read as "no changes". + + The flash is dropped from the geometry, so before this was reported the + comparison came back 0 changes at exit 0 and a real fabrication change + passed the gate invisibly. + """ + _write_board(tmp_path / "b", 0) + _write_board_with_undefined_aperture(tmp_path / "a") + result = _run("geomdiff", str(tmp_path / "b"), str(tmp_path / "a"), "--fail-on-diff") + assert result.exit_code == 2, result.output + assert "undefined aperture D11" in result.output + assert "0 changes" not in result.output diff --git a/tests/test_gerber_state.py b/tests/test_gerber_state.py index aaf039a..23b2421 100644 --- a/tests/test_gerber_state.py +++ b/tests/test_gerber_state.py @@ -12,8 +12,10 @@ def test_parse_minimal() -> None: - # FSLAX25Y25, MOMM unit, circle D10, linear draw, end - content = "%FSLAX25Y25*%%MOMM*%%ADD10C,1.0*%G01*X100000Y100000D01*M02*" + # FSLAX25Y25, MOMM unit, circle D10, select it, linear draw, end. + # The D10* selection is load-bearing: without it the stroke carries + # aperture 0, which is undefined, and expands to no geometry at all. + content = "%FSLAX25Y25*%%MOMM*%%ADD10C,1.0*%D10*G01*X100000Y100000D01*M02*" img = parse_gerber(content) assert img.bounding_box.is_valid assert len(img.draw_ops) > 0 @@ -416,3 +418,67 @@ def test_sr_parse_records_step_and_repeat_on_layer() -> None: assert sr.y == 2 assert abs(sr.dist_x - 1.0) < 1e-9 assert abs(sr.dist_y - 0.5) < 1e-9 + + +# --------------------------------------------------------------------------- +# Undefined apertures (issue #17) +# +# A draw op whose aperture was never defined is dropped by every downstream +# consumer -- both geometry emit helpers and the raster renderer treat a +# missing aperture as "nothing to draw". Before the parser reported it, a +# real fabrication change could pass a --fail-on-diff gate at exit 0. +# --------------------------------------------------------------------------- + +_HDR = "%FSLAX26Y26*%\n%MOIN*%\n" + + +def _errors(gerber: str) -> list[str]: + img = parse_gerber(gerber) + return [d.message for d in img.diagnostics if d.severity == DiagnosticSeverity.Error] + + +def test_flash_with_undefined_aperture_is_an_error() -> None: + errs = _errors(_HDR + "D11*\nX1000Y1000D03*\nM02*\n") + assert errs == ["Draw operation uses undefined aperture D11"] + + +def test_stroke_with_undefined_aperture_is_an_error() -> None: + errs = _errors(_HDR + "D11*\nX0Y0D02*\nX1000Y1000D01*\nM02*\n") + assert errs == ["Draw operation uses undefined aperture D11"] + + +def test_flash_before_any_aperture_is_selected_is_an_error() -> None: + """No D-code at all leaves _current_aperture at 0, which is also undefined.""" + errs = _errors(_HDR + "X1000Y1000D03*\nM02*\n") + assert errs == ["Draw operation before any aperture was selected"] + + +def test_undefined_aperture_error_carries_the_line_of_the_draw_op() -> None: + img = parse_gerber(_HDR + "%ADD10C,0.1*%\nD10*\nX0Y0D03*\nD11*\nX1000Y1000D03*\nM02*\n") + errs = [d for d in img.diagnostics if d.severity == DiagnosticSeverity.Error] + assert len(errs) == 1 + # Line 7 is the flash, not line 6 where D11 was selected. + assert errs[0].line == 7 + + +def test_move_with_undefined_aperture_is_not_an_error() -> None: + """D02 only repositions the cursor -- it consumes no aperture.""" + assert _errors(_HDR + "D11*\nX1000Y1000D02*\nM02*\n") == [] + + +def test_region_fill_needs_no_aperture() -> None: + """G36/G37 contours are filled from their outline, so none is selected.""" + gerber = _HDR + "G36*\nX0Y0D02*\nX1000Y0D01*\nX1000Y1000D01*\nX0Y0D01*\nG37*\nM02*\n" + assert _errors(gerber) == [] + + +def test_defined_aperture_draws_without_error() -> None: + """Guard against the check firing on every file.""" + assert _errors(_HDR + "%ADD10C,0.1*%\nD10*\nX1000Y1000D03*\nM02*\n") == [] + + +def test_undefined_aperture_is_reported_once_per_code() -> None: + """One bad aperture used by many flashes must not emit one error each.""" + body = "".join(f"X{i * 100}Y0D03*\n" for i in range(200)) + errs = _errors(_HDR + "D11*\n" + body + "M02*\n") + assert errs == ["Draw operation uses undefined aperture D11"]