Fix __hash__/__eq__ inconsistency in HConfigChild (#185) (#236)

* Fix __hash__/__eq__ inconsistency in HConfigChild (#185)

__hash__ included new_in_config and order_weight but __eq__ intentionally
excluded them, violating the Python invariant that a == b implies hash(a) == hash(b).
__eq__ also checked tags but __hash__ did not include them.

Align __hash__ to use the same fields as __eq__: text, tags, and children.

Add five tests covering each dimension of the inconsistency and its practical
impact on set deduplication and dict key lookup.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Fix pre-existing lint errors in next branch

- test_benchmarks.py: replace append loops with extend (PERF401), add
  @staticmethod to methods that don't use self (PLR6301), suppress
  intentional print calls with noqa: T201
- test_child.py: suppress pylint too-many-lines (C0302)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: jtdub

tests/benchmarks/test_benchmarks.py

@@ -54,8 +54,10 @@ def _generate_large_ios_config(num_interfaces: int = 1000) -> str:
             " auto-cost reference-bandwidth 100000",
         ]
     )
-    for i in range(num_interfaces):
-        lines.append(f" network 10.{i // 256}.{i % 256}.0 0.0.0.3 area 0")
+    lines.extend(
+        f" network 10.{i // 256}.{i % 256}.0 0.0.0.3 area 0"
+        for i in range(num_interfaces)
+    )
     lines.extend(
         [
             "!",
@@ -125,8 +127,10 @@ def _generate_large_xr_config(num_interfaces: int = 1000) -> str:
             " router-id 10.0.0.1",
         ]
     )
-    for i in range(num_interfaces):
-        lines.append(f" area 0 interface GigabitEthernet0/0/0/{i} cost 100")
+    lines.extend(
+        f" area 0 interface GigabitEthernet0/0/0/{i} cost 100"
+        for i in range(num_interfaces)
+    )
     lines.append("!")
     return "\n".join(lines)
 
@@ -145,33 +149,37 @@ def _time_fn(fn: Callable[[], object], iterations: int = 3) -> float:
 class TestParsingBenchmarks:
     """Benchmarks for config parsing."""
 
-    def test_parse_large_ios_config(self) -> None:
+    @staticmethod
+    def test_parse_large_ios_config() -> None:
         """Parse a ~10k line IOS config via get_hconfig."""
         config_text = _generate_large_ios_config()
         elapsed = _time_fn(lambda: get_hconfig(Platform.CISCO_IOS, config_text))
         line_count = config_text.count("\n")
-        print(f"\nget_hconfig: {line_count} lines in {elapsed:.4f}s")
+        print(f"\nget_hconfig: {line_count} lines in {elapsed:.4f}s")  # noqa: T201
         assert elapsed < 5.0, f"Parsing took {elapsed:.2f}s, expected < 5s"
 
-    def test_parse_large_xr_config(self) -> None:
+    @staticmethod
+    def test_parse_large_xr_config() -> None:
         """Parse a ~10k line XR config via get_hconfig."""
         config_text = _generate_large_xr_config()
         elapsed = _time_fn(lambda: get_hconfig(Platform.CISCO_XR, config_text))
         line_count = config_text.count("\n")
-        print(f"\nget_hconfig (XR): {line_count} lines in {elapsed:.4f}s")
+        print(f"\nget_hconfig (XR): {line_count} lines in {elapsed:.4f}s")  # noqa: T201
         assert elapsed < 5.0, f"Parsing took {elapsed:.2f}s, expected < 5s"
 
-    def test_fast_load_large_ios_config(self) -> None:
+    @staticmethod
+    def test_fast_load_large_ios_config() -> None:
         """Parse a ~10k line IOS config via get_hconfig_fast_load."""
         config_text = _generate_large_ios_config()
         config_lines = tuple(config_text.splitlines())
         elapsed = _time_fn(
             lambda: get_hconfig_fast_load(Platform.CISCO_IOS, config_lines),
         )
-        print(f"\nget_hconfig_fast_load: {len(config_lines)} lines in {elapsed:.4f}s")
+        print(f"\nget_hconfig_fast_load: {len(config_lines)} lines in {elapsed:.4f}s")  # noqa: T201
         assert elapsed < 5.0, f"Fast load took {elapsed:.2f}s, expected < 5s"
 
-    def test_fast_load_vs_get_hconfig(self) -> None:
+    @staticmethod
+    def test_fast_load_vs_get_hconfig() -> None:
         """get_hconfig_fast_load should be faster than get_hconfig."""
         config_text = _generate_large_ios_config()
         config_lines = tuple(config_text.splitlines())
@@ -181,7 +189,7 @@ def test_fast_load_vs_get_hconfig(self) -> None:
             lambda: get_hconfig_fast_load(Platform.CISCO_IOS, config_lines),
         )
         ratio = time_full / time_fast if time_fast > 0 else float("inf")
-        print(
+        print(  # noqa: T201
             f"\nget_hconfig: {time_full:.4f}s, "
             f"fast_load: {time_fast:.4f}s, "
             f"ratio: {ratio:.1f}x"
@@ -195,7 +203,8 @@ def test_fast_load_vs_get_hconfig(self) -> None:
 class TestRemediationBenchmarks:
     """Benchmarks for config_to_get_to remediation."""
 
-    def test_remediation_small_diff(self) -> None:
+    @staticmethod
+    def test_remediation_small_diff() -> None:
         """Remediation with ~5% of interfaces changed."""
         running_text = _generate_large_ios_config()
         running = get_hconfig(Platform.CISCO_IOS, running_text)
@@ -207,10 +216,11 @@ def test_remediation_small_diff(self) -> None:
         generated = get_hconfig(Platform.CISCO_IOS, generated_text)
 
         elapsed = _time_fn(lambda: running.config_to_get_to(generated))
-        print(f"\nRemediation (10% diff): {elapsed:.4f}s")
+        print(f"\nRemediation (10% diff): {elapsed:.4f}s")  # noqa: T201
         assert elapsed < 5.0, f"Remediation took {elapsed:.2f}s, expected < 5s"
 
-    def test_remediation_large_diff(self) -> None:
+    @staticmethod
+    def test_remediation_large_diff() -> None:
         """Remediation with ~100% of interfaces changed."""
         running = get_hconfig(Platform.CISCO_IOS, _generate_large_ios_config())
         generated_text = _generate_large_ios_config().replace(
@@ -219,10 +229,11 @@ def test_remediation_large_diff(self) -> None:
         generated = get_hconfig(Platform.CISCO_IOS, generated_text)
 
         elapsed = _time_fn(lambda: running.config_to_get_to(generated))
-        print(f"\nRemediation (100% diff): {elapsed:.4f}s")
+        print(f"\nRemediation (100% diff): {elapsed:.4f}s")  # noqa: T201
         assert elapsed < 10.0, f"Remediation took {elapsed:.2f}s, expected < 10s"
 
-    def test_remediation_completely_different(self) -> None:
+    @staticmethod
+    def test_remediation_completely_different() -> None:
         """Remediation between two entirely different configs."""
         running = get_hconfig(Platform.CISCO_IOS, _generate_large_ios_config(500))
         # Generate a completely different config
@@ -238,35 +249,38 @@ def test_remediation_completely_different(self) -> None:
         generated = get_hconfig(Platform.CISCO_IOS, "\n".join(lines))
 
         elapsed = _time_fn(lambda: running.config_to_get_to(generated))
-        print(f"\nRemediation (completely different): {elapsed:.4f}s")
+        print(f"\nRemediation (completely different): {elapsed:.4f}s")  # noqa: T201
         assert elapsed < 10.0, f"Remediation took {elapsed:.2f}s, expected < 10s"
 
 
 class TestIterationBenchmarks:
     """Benchmarks for tree traversal and iteration."""
 
-    def test_all_children_sorted(self) -> None:
+    @staticmethod
+    def test_all_children_sorted() -> None:
         """Iterate all_children_sorted on a large config."""
         config = get_hconfig(Platform.CISCO_IOS, _generate_large_ios_config())
 
         elapsed = _time_fn(lambda: list(config.all_children_sorted()))
         child_count = len(list(config.all_children()))
-        print(f"\nall_children_sorted: {child_count} nodes in {elapsed:.4f}s")
+        print(f"\nall_children_sorted: {child_count} nodes in {elapsed:.4f}s")  # noqa: T201
         assert elapsed < 2.0, f"Iteration took {elapsed:.2f}s, expected < 2s"
 
-    def test_dump_simple(self) -> None:
+    @staticmethod
+    def test_dump_simple() -> None:
         """Dump a large config to simple text."""
         config = get_hconfig(Platform.CISCO_IOS, _generate_large_ios_config())
 
         elapsed = _time_fn(config.dump_simple)
         line_count = len(config.dump_simple())
-        print(f"\ndump_simple: {line_count} lines in {elapsed:.4f}s")
+        print(f"\ndump_simple: {line_count} lines in {elapsed:.4f}s")  # noqa: T201
         assert elapsed < 2.0, f"dump_simple took {elapsed:.2f}s, expected < 2s"
 
-    def test_deep_copy(self) -> None:
+    @staticmethod
+    def test_deep_copy() -> None:
         """Deep copy a large config tree."""
         config = get_hconfig(Platform.CISCO_IOS, _generate_large_ios_config())
 
         elapsed = _time_fn(config.deep_copy)
-        print(f"\ndeep_copy: {elapsed:.4f}s")
+        print(f"\ndeep_copy: {elapsed:.4f}s")  # noqa: T201
         assert elapsed < 5.0, f"deep_copy took {elapsed:.2f}s, expected < 5s"

tests/unit/test_child.py

@@ -1,4 +1,5 @@
 """Tests for HConfigChild functionality."""
+# pylint: disable=too-many-lines
 
 import types
 
@@ -1146,3 +1147,112 @@ def test_idempotency_key_regex_trimmed_to_no_match() -> None:
     assert key == ("text|logging console",)
 
 
+
+def test_child_hash_eq_consistency_new_in_config() -> None:
+    """Test that equal HConfigChild objects have equal hashes regardless of new_in_config.
+
+    Validates the bug in issue #185: __hash__ includes new_in_config but __eq__ does not,
+    violating the Python invariant that a == b implies hash(a) == hash(b).
+    """
+    platform = Platform.CISCO_IOS
+    config = get_hconfig(platform)
+    child1 = config.add_child("interface GigabitEthernet0/0")
+    config2 = get_hconfig(platform)
+    child2 = config2.add_child("interface GigabitEthernet0/0")
+
+    child1.new_in_config = False
+    child2.new_in_config = True
+
+    # These two children compare as equal (same text, no tags, no children)
+    assert child1 == child2
+    # Python invariant: equal objects must have equal hashes
+    assert hash(child1) == hash(child2)
+
+
+def test_child_hash_eq_consistency_order_weight() -> None:
+    """Test that equal HConfigChild objects have equal hashes regardless of order_weight.
+
+    Validates the bug in issue #185: __hash__ includes order_weight but __eq__ does not,
+    violating the Python invariant that a == b implies hash(a) == hash(b).
+    """
+    platform = Platform.CISCO_IOS
+    config = get_hconfig(platform)
+    child1 = config.add_child("interface GigabitEthernet0/0")
+    config2 = get_hconfig(platform)
+    child2 = config2.add_child("interface GigabitEthernet0/0")
+
+    child1.order_weight = 0
+    child2.order_weight = 100
+
+    # These two children compare as equal (same text, no tags, no children)
+    assert child1 == child2
+    # Python invariant: equal objects must have equal hashes
+    assert hash(child1) == hash(child2)
+
+
+def test_child_hash_eq_consistency_tags() -> None:
+    """Test that __hash__ and __eq__ agree on whether tags affect equality.
+
+    Validates the bug in issue #185: __eq__ checks tags but __hash__ does not include
+    tags, meaning two objects that compare unequal could have the same hash (not a
+    correctness violation, but inconsistent) while also raising the question of whether
+    tags should be part of the hash.
+    """
+    platform = Platform.CISCO_IOS
+    config = get_hconfig(platform)
+    child1 = config.add_child("interface GigabitEthernet0/0")
+    config2 = get_hconfig(platform)
+    child2 = config2.add_child("interface GigabitEthernet0/0")
+
+    child1.tags = frozenset({"safe"})
+    child2.tags = frozenset()
+
+    # __eq__ considers tags, so these are unequal
+    assert child1 != child2
+    # Since they are unequal, their hashes should differ to avoid excessive collisions
+    # (not strictly required by the invariant, but required for correctness in reverse:
+    # if hash(a) != hash(b) then a != b must hold — currently tags are in __eq__ but
+    # not __hash__, so unequal objects can share a hash, which means dict/set lookup
+    # will fall back to __eq__ unexpectedly)
+    assert hash(child1) != hash(child2)
+
+
+def test_child_set_deduplication_with_new_in_config() -> None:
+    """Test that equal HConfigChild objects are deduplicated correctly in sets.
+
+    Validates the practical impact of issue #185: when new_in_config differs,
+    two logically equal children occupy different set buckets, causing duplicates.
+    """
+    platform = Platform.CISCO_IOS
+    config = get_hconfig(platform)
+    child1 = config.add_child("interface GigabitEthernet0/0")
+    config2 = get_hconfig(platform)
+    child2 = config2.add_child("interface GigabitEthernet0/0")
+
+    child1.new_in_config = False
+    child2.new_in_config = True
+
+    assert child1 == child2
+    # Equal objects must collapse to one entry in a set
+    assert len({child1, child2}) == 1
+
+
+def test_child_dict_key_lookup_with_order_weight() -> None:
+    """Test that HConfigChild objects with differing order_weight work as dict keys.
+
+    Validates the practical impact of issue #185: when order_weight differs, a
+    logically equal child cannot be found as a dict key.
+    """
+    platform = Platform.CISCO_IOS
+    config = get_hconfig(platform)
+    child1 = config.add_child("interface GigabitEthernet0/0")
+    config2 = get_hconfig(platform)
+    child2 = config2.add_child("interface GigabitEthernet0/0")
+
+    child1.order_weight = 0
+    child2.order_weight = 100
+
+    assert child1 == child2
+    lookup: dict[HConfigChild, str] = {child1: "found"}
+    # child2 is equal to child1, so it must find the same dict entry
+    assert lookup[child2] == "found"