Bind math_Vector-family templates + dependent OCCT surface; fix embind cross-inheritance overload aliasing#2
Conversation
Let template typedefs outside the NCollection family — e.g. math_VectorBase<double> (= math_Vector) and math_MatrixBase<double> (= math_Matrix) — be discovered and compiled. Two coupled changes: - discover.py: add EXTRA_TEMPLATE_CONTAINERS (math_VectorBase, math_MatrixBase) and ADMIT_TEMPLATE_CONTAINERS = NCOLLECTION_CONTAINERS | EXTRA (existing behaviour stays a strict subset, so it cannot regress), plus pass-1 canonical resolution so a method param spelled via a typedef (const math_Vector&) resolves to its canonical container before bailing. Implements the long-standing generic-template-discovery follow-up. - ast/template_args.py: substitute the canonical forward-declaration template parameter name. OCCT forward-declares `template<typename T> class math_VectorBase;` then defines it with TheItemType; libclang renders a method's self-type param (const math_VectorBase&) using the forward-decl name T, while processTemplate keys templateArgs from the definition (TheItemType), so the T was never substituted and an uncompilable math_VectorBase<T> leaked into select_overload<> signatures and val-dispatch arg.as<> lambdas. Mapping the canonical param name onto the same ordinal fixes every self-type-param site at once. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… fix) embind keys method overloads by arg-count -> arg-signature only; the `this` type is not in the key. When a derived class registers an override of a method whose overload dispatcher / per-signature map is inherited from a base, ensureOverloadTable / ensureOverloadSignatureTable did not fork an own copy, so the write landed in the base's shared structures. With a base method overridden by multiple subclasses, all of them shared one per-signature slot and the last registrant won -- so the base and non-overriding siblings dispatched to the wrong invoker and threw "Expected null or instance of <sibling>, got an instance of <base>". Both helpers now fork inherited overload structures so each registering class owns its own dispatcher / per-signature maps (tagged ocjsSignatureOwner); writes never escape to a base. Non-inherited paths are unchanged. This is distinct from the sub-2b constructor sibling-aliasing handled by predicates/sibling_aliasing.py (that is within one class; this is across inheritance). Regenerated src/patches/libembind-overloading.patch against the vendored stock-emscripten pristine snapshot (round-trips exactly) and updated libembind-overloading.expected.sha256 to the new post-patch result. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@marwndev is attempting to deploy a commit to the TauCAD Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Regenerate build-configs/full*.yml via scripts/enumerate-symbols.py now that the
bindgen template-discovery change + the bindgen-filters.yaml un-exclusions make
the math_Vector-dependent surface bindable. The three configs share one bindings
list (only emcc flags differ); each is the verbatim enumerator output.
Adds vs the previous full.yml:
- walking-line approximators (BRepApprox/GeomInt TheComputeLine{,Bezier}),
2D tangent-constraint solvers (GccAna_*/Geom2dGcc_*/GccEnt), and the ProjLib
projection package — previously excluded only because math_VectorBase<double>
was unbound;
- 6 IMeshData_*Handle the previous (stale) full.yml had missed; the enumerator
emits these independently of this change.
math_VectorBase<double> is not emitted as a standalone symbol by the enumerator,
so it is not listed here; the classes that consume it are.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
70d1fd7 to
c068b04
Compare
|
Thanks for the contribution @marwndev , I'll review and seek to incorporate these changes over the next few days! The math APIs are something I hadn't got a chance to properly exercise, so it's great you have on your downstream. I'll look to add additional smoke tests for these APIs too. Cheers Richard |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/ocjs_bindgen/discover.py (1)
461-467: 🎯 Functional Correctness | 🟠 MajorMandatory fix: Use
_extract_template_argsto validate template parameters and prevent zero-argument base bindings.The current logic relies on string parsing of
canonical.spellingto derive template arguments. For aliased typedefs, libclang's rendering can strip parameters (e.g.,math_VectorBaseinstead ofmath_VectorBase<double>), resulting in empty argument lists and the generation of uncompilable zero-argument bindings for base classes.Query the AST directly using
_extract_template_args(canonical)to ensure theargslist reflects the actual template parameters. If no arguments remain after extraction, skip the entity to avoid admitting raw base classes likemath_VectorBase.container, args = parsed canonical_container = CONTAINER_ALIASES.get(container, container) + # Prefer AST extraction over string parsing to handle typedef aliases correctly. + extracted_args = _extract_template_args(canonical) + if extracted_args: + args = extracted_args if canonical_container not in ADMIT_TEMPLATE_CONTAINERS: continue + # Reject base classes with zero template arguments. + if not args: continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ocjs_bindgen/discover.py` around lines 461 - 467, The template-argument detection in discover.py is relying on parsing canonical.spelling, which can drop aliased typedef parameters and admit zero-argument base bindings. Update the template validation path around _parse_template_spelling and the canonical_container check to use _extract_template_args(canonical) as the source of args, then skip the entity whenever the extracted args list is empty so raw base classes like math_VectorBase are not admitted.src/patches/libembind-overloading.patch (4)
20-23: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrefer exact signature matches before optional wildcards.
Because optional slots return
truebefore exact primitive/class checks, overloads likeoptional<T>andTwith the same arity become registration-order dependent. Defer wildcard matches until no exact key matches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/patches/libembind-overloading.patch` around lines 20 - 23, The overload matching in the embind patch is treating optional fields as an early wildcard, which can make same-arity overloads registration-order dependent. Update the matching logic around the fieldType lookup in the overload resolution code so exact primitive/class signature checks are evaluated before any optional wildcard acceptance, and only fall back to the optional path when no exact registered type matches.
528-532: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSeed the raw signature for first static methods.
The instance-method path sets
unboundTypesHandler.signature; this first static-method path does not. If a same-arity overload promotes the unresolved function,ensureOverloadSignatureTablestores the first entry underundefined.Proposed fix
if (undefined === proto[methodName]) { // This is the first function to be registered with this name. unboundTypesHandler.argCount = argCount-1; + unboundTypesHandler.signature = rawSignatureString; proto[methodName] = unboundTypesHandler;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/patches/libembind-overloading.patch` around lines 528 - 532, The first static-method registration path in the overload handling logic is missing the raw signature initialization that the instance-method path already sets on unboundTypesHandler. Update the proto[methodName] first-registration branch in the overloading patch so it seeds the signature for static methods before storing the handler, using the same mechanism as the instance-method path, to prevent ensureOverloadSignatureTable from recording the first overload under undefined when a same-arity overload is promoted.
433-442: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard final constructor signature collisions.
Line 406 checks raw type IDs, but Line 441 stores the resolved JS signature. Distinct C++ overloads such as
intanddoubleboth resolve tonumber, so the later constructor silently overwrites the earlier one.Proposed fix
if (undefined !== classType.registeredClass.constructor_body[argCount - 1].func) { classType.registeredClass.constructor_body[argCount - 1].func = func; } + if (undefined !== classType.registeredClass.constructor_body[argCount - 1].signatures[signatureString]) { + throw new BindingError(`Cannot register multiple constructors with identical javascript types of parameters for class '${classType.name}'!`); + } classType.registeredClass.constructor_body[argCount - 1].signatures[signatureString] = func; classType.registeredClass.constructor_body[argCount - 1].signaturesArray.push(signatureArray);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/patches/libembind-overloading.patch` around lines 433 - 442, The constructor overloading logic in libembind-overloading.patch is still allowing resolved JavaScript signatures to collide, so distinct C++ overloads like int and double both map to number and overwrite each other. Update the constructor registration path around craftInvokerFunction and constructor_body so that final signature storage detects duplicate cppTypeToJsType-derived signature strings before writing to signatures or signaturesArray, and preserve both overloads by keeping separate entries or preventing the later one from replacing the earlier one.
506-508: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDon’t silently replace same-class overloads with the same JS signature.
These paths unconditionally assign
signatures[signatureString]. That is valid for an inherited override, but same-class overloads whose C++ types collapse to the same JS type will dispatch to the last registered invoker. Preserve enough owner/class metadata to allow inherited overrides while throwing for same-class duplicate JS signatures.Also applies to: 565-567
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/patches/libembind-overloading.patch` around lines 506 - 508, The overload registration in the embind patch is overwriting same-class entries in the overload table when two C++ overloads collapse to the same JS signature. Update the registration logic around the overloadTable/signatures handling to distinguish same-class duplicates from inherited overrides by using owner/class metadata, so inherited methods can still replace as intended while duplicate JS signatures declared on the same class are rejected instead of silently dispatching to the last invoker. Apply the same fix in the other mirrored overload registration block referenced by this patch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/ocjs_bindgen/discover.py`:
- Around line 461-467: The template-argument detection in discover.py is relying
on parsing canonical.spelling, which can drop aliased typedef parameters and
admit zero-argument base bindings. Update the template validation path around
_parse_template_spelling and the canonical_container check to use
_extract_template_args(canonical) as the source of args, then skip the entity
whenever the extracted args list is empty so raw base classes like
math_VectorBase are not admitted.
In `@src/patches/libembind-overloading.patch`:
- Around line 20-23: The overload matching in the embind patch is treating
optional fields as an early wildcard, which can make same-arity overloads
registration-order dependent. Update the matching logic around the fieldType
lookup in the overload resolution code so exact primitive/class signature checks
are evaluated before any optional wildcard acceptance, and only fall back to the
optional path when no exact registered type matches.
- Around line 528-532: The first static-method registration path in the overload
handling logic is missing the raw signature initialization that the
instance-method path already sets on unboundTypesHandler. Update the
proto[methodName] first-registration branch in the overloading patch so it seeds
the signature for static methods before storing the handler, using the same
mechanism as the instance-method path, to prevent ensureOverloadSignatureTable
from recording the first overload under undefined when a same-arity overload is
promoted.
- Around line 433-442: The constructor overloading logic in
libembind-overloading.patch is still allowing resolved JavaScript signatures to
collide, so distinct C++ overloads like int and double both map to number and
overwrite each other. Update the constructor registration path around
craftInvokerFunction and constructor_body so that final signature storage
detects duplicate cppTypeToJsType-derived signature strings before writing to
signatures or signaturesArray, and preserve both overloads by keeping separate
entries or preventing the later one from replacing the earlier one.
- Around line 506-508: The overload registration in the embind patch is
overwriting same-class entries in the overload table when two C++ overloads
collapse to the same JS signature. Update the registration logic around the
overloadTable/signatures handling to distinguish same-class duplicates from
inherited overrides by using owner/class metadata, so inherited methods can
still replace as intended while duplicate JS signatures declared on the same
class are rejected instead of silently dispatching to the last invoker. Apply
the same fix in the other mirrored overload registration block referenced by
this patch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa18b161-b2a3-427f-8e68-d6fb466f8d00
📒 Files selected for processing (8)
bindgen-filters.yamlbuild-configs/full.ymlbuild-configs/full_multi.ymlbuild-configs/full_multi_browser.ymlsrc/ocjs_bindgen/ast/template_args.pysrc/ocjs_bindgen/discover.pysrc/patches/libembind-overloading.expected.sha256src/patches/libembind-overloading.patch
💤 Files with no reviewable changes (1)
- bindgen-filters.yaml
Summary
Three related changes that make the
math_Vector/ TKMath template surface bindable, surface the OCCT classes that depend on it, and fix an embind overload-resolution bug exposed along the way.1. bindgen: discover + emit non-NCollection template instantiations
Lets template typedefs outside the NCollection family — e.g.
math_VectorBase<double>(=math_Vector) andmath_MatrixBase<double>(=math_Matrix) — be discovered and compiled.discover.py: addEXTRA_TEMPLATE_CONTAINERS(math_VectorBase,math_MatrixBase) andADMIT_TEMPLATE_CONTAINERS = NCOLLECTION_CONTAINERS | EXTRA(existing behaviour stays a strict subset, so it cannot regress), plus pass‑1 canonical resolution so a method param spelled via a typedef (const math_Vector&) resolves to its canonical container before bailing. Implements the long‑standing generic‑template‑discovery follow‑up.ast/template_args.py: substitute the canonical forward‑declaration template parameter name. OCCT forward‑declarestemplate<typename T> class math_VectorBase;then defines it withTheItemType; libclang renders a method's self‑type param (const math_VectorBase&) using the forward‑decl nameT, whileprocessTemplatekeystemplateArgsfrom the definition, soTwas never substituted and an uncompilablemath_VectorBase<T>leaked intoselect_overload<>signatures and val‑dispatcharg.as<>lambdas. Mapping the canonical param name onto the same ordinal fixes every self‑type‑param site at once.2. libembind: fork inherited overload tables (cross‑inheritance aliasing fix)
embind keys method overloads by arg‑count → arg‑signature only; the
thistype is not in the key. When a derived class registers an override of a method whose overload dispatcher / per‑signature map is inherited from a base,ensureOverloadTable/ensureOverloadSignatureTabledid not fork an own copy, so the write landed in the base's shared structures. With a base method overridden by multiple subclasses, they shared one per‑signature slot and the last registrant won — so the base and non‑overriding siblings dispatched to the wrong invoker and threwExpected null or instance of <sibling>, got an instance of <base>.Concrete case:
BOPAlgo_Builder::Performis overridden by siblingBOPAlgo_Splitterand inherited byBOPAlgo_CellsBuilder; before this fix, bothnew BOPAlgo_CellsBuilder().Perform(...)andnew BOPAlgo_Builder().Perform(...)threwExpected ... BOPAlgo_Splitter, got ... BOPAlgo_Options.Fix: both helpers now fork inherited overload structures so each registering class owns its own dispatcher / per‑signature maps (tagged
ocjsSignatureOwner); writes never escape to a base. Non‑inherited paths are byte‑for‑byte unchanged, so single‑class methods cannot regress. This is distinct from the sub‑2b constructor sibling‑aliasing handled bypredicates/sibling_aliasing.py(that is within one class; this is across inheritance). The change is folded intosrc/patches/libembind-overloading.patch(regenerated against the vendored stock‑emscripten pristine snapshot — round‑trips exactly) with an updatedlibembind-overloading.expected.sha256.3. full builds: bind the math_Vector‑dependent OCCT surface
Now that
math_VectorBase<double>is discoverable + compilable, the classes excluded only because that base was unbound can be re‑included:bindgen-filters.yaml: un‑exclude the walking‑line approximators (BRepApprox/GeomIntTheComputeLine{,Bezier}), the 2D tangent‑constraint solvers (GccAna_*/Geom2dGcc_*/GccEnt), and theProjLibprojection package. (Geom2dGcc_FunctionTanCuCuCustays excluded — an internal undefined‑symbol functor, not public API.)build-configs/full*.yml: add the now‑bindable symbols (math_VectorBase_double+ the re‑included approximators / Gcc solvers / ProjLib classes), regenerated viascripts/enumerate-symbols.py.Validation
BOPAlgo_CellsBuilder.Perform, the 2D Gcc tangent‑constraint solvers, ProjLib projection, and directmath_Vectornumeric use all work. A runtime audit of 157 signature dispatchers across 607 classes shows every one resolves to an ancestor‑or‑self owner (zero sibling mis‑resolution).full.ymlbuild is the authoritative full‑link check.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes