From b9154e851b80ef812061de657febb77c866df899 Mon Sep 17 00:00:00 2001 From: Ryan Malloy Date: Fri, 13 Feb 2026 01:34:30 -0700 Subject: [PATCH] Address all critical and important findings from safety review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C1: Port count mismatch now emits ERROR to stderr and marks unconnected ports as __UNCONNECTED___ (never silent) C2: Single-module pin mapping built in lockstep — cable_wires always matches header_pins length, warns on unmappable nets C3: VSS removed from is_power_net (it's ground per CMOS convention), dead _POWER_PATTERN regex replaced with _KNOWN_NET_PATTERN C4: apply_filters dead computation removed, docstring clarifies that net-level filtering is a mapper concern I3: WireViz render catches (ValueError, TypeError, OSError) with diagnostic context instead of bare Exception I5: Invalid --format flags now warn and error instead of silently falling through to defaults I6: Model/value heuristic warns on stderr when it triggers, since signal names like ALERT or DATA could be misidentified New tests: VSS classification, port count mismatch (C1), model/value heuristic warning (I6), duplicate refs (S3), empty subcircuit (S2), full pipeline determinism across 3 runs (S1) 115 tests pass, ruff clean --- src/spice2wireviz/cli.py | 36 ++++++++++-- src/spice2wireviz/filter.py | 21 +++---- src/spice2wireviz/mapper/single_module.py | 53 ++++++++++------- src/spice2wireviz/parser/models.py | 4 +- src/spice2wireviz/parser/netlist.py | 54 +++++++++++++---- tests/test_netlist_parser.py | 61 +++++++++++++++++++ tests/test_roundtrip.py | 71 +++++++++++++++++++++++ 7 files changed, 254 insertions(+), 46 deletions(-) diff --git a/src/spice2wireviz/cli.py b/src/spice2wireviz/cli.py index c50f255..85f26c5 100644 --- a/src/spice2wireviz/cli.py +++ b/src/spice2wireviz/cli.py @@ -288,11 +288,31 @@ def _render_wireviz( sys.exit(1) # Map format flags to WireViz output_formats - format_map = {"h": "html", "p": "png", "s": "svg", "g": "gv", "c": "csv", "t": "tsv"} - formats = tuple(format_map[f] for f in format_flags.lower() if f in format_map) + format_map = { + "h": "html", "p": "png", "s": "svg", + "g": "gv", "c": "csv", "t": "tsv", + } + + # Warn about unrecognized format characters + invalid_chars = [f for f in format_flags.lower() if f not in format_map] + if invalid_chars: + click.echo( + f"Warning: unrecognized format flags: {''.join(invalid_chars)} " + f"(valid: {''.join(format_map.keys())})", + err=True, + ) + + formats = tuple( + format_map[f] for f in format_flags.lower() if f in format_map + ) if not formats: - formats = ("html", "png", "svg") + click.echo( + "Error: no valid format flags provided. " + f"Valid flags: {''.join(format_map.keys())}", + err=True, + ) + sys.exit(1) output_dir = output.parent if output else Path.cwd() output_name = output.stem if output else "spice2wireviz_output" @@ -308,6 +328,12 @@ def _render_wireviz( f"Rendered: {', '.join(f'{output_name}.{fmt}' for fmt in formats)}", err=True, ) - except Exception as exc: - click.echo(f"WireViz render error: {exc}", err=True) + except (ValueError, TypeError, OSError) as exc: + click.echo( + f"WireViz render error ({type(exc).__name__}): {exc}\n" + f" Output dir: {output_dir}\n" + f" Output name: {output_name}\n" + f" Formats: {formats}", + err=True, + ) sys.exit(1) diff --git a/src/spice2wireviz/filter.py b/src/spice2wireviz/filter.py index 6dc89df..8aa69fb 100644 --- a/src/spice2wireviz/filter.py +++ b/src/spice2wireviz/filter.py @@ -124,7 +124,12 @@ def filter_net(net: str, config: FilterConfig, netlist: ParsedNetlist) -> bool: def apply_filters(netlist: ParsedNetlist, config: FilterConfig) -> ParsedNetlist: - """Return a new ParsedNetlist with filters applied. + """Return a new ParsedNetlist with component/instance-level filters applied. + + Filters components by prefix/ref and instances by prefix/ref/subcircuit. + Net-level filtering (ground, power, glob patterns) is handled by the + mappers during connection tracing, not here — because net filtering + affects pin lists and cable generation which are mapper concerns. Produces warnings on stderr for significant exclusions. """ @@ -138,17 +143,13 @@ def apply_filters(netlist: ParsedNetlist, config: FilterConfig) -> ParsedNetlist inst for inst in netlist.instances if filter_instance(inst, config, netlist) ] - # Filter nets within surviving components/instances - excluded_net_count = 0 - for comp in filtered_components: - original_count = len(comp.nodes) - comp = comp.model_copy() - new_nodes = [n for n in comp.nodes if filter_net(n, config, netlist)] - excluded_net_count += original_count - len(new_nodes) + excluded_comp_count = len(netlist.top_level_components) - len(filtered_components) + excluded_inst_count = len(netlist.instances) - len(filtered_instances) - if excluded_net_count > 0: + if excluded_comp_count > 0 or excluded_inst_count > 0: print( - f"Note: {excluded_net_count} net connections hidden by filters", + f"Note: filters excluded {excluded_comp_count} components " + f"and {excluded_inst_count} instances", file=sys.stderr, ) diff --git a/src/spice2wireviz/mapper/single_module.py b/src/spice2wireviz/mapper/single_module.py index eb9fa4c..ed5ed24 100644 --- a/src/spice2wireviz/mapper/single_module.py +++ b/src/spice2wireviz/mapper/single_module.py @@ -12,6 +12,7 @@ This shows the external interface of a single board/module. from __future__ import annotations +import sys from typing import Any from ..emitter.yaml_emitter import ( @@ -103,31 +104,43 @@ def map_single_module( shared_nets = _find_shared_nets(subckt, comp, config, netlist) if shared_nets and header_name in connectors: - cable_name = f"W_{comp_name}" - wire_colors = [_net_wire_color(net, netlist) for net in shared_nets] - wire_labels = list(shared_nets) + # Build pin mappings in lockstep — every entry must resolve + mapped_header: list[int] = [] + mapped_comp: list[int] = [] + mapped_nets: list[str] = [] - cables[cable_name] = build_cable( - cable_name, - wirecount=len(shared_nets), - colors=wire_colors if any(wire_colors) else None, - wirelabels=wire_labels, - category="bundle", - notes=f"Nets: {', '.join(shared_nets)}", - ) + for net in shared_nets: + if net in port_labels and net in comp.nodes: + mapped_header.append(port_labels.index(net) + 1) + mapped_comp.append(comp.nodes.index(net) + 1) + mapped_nets.append(net) + else: + print( + f"Warning: net '{net}' shared between " + f"{header_name} and {comp_name} but not " + f"resolvable in both pin lists", + file=sys.stderr, + ) - # Map pin indices for connection - header_pins = [port_labels.index(net) + 1 for net in shared_nets if net in port_labels] - comp_pins = [ - comp.nodes.index(net) + 1 for net in shared_nets if net in comp.nodes - ] - cable_wires = list(range(1, len(shared_nets) + 1)) + if mapped_nets: + cable_name = f"W_{comp_name}" + wire_colors = [ + _net_wire_color(net, netlist) for net in mapped_nets + ] + cables[cable_name] = build_cable( + cable_name, + wirecount=len(mapped_nets), + colors=wire_colors if any(wire_colors) else None, + wirelabels=mapped_nets, + category="bundle", + notes=f"Nets: {', '.join(mapped_nets)}", + ) - if header_pins and comp_pins and len(header_pins) == len(comp_pins): + cable_wires = list(range(1, len(mapped_nets) + 1)) connections.append(build_connection( - header_name, header_pins, + header_name, mapped_header, cable_name, cable_wires, - comp_name, comp_pins, + comp_name, mapped_comp, )) return assemble_wireviz_doc(connectors, cables, connections, metadata) diff --git a/src/spice2wireviz/parser/models.py b/src/spice2wireviz/parser/models.py index b5704ad..47d3fb3 100644 --- a/src/spice2wireviz/parser/models.py +++ b/src/spice2wireviz/parser/models.py @@ -96,8 +96,10 @@ class ParsedNetlist(BaseModel): def is_power_net(self, net: str) -> bool: upper = net.upper() - return upper in {"VCC", "VDD", "V+", "V-", "VEE", "VSS", "AVCC", "AVDD", "DVCC", "DVDD"} + # VSS is ground in CMOS conventions, not power — see is_ground_net + return upper in {"VCC", "VDD", "V+", "V-", "VEE", "AVCC", "AVDD", "DVCC", "DVDD"} def is_ground_net(self, net: str) -> bool: upper = net.upper() + # VSS is ground (CMOS convention: VSS = negative supply = ground) return upper in {"GND", "AGND", "DGND", "GND!", "EARTH", "VSS"} or net == "0" diff --git a/src/spice2wireviz/parser/netlist.py b/src/spice2wireviz/parser/netlist.py index b9794aa..3fa48bd 100644 --- a/src/spice2wireviz/parser/netlist.py +++ b/src/spice2wireviz/parser/netlist.py @@ -36,13 +36,17 @@ BOUNDARY_PREFIXES = {"J", "TP", "P"} # Prefixes we recognize as external-facing components RECOGNIZED_PREFIXES = {"J", "TP", "P", "X"} -# Known power net patterns -_POWER_PATTERN = re.compile( - r"^(V(CC|DD|EE|SS|[+-])|A?V(CC|DD)|D?V(CC|DD))$", re.IGNORECASE -) +# Known ground net patterns (used in value/model heuristic) +_GROUND_PATTERN = re.compile(r"^(GND!?|AGND|DGND|EARTH|0)$", re.IGNORECASE) -# Known ground net patterns -_GROUND_PATTERN = re.compile(r"^(GND!?|AGND|DGND|EARTH|VSS|0)$", re.IGNORECASE) +# Known power net patterns (used in value/model heuristic) +_POWER_NET_NAMES = {"VCC", "VDD", "V+", "V-", "VEE", "AVCC", "AVDD", "DVCC", "DVDD"} + +# Combined: nets the heuristic should never misidentify as model/value names +_KNOWN_NET_PATTERN = re.compile( + r"^(V(CC|DD|EE|[+-])|AV(CC|DD)|DV(CC|DD)|GND!?|AGND|DGND|EARTH|VSS|0)$", + re.IGNORECASE, +) def _strip_comment(line: str) -> str: @@ -289,9 +293,30 @@ def _parse_x_instance( port_to_net: dict[str, str] = {} subckt_def = known_subcircuits.get(subckt_name) if subckt_def: + expected = len(subckt_def.port_names) + actual = len(nodes) + if expected != actual: + print( + f"ERROR: port count mismatch for {ref}: " + f"subcircuit '{subckt_name}' defines {expected} ports " + f"({', '.join(subckt_def.port_names)}) but instance " + f"provides {actual} nodes ({', '.join(nodes)})", + file=sys.stderr, + ) for i, port_name in enumerate(subckt_def.port_names): if i < len(nodes): port_to_net[port_name] = nodes[i] + else: + # Mark unconnected ports visibly + port_to_net[port_name] = f"__UNCONNECTED_{port_name}__" + # Warn about extra nodes beyond the definition + if len(nodes) > expected: + extras = nodes[expected:] + print( + f"Warning: {ref} has {len(extras)} extra nodes " + f"beyond subcircuit definition: {', '.join(extras)}", + file=sys.stderr, + ) else: # No definition available; use positional indices as port names for i, node in enumerate(nodes): @@ -326,18 +351,27 @@ def _parse_boundary_component(tokens: list[str], prefix: str) -> SpiceComponent positional.append(token) # Heuristic: if last positional looks like a model name (contains only - # alphanumeric/underscore and doesn't match net patterns), treat it as value + # alphanumeric/underscore and doesn't match net patterns), treat it as value. + # WARNING: This can misidentify signal net names (e.g., ALERT, ENABLE, DATA) + # as model names. We warn when the heuristic triggers. value = "" nodes = positional if len(positional) >= 2: last = positional[-1] # If it's not purely numeric and not a known net pattern, it's likely a value/model - if re.match(r"^[A-Za-z_]\w*$", last) and not _POWER_PATTERN.match( - last - ) and not _GROUND_PATTERN.match(last): + if ( + re.match(r"^[A-Za-z_]\w*$", last) + and not _KNOWN_NET_PATTERN.match(last) + ): value = last nodes = positional[:-1] + print( + f"Note: treating '{last}' as model/value for {ref} " + f"(not as net name). If this is wrong, the net " + f"connection to '{last}' will be missing.", + file=sys.stderr, + ) pins = [ SpicePin( diff --git a/tests/test_netlist_parser.py b/tests/test_netlist_parser.py index 5f7db27..e15616d 100644 --- a/tests/test_netlist_parser.py +++ b/tests/test_netlist_parser.py @@ -146,6 +146,13 @@ class TestNetClassification: assert not netlist.is_power_net("GND") assert not netlist.is_power_net("SIGNAL") + def test_vss_is_ground_not_power(self): + """VSS should be classified as ground (CMOS convention), not power.""" + text = ".subckt test VSS VDD\n.ends test\n" + netlist = parse_netlist(text) + assert netlist.is_ground_net("VSS") + assert not netlist.is_power_net("VSS") + class TestEdgeCases: def test_nonexistent_file(self): @@ -169,3 +176,57 @@ X1 A B C undefined_subckt # Without definition, uses positional port names assert "port1" in inst.port_to_net assert inst.port_to_net["port1"] == "NET1" + + def test_port_count_mismatch_fewer_nodes(self, capsys): + """C1: When instance has fewer nodes than subcircuit ports, warn and mark unconnected.""" + text = """\ +.subckt amp VIN GND VOUT ENABLE +.ends amp +X1 NET1 NET2 amp +""" + netlist = parse_netlist(text) + inst = netlist.instances[0] + captured = capsys.readouterr() + assert "ERROR: port count mismatch" in captured.err + assert "4 ports" in captured.err + assert "2 nodes" in captured.err + # Unconnected ports should be marked + assert inst.port_to_net["VIN"] == "NET1" + assert inst.port_to_net["GND"] == "NET2" + assert "__UNCONNECTED_VOUT__" in inst.port_to_net["VOUT"] + assert "__UNCONNECTED_ENABLE__" in inst.port_to_net["ENABLE"] + + def test_port_count_mismatch_more_nodes(self, capsys): + """C1: When instance has more nodes than subcircuit ports, warn about extras.""" + text = """\ +.subckt small A B +.ends small +X1 NET1 NET2 NET3 NET4 small +""" + parse_netlist(text) + captured = capsys.readouterr() + assert "ERROR: port count mismatch" in captured.err + assert "extra nodes" in captured.err + + def test_model_value_heuristic_warning(self, capsys): + """I6: The model/value heuristic should warn when it triggers.""" + text = """\ +.subckt test A B +J1 A B SOME_MODEL +.ends test +""" + parse_netlist(text) + captured = capsys.readouterr() + assert "SOME_MODEL" in captured.err + assert "model/value" in captured.err + + def test_duplicate_refs_both_parsed(self): + """S3: Duplicate reference designators should both be parsed.""" + text = """\ +J1 NET_A NET_B CONN_TYPE +J1 NET_C NET_D CONN_TYPE +""" + netlist = parse_netlist(text) + # Both should appear (parser doesn't deduplicate) + j1_refs = [c for c in netlist.top_level_components if c.reference == "J1"] + assert len(j1_refs) == 2 diff --git a/tests/test_roundtrip.py b/tests/test_roundtrip.py index 9b97f69..6049aa3 100644 --- a/tests/test_roundtrip.py +++ b/tests/test_roundtrip.py @@ -97,6 +97,77 @@ class TestYamlValidity: assert isinstance(entry, dict), f"Connection set {i} entry is not a dict" +class TestPipelineDeterminism: + """S1: Verify the full pipeline produces byte-identical output on repeated runs.""" + + def test_inter_module_deterministic(self): + """Parse the same file multiple times; output must be identical.""" + outputs = [] + for _ in range(3): + netlist = parse_netlist(FIXTURES / "hierarchical.net") + result = map_inter_module(netlist) + yaml_str = emit_yaml(result) + outputs.append(yaml_str) + assert outputs[0] == outputs[1] == outputs[2] + + def test_single_module_deterministic(self): + outputs = [] + for _ in range(3): + netlist = parse_netlist(FIXTURES / "simple_board.net") + result = map_single_module(netlist, "amplifier_board") + yaml_str = emit_yaml(result) + outputs.append(yaml_str) + assert outputs[0] == outputs[1] == outputs[2] + + def test_filtered_deterministic(self): + config = FilterConfig(show_ground=False, show_power=False) + outputs = [] + for _ in range(3): + netlist = parse_netlist(FIXTURES / "multi_board.net") + result = map_inter_module(netlist, config) + yaml_str = emit_yaml(result) + outputs.append(yaml_str) + assert outputs[0] == outputs[1] == outputs[2] + + +class TestEmptySubcircuit: + """S2: Subcircuit with ports but no boundary components.""" + + def test_empty_subcircuit_produces_header_only(self): + text = """\ +.subckt bare_module A B C +* No J*/TP*/P* inside — just passives +R1 A B 10k +.ends bare_module +""" + netlist = parse_netlist(text) + result = map_single_module(netlist, "bare_module") + # Should produce a header connector but no cables or connections + assert "bare_module" in result["connectors"] + assert len(result["cables"]) == 0 + assert len(result["connections"]) == 0 + + +class TestDuplicateRefInMapper: + """S3: Duplicate reference designators in the mapper.""" + + def test_duplicate_ref_last_wins_in_inter_module(self): + """When two top-level components share a reference, the mapper + builds connectors from both but dict key collision means last wins. + This is a known limitation that should at least not crash.""" + text = """\ +.subckt mod A B +.ends mod +X1 NET1 NET2 mod +J1 NET1 NET2 CONN_A +J1 NET3 NET4 CONN_B +""" + netlist = parse_netlist(text) + result = map_inter_module(netlist) + # Should not crash; J1 connector exists (last definition wins) + assert "J1" in result["connectors"] + + @pytest.mark.skipif(not _has_wireviz(), reason="WireViz not installed") class TestWireVizRoundtrip: """Feed generated YAML to WireViz's parse() to verify full compatibility."""