diff --git a/python/kicad_interface.py b/python/kicad_interface.py index cd2d147..aa26339 100644 --- a/python/kicad_interface.py +++ b/python/kicad_interface.py @@ -1471,6 +1471,28 @@ class KiCADInterface: rf'\1"{escaped_r}"', block_text, ) + # Also update the (reference "...") leaves inside the symbol's + # (instances) → (project) → (path) subtree. KiCad reads those + # entries — not the (property "Reference" ...) field — when + # generating netlists and syncing the PCB via "Update PCB from + # Schematic", so leaving them stale produces a silent + # reference mismatch where eeschema shows the new ref but ERC + # / netlist export / PCB sync all use the old one. See #126. + instances_pos = block_text.find("(instances") + if instances_pos >= 0: + instances_end = self._find_matching_paren(block_text, instances_pos) + if instances_end >= 0: + instances_block = block_text[instances_pos : instances_end + 1] + updated_instances = re.sub( + r'(\(reference\s+)"' + re.escape(reference) + r'"', + rf'\1"{escaped_r}"', + instances_block, + ) + block_text = ( + block_text[:instances_pos] + + updated_instances + + block_text[instances_end + 1 :] + ) if field_positions is not None: for field_name, pos in field_positions.items(): x = pos.get("x", 0) diff --git a/tests/test_edit_schematic_component_instances.py b/tests/test_edit_schematic_component_instances.py new file mode 100644 index 0000000..8103677 --- /dev/null +++ b/tests/test_edit_schematic_component_instances.py @@ -0,0 +1,279 @@ +""" +Regression tests for edit_schematic_component newReference: must also update +the (reference "...") leaves inside the symbol's (instances) → (project) → +(path) subtree, not just the (property "Reference" "...") field. + +Background +---------- +A KiCad schematic stores a placed symbol's reference designator in two places: + + (symbol + (lib_id "Device:R") + (uuid "abcd-1234-…") + ... + (property "Reference" "R5" ← visible-in-eeschema field + (at … …) + (effects …) + ) + ... + (instances + (project "MyProject" + (path "/sheet-uuid/symbol-uuid" + (reference "R5") ← used by netlist + PCB sync + (unit 1) + ) + ) + ) + ) + +Before the fix, `edit_schematic_component` with `newReference` updated only the +(property "Reference" ...) field. The (reference "...") inside (instances) stayed +on the old value. eeschema rendered the new ref correctly and ERC passed, but +netlist export and "Update PCB from Schematic" both read from (instances) and +silently used the OLD reference — producing destructive PCB diffs on what users +thought was a clean rename. + +The fix walks the (instances) subtree after updating the property field and +replaces every (reference "OLD") leaf with (reference "NEW"). +""" + +import sys +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent / "python")) + + +PLACED_RESISTOR_WITH_INSTANCES = """\ + (symbol (lib_id "Device:R") (at 50 50 0) (unit 1) + (in_bom yes) (on_board yes) (dnp no) + (uuid "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee") + (property "Reference" "R1" (at 51.27 47.46 0) + (effects (font (size 1.27 1.27))) + ) + (property "Value" "10k" (at 51.27 52.54 0) + (effects (font (size 1.27 1.27))) + ) + (property "Footprint" "Resistor_SMD:R_0603_1608Metric" (at 50 50 0) + (effects (font (size 1.27 1.27)) hide) + ) + (property "Datasheet" "~" (at 50 50 0) + (effects (font (size 1.27 1.27)) hide) + ) + (instances + (project "MyProject" + (path "/abcdef12-3456-7890-abcd-ef1234567890/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + (reference "R1") + (unit 1) + ) + ) + ) + ) +""" + +PLACED_RESISTOR_HIERARCHICAL = """\ + (symbol (lib_id "Device:R") (at 50 50 0) (unit 1) + (in_bom yes) (on_board yes) (dnp no) + (uuid "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee") + (property "Reference" "R1" (at 51.27 47.46 0) + (effects (font (size 1.27 1.27))) + ) + (property "Value" "10k" (at 51.27 52.54 0) + (effects (font (size 1.27 1.27))) + ) + (property "Footprint" "Resistor_SMD:R_0603_1608Metric" (at 50 50 0) + (effects (font (size 1.27 1.27)) hide) + ) + (property "Datasheet" "~" (at 50 50 0) + (effects (font (size 1.27 1.27)) hide) + ) + (instances + (project "MyProject" + (path "/sheet-a-uuid/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + (reference "R1") + (unit 1) + ) + (path "/sheet-b-uuid/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + (reference "R1") + (unit 1) + ) + ) + ) + ) +""" + +PLACED_RESISTOR_NO_INSTANCES = """\ + (symbol (lib_id "Device:R") (at 50 50 0) (unit 1) + (in_bom yes) (on_board yes) (dnp no) + (uuid "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee") + (property "Reference" "R1" (at 51.27 47.46 0) + (effects (font (size 1.27 1.27))) + ) + (property "Value" "10k" (at 51.27 52.54 0) + (effects (font (size 1.27 1.27))) + ) + (property "Footprint" "Resistor_SMD:R_0603_1608Metric" (at 50 50 0) + (effects (font (size 1.27 1.27)) hide) + ) + (property "Datasheet" "~" (at 50 50 0) + (effects (font (size 1.27 1.27)) hide) + ) + ) +""" + + +def _write_schematic(tmp_path: Path, placed_block: str) -> Path: + """Build a minimal kicad_sch wrapper around one placed-symbol block.""" + sch = tmp_path / "test.kicad_sch" + sch.write_text( + "(kicad_sch\n" + ' (version 20260306)\n' + ' (generator "eeschema")\n' + ' (uuid "abcdef12-3456-7890-abcd-ef1234567890")\n' + ' (paper "A4")\n' + ' (lib_symbols)\n' + f"{placed_block}" + ")\n", + encoding="utf-8", + ) + return sch + + +def _interface() -> Any: + from kicad_interface import KiCADInterface + + iface = KiCADInterface.__new__(KiCADInterface) + # _handle_edit_schematic_component does not require backend setup since it + # operates on the .kicad_sch file directly via text-based editing. + iface._update_command_handlers = MagicMock() + return iface + + +@pytest.mark.unit +class TestEditSchematicComponentInstancesReference: + """The fix: newReference must update (reference "...") inside (instances) too.""" + + def test_instances_reference_updated_on_rename(self, tmp_path: Any) -> None: + """End-to-end: rename R1→R5, both property and instances must reflect R5.""" + sch = _write_schematic(tmp_path, PLACED_RESISTOR_WITH_INSTANCES) + + result = _interface()._handle_edit_schematic_component( + { + "schematicPath": str(sch), + "reference": "R1", + "newReference": "R5", + } + ) + + assert result["success"] is True, result + content = sch.read_text(encoding="utf-8") + + # The visible property is updated (existing behaviour). + assert '(property "Reference" "R5"' in content + assert '(property "Reference" "R1"' not in content + + # The fix: the (instances) leaf is updated too. The PCB sync / + # netlist export path reads this entry. + assert '(reference "R5")' in content + assert '(reference "R1")' not in content + + def test_hierarchical_multiple_paths_all_updated(self, tmp_path: Any) -> None: + """A symbol reused across hierarchical sub-sheets has one (path) per + instance, each with its own (reference "X"). All must be updated + atomically — leaving any stale produces the same mismatch bug on + only-some-of-the-instances.""" + sch = _write_schematic(tmp_path, PLACED_RESISTOR_HIERARCHICAL) + + result = _interface()._handle_edit_schematic_component( + { + "schematicPath": str(sch), + "reference": "R1", + "newReference": "R5", + } + ) + + assert result["success"] is True, result + content = sch.read_text(encoding="utf-8") + + # Both (path) entries inside (instances) must now show R5. + assert content.count('(reference "R5")') == 2 + assert '(reference "R1")' not in content + + def test_no_instances_block_does_not_crash(self, tmp_path: Any) -> None: + """Symbols saved by older KiCad versions (or partial schematics + constructed by external tools) may not have an (instances) block. + The rename must still succeed — the instances walk is a no-op.""" + sch = _write_schematic(tmp_path, PLACED_RESISTOR_NO_INSTANCES) + + result = _interface()._handle_edit_schematic_component( + { + "schematicPath": str(sch), + "reference": "R1", + "newReference": "R5", + } + ) + + assert result["success"] is True, result + content = sch.read_text(encoding="utf-8") + assert '(property "Reference" "R5"' in content + # No (instances) entry → no (reference …) leaves to update; the + # property update alone is sufficient. + assert "(instances" not in content + + def test_property_reference_not_clobbered_by_instances_pass(self, tmp_path: Any) -> None: + """The instances-update regex must match (reference "X") specifically, + not (property "Reference" "X"). If the regex were too loose it would + double-substitute or, worse, leave the property field untouched after + a separate over-eager edit. Pin the shape.""" + sch = _write_schematic(tmp_path, PLACED_RESISTOR_WITH_INSTANCES) + + _interface()._handle_edit_schematic_component( + { + "schematicPath": str(sch), + "reference": "R1", + "newReference": "R42", + } + ) + + content = sch.read_text(encoding="utf-8") + # property line uses (property "Reference" "X") — visible-in-eeschema + # form, with a leading "(property" not "(reference". + assert '(property "Reference" "R42"' in content + # instances leaf uses (reference "X") form. + assert '(reference "R42")' in content + + def test_other_edits_still_work_when_only_newReference_provided( + self, tmp_path: Any + ) -> None: + """Verify the rename leaves other field values intact.""" + sch = _write_schematic(tmp_path, PLACED_RESISTOR_WITH_INSTANCES) + + _interface()._handle_edit_schematic_component( + { + "schematicPath": str(sch), + "reference": "R1", + "newReference": "R5", + } + ) + + content = sch.read_text(encoding="utf-8") + assert '(property "Value" "10k"' in content + assert '(property "Footprint" "Resistor_SMD:R_0603_1608Metric"' in content + + def test_response_payload_reports_reference_change(self, tmp_path: Any) -> None: + """The response payload's `updated.reference` should reflect the new + reference, preserving the existing handler contract.""" + sch = _write_schematic(tmp_path, PLACED_RESISTOR_WITH_INSTANCES) + + result = _interface()._handle_edit_schematic_component( + { + "schematicPath": str(sch), + "reference": "R1", + "newReference": "R5", + } + ) + + assert result["updated"]["reference"] == "R5"