Skip to content

Share ctypes aggregate parameter conversion - #9657

Merged
youknowone merged 1 commit into
RustPython:mainfrom
youknowdot:fix/ctypes-aggregate-conversion-20261103
Nov 5, 2026
Merged

youknowone merged 1 commit into
RustPython:mainfrom
youknowdot:fix/ctypes-aggregate-conversion-20261103

Conversation

@youknowdot

@youknowdot youknowdot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Share aggregate from_param conversion across array, structure, union, and pointer fallback paths, removing duplicated parsing and adaptation logic.
  • Propagate _as_parameter_ lookup errors and use the existing recursion guard. Recursive adaptation stays in the generic converter rather than restarting pointer-only adaptations.
  • Match array/byref prototype checks and use initialized pointer metadata instead of mutable _type_ attributes. Snapshot metadata before callback-capable type checks.

No simple.rs, function.rs, tests, snippets, or workflows are changed. This composes with the separate scalar conversion work in #9654.

Validation

  • Focused VM library cargo check and strict threading Clippy passed on the final source.
  • Isolated CPython 3.15.0 reference probes cover lookup errors, cycles, direct versus wrapped pointer inputs, array/byref compatibility, mutated _type_, and incomplete pointers.
  • Independent source review, rustfmt, diff checks, and normal commit hooks passed.
  • RustPython runtime tests, full workspace tests, C-API tests, and non-Linux execution were not run locally because the native execution guard is unavailable. Hosted CI is still required.

Aggregate CArg inputs with no initialized prototype take a safe TypeError path. This intentionally does not reproduce CPython's NULL-prototype crash.

Reference: CPython 3.15 ctypes conversion.

AI assistance

OpenAI Codex assisted with source analysis, implementation, reference-case preparation, review, and this description. Model version was not exposed by this runtime.

Summary by CodeRabbit

  • Bug Fixes
    • CTypes arrays, structures, and unions now use consistent parameter conversion, including support for matching instances and values exposed through _as_parameter_.
    • Pointer parameters continue to accept None and compatible pointer or array instances; values without an initialized pointer prototype now raise a TypeError.

Reuse generic from_param adaptation, preserve lookup errors and recursion bounds, and follow initialized prototype metadata for pointer and byref inputs.

Assisted-by: OpenAI Codex:model version not exposed
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

ctypes parameter conversion is centralized in PyCData::from_param. Array, structure, and union converters delegate to it. The pointer converter retains prototype-specific handling and delegates other values to the shared converter.

Changes: ctypes Parameter Conversion

Layer / File(s) Summary
Shared parameter conversion
crates/vm/src/stdlib/_ctypes/base.rs
PyCData::from_param handles matching instances, compatible CArgObject values, recursive _as_parameter_ conversion with recursion tracking, and type errors.
Converter integration
crates/vm/src/stdlib/_ctypes/array.rs, crates/vm/src/stdlib/_ctypes/structure.rs, crates/vm/src/stdlib/_ctypes/union.rs, crates/vm/src/stdlib/_ctypes/pointer.rs
Array, structure, and union converters delegate conversion to PyCData::from_param. The pointer converter uses initialized prototype metadata, retains its None, by-reference, and compatible pointer or array handling, and delegates other values.

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Refactor

Possibly related PRs

  • RustPython/RustPython#9685: Invokes and caches argtypes converters during foreign-call argument conversion, which uses the ctypes argument-conversion contract.

Suggested reviewers: youknowone

Merge Risk: Minimal · up to 93bac

The shared ctypes conversion has no established merge-blocking issue. Merge after normal checks pass.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title "Share ctypes aggregate parameter conversion" matches the main change. The PR moves from_param conversion for arrays, structures, unions, and the pointer fallback path into a shared PyCData:…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title "Share ctypes aggregate parameter conversion" matches the main change. The PR moves from_param conversion for arrays, structures, unions, and the pointer fallback path into a shared PyCData::from_param in base.rs, and removes the duplicated per-type logic. The title names the subject (ctypes aggregate parameter conversion) and the action (sharing it), and it is consistent with the PR objectives. It does not cover details such as the recursion guard, metadata snapshotting, or the TypeError path for prototype-less CArg inputs, which is acceptable for a title.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 5 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 7e1c3e9 and 93bac84.

Files selected for processing: 5
  • crates/vm/src/stdlib/_ctypes/array.rs
  • crates/vm/src/stdlib/_ctypes/base.rs
  • crates/vm/src/stdlib/_ctypes/pointer.rs
  • crates/vm/src/stdlib/_ctypes/structure.rs
  • crates/vm/src/stdlib/_ctypes/union.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

@youknowone
youknowone marked this pull request as ready for review November 4, 2026 12:39
@youknowone
youknowone enabled auto-merge (squash) November 4, 2026 12:39
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-04T12:44:39.885005Z 93bac84 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@youknowone
youknowone merged commit 1be6414 into RustPython:main Nov 5, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants