Prevent empty registry rows from retaining unused entities

CHANGELOG.md

@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Component reads and tag cleanup no longer retain unused entities through empty registry rows.
+- Failed component removals no longer create empty rows, and clearing removes existing empty rows.
 - Replacing component relations from an inherited view now preserves values which depend on the destination.
 - Removing a relation tag or component now preserves query membership when the other relation still exists.
 - Queries collecting component rows now resolve inherited components instead of raising `KeyError` for prefab instances.

tcod/ecs/entity.py

@@ -420,9 +420,12 @@ def __getitem__(self, key: ComponentKey[T]) -> T:
         assert self.__assert_key(key)
         _components_by_entity = self.entity.registry._components_by_entity
         for entity in _traverse_entities(self.entity, self.traverse):
+            components = _components_by_entity.get(entity)
+            if components is None:
+                continue
             try:
-                return _components_by_entity[entity][key]  # type: ignore[no-any-return]
-            except KeyError:  # noqa: PERF203
+                return components[key]  # type: ignore[no-any-return]
+            except KeyError:
                 pass
         raise KeyError(key)
 
@@ -444,10 +447,13 @@ def __delitem__(self, key: TypeForm[object] | tuple[object, TypeForm[object]]) -
         """Delete a directly held component from an entity."""
         assert self.__assert_key(key)
 
-        old_value = self.entity.registry._components_by_entity[self.entity].get(key)
+        components = self.entity.registry._components_by_entity.get(self.entity)
+        if components is None:
+            raise KeyError(key)
+        old_value = components.get(key)
 
-        del self.entity.registry._components_by_entity[self.entity][key]
-        if not self.entity.registry._components_by_entity[self.entity]:
+        del components[key]
+        if not components:
             del self.entity.registry._components_by_entity[self.entity]
 
         del self.entity.registry._components_by_type[key][self.entity]
@@ -589,8 +595,12 @@ def pop(
     def clear(self) -> None:
         """Remove any components stored directly in this entity."""
         if self.traverse:
-            return self(traverse=()).clear()
-        return super().clear()
+            self(traverse=()).clear()
+            return
+        super().clear()
+        # MutableMapping.clear() only calls __delitem__ for existing components,
+        # so it leaves an already empty registry row behind.
+        self.entity.registry._components_by_entity.pop(self.entity, None)
 
 
 @attrs.define(eq=False, frozen=True, weakref_slot=False)
@@ -626,7 +636,7 @@ def add(self, tag: object) -> None:
 
     def discard(self, tag: object) -> None:
         """Discard a tag directly held by an entity."""
-        if tag not in self.entity.registry._tags_by_entity[self.entity]:
+        if tag not in self.entity.registry._tags_by_entity.get(self.entity, ()):
             return  # Already doesn't have tag
         tcod.ecs.query._touch_tag(self.entity.registry, tag)  # Tag removed
 
@@ -648,8 +658,9 @@ def remove(self, tag: object) -> None:
     def clear(self) -> None:
         """Remove all tags directly in this entity."""
         _tags_by_entity = self.entity.registry._tags_by_entity
-        while _tags_by_entity[self.entity]:
-            self.discard(next(iter(_tags_by_entity[self.entity])))
+        while tags := _tags_by_entity.get(self.entity):
+            self.discard(next(iter(tags)))
+        _tags_by_entity.pop(self.entity, None)
 
     def __contains__(self, x: object) -> bool:
         """Return True if this entity has the given tag."""
@@ -996,10 +1007,16 @@ def __setitem__(self, target: Entity, component: T) -> None:
     def __delitem__(self, target: Entity) -> None:
         """Delete a component assigned to the target entity."""
         registry = self.entity.registry
-        del registry._relation_components_by_entity[self.entity][self.key][target]
-        if not registry._relation_components_by_entity[self.entity][self.key]:
-            del registry._relation_components_by_entity[self.entity][self.key]
-        if not registry._relation_components_by_entity[self.entity]:
+        by_entity = registry._relation_components_by_entity.get(self.entity)
+        if by_entity is None:
+            raise KeyError(target)
+        by_key = by_entity.get(self.key)
+        if by_key is None:
+            raise KeyError(target)
+        del by_key[target]
+        if not by_key:
+            del by_entity[self.key]
+        if not by_entity:
             del registry._relation_components_by_entity[self.entity]
 
         _relations_lookup_discard(registry, self.entity, self.key, target)

tests/test_entity_lifecycle.py

@@ -0,0 +1,83 @@
+"""Regression tests for unreferenced entity lifetimes."""
+
+from __future__ import annotations
+
+import gc
+import weakref
+
+import pytest
+
+from tcod.ecs import Registry
+
+
+@pytest.mark.parametrize(
+    "operation",
+    [
+        "clear",
+        "clear_empty",
+        "clear_tags",
+        "discard_missing_tag",
+        "read_missing_component",
+        "read_inherited_component",
+        "delete_missing_component",
+        "delete_missing_relation_component",
+        "delete_missing_relation_component_type",
+    ],
+)
+def test_unused_entity_is_released(operation: str) -> None:
+    """Reads and cleanup must leave no registry reference to an unused entity."""
+    registry = Registry()
+    prefab = registry["prefab"]
+    prefab_value = 7
+    prefab.components[int] = prefab_value
+    entity = prefab.instantiate() if operation == "read_inherited_component" else registry.new_entity()
+    if operation == "clear":
+        entity.components[int] = 3
+        entity.tags.add("inventory")
+        entity.clear()
+    elif operation == "clear_empty":
+        entity.clear()
+    elif operation == "clear_tags":
+        entity.tags.add("inventory")
+        entity.tags.clear()
+    elif operation == "discard_missing_tag":
+        entity.tags.discard("inventory")
+    elif operation == "read_missing_component":
+        assert entity.components.get(str) is None
+    elif operation == "read_inherited_component":
+        assert entity.components[int] == prefab_value
+        entity.relation_tags_many.clear()
+    elif operation == "delete_missing_component":
+        with pytest.raises(KeyError, match="str"):
+            del entity.components[str]
+    elif operation == "delete_missing_relation_component":
+        with pytest.raises(KeyError, match="target"):
+            del entity.relation_components[str][registry["target"]]
+    else:
+        assert operation == "delete_missing_relation_component_type"
+        entity.relation_components[int][prefab] = 3
+        with pytest.raises(KeyError, match="target"):
+            del entity.relation_components[str][registry["target"]]
+        assert entity.relation_components[int] == {prefab: 3}
+        del entity.relation_components[int][prefab]
+    reference = weakref.ref(entity)
+    del entity
+    gc.collect()
+    assert reference() is None
+    assert registry["prefab"] is prefab
+    assert prefab.components[int] == prefab_value
+
+
+def test_incoming_relation_retains_cleared_entity() -> None:
+    """An incoming relation intentionally keeps its target alive until removed."""
+    registry = Registry()
+    owner, target = registry["owner"], registry.new_entity()
+    owner.relation_tag["target"] = target
+    reference = weakref.ref(target)
+    target.clear()
+    del target
+    gc.collect()
+    assert reference() is not None
+    del owner.relation_tag["target"]
+    gc.collect()
+    assert reference() is None