Add test for scoped enum forward declaration bug#221
Conversation
This test exposes a bug where scoped enums (enum class) generate an incorrect forward declaration in multi-module scenarios: - PXD file generates: `cdef class Status: pass` (Cython extension type) - PYX file generates: `class Status(_PyEnum): ...` (Python class) This mismatch occurs because: 1. CodeGenerator.py always generates `cdef class` forward declarations for all enums (lines 447-454) 2. But scoped enums are implemented as regular Python classes that inherit from _PyEnum (lines 475-497) When write_pxd is True (multi-module scenario) and another module does `from EnumModule cimport Status`, Cython expects a cdef class but gets a regular Python class, which can cause compilation issues. The test creates a two-module scenario with a scoped enum in one module that is used by another module to verify this behavior.
|
Warning Rate limit exceeded@timosachsenberg has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 33 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 (3)
WalkthroughAdds tests and fixtures to ensure scoped C++ enums (enum class) implemented as Python Enum subclasses do not produce cdef class forward declarations in generated .pxd; adjusts codegen to skip emitting cdef forward declarations for scoped enums. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/test_code_generator.py(2 hunks)tests/test_files/enum_forward_decl/ConsumerModule.hpp(1 hunks)tests/test_files/enum_forward_decl/ConsumerModule.pxd(1 hunks)tests/test_files/enum_forward_decl/EnumModule.hpp(1 hunks)tests/test_files/enum_forward_decl/EnumModule.pxd(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_files/enum_forward_decl/ConsumerModule.hpp (1)
tests/test_files/enum_forward_decl/EnumModule.hpp (2)
s(23-25)s(23-23)
⏰ 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.11)
- GitHub Check: test (==3.2.0, 3.13)
- GitHub Check: test (==3.2.0, 3.10)
- GitHub Check: test (==3.2.0, 3.12)
- GitHub Check: test (==3.1.0, 3.13)
- GitHub Check: test (==3.1.0, 3.12)
- GitHub Check: test (==3.1.0, 3.10)
- GitHub Check: test (==3.1.0, 3.11)
🔇 Additional comments (6)
tests/test_code_generator.py (2)
43-43: LGTM!The import of
autowrap.Mainis correctly added to support the new test function that usescollect_manual_code,register_converters,generate_code, andrun_cython.
475-601: Well-designed test that correctly reproduces the bug.The test logic is sound and serves a dual purpose:
- Reproduces the bug: Detects when scoped enums have mismatched declarations (
cdef classin .pxd vsclass X(_PyEnum)in .pyx) and fails explicitly with a clear diagnostic message- Regression test: Once the bug is fixed in CodeGenerator.py, this test will pass and prevent future regressions
The test properly:
- Sets up a multi-module scenario with EnumModule and ConsumerModule
- Parses declarations and generates code with
write_pxd=True(viaall_declparameter)- Inspects generated .pxd/.pyx content to detect the mismatch
- Cleans up resources in the finally block
tests/test_files/enum_forward_decl/ConsumerModule.pxd (1)
1-15: LGTM!This Cython declaration file correctly sets up the test scenario by importing the
Statusenum fromEnumModulevia cimport and using it inStatusConsumermethod signatures. This is exactly what's needed to expose the forward declaration bug when another module depends on a scoped enum.tests/test_files/enum_forward_decl/EnumModule.hpp (1)
1-33: LGTM!This minimal C++ header correctly defines a scoped enum (
enum class Status) and a class that uses it. The scoped enum is the key element needed to reproduce the forward declaration bug, as scoped enums are wrapped as Python classes inheriting from_PyEnumrather than as Cython extension types.tests/test_files/enum_forward_decl/ConsumerModule.hpp (1)
1-28: LGTM!This C++ header correctly demonstrates cross-module enum usage by including
EnumModule.hppand using theStatusenum in method signatures. TheStatusConsumerclass provides both consumption (isOk) and production (getDefaultStatus) of enum values, which is appropriate for testing the forward declaration scenario.tests/test_files/enum_forward_decl/EnumModule.pxd (1)
1-21: LGTM! Clear documentation of the bug trigger.This Cython declaration file correctly sets up the condition that triggers the bug. The
cpdef enum class Statuson line 13 declares a scoped enum that is NOT attached to any class (as noted in the comment on lines 11-12). This is precisely what causes CodeGenerator.py to emit acdef classforward declaration in the .pxd file while implementing it as a regular Pythonclassinheriting from_PyEnumin the .pyx file.The clear comments help document the test scenario for future maintainers.
| `from EnumModule cimport Status` would expect a cdef class but get a Python class. | ||
| """ | ||
| import shutil | ||
| import subprocess |
There was a problem hiding this comment.
Remove unused import.
The subprocess module is imported but never used in the test function.
Apply this diff:
- import shutil
- import subprocess
+ import shutilCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/test_code_generator.py around line 495, the file imports the subprocess
module but it is not used anywhere in the test; remove the unused "import
subprocess" line to clean up imports and avoid linter warnings. Ensure no other
references to subprocess exist in the file before removing the import.
The bug was that scoped enums (enum class) were incorrectly generating a `cdef class` forward declaration in the .pxd file, but their actual implementation in the .pyx file was a regular Python `class` inheriting from `_PyEnum`. This mismatch could cause issues in multi-module scenarios. Fix: Only generate `cdef class` forward declarations for unscoped enums, which are actually implemented as `cdef class` in the .pyx file. Scoped enums don't need a forward declaration since they're regular Python classes. Also improved the test to use regex for more precise matching.
…test - Add comprehensive docstring explaining the bug, its impact, and the fix - Document the test setup (two-module scenario) - Update test file comments to explain their role in the regression test
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/test_code_generator.py (1)
494-495: Remove unused import.The
subprocessmodule is imported but never used in the test function.Apply this diff:
import shutil - import subprocess
🧹 Nitpick comments (1)
tests/test_code_generator.py (1)
567-589: Consider verifying actual cross-module enum usage.The test currently checks for the declaration mismatch by inspecting generated file content, which correctly detects the bug. To strengthen the test, consider also verifying that
ConsumerModulecan successfully import and use theStatusenum fromEnumModuleafter the fix is applied. This would validate the end-to-end behavior in addition to the implementation details.For example, after the Cython compilation step, you could:
# After line 598, add cross-module usage verification: # Import the compiled modules and verify enum can be used across modules enum_mod = autowrap.Utils.compile_and_import( "EnumModule", ["EnumModule.pyx"], autowrap_include_dirs, ) consumer_mod = autowrap.Utils.compile_and_import( "ConsumerModule", ["ConsumerModule.pyx"], autowrap_include_dirs, ) # Verify that Status enum is accessible and usable assert hasattr(enum_mod, 'Status')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
autowrap/CodeGenerator.py(1 hunks)tests/test_code_generator.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- autowrap/CodeGenerator.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.2.0, 3.13)
- GitHub Check: test (==3.2.0, 3.12)
- GitHub Check: test (==3.2.0, 3.10)
- GitHub Check: test (==3.2.0, 3.11)
- GitHub Check: test (==3.1.0, 3.11)
- GitHub Check: test (==3.1.0, 3.10)
- GitHub Check: test (==3.1.0, 3.12)
- GitHub Check: test (==3.1.0, 3.13)
🔇 Additional comments (1)
tests/test_code_generator.py (1)
43-43: LGTM!The import is correctly added to support the new test function's use of
collect_manual_code,register_converters, andrun_cython.
|
Wow. Not bad. |
3 participants
This test exposes a bug where scoped enums (enum class) generate
an incorrect forward declaration in multi-module scenarios:
cdef class Status: pass(Cython extension type)class Status(_PyEnum): ...(Python class)This mismatch occurs because:
cdef classforward declarationsfor all enums (lines 447-454)
inherit from _PyEnum (lines 475-497)
When write_pxd is True (multi-module scenario) and another module
does
from EnumModule cimport Status, Cython expects a cdef classbut gets a regular Python class, which can cause compilation issues.
The test creates a two-module scenario with a scoped enum in one
module that is used by another module to verify this behavior.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.