Add exclude_inherited_overrides to skip redundant override bindings#104
Open
kwabenantim wants to merge 5 commits into
Open
Add exclude_inherited_overrides to skip redundant override bindings#104kwabenantim wants to merge 5 commits into
kwabenantim wants to merge 5 commits into
Conversation
cppwg emits a pybind11 .def for every overridden virtual, so a method overriding a virtual already wrapped on a wrapped base class is bound twice - on the base and on each derived class. The derived binding is pure duplication: pybind11 inheritance plus C++ virtual dispatch already expose it through the base binding. In pychaste this is pervasive (9,724 .defs, a large fraction redundant). Add a package-level opt-in boolean `exclude_inherited_overrides` (default False, mirroring exclude_default_args) that skips such bindings. Only the .def is dropped; the virtual trampoline (built separately in virtual_overrides) is left intact, so Python subclasses can still override the method. A method is skipped only when a base - wrapped in this package and not class-excluded - declares its own virtual of the same name, argument types and const-ness (the return type is not compared, so covariant-return overrides still match). If the base lists the method in its excluded_methods it is kept, since the base does not wrap it; this is checked via a new decl->class_info map built alongside package_classes in the module writer. Validated on pychaste: 9,724 -> 7,321 .defs (~25% fewer); the dropped derived overrides remain bound on their base and reachable via virtual dispatch, and trampolines are unchanged. Compiling a binding-heavy wrapper (NodeBasedCellPopulation, 195 -> 147 defs) dropped from ~16.3s to ~12.3s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new package-level configuration option, exclude_inherited_overrides, to reduce generated pybind11 wrapper size by skipping redundant .def(...) bindings for virtual overrides that are already exposed via wrapped base-class bindings.
Changes:
- Parse and plumb
exclude_inherited_overridesthroughPackageInfoandPackageInfoParser. - Track wrapped base-class
CppClassInfoobjects package-wide and use that data inCppClassWrapperWriterto decide when to skip redundant override bindings. - Add documentation and tests covering config parsing and the inherited-override detection predicate.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_package_info_parser.py | Adds tests verifying the new package-level option parses correctly and defaults to False. |
| tests/test_class_writer.py | Adds unit tests for the _is_inherited_override predicate behavior under various conditions. |
| README.md | Documents the new exclude_inherited_overrides option and its behavior. |
| cppwg/writers/module_writer.py | Builds package_class_infos (decl → class_info) and passes it into class writers. |
| cppwg/writers/class_writer.py | Implements _is_inherited_override and skips emitting redundant .def(...) bindings when enabled. |
| cppwg/parsers/package_info_parser.py | Adds default + bool conversion for exclude_inherited_overrides. |
| cppwg/info/package_info.py | Stores the new option on PackageInfo and documents it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two correctness fixes so skipping a redundant override never leaves a method uncallable from Python via pybind11 name-shadowing (a derived binding of a name shadows the inherited base binding for that name): - Overload-shadowing guard: only skip an override if every other public overload of the same name on the class is also a skippable override. Otherwise the surviving sibling overload would shadow the base's binding of this one. Factor the per-method test into _overrides_wrapped_base_virtual and add the guard in _is_inherited_override. - Public-access filter: only a public base virtual is actually wrapped, so a protected/private base virtual of the same signature must not make a derived override redundant (else the override, the only binding, would be dropped). Add tests for the mixed-overload cases (kept when a sibling survives, skipped when all overloads are overrides) and the protected-base case. Validated on pychaste: 9,724 -> 7,361 .defs; PlanarPolarisedFarhadifarForce now binds both GetLineTensionParameter overloads, and ~28 overrides of protected base virtuals are correctly kept. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
exclude_inherited_overrides treated any base wrapped anywhere in the package as providing an inherited binding, but bases_block only emits the pybind11 base link for a cross-module base when the module enables cross-module inheritance via imports. Without that link the base binding is not inherited, so skipping the derived override stranded the method. Gate the redundancy check so a base wrapped in another module counts only when imports is set; a same-module base is always linked. No effect on single-module packages (e.g. pychaste) or correctly-configured multi-module ones. Note the deeper follow-up (warn on a wrapped cross-module base with imports unset) in a code comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cppwg/writers/class_writer.py:521
- The inherited-override skip only checks base_info.excluded_methods to decide whether the base actually wraps the matching virtual. But methods can also be excluded from wrapping via other class-level filters (e.g. arg_type_excludes/return_type_excludes/calldef_excludes, private/parent checks inside CppMethodWrapperWriter.exclude()). In those cases the base will not emit a .def, and skipping the derived override here would drop the only binding and make the method unreachable from Python.
# The base declares a matching virtual. Keep the override only if
# the base excludes this method by name (then it is unwrapped
# there, so this override is the sole binding).
base_info = self.package_class_infos.get(base_decl)
if base_info is not None and name in (base_info.excluded_methods or []):
_FakeDerivedDecl.member_functions ignored the `function` predicate, so a
non-public overload leaked into the overload-shadowing scan that production
filters out via access_type_matcher_t("public"). The real matcher can't be
called on a fake decl (it needs a class_t parent), so mirror it by reading
the matcher's target access_type and filtering on each method's own
access_type. Add a test with a protected sibling overload that only passes
when the predicate is honoured.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cppwg/writers/class_writer.py:523
- The redundancy check only consults base_info.excluded_methods to decide whether the base actually provides a Python binding. But base methods can also be excluded by other filters used during wrapper generation (e.g. return_type_excludes/arg_type_excludes in CppMethodWrapperWriter.exclude). In particular, if the base method is excluded by return_type_excludes and the derived method has a covariant return type (return types intentionally aren’t compared), this code can incorrectly treat the base as “wrapped” and skip the derived .def, removing the binding entirely.
Consider determining whether the specific base_method would be emitted by the base wrapper (using the base class’s CppClassInfo + the same exclusion logic as CppMethodWrapperWriter.exclude) before returning True here, rather than checking excluded_methods by name only.
# The base declares a matching virtual. Keep the override only if
# the base excludes this method by name (then it is unwrapped
# there, so this override is the sole binding).
base_info = self.package_class_infos.get(base_decl)
if base_info is not None and name in (base_info.excluded_methods or []):
continue
return True
The inherited-override overload-shadowing guard scanned every public same-name overload, but an overload excluded from wrapping (excluded_methods / return_type_excludes / arg_type_excludes) emits no binding and cannot shadow the base. Counting it kept a redundant derived .def that itself shadowed the base's remaining overloads, leaving them unreachable. Extract the exclusion logic into a reusable CppMethodWrapperWriter.method_is_excluded static method (exclude() now delegates to it) and skip excluded siblings in the guard before treating a surviving overload as forcing the override to be kept. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment on lines
+517
to
+523
| # The base declares a matching virtual. Keep the override only if | ||
| # the base excludes this method by name (then it is unwrapped | ||
| # there, so this override is the sole binding). | ||
| base_info = self.package_class_infos.get(base_decl) | ||
| if base_info is not None and name in (base_info.excluded_methods or []): | ||
| continue | ||
| return True |
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
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.
Fixes #101