Fix cross-module enum class imports for type assertions #237
Conversation
📝 WalkthroughWalkthroughAdds generation of cross-module Python imports for scoped/wrapped enums, updates enum-to-Python conversion to use Python enum wrappers for scoped enums, invokes the import generation during .pyx output, and adds unit/integration tests and fixtures. A duplicate definition of the new method was introduced accidentally. Changes
Sequence Diagram(s)(Skipped — changes are code-generation and tests; they do not introduce a new multi-component runtime control flow that benefits from a sequence diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
autowrap/CodeGenerator.py
🧰 Additional context used
🧬 Code graph analysis (1)
autowrap/CodeGenerator.py (2)
autowrap/Code.py (2)
Code(41-90)add(56-75)autowrap/DeclResolver.py (1)
ResolvedEnum(128-139)
⏰ 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.1.0, 3.10)
- GitHub Check: test (==3.2.0, 3.11)
- GitHub Check: test (==3.2.0, 3.13)
- GitHub Check: test (==3.2.0, 3.10)
- GitHub Check: test (==3.1.0, 3.11)
- GitHub Check: test (==3.2.0, 3.12)
- GitHub Check: test (==3.1.0, 3.12)
- GitHub Check: test (==3.1.0, 3.13)
🔇 Additional comments (2)
autowrap/CodeGenerator.py (2)
281-281: LGTM! Appropriate placement for cross-module enum imports.The call is correctly positioned after
create_foreign_cimports()and before code generation, ensuring enum imports are available when needed.
2040-2067: Inconsistency: AI summary mentions duplicate method, but only one definition found.The AI summary states "A second identical public method create_foreign_enum_imports(self) was added (duplicated definition)", but only one definition is visible in the provided code (lines 2040-2067). If a duplicate exists elsewhere, it should be removed to avoid conflicts.
This commit fixes two issues with enum class support in autowrap: 1. Cross-module imports (CodeGenerator.py): When autowrap splits classes across multiple output modules, enum classes may be defined in one module but used in type assertions in another. Added create_foreign_enum_imports() to generate necessary Python imports. 2. Return type conversion (ConversionProvider.py): When C++ methods return enum class types, the generated code was returning raw int values instead of wrapping them in the Python enum class. This caused type mismatches when passing return values to setter methods. Before: py_result = <int>_r After: py_result = _PyEnumName(<int>_r) Fixes #236 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
timosachsenberg
force-pushed
the
fix-cross-module-enum-imports
branch
from
bde92f7 to
0a38f40
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @autowrap/CodeGenerator.py:
- Around line 2040-2067: In create_foreign_enum_imports(), restrict Python-level
imports to scoped enums only (i.e., check resolved.is_scoped or equivalent)
since unscoped enums use Cython cimport; pick the correct import symbol name
based on wrap_attach (use "_Py" + resolved.name when resolved.wrap_attach is
true, otherwise use resolved.name), keep the wrap_ignore guard, and append the
generated Code() to self.top_level_pyx_code (not self.top_level_code) so the
import lands in the .pyx output.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
autowrap/CodeGenerator.pyautowrap/ConversionProvider.py
⏰ 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.1.0, 3.13)
- GitHub Check: test (==3.2.0, 3.13)
- GitHub Check: test (==3.2.0, 3.12)
- GitHub Check: test (==3.1.0, 3.12)
- GitHub Check: test (==3.2.0, 3.10)
- GitHub Check: test (==3.1.0, 3.11)
- GitHub Check: test (==3.2.0, 3.11)
- GitHub Check: test (==3.1.0, 3.10)
🔇 Additional comments (2)
autowrap/ConversionProvider.py (1)
434-442: LGTM! Correct enum output conversion for scoped enums.The implementation properly wraps scoped enum values in their Python enum class wrapper while maintaining backward compatibility for non-scoped enums. The naming logic (
_Pyprefix for wrap-attach, original name otherwise) is consistent withmatching_python_type_full(lines 400-403) andtype_check_expression(lines 410-416).autowrap/CodeGenerator.py (1)
2040-2067: No action needed. Verification confirms only one definition ofcreate_foreign_enum_imports(self)exists at line 2040 in autowrap/CodeGenerator.py. There is no duplicate method definition.Likely an incorrect or invalid review comment.
|
Check coderabbit and if possible add tests. |
…tination Address three issues identified by CodeRabbit review: 1. Add scoped enum check: Only scoped enums (Python IntEnum classes) need Python imports. Unscoped enums are cdef classes and use cimport. 2. Fix naming logic: Use _Py prefix only when wrap-attach annotation is present, otherwise use the original enum name. 3. Fix output destination: Append to top_level_pyx_code (for .pyx) instead of top_level_code (for .pxd) since these are Python imports. Add tests: - Unit test: test_create_foreign_enum_imports() tests the method in isolation - Integration test: test_cross_module_scoped_enum_imports() tests full multi-module scenario with both wrap-attach and non-wrap-attach enums Co-Authored-By: Claude Opus 4.5 <[email protected]>
tests/test_code_generator.py
Outdated
| inc_dirs=autowrap_include_dirs, extra_opts=None, out=m_filename | ||
| ) | ||
|
|
||
| print("Test passed: Cross-module scoped enum imports work correctly") |
There was a problem hiding this comment.
Can we import the modules in the end and do some real python tests?
Otherwise looks great
Extend test_cross_module_scoped_enum_imports to: - Use cythonize for proper package compilation with relative imports - Run runtime tests that verify enums work across module boundaries - Test Priority enum (no wrap-attach) and Task.TaskStatus (with wrap-attach) - Test isinstance type checking with wrong enum types - Skip runtime tests gracefully if build environment is incomplete The generated code verification (import statements, isinstance checks) runs unconditionally; runtime tests are skipped if Cython compilation fails due to missing dependencies like AutowrapRefHolder. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/test_code_generator.py (4)
766-769: Weak assertion may mask false positives.The condition
"Priority" not in pxd_generated_code or "cimport" in pxd_generated_codecould pass incorrectly if the pxd contains both an erroneousfrom ... import Prioritystatement AND an unrelatedcimportstatement elsewhere. Consider a stricter check:Suggested stricter assertion
- assert "Priority" not in pxd_generated_code or "cimport" in pxd_generated_code, ( - f"Enum Python imports should not be in top_level_code (pxd). " - f"Generated pxd code:\n{pxd_generated_code}" - ) + # Ensure no Python-style imports for enums appear in pxd (cimports are fine) + assert "from module1 import Priority" not in pxd_generated_code, ( + f"Enum Python imports should not be in top_level_code (pxd). " + f"Generated pxd code:\n{pxd_generated_code}" + )
945-951: Minor cleanup: unused variable and subprocess best practice.The
stdoutvariable is captured but never used. Also, whileshell=Trueis safe here since the command uses onlysys.executable, consider using a list form for clarity and to avoid shell parsing.Suggested cleanup
result = subprocess.Popen( - f"{sys.executable} setup.py build_ext --force --inplace", - shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE + [sys.executable, "setup.py", "build_ext", "--force", "--inplace"], + stdout=subprocess.PIPE, stderr=subprocess.PIPE ) - stdout, stderr = result.communicate() + _, stderr = result.communicate()
996-1006: Replaceassert Falsewithpytest.fail()for robustness.Using
assert Falsecan be stripped bypython -O. Since this is a pytest test,pytest.fail()is more appropriate and semantically clearer.Suggested fix
# Runtime Test 7: Type checking works - wrong enum type should raise try: runner.isHighPriority(EnumProvider.Task.TaskStatus.PENDING) - assert False, "Should have raised AssertionError for wrong enum type" + pytest.fail("Should have raised AssertionError for wrong enum type") except AssertionError as e: assert "wrong type" in str(e) try: runner.isCompleted(EnumProvider.Priority.HIGH) - assert False, "Should have raised AssertionError for wrong enum type" + pytest.fail("Should have raised AssertionError for wrong enum type") except AssertionError as e: assert "wrong type" in str(e)
975-981: Stylistic: explicit boolean comparisons.Static analysis flags
== True/== Falsecomparisons. While acceptable in tests for clarity, the idiomatic Python style uses truthy/falsy checks. This is a minor nitpick.Optional: idiomatic boolean checks
- assert runner.isHighPriority(EnumProvider.Priority.HIGH) == True - assert runner.isHighPriority(EnumProvider.Priority.LOW) == False + assert runner.isHighPriority(EnumProvider.Priority.HIGH) + assert not runner.isHighPriority(EnumProvider.Priority.LOW) assert runner.getDefaultPriority() == EnumProvider.Priority.MEDIUM # Runtime Test 4: TaskRunner can use Task.TaskStatus enum from EnumProvider - assert runner.isCompleted(EnumProvider.Task.TaskStatus.COMPLETED) == True - assert runner.isCompleted(EnumProvider.Task.TaskStatus.PENDING) == False + assert runner.isCompleted(EnumProvider.Task.TaskStatus.COMPLETED) + assert not runner.isCompleted(EnumProvider.Task.TaskStatus.PENDING)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_code_generator.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_code_generator.py (2)
autowrap/CodeGenerator.py (2)
CodeGenerator(106-2227)create_foreign_enum_imports(2040-2072)autowrap/DeclResolver.py (1)
ResolvedEnum(128-139)
🪛 Ruff (0.14.10)
tests/test_code_generator.py
945-945: subprocess call with shell=True identified, security issue
(S602)
949-949: Unpacked variable stdout is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
975-975: Avoid equality comparisons to True; use runner.isHighPriority(EnumProvider.Priority.HIGH): for truth checks
Replace with runner.isHighPriority(EnumProvider.Priority.HIGH)
(E712)
976-976: Avoid equality comparisons to False; use not runner.isHighPriority(EnumProvider.Priority.LOW): for false checks
Replace with not runner.isHighPriority(EnumProvider.Priority.LOW)
(E712)
980-980: Avoid equality comparisons to True; use ...: for truth checks
Replace comparison
(E712)
981-981: Avoid equality comparisons to False; use not ...: for false checks
Replace comparison
(E712)
998-998: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
1004-1004: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
⏰ 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.12)
- GitHub Check: test (==3.2.0, 3.11)
- GitHub Check: test (==3.1.0, 3.12)
- GitHub Check: test (==3.1.0, 3.10)
- GitHub Check: test (==3.1.0, 3.11)
- GitHub Check: test (==3.2.0, 3.13)
- GitHub Check: test (==3.1.0, 3.13)
- GitHub Check: test (==3.2.0, 3.10)
🔇 Additional comments (1)
tests/test_code_generator.py (1)
645-774: Comprehensive test coverage for cross-module enum imports.These tests thoroughly validate the new
create_foreign_enum_imports()functionality:
- Unit test (
test_create_foreign_enum_imports): Covers wrap-attach prefix naming, non-wrapped enums, unscoped enum exclusion, wrap-ignore exclusion, and correct placement in pyx vs pxd.- Integration test (
test_cross_module_scoped_enum_imports): End-to-end validation with actual Cython compilation and runtime type checking across module boundaries.The graceful skip on compilation failure (line 951) is a good pattern for CI environments with incomplete toolchains.
Also applies to: 777-1018
Test VerificationVerified that the unit test correctly catches the bug by temporarily reverting to the buggy code:
The test validates all three fixes:
Error message with buggy code: |
Fix for Cross-Module Scoped Enum Type CheckingI encountered and fixed an issue with scoped enums (enum class) in multi-module builds (e.g., pyOpenMS with ProblemScoped enums are wrapped as Python IntEnum classes (e.g., assert isinstance(in_0, _PySpectrumType)the Python class Issue 1 - Compile-time: Cython needs the name to be declared/imported, otherwise you get: Issue 2 - Runtime: Adding module-level imports like: from ._pyopenms_3 import _PySpectrumTypecauses circular import errors. The modules form an import chain during initialization (1 → 8 → 7 → ... → 2), and when module 2 tries to import from module 3, module 3 hasn't finished initializing yet. SolutionInstead of module-level imports, use assert isinstance(in_0, globals().get('_PySpectrumType', int))This approach:
ImplementationConversionProvider.py - Modified # Before:
return "isinstance(%s, %s)" % (argument_var, name)
# After:
return "isinstance(%s, globals().get('%s', int))" % (argument_var, name)CodeGenerator.py - The ResultsAll 491 pyOpenMS unit tests pass, including 82 static method tests that specifically test enum conversion functions. |
PR #237 introduced module-level Python imports for scoped enum classes across modules. While this fixed the Cython compile-time error, it causes circular import errors at runtime when modules form import chains during initialization. Problem: - Module 1 starts loading and imports from Module 8 - Module 8 imports from Module 7, etc. - When Module 2 tries to import from Module 3, Module 3 hasn't finished initializing yet → ImportError Solution: - Make create_foreign_enum_imports() a no-op (remove module-level imports) - Use globals().get() for late binding in type assertions: `isinstance(x, globals().get('_PyEnumName', int))` This approach: - Compiles successfully (globals().get() is always valid Python) - Resolves the enum class at runtime after all modules are loaded - Falls back to 'int' which works for IntEnum (inherits from int) Also updated output_conversion() to use the same pattern for wrapping return values in the enum class. Tested with pyOpenMS (491 tests pass including 82 static method tests). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…#238) PR #237 introduced module-level Python imports for scoped enum classes across modules. While this fixed the Cython compile-time error, it causes circular import errors at runtime when modules form import chains during initialization. Problem: - Module 1 starts loading and imports from Module 8 - Module 8 imports from Module 7, etc. - When Module 2 tries to import from Module 3, Module 3 hasn't finished initializing yet → ImportError Solution: - Make create_foreign_enum_imports() a no-op (remove module-level imports) - Use globals().get() for late binding in type assertions: `isinstance(x, globals().get('_PyEnumName', int))` This approach: - Compiles successfully (globals().get() is always valid Python) - Resolves the enum class at runtime after all modules are loaded - Falls back to 'int' which works for IntEnum (inherits from int) Also updated output_conversion() to use the same pattern for wrapping return values in the enum class. Tested with pyOpenMS (491 tests pass including 82 static method tests). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.5 <[email protected]>
Changes in this release: - Fix inheritance documentation with Sphinx RST syntax and method grouping (#231) - Fix circular import issue with cross-module scoped enum type checking (#238) - Fix cross-module enum class imports for type assertions (#237) - Add test for enum-based overload resolution (#234) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Summary
When autowrap splits classes across multiple output modules for parallel compilation, enum classes (e.g.,
_PyPolarity,_PySpectrumType) may be defined in one module but used inisinstance()type assertions in another module. This causes Cython compilation errors like:Solution
This PR adds
create_foreign_enum_imports()which generates the necessary Python-level imports for enum classes across modules:This is separate from
create_foreign_cimports()which only handles Cython-level cimports. The_PyEnumNameclasses are pure PythonIntEnumsubclasses and need regular Python imports.Testing
Tested with pyOpenMS build on OpenMS develop branch (PR converting plain enums to
enum class):SpectrumSettings.setType(),IonSource.setPolarity(), etc.Fixes #236
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.