Add wrap-view annotation for in-place member modification #245
Conversation
Adds a new `wrap-view` annotation that generates companion View classes alongside regular wrapper classes. View classes provide direct access to nested C++ objects without creating copies. Key features: - `obj.view()` returns a view for in-place member modification - View properties return nested views (not copies) for wrapped classes - Methods returning `T&` (mutable reference) return views on view class - Views keep parent objects alive via Python reference chain Implementation details: - View classes use raw pointers + Python parent references (avoids Cython issues with shared_ptr aliasing across types) - Methods marked wrap-ignore that return T& are still available on view classes (T& returns don't work on main classes due to Cython limitations, but work on view classes via pointer access) - Forward references handled by pre-computing classes_with_views Includes unit tests for code generation and integration tests that compile actual Cython code and verify runtime behavior. Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Warning Rate limit exceeded@timosachsenberg has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 25 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a "wrap-view" annotation that generates companion View classes for annotated C++ wrappers, updates resolution to preserve certain reference-returning methods, extends code generation to emit view classes/properties/methods, and includes documentation and comprehensive tests for the feature. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as PXD Parser
participant Resolver as DeclResolver
participant Generator as CodeGenerator
participant Compiler as Cython
Parser->>Resolver: Parse declarations with `wrap-view`
Resolver->>Resolver: Set `wrap_view=True` and retain mutable-ref methods
Resolver->>Generator: Provide resolved classes (with wrap_view flag)
Generator->>Generator: Pre-compute `classes_with_views`
Generator->>Generator: Generate main wrapper class code
alt wrap_view enabled
Generator->>Generator: _create_view_class()
Generator->>Generator: _create_view_property_for_attribute() for attributes
Generator->>Generator: _should_generate_view_method() for methods
Generator->>Generator: _create_view_method() to wrap mutable-ref returns
end
Generator->>Compiler: Emit combined PYX/PXD/typing stubs for compilation
Compiler->>Compiler: Compile extension module
sequenceDiagram
participant User as Python User
participant Main as Main Wrapper Class
participant View as View Class
participant Cpp as C++ Object
User->>Main: obj = MainClass()
Main->>Cpp: construct C++ instance
User->>Main: view = obj.view()
Main->>View: instantiate View with _ptr and _parent
User->>View: view.attribute = new_value
View->>Cpp: modify underlying C++ via pointer
User->>Main: copy = obj.get_attr() -- (by-value getter)
Main->>Cpp: return copy
View->>Cpp: modifications persist on parent object
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Verifies that deep views remain valid and functional even after: - Intermediate views are deleted - The root container object is deleted This confirms that all nested views hold a reference to the root object, keeping the entire C++ object tree alive. Co-Authored-By: Claude Opus 4.5 <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @autowrap/CodeGenerator.py:
- Around line 1034-1071: Currently the except blocks around calls to
_create_view_property_for_attribute and _create_view_method swallow all
exceptions and merely log warnings, which can hide failures; change these
handlers to fail fast by either removing the broad try/except or re-raising
after logging (e.g., raise with additional context), so that errors during
generation of view properties/methods for r_class.attribute or r_class.methods
are propagated instead of silently ignored; ensure the log includes the
attribute.cpp_decl.name or method.name and the original exception when
re-raising so view_code and view_typestub_code generation halts with a clear
error.
- Around line 1128-1134: The type stub generation incorrectly uses $name and the
original $py_typing_type when attr_has_view is true; update the stubs.add call
so that when attr_has_view is true the stub property name uses $wrap_as (not
$name) and its type is the view type returned at runtime (e.g., ${AttrBase}View)
instead of $py_typing_type; adjust the logic around stubs.add in
CodeGenerator.py to choose between $py_typing_type and the view type based on
attr_has_view and ensure the stub signature matches the __get__ runtime return.
- Around line 285-293: The pre-computation that builds self.classes_with_views
currently iterates over self.resolved, which only contains declarations from the
current module; change it to iterate over self.all_resolved so cross-module
classes with wrap-view are detected; specifically update the loop in
CodeGenerator that checks isinstance(resolved, ResolvedClass) and
resolved.wrap_view to use self.all_resolved instead of self.resolved so classes
defined in other modules are included in classes_with_views.
In @tests/test_files/wrap_view_test.hpp:
- Around line 52-58: The method Outer::getItemAt(int) accepts negative indices
which leads to signed→unsigned conversion and out-of-bounds access at
items_[index]; fix by validating the input at the top of getItemAt: if index is
negative either throw std::out_of_range (or assert) or handle it explicitly, and
only call items_.resize(index + 1) and return items_[index] after the check so
negative values never reach items_[] or items_.resize with a wrong size.
🧹 Nitpick comments (3)
tests/test_decl_resolver.py (1)
844-844: Prefix unused unpacked variable with underscore.Static analysis correctly identifies that
map_Iis never used in either test function. Convention is to prefix with underscore to indicate intentional discard.♻️ Suggested fix
- (instance,), map_I = DeclResolver.resolve_decls_from_string( + (instance,), _map = DeclResolver.resolve_decls_from_string(Apply to both lines 844 and 860.
Also applies to: 860-860
tests/test_wrap_view.py (1)
43-52: Consider more specific exception handling in fixture cleanup.The static analysis correctly flags the bare
excepton line 51. While cleanup failures are generally non-critical, catching everything silently can hide unexpected issues during test development.♻️ Suggested fix
finally: # Cleanup generated file if os.path.exists(target): try: os.remove(target) - except: - pass + except OSError: + pass # Cleanup failure is non-critical in testsautowrap/CodeGenerator.py (1)
925-1018: View class scaffolding: fix misleading doc + avoid embedded-newlinepxd_comment+ consider nulling_ptrin__dealloc__.
- Docstrings mention “shared_ptr” but implementation is raw pointer + Python parent ref.
pxd_commentcontains a newline without|markers → may render oddly._is_validnever flips to false because_ptris never nulled.Proposed fix (minimal)
- view_docstring = ( + view_docstring = ( "View class for in-place modification of %s members.\n\n" " This class holds a reference to a %s instance and provides\n" " direct access to its members without creating copies. Changes\n" " made through the view are reflected in the original object.\n\n" - " The view keeps the parent object alive via shared_ptr." + " The view keeps the parent object alive via a Python parent reference." ) % (cname, cname) - pxd_comment = ( - "# see .pxd file for cdef of _ptr and _parent" - if self.write_pxd - else "cdef %s* _ptr\n cdef object _parent" % cy_type - ) - view_code.add( """ | |cdef class $view_name: @@ - | $pxd_comment + | # Fields declared in .pxd | + | def __dealloc__(self): + | self._ptr = NULL + | self._parent = None + | | def __repr__(self): @@ """, locals(), ) + if not self.write_pxd: + view_code.add(f" cdef {cy_type}* _ptr") + view_code.add(" cdef object _parent")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
autowrap/CodeGenerator.pyautowrap/DeclResolver.pydocs/README.mdtests/test_code_generator.pytests/test_decl_resolver.pytests/test_files/wrap_view_test.hpptests/test_files/wrap_view_test.pxdtests/test_pxd_parser.pytests/test_wrap_view.py
🧰 Additional context used
🧬 Code graph analysis (4)
tests/test_decl_resolver.py (2)
tests/test_pxd_parser.py (1)
test_wrap_view_annotation(647-661)autowrap/DeclResolver.py (1)
resolve_decls_from_string(303-304)
tests/test_pxd_parser.py (2)
tests/test_decl_resolver.py (1)
test_wrap_view_annotation(842-855)autowrap/PXDParser.py (1)
parse_str(609-618)
autowrap/CodeGenerator.py (2)
autowrap/DeclResolver.py (1)
ResolvedClass(168-225)autowrap/Code.py (2)
add(56-75)Code(41-90)
tests/test_wrap_view.py (1)
autowrap/__init__.py (1)
parse_and_generate_code(106-119)
🪛 Clang (14.0.6)
tests/test_files/wrap_view_test.hpp
[error] 4-4: 'string' file not found
(clang-diagnostic-error)
🪛 markdownlint-cli2 (0.18.1)
docs/README.md
288-288: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
🪛 Ruff (0.14.10)
tests/test_decl_resolver.py
844-844: Unpacked variable map_I is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
860-860: Unpacked variable map_I is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
autowrap/CodeGenerator.py
1043-1043: Do not catch blind exception: Exception
(BLE001)
1066-1066: Do not catch blind exception: Exception
(BLE001)
1324-1324: f-string without any placeholders
Remove extraneous f prefix
(F541)
tests/test_wrap_view.py
43-43: Do not catch blind exception: Exception
(BLE001)
51-51: Do not use bare except
(E722)
51-52: try-except-pass detected, consider logging the exception
(S110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test (==3.2.0, 3.10)
- GitHub Check: test (==3.2.0, 3.11)
- GitHub Check: test (==3.2.0, 3.12)
- GitHub Check: test (==3.2.0, 3.13)
- GitHub Check: test (==3.1.0, 3.10)
- GitHub Check: test (==3.1.0, 3.13)
- GitHub Check: test (==3.1.0, 3.11)
- GitHub Check: test (==3.1.0, 3.12)
🔇 Additional comments (17)
tests/test_decl_resolver.py (1)
842-869: Good test coverage for wrap-view resolution.The tests correctly verify both the positive case (wrap-view annotation sets
wrap_view=True) and the default case (wrap_view=Falsewhen absent). The test structure follows existing patterns in this file.tests/test_pxd_parser.py (1)
647-684: Comprehensive tests for wrap-view annotation parsing.Good coverage testing both standalone
wrap-viewannotation parsing as booleanTrue, and its coexistence with other annotations likewrap-docandwrap-hash. The assertions correctly verify the annotation value types.docs/README.md (2)
162-166: Clear documentation for wrap-view directive.The directive description clearly explains the semantics: main wrapper returns copies, view class allows in-place modification, and views keep parents alive. The bullet points effectively summarize the key behaviors.
288-315: Helpful example demonstrating wrap-view usage.The example effectively shows both the Cython declaration syntax and Python usage patterns, clearly contrasting copy behavior (
outer.inner_member.value = 42) with in-place modification through views (view.inner_member.value = 42). This will help users understand when and how to use the feature.autowrap/DeclResolver.py (2)
180-180: Correctly adds wrap_view attribute to ResolvedClass.The
wrap_viewboolean attribute is properly declared with type hint and initialized from the annotation dictionary with a sensibleFalsedefault. This aligns with the existing pattern for other annotations likewrap_ignore.Also applies to: 201-201
569-576: Key logic change: retain wrap-ignore methods returning mutable references.This change enables view classes to expose methods that return
T&even when they're markedwrap-ignoreon the main class (since Cython can't handle reference returns directly). The conditionresult_type.is_ref and not result_type.is_constcorrectly identifies mutable references, while non-reference and const-reference wrap-ignore methods are still filtered out as intended.tests/test_code_generator.py (4)
1014-1102: Comprehensive test for View class generation.This test correctly verifies the core wrap-view functionality:
- View class (
TestClassView) is generated- Main class has
view()method- View class has expected structure (
_ptr,_parent,_is_valid,__repr__)Good use of assertions with descriptive failure messages.
1105-1151: Good negative test case.Correctly verifies that View classes are NOT generated when
wrap-viewannotation is absent, preventing unintended code generation.
1154-1265: Thorough property and const attribute tests.Tests properly verify:
- Properties are generated for each attribute
- Properties use
_ptr(notinst) for view access- Both getter and setter are generated for mutable attributes
- Const attributes raise
AttributeErroron set attempts
1268-1388: Good coverage for mutable reference methods.Tests verify that:
- Methods returning
T&generate view-returning methods- View methods return appropriate View types (e.g.,
InnerView)- Methods with arguments correctly pass those arguments through
tests/test_files/wrap_view_test.pxd (1)
1-57: Well-designed test fixture for wrap-view integration tests.This PXD file provides good coverage of wrap-view scenarios:
- Basic attributes (
value,name)- Mutable reference getters with
wrap-ignore(getInner(),getItemAt())- Const reference getters (
getConstInner())- Value copy getters (
getInnerCopy())- Multi-level nesting (
Container→Outer→Inner)The inline comments explaining why
wrap-ignoreis needed for reference returns are helpful for maintainability.tests/test_wrap_view.py (1)
55-254: Excellent integration test coverage for wrap-view feature.The test classes comprehensively exercise the wrap-view functionality:
- Basic tests: Verify View classes exist,
view()method works, correct types returned- In-place modification: Verify views modify originals while copies don't
- Mutable ref methods: Verify
T&returns work correctly through views- Lifetime management: Verify views keep parents alive (critical for memory safety)
- Deep nesting: Three-level nesting test (
Container→Outer→Inner)- String attributes: Verify string handling through views
The tests effectively validate both the code generation and runtime behavior.
tests/test_files/wrap_view_test.hpp (1)
4-6: Clang can’t find<string>: likely CI/toolchain config, but please verify.
The header itself looks fine; this error usually means the static-analysis compile command isn’t using a C++ driver/stdlib include paths (or is compiling as C).autowrap/CodeGenerator.py (4)
831-846: Filteringwrap-ignoremethods out of main-class generation looks right.
This aligns with the intent that ref-returning access is provided via the View class, not the main wrapper.
886-889: Hooking view-class emission behindr_class.wrap_viewis a good integration point.
Please double-check behavior forwrap-manual-memoryclasses andwrite_pxd=Falsebuilds (the addedview()usesself.inst.get()).
2735-2738:uintptr_tcimport addition is appropriate for pointer-based__repr__.
Just ensure this remains compatible with the project’s supported Cython/Python toolchains.
1233-1262: Fix missing cleanup handling, arg-name normalization, and overload-method collision in view method generation.In
_create_view_method, three issues prevent correct code generation:
Cleanup never executed:
converter.input_conversion(...)returns a cleanup tuple element, but it's captured and discarded. Unlike other method-generation code in the codebase (constructors, regular methods), cleanups are never collected or applied, risking temporary object leaks.Missing argument-name normalization: The code iterates
for arg_name, arg_type in method.arguments:directly. However,method.argumentscan contain missing or invalid names. Callaugment_arg_names(method)first—as done in_create_overloaded_method_decland regular method wrapping—to normalize unnamed arguments toin_0,in_1, etc.Overloaded methods will collide: The loop
for method in methods:generates multiple method definitions with identical names when overloads exist, causing all but the last to be silently overwritten. Without overload dispatch (pending), skip overloaded view methods entirely.Ruff F541: Line 1321 has
f" Returns a view..."with no{}placeholders—should be a plain string.Suggested fixes
- # Build argument list + # Build argument list (normalize missing names) + args = augment_arg_names(method) arg_names = [] arg_types = [] arg_conversions = Code() call_args = [] + cleanups = [] - for arg_name, arg_type in method.arguments: + for arg_type, arg_name in args: converter = self.cr.get(arg_type) py_type = converter.matching_python_type(arg_type) arg_names.append(arg_name) arg_types.append(py_type) # Generate input conversion conv_code, call_as, cleanup = converter.input_conversion( arg_type, arg_name, len(call_args) ) if conv_code: arg_conversions.add(conv_code) call_args.append(call_as) + cleanups.append(cleanup)- docstring += f" Returns a view for in-place modification instead of a copy.\n" + docstring += " Returns a view for in-place modification instead of a copy.\n"code.add( """ | # Call C++ method and wrap result in view | cdef $view_type v = $view_type.__new__($view_type) | v._ptr = &self._ptr.$cpp_name($call_args_str) | v._parent = self._parent # Propagate parent reference | return v """, locals(), ) + for cleanup in reversed(cleanups): + if not cleanup: + continue + if isinstance(cleanup, (str, bytes)): + cleanup = " %s" % cleanup + code.add(cleanup)In
_create_view_class, skip overloaded methods for now:for method_name, methods in r_class.methods.items(): # Skip constructors (same name as class) if method_name == cname: continue + if len(methods) > 1: + L.warning(f"Skipping overloaded view method {cname}.{method_name}; overload dispatch not yet implemented") + continue for method in methods:Likely an incorrect or invalid review comment.
- Add note about T& methods needing wrap-ignore on main class - Document chaining behavior with example - Document lifetime guarantees (views keep root alive) - Add example showing views outlive their source objects Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_wrap_view.py (2)
43-44: Consider catching more specific exceptions or logging the failure reason.While catching broad
Exceptionis often discouraged, in test fixtures where compilation can fail for many reasons (missing compiler, incompatible Cython version, etc.), this pattern is acceptable. However, you might want to log the exception details for debugging failed CI runs.♻️ Optional: Add logging for better diagnostics
except Exception as e: + import logging + logging.warning(f"wrap_view_module compilation failed: {e}") pytest.skip(f"Compilation failed: {e}")
46-52: Avoid bareexceptclause - use specific exception or at minimumException.The bare
except:catches all exceptions includingKeyboardInterruptandSystemExit, which can mask serious issues during test cleanup. Additionally, silently passing on cleanup errors can hide important diagnostic information.♻️ Proposed fix
finally: # Cleanup generated file if os.path.exists(target): try: os.remove(target) - except: - pass + except OSError: + pass # File may be locked or already removed
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/README.mdtests/test_wrap_view.py
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/README.md
288-288: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
🪛 Ruff (0.14.10)
tests/test_wrap_view.py
43-43: Do not catch blind exception: Exception
(BLE001)
51-51: Do not use bare except
(E722)
51-52: try-except-pass detected, consider logging the exception
(S110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test (==3.2.0, 3.10)
- GitHub Check: test (==3.1.0, 3.12)
- GitHub Check: test (==3.1.0, 3.13)
- GitHub Check: test (==3.2.0, 3.12)
- GitHub Check: test (==3.2.0, 3.11)
- GitHub Check: test (==3.2.0, 3.13)
- GitHub Check: test (==3.1.0, 3.11)
- GitHub Check: test (==3.1.0, 3.10)
🔇 Additional comments (5)
docs/README.md (2)
162-166: Clear and informative directive documentation.The wrap-view directive documentation is well-structured, covering all key aspects: companion class generation, in-place modification semantics, the requirement for
wrap-ignoreonT&methods, and lifetime guarantees.
288-336: Comprehensive example section with good coverage of edge cases.The wrap-view example section effectively demonstrates:
- Basic usage with
wrap-ignorefor reference-returning methods- The critical distinction between copy vs. in-place modification
- Chaining syntax
- Lifetime semantics with practical code examples
This will help users understand when and how to use the feature.
tests/test_wrap_view.py (3)
55-82: Well-structured basic tests for view class generation.The tests properly verify that View classes are generated with expected naming conventions and that main classes expose the
view()method.
192-250: Excellent lifetime tests - critical for verifying reference chain safety.These tests thoroughly verify the core safety guarantee of the wrap-view feature: that views keep parent objects alive. The
test_deep_view_survives_intermediate_and_root_deletiontest is particularly valuable as it confirms the reference chain behavior described in the PR objectives.
1-288: Comprehensive test coverage for the wrap-view feature.The test module provides good coverage across all key aspects:
- Class generation and naming
- In-place modification semantics
- Mutable reference method behavior
- Lifetime/safety guarantees
- Deep nesting scenarios
- String attribute handling
This aligns well with the PR objectives and documentation.
- Use all_resolved instead of resolved for cross-module wrap-view detection - Fail fast on view property/method generation errors instead of swallowing - Fix type stubs to show View types when attr_has_view is true - Add bounds check for negative index in test file getItemAt() Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Nice! This would work already. I am wondering if it would be nicer to add the annotation to the method to say that the return type should be wrapped as a View. |
|
ah true. so you would suggest having the annotation on each method, and this will mean that that the returned type needs to wrapped as a view? |
|
I mean it is up for discussion. My initial idea was to add it to the method. Then autowrap has to check which methods are annotated with "view" and generate additional View wrapper classes/proxies for there return types accordingly. EDIT: Yes I think it should work with chaining, too. |
|
How will python user figure they got a view? will this be annotated in the Class or Method name? and should we decide between no view XOR view or do should we have both if it is annotated (of course only works if we have either a view() function or a generated name)? |
|
In a unified class? By doing object.parent != None. Or you expose a member function that does that (e.g. isView()). |
Summary
wrap-viewannotation that generates companion${ClassName}ViewclassesT&(mutable reference) return views instead of copies on view classesFeatures
Implementation Details
wrap-ignorethat returnT&are still available on view classes (T& returns don't work on main classes due to Cython limitations)classes_with_viewsbefore class generationTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.