GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Fix f-string conversion flag ValueError when compiling AST by youknowone · Pull Request #6534 · RustPython/RustPython

Conversation

youknowone commented Dec 26, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

The ast_from_object for ConversionFlag was using bytecode::ConvertValueOparg::from_op_arg() which only accepts internal oparg values (0, 1, 2, 3, 255), but Python's AST uses ASCII codes ('s'=115, 'r'=114, 'a'=97, -1=None).

This caused compile() on parsed AST with conversion flags to fail with "invalid conversion flag".

@ShaharNaveh You probably knows better about ConvertValueOparg. Will this be okay? Do we need to handle ConvertValueOparg differently?

Fix #6533

Summary by CodeRabbit

Refactor
- Improved internal deserialization logic for better code maintainability.
Chores
- Removed unused import dependencies.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Fix f-string conversion flag ValueError when compiling AST (RustPytho…

ae63092

…n#6533)

The ast_from_object for ConversionFlag was using bytecode::ConvertValueOparg::from_op_arg()
which only accepts internal oparg values (0, 1, 2, 3, 255), but Python's AST uses ASCII codes
('s'=115, 'r'=114, 'a'=97, -1=None).

This caused compile() on parsed AST with conversion flags to fail with "invalid conversion flag".

youknowone requested a review from ShaharNaveh

December 26, 2025 01:17

coderabbitai Bot commented Dec 26, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

📝 Walkthrough

Walkthrough

This PR fixes conversion flag deserialization in f-strings by replacing bytecode-based mapping with direct ASCII-code matching. An unused import is removed, and the deserialization logic now maps -1 or 255 to None, 's' to Str, 'r' to Repr, and 'a' to Ascii, while preserving serialization behavior.

Changes

Cohort / File(s)	Summary
Import cleanup `crates/vm/src/stdlib/ast.rs`	Removed unused import of OpArgType from compiler::core::bytecode module
Conversion flag deserialization `crates/vm/src/stdlib/ast/other.rs`	Replaced ToPrimitive-based deserialization with direct ASCII-code matching for ConversionFlag in ast_from_object; maps 's'→Str, 'r'→Repr, 'a'→Ascii, -1/255→None; removed bytecode dependency; ast_to_object unchanged

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Align f-string related bytecodes with 3.13 #6321: Modifies crates/vm/src/stdlib/ast/other.rs conversion flag handling with different approach (bytecode-based mapping vs. ASCII-code matching).

Poem

🐰 String conversions dance with glee,
ASCII codes now match with spree!
No more flags that fail to parse,
F-strings shine with ASCII ars! ✨

Pre-merge checks and finishing touches

❌ 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%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ 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 clearly and concisely describes the main change: fixing a ValueError related to f-string conversion flags in AST compilation.
Linked Issues check	✅ Passed	The changes directly address the issue by replacing bytecode oparg handling with ASCII code mapping for ConversionFlag deserialization, allowing compile() to handle AST conversion flags correctly.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the ConversionFlag deserialization issue reported in #6533; the import removal in ast.rs is a necessary cleanup.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

youknowone marked this pull request as ready for review

December 26, 2025 02:12

coderabbitai Bot reviewed

Dec 26, 2025

View reviewed changes

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c9bf8df and ae63092.

📒 Files selected for processing (2)

crates/vm/src/stdlib/ast.rs
crates/vm/src/stdlib/ast/other.rs

💤 Files with no reviewable changes (1)

crates/vm/src/stdlib/ast.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

crates/vm/src/stdlib/ast/other.rs

🔇 Additional comments (1)

crates/vm/src/stdlib/ast/other.rs (1)

2-2: LGTM!

The import cleanup correctly removes unused dependencies and retains only SourceFile, which is required for the updated AST conversion logic.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

crates/vm/src/stdlib/ast/other.rs

Comment on lines +14 to +22

		// Python's AST uses ASCII codes: 's', 'r', 'a', -1=None
		// Note: 255 is -1i8 as u8 (ruff's ConversionFlag::None)
		match i32::try_from_object(vm, object)? {
		-1 \| 255 => Ok(Self::None),
		x if x == b's' as i32 => Ok(Self::Str),
		x if x == b'r' as i32 => Ok(Self::Repr),
		x if x == b'a' as i32 => Ok(Self::Ascii),
		_ => Err(vm.new_value_error("invalid conversion flag")),
		}

coderabbitai Bot Dec 26, 2025

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Search for ConversionFlag enum definition
rg -n "enum ConversionFlag" --type rust -A 10