From 38e0e0e297e4f1f279cfa890f58711f49e0b454b Mon Sep 17 00:00:00 2001 From: Dion Moult Date: Wed, 5 Aug 2026 11:10:11 +1000 Subject: [PATCH] Compare entity instances in one file by identity again entity_instance.file is a property backed by a fresh SWIG wrapper on every access, and ifcopenshell::file had no __eq__, so `self.file != other.file` in entity_instance.__eq__ compared two throwaway wrappers and was always true - even for an instance against itself. Every entity comparison therefore took the deep get_info() branch, making distinct but structurally identical instances compare equal and leaving the final `return False` unreachable. Bonsai's TestAddRepresentationItemToShapeAspect showed this as two separate IfcShapeAspects being treated as one, so the stale aspect was never removed. Restore the file_pointer() pair that was commented out on both ifcopenshell::file and express::Base - IfcParseWrapper.i already described it as the way to "trace file ownership of instances on the python side" - and give file the __eq__/__hash__ it was missing. The express::Base one needs $self->file() now that file_ lives on instance_data. This also repairs rocksdb_lazy_instance.__eq__, which already called file_pointer(). EXPRESS `=` is value comparison and `:=:` is instance comparison, but rule_compiler emits `==` for both (see the @todo on process_rel_op), and derived attributes build their operands in the shared global file, so rules compare same-file instances and need value semantics. Restore those for the duration of rule execution with settings.compare_instances_by_value, alongside the existing unpack_non_aggregate_inverses. Co-Authored-By: Claude Opus 5 (1M context) --- .../ifcopenshell/entity_instance.py | 6 ++++- .../ifcopenshell/express/rule_executor.py | 7 +++++ .../ifcopenshell/ifcopenshell_wrapper.pyi | 2 ++ .../ifcopenshell/settings.py | 21 +++++++++++++++ .../test/test_entity_instance.py | 14 ++++++++++ src/ifcwrap/IfcParseWrapper.i | 26 ++++++++++++++----- 6 files changed, 69 insertions(+), 7 deletions(-) diff --git a/src/ifcopenshell-python/ifcopenshell/entity_instance.py b/src/ifcopenshell-python/ifcopenshell/entity_instance.py index 1a102faa8e..c3f7003f4d 100644 --- a/src/ifcopenshell-python/ifcopenshell/entity_instance.py +++ b/src/ifcopenshell-python/ifcopenshell/entity_instance.py @@ -218,7 +218,11 @@ class entity_instance_mixin: if self.is_a(True) != other.is_a(True): return False - if self.file != other.file or not self.is_entity(): + if ( + settings.compare_instances_by_value + or self.file_pointer() != other.file_pointer() + or not self.is_entity() + ): return self.get_info(recursive=True, include_identifier=False) == other.get_info( recursive=True, include_identifier=False ) diff --git a/src/ifcopenshell-python/ifcopenshell/express/rule_executor.py b/src/ifcopenshell-python/ifcopenshell/express/rule_executor.py index 68240aeddd..1b4815a430 100644 --- a/src/ifcopenshell-python/ifcopenshell/express/rule_executor.py +++ b/src/ifcopenshell-python/ifcopenshell/express/rule_executor.py @@ -90,6 +90,12 @@ def run(f: ifcopenshell.file, logger: Logger) -> None: orig = ifcopenshell.settings.unpack_non_aggregate_inverses ifcopenshell.settings.unpack_non_aggregate_inverses = True + # Rules are transpiled with EXPRESS `=` emitted as Python `==`, so `==` has + # to mean value comparison for the duration. See the @todo on + # rule_compiler.process_rel_op. + orig_compare = ifcopenshell.settings.compare_instances_by_value + ifcopenshell.settings.compare_instances_by_value = True + fn = os.path.join(os.path.dirname(__file__), "rules", f"{f.schema_identifier}.py") try: source = open(fn, "r").read() @@ -272,6 +278,7 @@ def run(f: ifcopenshell.file, logger: Logger) -> None: ) ifcopenshell.settings.unpack_non_aggregate_inverses = orig + ifcopenshell.settings.compare_instances_by_value = orig_compare if __name__ == "__main__": diff --git a/src/ifcopenshell-python/ifcopenshell/ifcopenshell_wrapper.pyi b/src/ifcopenshell-python/ifcopenshell/ifcopenshell_wrapper.pyi index ec9a591768..26e9b42e59 100644 --- a/src/ifcopenshell-python/ifcopenshell/ifcopenshell_wrapper.pyi +++ b/src/ifcopenshell-python/ifcopenshell/ifcopenshell_wrapper.pyi @@ -750,6 +750,7 @@ class entity_instance(entity_instance_mixin): def declaration(self) -> declaration: ... @property def file(self) -> ifcopenshell.file: ... + def file_pointer(self) -> int: ... def get_argument(self, *args: int | str) -> Any: ... def get_argument_index(self, a: str) -> int: ... def attribute_name(self, i: int) -> str: ... @@ -836,6 +837,7 @@ class face: def print_impl(self, o, indent): ... class file(file_mixin): + def file_pointer(self) -> int: ... def fresh_id(self) -> int: ... def __init__( self, diff --git a/src/ifcopenshell-python/ifcopenshell/settings.py b/src/ifcopenshell-python/ifcopenshell/settings.py index 1364f59872..bf95d20e53 100644 --- a/src/ifcopenshell-python/ifcopenshell/settings.py +++ b/src/ifcopenshell-python/ifcopenshell/settings.py @@ -34,3 +34,24 @@ element or None. Example: """ unpack_non_aggregate_inverses = False + +""" +When true compare entity instances by value rather than by identity, even +when they belong to the same file. EXPRESS uses `=` for value comparison and +`:=:` for instance comparison, whereas the Python API only has `==`, which +normally means "the same instance". Example: + +>>> import ifcopenshell +>>> f = ifcopenshell.file(schema='ifc2x3') +>>> f.createIfcCartesianPoint((0., 0.)) +#1=IfcCartesianPoint((0.,0.)) +>>> f.createIfcCartesianPoint((0., 0.)) +#2=IfcCartesianPoint((0.,0.)) +>>> f[1] == f[2] +False +>>> ifcopenshell.settings.compare_instances_by_value = True +>>> f[1] == f[2] +True +""" + +compare_instances_by_value = False diff --git a/src/ifcopenshell-python/test/test_entity_instance.py b/src/ifcopenshell-python/test/test_entity_instance.py index 33bb23b4e8..e0e83ee7e1 100644 --- a/src/ifcopenshell-python/test/test_entity_instance.py +++ b/src/ifcopenshell-python/test/test_entity_instance.py @@ -75,6 +75,20 @@ def test_equality(): assert f[1] == g[1] g[1].Coordinates = (1.0, 0.0) assert f[1] != g[1] + f.createIfcCartesianPoint((0.0, 0.0)) + assert f[1] == f[1] + assert f[1] != f[2] + + +def test_equality_of_owning_file(): + f = ifcopenshell.file() + g = ifcopenshell.file() + f.createIfcCartesianPoint((0.0, 0.0)) + f.createIfcCartesianPoint((0.0, 0.0)) + g.createIfcCartesianPoint((0.0, 0.0)) + assert f[1].file == f[1].file + assert f[1].file == f[2].file + assert f[1].file != g[1].file def test_setting_logical(): diff --git a/src/ifcwrap/IfcParseWrapper.i b/src/ifcwrap/IfcParseWrapper.i index 04ac4db0b8..22d51bd65a 100644 --- a/src/ifcwrap/IfcParseWrapper.i +++ b/src/ifcwrap/IfcParseWrapper.i @@ -263,13 +263,29 @@ private: %newobject ifcopenshell::file::create_uninitialized; %extend ifcopenshell::file { - /* // Use to correlate to entity_instance.file_pointer, so that we // can trace file ownership of instances on the python side. size_t file_pointer() const { return reinterpret_cast($self); } - */ + + %pythoncode %{ + def __eq__(self, other): + # Attribute access hands out a new wrapper every time, so identity + # of the wrapper says nothing about the file it refers to. + if self is other: + return True + if not isinstance(other, file): + return NotImplemented + return self.file_pointer() == other.file_pointer() + + def __ne__(self, other): + result = self.__eq__(other) + return result if result is NotImplemented else not result + + def __hash__(self): + return self.file_pointer() + %} file(const std::string& schema) { return new ifcopenshell::file(ifcopenshell::schema_by_name(schema)); @@ -552,12 +568,10 @@ private: return oss.str(); } - /* - // Just something to have a somewhat sensible value to hash + // Identifies the file owning this instance, zero when it has no owner. size_t file_pointer() const { - return reinterpret_cast($self->file_); + return reinterpret_cast($self->file()); } - */ unsigned get_argument_index(const std::string& a) const { if ($self->declaration().as_entity()) {