validate_stub: resolve property/staticmethod wrappers deterministically - #8946
Open
BIMvoice wants to merge 1 commit into
Open
validate_stub: resolve property/staticmethod wrappers deterministically#8946BIMvoice wants to merge 1 commit into
BIMvoice wants to merge 1 commit into
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
get_names_tree matched a property()/staticmethod() wrapper's underlying function by scanning a set of candidate strings with next(), then removing the match with subnames.remove(). Set iteration order for strings depends on PYTHONHASHSEED, which CPython randomizes per process. When more than one raw definition could match the same name (e.g. a getter and setter sharing a name, or several duplicate overload signatures), which one gets picked and removed varied between runs, and a later required lookup for the same name could then find nothing left, raising an unconditional AssertionError. This is replaced with a functions_by_name index built in source order (never hash order), so a wrapper is resolved by position (getter first, setter second) and, when several definitions share a name, by parameter count, picking the lowest-arity one for the getter slot. Only one definition is claimed per distinct name per statement, matching the existing description = property(description, description) dedup case. This makes the resolution a pure function of the parsed source text, independent of the interpreter's hash seed. Separately, get_function_node_name skipped every underscore-prefixed function up front, before it could ever be referenced by a later property()/staticmethod() assignment. src/ifcwrap/IfcGeomWrapper.i used to define such a helper, _geometry_with_backref, referenced by geometry = property(_geometry_with_backref), which made this assert deterministically for any build including that code path. PR IfcOpenShell#8763 correctly diagnosed this exact case and proposed renaming the helper; it was closed partly on the reasoning that the test passes cleanly, but the test is flaky, so a clean local run does not prove the bug is gone. As of commit 2f1b2f9 (today, unrelated PR by Andrej730) that specific helper was replaced with SWIG's %feature("shadow"), which no longer hits this path, so that one instance is already fixed upstream. The underscore skip itself is still present in this file for any other wrapped helper, so this change also generalizes the fix: underscore-prefixed definitions are now collected like any other and only hidden from the final stub diff if a property()/staticmethod() never claimed them. Extensive local testing (a formal analysis plus a 300-case fuzzer across 8 hash seeds, and 400 randomized cases comparing old vs new output) could not reproduce hash-seed-dependent flakiness from the missing-candidate path alone for a fixed input file, since candidate counts are consumed in a fixed, source-order sequence. The fix is shipped regardless because it removes the hash-order dependency outright, generalizes the underscore-handling defect instead of leaving it for the next occurrence, and was verified to produce byte identical output to the previous implementation across every real generated ifcopenshell_wrapper.py/.pyi pair available locally. Generated with the assistance of an AI coding tool.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
validate_stub skips any function whose name begins with an underscore, treating it as unused. When a property() or staticmethod() wrapper references such a function, the lookup can never resolve it and the check fails with a bare AssertionError at validate_stub.py:154.
Our own commit 824c1fc (2026-07-17, the #1124 owning-element fix) introduced exactly that pattern in IfcGeomWrapper.i, via geometry = property(_geometry_with_backref). That broke compile-and-test on v0.8.0, and it stayed broken for about a week. @Andrej730 fixed the specific instance in 88c8bd0 on 2026-07-24 by dropping the underscore, with the commit message "ifcwrap: fix breaking validate_stub (824c1fc)". Thanks for cleaning that up, and apologies for causing it.
This PR does not re-fix that instance. It hardens validate_stub itself so the same class of breakage cannot recur the next time someone writes an underscore-prefixed wrapped helper.
What changed
get_names_tree resolved a wrapper by matching the wrapped function's name against a set of candidate definitions using next(), then destructively removing the match. That is replaced with a functions_by_name index built in source order, resolving getter (position 0, lowest arity) against setter (position 1) deterministically, and preserving the property(description, description) dedup case. Underscore-prefixed definitions are no longer excluded before matching; they are only hidden from the final output if never claimed.
An honest negative result
An earlier hypothesis, including in our own comments on #8870, was that this failure was intermittent and driven by PYTHONHASHSEED randomising set iteration order. We could not reproduce that. A 300 case fuzzer across 8 hash seeds, plus a scan of 9 real generated ifcopenshell_wrapper.py/.pyi pairs, found zero hash-driven flakiness: for a fixed input the outcome is deterministic, because supply and demand for a given name are consumed in fixed statement order regardless of which set member next() returns.
So the apparent intermittency across PRs was branch base, not randomness. Branches cut before 88c8bd0 fail; branches cut after it pass. We are not claiming this PR fixes a flaky test, because on the evidence the test was not flaky. The set-based matching is still genuinely fragile and worth removing on its own merits, which is what this PR does.
Verification
Credit
#8763 diagnosed the underscore-skip cause correctly back when this first broke. We closed it earlier today partly on the reasoning that the test passes cleanly, which was not a sound basis given the failure depends on branch base. That diagnosis is carried forward here.
Context for #8870. Not marked as fixing it, since that issue tracks several workflows.
This change was made with the assistance of an AI tool.