Fix f-string conversion flag ValueError when compiling AST #6534
Conversation
…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".
📝 WalkthroughWalkthroughThis PR fixes conversion flag deserialization in f-strings by replacing bytecode-based mapping with direct ASCII-code matching. An unused import is removed, 8000 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
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/vm/src/stdlib/ast.rscrates/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 runningcargo fmtto 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.
| // 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")), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for ConversionFlag enum definition
rg -n "enum ConversionFlag" --type rust -A 10Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Search for ast_to_object implementation in the file
rg -n "fn ast_to_object" crates/vm/src/stdlib/ast/other.rs -A 5Repository: RustPython/RustPython
Length of output: 1205
🏁 Script executed:
# Check if there are any tests for ConversionFlag round-trip conversion
rg -n "ConversionFlag" crates/vm/src/stdlib/ast/ --type rust -B 2 -A 2Repository: RustPython/RustPython
Length of output: 1254
🏁 Script executed:
# Check cargo.toml for ruff dependency
rg -n "ruff" Cargo.toml -A 2 -B 2Repository: RustPython/RustPython
Length of output: 1048
🏁 Script executed:
# Search for ConversionFlag in ruff-related files or comments
rg -n "ConversionFlag" --type rust -B 3 -A 3 | head -50Repository: RustPython/RustPython
Length of output: 3179
🏁 Script executed:
# Check if there's any documentation or external reference about ConversionFlag values
find . -name "*.rs" -type f | xargs grep -l "ruff::ConversionFlag" | head -5Repository: RustPython/RustPython
Length of output: 137
🏁 Script executed:
# Search for usage of ast_to_object to understand if it's used for Python round-trips
rg -n "ast_to_object" crates/vm/src/stdlib/ast/ --type rust -B 2 -A 2 | head -40Repository: RustPython/RustPython
Length of output: 2701
🏁 Script executed:
# Check if there are tests for AST conversion
fd "test" crates/vm/src/stdlib/ast/ --type rustRepository: RustPython/RustPython
Length of output: 236
🏁 Script executed:
# Look at the unparse.rs file since it imports ConversionFlag - check how it uses it
head -50 crates/codegen/src/unparse.rsRepository: RustPython/RustPython
Length of output: 1461
🏁 Script executed:
# Search for where FormattedValue is used and how conversion field is accessed
rg -n "FormattedValue" crates/vm/src/stdlib/ast/ -B 2 -A 5Repository: RustPython/RustPython
Length of output: 7673
🏁 Script executed:
# Check how conversion flag is used in string formatting code
rg -n "conversion" crates/vm/src/stdlib/ast/string.rs -B 2 -A 2Repository: RustPython/RustPython
Length of output: 1670
🏁 Script executed:
# Search for any documentation about Python's ConversionFlag values
rg -n "conversion.*flag\|conversion.*-1\|conversion.*255" --ignore-case -iRepository: RustPython/RustPython
Length of output: 47
Fix ast_to_object to output -1 for ConversionFlag::None to preserve round-trip compatibility.
The round-trip conversion is real: FormattedValue uses conversion.ast_to_object() to serialize to Python AST objects (line 318 in string.rs), and later deserializes via ast_from_object() (line 340). While ast_from_object correctly accepts both -1 and 255 for None, the reverse conversion breaks round-trip compatibility:
ast_to_objectoutputs 255 (viaself as u8) forConversionFlag::None- Python's
_ast.FormattedValueexpects -1 for no conversion - Result: Python AST objects get
conversion=255instead of-1, breaking compatibility
Fix by special-casing None to output -1 as a signed integer:
fn ast_to_object(self, vm: &VirtualMachine, _source_file: &SourceFile) -> PyObjectRef {
let val = match self {
Self::None => -1i32,
_ => self as u8 as i32,
};
vm.ctx.new_int(val).into()
}🤖 Prompt for AI Agents
In crates/vm/src/stdlib/ast/other.rs around lines 14 to 22, ast_to_object
currently emits 255 for ConversionFlag::None (via casting to u8) which breaks
round-trip with Python AST expecting -1; change ast_to_object to special-case
Self::None and produce -1i32, otherwise cast the enum to u8 then to i32, and
return vm.ctx.new_int(val).into() so the Python AST sees -1 for None and other
flags remain unchanged.
I don't think that we do |
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
Chores
✏️ Tip: You can customize this high-level summary in your review settings.