Skip to content

Accept __index__-conforming objects for compile() flags / optimize#7728

Merged
youknowone merged 1 commit into
RustPython:mainfrom
changjoon-park:fix-compile-index-args
Apr 29, 2026
Merged

Accept __index__-conforming objects for compile() flags / optimize#7728
youknowone merged 1 commit into
RustPython:mainfrom
changjoon-park:fix-compile-index-args

Conversation

@changjoon-park
Copy link
Copy Markdown
Contributor

@changjoon-park changjoon-park commented Apr 29, 2026

Background

CPython's compile() (Python/Python-ast.c) accepts any object with __index__ for the flags and optimize arguments — not just exact int. RustPython's CompileArgs typed both fields as OptionalArg<PyIntRef>, so a class with only __index__ raised TypeError: Expected type 'int' but '<X>' found. during arg binding.

bpo-36907's regression test (test_call.test_fastcall_clearing_dict) exercises exactly this: an IntWithDict.__index__ that mutates self.kwargs mid-call. CPython parses both args via __index__, doesn't crash even when the kwargs dict is cleared between binding and use.

Repro

import gc
class IntWithDict:
    __slots__ = ['kwargs']
    def __init__(self, **kwargs): self.kwargs = kwargs
    def __index__(self):
        self.kwargs.clear()
        gc.collect()
        return 0

x = IntWithDict(optimize=IntWithDict())
compile("pass", "", "exec", x, **x.kwargs)
# CPython 3.14: succeeds
# RustPython:   TypeError: Expected type 'int' but 'IntWithDict' found.   (before this PR)

Fix

Change flags and optimize in CompileArgs from OptionalArg<PyIntRef> to OptionalArg<ArgPrimitiveIndex<i32>>. ArgPrimitiveIndex (already used by range, slice, bytes.__mul__, hex, oct, ...) calls __index__ then converts to the primitive in one step. Three call sites simplify from .map_or(Ok(default), |v| v.try_to_primitive(vm))? to .map_or(default, |v| v.value).

Tests unmasked

  • test_call.FastCallTests.test_fastcall_clearing_dict

Verification

  • CPython 3.14.4 byte-identical for the bpo-36907 repro
  • Normal cases unchanged: compile("...", "<t>", "eval", 0), compile(..., flags=0), compile(..., optimize=1)
  • Non-int, non-__index__ objects still raise TypeError (now from try_index instead of strict PyIntRef check)
  • No regressions across test_call, test_compile, test_int, test_index, test_descr, test_typing, test_ast (~1,571 tests)
  • Cross-module grep for the same TODO marker text — test_termios.py has 4 instances of Expected type 'int' but 'FileIO' found., but those are a different fix path (file-descriptor protocol via fileno(), not __index__); out of scope.

Summary by CodeRabbit

  • Refactor
    • Improved compile() function's argument handling to accept a broader range of numeric types, enhancing compatibility with Python conventions.

CPython's compile() (Python/Python-ast.c) accepts any object with
__index__ for the flags and optimize arguments. RustPython's CompileArgs
typed both fields as OptionalArg<PyIntRef>, so a class with only
__index__ raised 'TypeError: Expected type int but X found' during arg
binding.

bpo-36907's regression test (test_call.test_fastcall_clearing_dict)
exercises exactly this: an IntWithDict.__index__ that mutates
self.kwargs mid-call. CPython parses both args via __index__ and doesn't
crash even when the kwargs dict is cleared between binding and use.

Switch flags and optimize to OptionalArg<ArgPrimitiveIndex<i32>>, the
same helper already used by range, slice, bytes.__mul__, hex, oct, etc.
ArgPrimitiveIndex calls try_index (= __index__ protocol) and converts
to the requested primitive in one step, so the three call sites in
compile() simplify from .map_or(Ok(d), |v| v.try_to_primitive(vm))? to
.map_or(d, |v| v.value).

Unmasks test_call.FastCallTests.test_fastcall_clearing_dict.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 29, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: d39145a7-1f98-4e85-81d3-c74a44156529

📥 Commits

Reviewing files that changed from the base of the PR and between 3f718f9 and c440ae7.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_call.py is excluded by !Lib/**
📒 Files selected for processing (1)
  • crates/vm/src/stdlib/builtins.rs

📝 Walkthrough

Walkthrough

The compile() builtin's argument parsing for flags and optimize is refactored to use ArgPrimitiveIndex<i32> instead of OptionalArg<PyIntRef>, enabling flexible conversion from __index__-compatible objects. Conversion logic is simplified by directly accessing parsed primitive values with appropriate defaults when arguments are missing.

Changes

Cohort / File(s) Summary
Argument Parsing Refactor
crates/vm/src/stdlib/builtins.rs
Updated compile() builtin to parse flags and optimize using ArgPrimitiveIndex<i32> for improved type handling. Simplified conversion logic by removing error-propagating mapping and directly reading .value with defaults (-1 for optimize, 0 for flags). Added CPython compatibility comment.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 With arguments parsed oh so neat,
The compile builtin skips the heat,
No more wrapping, no more fuss,
Just __index__ objects for us!
Simpler code, defaults so clean,
Best refactor I have seen! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: enabling the compile() builtin to accept index-conforming objects for the flags and optimize parameters, matching CPython behavior.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[ ] test: cpython/Lib/test/test_call.py

dependencies:

dependent tests: (no tests depend on call)

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

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