From 091a2ad0cf94b51c560f161c8793b44ea2295adc Mon Sep 17 00:00:00 2001 From: stroblme Date: Mon, 10 Aug 2026 19:23:27 +0200 Subject: [PATCH] Housekeeping ahead of the Inc 1-4 coverage work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - inventory.py --methods: report the instance methods the app calls per class, plus chained calls that constrain a return type. --check says which classes to bind; this says what to bind on them, which is what writing ~38 module TUs needs. - Drop StlAPI from the inventory: StlAPI_Writer has no app call site (the only use was a test fixture, now on the app's own STL writer). 138 symbols / 47 modules. - adding-symbols.md: scope the executing-constructor ban to the BRepAlgoAPI booleans, which are the only classes with a deferred Set*/Build form — BRepMesh_IncrementalMesh, GeomAPI_*, BRepCheck_Analyzer and friends compute in their constructor by design and bind as stock. Replace the per-increment app-test guidance: backend/tests/conftest.py imports n3xd.main, so no app test can collect until the last module is bound. Increments gate on stock-recorded fixtures here; the app suite is the Inc 4 gate. - parity_venv.sh: run the ocp suite in the swapped venv (it imports only OCP/n3xd_ocp, so it works throughout). - Fix a stale macro name in occt_handle.h (ocp_new, not OCP_TRANSIENT_NEW) and drop the unused ocp_transient_class helper. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DfriM8XUkn7uYf5Dwe2xo6 --- docs/adding-symbols.md | 47 +++++++++++++------ inventory.json | 3 -- src/common/occt_handle.h | 2 +- src/common/occt_transient.h | 7 --- tools/inventory.py | 93 +++++++++++++++++++++++++++++++++++++ tools/parity_venv.sh | 7 +++ 6 files changed, 134 insertions(+), 25 deletions(-) diff --git a/docs/adding-symbols.md b/docs/adding-symbols.md index bc75d7a..0235cf6 100644 --- a/docs/adding-symbols.md +++ b/docs/adding-symbols.md @@ -6,15 +6,22 @@ Read [design.md](design.md) first if you are touching the machinery instead. ## 1. Find what is missing ```bash -python tools/inventory.py --emit # re-scan the app for OCP usage -python tools/inventory.py --check # what the installed wheel lacks, by module +python tools/inventory.py --emit # re-scan the app for OCP usage +python tools/inventory.py --check # what the wheel lacks, by module +python tools/inventory.py --methods --only BRepAdaptor # what to bind on each class ``` -`--check` groups gaps by module, which is how increments are scoped. Note it -only sees symbols reached through an import (`TopExp.MapShapes_s`), not methods -called on instances (`shape.IsSame(...)`) — a green `--check` is necessary, not -sufficient. Running the app's own tests in the parity venv is what catches the -rest, loudly, as an `AttributeError`. +`--check` groups gaps by module, which is how increments are scoped. It only +sees symbols reached through an import (`TopExp.MapShapes_s`), so it answers +*which* classes to bind but not *what* to bind on them. + +`--methods` answers the second question: it resolves variables assigned +straight from a constructor and reports the methods called on them, plus +chained calls as `Klass.Outer() -> Inner` (those constrain `Outer`'s **return** +type — `adaptor.Cylinder().Radius()` is a requirement on `gp_Cylinder`). It is +a heuristic — it does not follow arguments, returns or attributes, and a local +reassigned from something else shows up as noise — so read it as a starting +surface, not a specification. ## 2. Write the module @@ -50,8 +57,13 @@ only in that a base class must precede its derived classes. `nb::class_` declaration and bind constructors with `ocp_new()`. Never `nb::init<>` — see design.md. - **Long kernel call** → `OCP_NOGIL`, but only if it cannot re-enter Python. -- **Executing constructor** → do not bind it. Bind the default constructor plus - the `SetX`/`Build` sequence. +- **Executing constructor** → banned only where a deferred `SetX`/`Build` API + exists and the constructor duplicates `Build()`: the `BRepAlgoAPI_*` booleans + and splitter. Bind their default constructor plus the `SetX`/`Build` + sequence. Classes that only compute in their constructor and have no deferred + form — `BRepMesh_IncrementalMesh`, `BRepCheck_Analyzer`, `GeomAPI_*`, + `BRepClass3d_SolidClassifier`, `BRepExtrema_DistShapeShape`, `GCPnts_*`, + `BRepBuilderAPI_Transform` — bind exactly as stock does. - **Enum** → `nb::is_arithmetic()` and `.export_values()`. - **`Message_ProgressRange` parameters** → omit them. The app never passes one (no `OCP.Message` import anywhere), and leaving them out keeps signatures @@ -82,11 +94,18 @@ make publish tools/parity_venv.sh && python tools/inventory.py --check ``` -Then run the slice of the app's suite the increment claims — Inc 1 is gated on -`backend/tests/test_geom_memo.py`, Inc 2 on the tessellation tests plus -`pytest -m perf`, Inc 4 on `test_cad_pool.py` and `test_derive.py`, and the -cutover on the full suite plus `backend/tools/rebuild_sweep.py --diff` over the -project store. +**The app's own tests cannot gate an individual increment.** `backend/tests/ +conftest.py` imports `n3xd.main`, which pulls in the whole app and therefore the +whole OCP surface, so every backend test fails at collection until the last +module is bound. Increments are gated here instead: `tools/gen_fixtures.py` +records reference values from the *stock* wheel (counts, `Modified`/`Generated`/ +`IsDeleted` history maps, measured floats) into `tests/data/manifest.json`, and +`tests/test_inc_*.py` reproduces the same constructions under our wheel. +Counts and history maps must match exactly; floats compare at rel 1e-9. + +The app's full suite is the **Inc 4** gate, run in the parity venv, alongside +`pytest -m perf` and `backend/tools/rebuild_sweep.py --diff` over the project +store. ## Adding to `n3xd_ocp` instead diff --git a/inventory.json b/inventory.json index bcbdcbd..ebc908d 100644 --- a/inventory.json +++ b/inventory.json @@ -212,9 +212,6 @@ "StdPrs_BRepFont": [], "StdPrs_BRepTextBuilder": [] }, - "StlAPI": { - "StlAPI_Writer": [] - }, "TColStd": { "TColStd_Array1OfInteger": [], "TColStd_Array1OfReal": [], diff --git a/src/common/occt_handle.h b/src/common/occt_handle.h index 70234dd..5b42079 100644 --- a/src/common/occt_handle.h +++ b/src/common/occt_handle.h @@ -22,7 +22,7 @@ That last sentence is only true if every wrapped transient was heap allocated and is handle-owned. See occt_transient.h — transient - constructors are bound through OCP_TRANSIENT_NEW, never nb::init<>, so + constructors are bound through ocp_new(), never nb::init<>, so Python never owns transient storage. from_python re-checks the invariant rather than trusting it, because the failure mode is a double free. */ diff --git a/src/common/occt_transient.h b/src/common/occt_transient.h index b898732..81a0a0e 100644 --- a/src/common/occt_transient.h +++ b/src/common/occt_transient.h @@ -24,13 +24,6 @@ namespace nb = nanobind; -/// Declare a bound transient class. Bind its constructors with ocp_new() — nb::init<> on such a class is a bug (see above). -template -nb::class_ ocp_transient_class(nb::handle scope, const char *name) { - return nb::class_(scope, name); -} - /// Heap-allocating constructor for a transient class: /// /// cls.def(ocp_new(), diff --git a/tools/inventory.py b/tools/inventory.py index f393200..b9a8111 100644 --- a/tools/inventory.py +++ b/tools/inventory.py @@ -80,6 +80,91 @@ def scan(app_root: pathlib.Path) -> dict: } +def methods(app_root: pathlib.Path, only: str | None) -> int: + """Report the *instance* methods the app calls on each OCP class. + + `scan` only sees names reached through an import, which is enough to know + *which* classes to bind but not *what* to bind on them. This fills that + gap well enough to write a module in one pass instead of discovering the + surface one AttributeError at a time: variables assigned straight from a + constructor carry their class through the file, so `a = BRepAdaptor_Surface(f)` + followed by `a.GetType()` is resolved. + + Chained calls are reported separately as `Klass.Outer() -> Inner`, because + what they constrain is the *return* type — `adaptor.Cylinder().Radius()` + says gp_Cylinder needs `Radius`, not that BRepAdaptor_Surface does. + + Heuristic by construction: it does not follow arguments, returns or + attributes, so treat a quiet class as "look again", not "nothing needed". + """ + direct: dict[str, set[str]] = collections.defaultdict(set) + chained: dict[str, set[str]] = collections.defaultdict(set) + + for path in sorted(app_root.rglob("*.py")): + if "__pycache__" in path.parts: + continue + try: + tree = ast.parse(path.read_text(encoding="utf-8")) + except (SyntaxError, UnicodeDecodeError): + continue + + classes = { + alias.asname or alias.name: alias.name + for node in ast.walk(tree) + if isinstance(node, ast.ImportFrom) + and node.module + and (node.module == "OCP" or node.module.startswith("OCP.")) + for alias in node.names + } + if not classes: + continue + + # local variable -> OCP class, from `v = Klass(...)` + env: dict[str, str] = {} + for node in ast.walk(tree): + if isinstance(node, ast.Assign) and isinstance(node.value, ast.Call): + fn = node.value.func + cls = classes.get(getattr(fn, "id", "")) + if cls: + for tgt in node.targets: + if isinstance(tgt, ast.Name): + env[tgt.id] = cls + + def owner(node: ast.expr) -> str | None: + """The OCP class an expression evaluates to, when knowable.""" + if isinstance(node, ast.Name): + return env.get(node.id) + if isinstance(node, ast.Call): + return classes.get(getattr(node.func, "id", "")) + return None + + for node in ast.walk(tree): + if not isinstance(node, ast.Attribute): + continue + cls = owner(node.value) + if cls: + direct[cls].add(node.attr) + continue + # .Outer().Inner — constrains Outer's return type + inner = node.value + if isinstance(inner, ast.Call) and isinstance(inner.func, ast.Attribute): + cls = owner(inner.func.value) + if cls: + chained[f"{cls}.{inner.func.attr}()"].add(node.attr) + + def dump(title: str, data: dict[str, set[str]]) -> None: + keys = [k for k in sorted(data) if only is None or only in k] + if not keys: + return + print(f"\n{title}") + for key in keys: + print(f" {key}: {', '.join(sorted(data[key]))}") + + dump("instance methods, by class:", direct) + dump("chained calls (constrain the return type):", chained) + return 0 + + def emit(app_root: pathlib.Path, out: pathlib.Path) -> int: data = scan(app_root) out.write_text(json.dumps(data, indent=2) + "\n") @@ -130,10 +215,18 @@ def main() -> int: ap.add_argument("--emit", action="store_true", help="rewrite the inventory") ap.add_argument("--check", action="store_true", help="compare the installed OCP against the inventory") + ap.add_argument("--methods", action="store_true", + help="report the instance methods the app calls per class") + ap.add_argument("--only", help="with --methods: substring filter on the class") ap.add_argument("--app", type=pathlib.Path, default=DEFAULT_APP) ap.add_argument("--inventory", type=pathlib.Path, default=DEFAULT_INVENTORY) args = ap.parse_args() + if args.methods: + if not args.app.is_dir(): + print(f"app sources not found: {args.app}", file=sys.stderr) + return 2 + return methods(args.app, args.only) if args.emit: if not args.app.is_dir(): print(f"app sources not found: {args.app}", file=sys.stderr) diff --git a/tools/parity_venv.sh b/tools/parity_venv.sh index ab98a4b..bf465ec 100755 --- a/tools/parity_venv.sh +++ b/tools/parity_venv.sh @@ -51,3 +51,10 @@ print('modules:', len(OCP._OCP.__all_modules__)) echo "--- coverage against the app's symbol inventory ---" uv run python "$HERE/inventory.py" --check || \ echo "(incomplete coverage is expected until the increments land)" + +# The binding's own suite, run from the swapped venv: this is what exercises the +# published artifact inside the app's exact dependency set. It imports only +# OCP/n3xd_ocp, so it works long before the app's own tests can collect (their +# conftest imports n3xd.main, i.e. the whole OCP surface). +echo "--- ocp suite under the swapped venv ---" +uv run pytest "$OCP_REPO/tests" -q