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"]