Bug: Variable name collision when wrapping nested containers (map<String, vector<String>>)

[timosachsenberg]

Description

When autowrap generates code for a method that takes a nested container like libcpp_map[String, StringList] (where StringList is vector<String>), it generates buggy Cython code that reuses the same variable name for both the outer and inner container.

Reproduction

Given a C++ method like:

USI buildUSI(const std::map<String, std::vector<String>>& identifier_to_msrunpath,
             const String& dataset_id,
             bool include_interpretation);

With pxd declaration:

USI buildUSI(const libcpp_map[String, StringList]& identifier_to_msrunpath,
             const String& dataset_id,
             bool include_interpretation) except + nogil

Generated Code (buggy)

def _buildUSI_1(self, dict identifier_to_msrunpath, dataset_id, bool include_interpretation):
    cdef libcpp_map[_String, StringList] * v0 = new libcpp_map[_String, StringList]()
    for _loop_key, _loop_value in identifier_to_msrunpath.items():
        cdef libcpp_vector[_String] * v0 = new libcpp_vector[_String]()  # BUG: redefines v0!
        cdef bytes item0
        for item0 in _loop_value:
           v0.push_back(_String(<char *>item0))
        deref(v0)[deref(<_String *>(<String>_loop_key).inst.get())] = deref(v0)
    del v0
    # ...

Error

pyopenms/_pyopenms_8.pyx:8924:17: cdef statement not allowed here

Cython doesn't allow cdef statements inside a loop body after non-cdef statements.

Expected Behavior

The inner vector should use a different variable name (e.g., v1 or _inner_v0):

def _buildUSI_1(self, dict identifier_to_msrunpath, dataset_id, bool include_interpretation):
    cdef libcpp_map[_String, StringList] * v0 = new libcpp_map[_String, StringList]()
    cdef libcpp_vector[_String] * v1  # Declare outside loop
    cdef bytes item0
    for _loop_key, _loop_value in identifier_to_msrunpath.items():
        v1 = new libcpp_vector[_String]()  # Use different name, assign inside loop
        for item0 in _loop_value:
           v1.push_back(_String(<char *>item0))
        deref(v0)[deref(<_String *>(<String>_loop_key).inst.get())] = deref(v1)
        del v1
    # ...

Workaround

Currently using # wrap-ignore on the affected methods.

Environment

• autowrap version: (from pip)
• Cython version: 3.x
• Python: 3.10

[coderabbitai]

📝 CodeRabbit Plan Mode

Generate an implementation plan and prompts that you can use with your favorite coding agent.

✅ Successfully generated implementation plan.

💡 Use @coderabbitai <feedback> to replan

Examples

• Example 1
• Example 2

---

🔗 Related PRs

#216 - Add lookup/membership tests for STL containers and fix variable name collision bug [merged]
#228 - Fix ArrayWrapper inlining to go to pyx only, not pxd [merged]
#237 - Fix cross-module enum class imports for type assertions [merged]

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[coderabbitai]

Coding Plan

Approach

Fix variable name collision in autowrap's code generation when handling nested containers like map<String, vector<String>>. The issue occurs because StdMapConverter passes hardcoded arg_num=0 to nested converters, causing both outer and inner containers to use the same variable name v0, and placing cdef declarations inside loops (illegal in Cython 3.x).

Observations

Autowrap is a Python extension module generator that creates Cython .pyx wrappers from annotated .pxd files. The core code generation lives in ConversionProvider.py which implements a converter registry pattern where each STL container type (vector, map, set, etc.) has a dedicated converter class. These converters generate three pieces of code: input conversion (Python→C++), call fragment, and cleanup code. Variable naming uses arg_num parameter to create unique names like v0, v1, etc. The StdVectorConverter already has a pattern for handling nested conversions using _rec suffixes and topmost_code/bottommost_code callbacks for declaration hoisting.

Assumptions

Assumption 1: Should we use suffix pattern or increment-based unique naming for nested converter arg_num?

Options Considered:

1. Suffix pattern like "%s_val" % arg_num and "%s_key" % arg_num - generates v0_val, v0_key
2. Use existing svec/specialized naming pattern - more targeted but inconsistent
3. Numeric increment tracking across nested calls - requires state management

Chosen Option: 1

Rationale: Suffix pattern aligns with the existing _rec pattern in StdVectorConverter, is predictable, and clearly indicates the variable's role in the generated code.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Create Failing Test Case

Following TDD principles, first create a test that demonstrates the bug exists before implementing any fixes. This test should fail with the current code, proving the issue is real and reproducible.

Create test case for map with vector values that demonstrates the bug

Add a test exercising the map<String, vector<String>> pattern that triggered this bug. This test should initially fail, validating that the bug exists.

• Add test declarations in tests/test_files/ directory following the existing pattern in libcpp_test.pxd for process211-214 tests
• Create a C++ function signature that takes map<string, vector<string>> as a parameter
• Add corresponding Python test in the test suite that attempts to:
  • Generate wrapper code for this nested container pattern
  • Compile the generated code (this should fail with "cdef statement not allowed here" error)
• Run the test to confirm it fails, demonstrating the bug is reproducible

🤖 Prompt for AI agents

Create a failing test case that demonstrates the nested container bug.

Add test declarations and function:
- Add test in `tests/test_files/` following the pattern in `libcpp_test.pxd`
- Create C++ function signature accepting `map<string, vector<string>>`
parameter

Create Python test:
- Add test that generates wrapper code for the nested container pattern
- Attempt to compile the generated code
- Verify the test fails with "cdef statement not allowed here" error, confirming
the bug exists

Phase 2: Fix Variable Naming in StdMapConverter

This phase addresses the root cause of the collision by ensuring nested converters receive unique arg_num values that generate distinct variable names.

Task 1: Modify arg_num passing in StdMapConverter.input_conversion()

Update the delegating converter calls in StdMapConverter to pass unique arg_num values instead of hardcoded 0.

• Locate the value converter delegation in autowrap/ConversionProvider.py around line 991 where input_conversion(tt_value, loop_value, 0) is called
• Change the arg_num parameter from 0 to "%s_val" % arg_num to generate unique names like v0_val
• Locate the key converter delegation around line 1002 and change arg_num from 0 to "%s_key" % arg_num
• Ensure the modified arg_num values propagate correctly to generate variables like v0_val, item0_val instead of v0, item0

Task 2: Handle cdef declaration hoisting for nested conversions

Extract cdef declarations from nested conversion code and place them before the loop to comply with Cython 3.x requirements.

• In the delegating converter path of StdMapConverter.input_conversion(), after receiving (conv_code, call_as, cleanup) from the nested converter
• Parse the returned conv_code to identify lines starting with cdef declarations
• Split the code into declaration lines and body/assignment lines
• Add declaration lines to the main code block BEFORE the loop iteration begins
• Keep only the non-declaration lines (assignments and operations) inside the loop body
• Ensure proper variable lifecycle: declare outside loop, allocate/assign inside loop, deallocate inside loop

🤖 Prompt for AI agents

Fix the variable name collision in `StdMapConverter.input_conversion()` in
`autowrap/ConversionProvider.py`.

Modify nested converter calls:
- Change value converter's `arg_num` parameter from hardcoded `0` to `"%s_val" %
arg_num`
- Change key converter's `arg_num` parameter from hardcoded `0` to `"%s_key" %
arg_num`

Implement cdef declaration hoisting:
- Parse nested converter's returned `conv_code` to extract lines starting with
`cdef `
- Place cdef declarations before the loop structure
- Keep assignment and operation code inside the loop body

Phase 3: Apply Same Fix to LibcppUnorderedMapConverter

The LibcppUnorderedMapConverter has the same hardcoded arg_num=0 issue and needs the identical fix pattern.

Update LibcppUnorderedMapConverter delegating calls

Apply the same arg_num suffix pattern to the unordered_map converter.

• Locate LibcppUnorderedMapConverter.input_conversion() in autowrap/ConversionProvider.py (around lines 2963 and 2976 based on exploration)
• Apply the same "%s_val" % arg_num and "%s_key" % arg_num pattern for nested converter calls
• Implement the same cdef hoisting pattern as in Task 2.2

🤖 Prompt for AI agents

Apply the same fix to `LibcppUnorderedMapConverter.input_conversion()` in
`autowrap/ConversionProvider.py`.

Modify nested converter calls:
- Change value converter's `arg_num` to `"%s_val" % arg_num`
- Change key converter's `arg_num` to `"%s_key" % arg_num`

Implement cdef declaration hoisting:
- Apply the same pattern as Phase 2 Task 2.2 to extract and hoist cdef
declarations

Phase 4: Verify Fix and Regression Testing

Confirm the fix resolves the issue and doesn't break existing functionality.

Task 1: Verify the failing test from Phase 1 now passes

Re-run the test created in Phase 1 to confirm the fix resolves the bug.

• Run the test case created in Task 1.1
• Verify the generated code compiles without "cdef statement not allowed here" errors
• Verify the conversion correctly passes Python dict[str, list[str]] to the C++ function
• Verify round-trip data integrity is maintained

Task 2: Verify existing nested container tests still pass

Ensure the changes don't break existing nested container functionality.

• Run the existing tests in tests/test_wrapped_containers.py that cover nested patterns
• Verify tests/test_code_generator_libcpp.py tests for process211/212/214 still pass
• Check that the existing map<Item, vector<int>> patterns continue to work correctly

🤖 Prompt for AI agents

Verify the fix works and ensure no regressions.

Verify Phase 1 test now passes:
- Run the test case from Task 1.1
- Confirm generated code compiles without "cdef statement not allowed here"
errors
- Validate Python `dict[str, list[str]]` converts correctly to C++ function
- Verify data integrity through round-trip

Run regression tests:
- Execute all tests in `tests/test_wrapped_containers.py`
- Run `tests/test_code_generator_libcpp.py` tests for process211/212/214
- Confirm `map<Item, vector<int>>` patterns still work

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Create a failing test case that demonstrates the nested container bug.

Add test declarations and function:
- Add test in `tests/test_files/` following the pattern in `libcpp_test.pxd`
- Create C++ function signature accepting `map<string, vector<string>>`
parameter

Create Python test:
- Add test that generates wrapper code for the nested container pattern
- Attempt to compile the generated code
- Verify the test fails with "cdef statement not allowed here" error, confirming
the bug exists
===============================================================================

Task: 2

Fix the variable name collision in `StdMapConverter.input_conversion()` in
`autowrap/ConversionProvider.py`.

Modify nested converter calls:
- Change value converter's `arg_num` parameter from hardcoded `0` to `"%s_val" %
arg_num`
- Change key converter's `arg_num` parameter from hardcoded `0` to `"%s_key" %
arg_num`

Implement cdef declaration hoisting:
- Parse nested converter's returned `conv_code` to extract lines starting with
`cdef `
- Place cdef declarations before the loop structure
- Keep assignment and operation code inside the loop body
===============================================================================

Task: 3

Apply the same fix to `LibcppUnorderedMapConverter.input_conversion()` in
`autowrap/ConversionProvider.py`.

Modify nested converter calls:
- Change value converter's `arg_num` to `"%s_val" % arg_num`
- Change key converter's `arg_num` to `"%s_key" % arg_num`

Implement cdef declaration hoisting:
- Apply the same pattern as Phase 2 Task 2.2 to extract and hoist cdef
declarations
===============================================================================

Task: 4

Verify the fix works and ensure no regressions.

Verify Phase 1 test now passes:
- Run the test case from Task 1.1
- Confirm generated code compiles without "cdef statement not allowed here"
errors
- Validate Python `dict[str, list[str]]` converts correctly to C++ function
- Verify data integrity through round-trip

Run regression tests:
- Execute all tests in `tests/test_wrapped_containers.py`
- Run `tests/test_code_generator_libcpp.py` tests for process211/212/214
- Confirm `map<Item, vector<int>>` patterns still work

💡 Iterate on the plan with: @coderabbitai <feedback>

Example Feedback
- @coderabbitai You can skip phase 3. Add a simple unit test case for phase 2.
- @coderabbitai For assumption 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!

[timosachsenberg]

@coderabbitai fist step should be test that validates that this is broken

[coderabbitai]

I've updated the plan above based on your feedback. Ready to proceed when you are!