-
Notifications
You must be signed in to change notification settings - Fork 1.4k
PEP 750 tstring #6744
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PEP 750 tstring #6744
Conversation
📝 WalkthroughWalkthroughAdds full support for template strings (t-strings / PEP 750) end-to-end: AST representation, codegen emitting new bytecode ops, new bytecode instructions, VM execution handlers, and runtime types PyInterpolation and PyTemplate. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant Codegen as Code Generator
participant Bytecode
participant VM as Virtual Machine
participant Runtime as Builtins
Parser->>Codegen: Emit TString AST node
Codegen->>Codegen: compile_expr_tstring()
Codegen->>Codegen: collect literal parts and interpolations
Codegen->>Bytecode: emit BuildInterpolation (per interpolation)
Codegen->>Bytecode: emit BuildTemplate
VM->>VM: execute BuildInterpolation
VM->>Runtime: construct PyInterpolation(value, expr, conversion, format_spec)
Runtime-->>VM: PyInterpolation object
VM->>VM: execute BuildTemplate
VM->>Runtime: construct PyTemplate(strings_tuple, interpolations_tuple)
Runtime-->>VM: PyTemplate object
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin tstring |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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: 2
🤖 Fix all issues with AI agents
In `@crates/vm/src/builtins/template.rs`:
- Around line 297-304: The __reduce__ for PyTemplateIter (method reduce)
currently returns (TemplateIterType, (template,)) which fails because
PyTemplateIter lacks a Constructor; update reduce to return the same
reconstruction pattern used by PyTemplate by returning the helper function
_template_unpickle as the callable and the template tuple as its args (i.e.,
return (_template_unpickle, (zelf.template.clone(),))) so unpickling calls the
helper instead of trying to call TemplateIterType directly.
In `@crates/vm/src/frame.rs`:
- Around line 706-755: PyTemplate::new and PyInterpolation::new currently skip
the invariant checks performed in their py_new implementations; update these
simple constructors to validate the same invariants: in PyTemplate::new, assert
strings.len() == interpolations.len() + 1 (use a debug_assert! or panic with a
clear message if you prefer non-debug enforcement) and in PyInterpolation::new
validate that conversion is either vm.ctx.none() or a single-character string in
{'s','r','a'} (mirror the checks and error text from interpolation.rs:py_new and
template.rs:py_new), failing fast with a clear message when violated so
directly-constructed objects cannot be malformed.
🧹 Nitpick comments (2)
crates/vm/src/stdlib/ast/string.rs (2)
329-356: Avoid quadratic iteration when consuming f-string parts.
iter_mut().nth(i)inside the loop re-traverses the slice each iteration. Iterating directly keeps this O(n) and reads cleaner.♻️ Proposed refactor
- for i in 0..value.as_slice().len() { - let part = core::mem::replace(value.iter_mut().nth(i).unwrap(), default_part.clone()); + for part in value.iter_mut() { + let part = core::mem::replace(part, default_part.clone());
575-589: Avoid quadratic iteration intstring_to_object.Same pattern as above—direct iteration over
value.iter_mut()avoids O(n²) traversal.♻️ Proposed refactor
- for i in 0..value.as_slice().len() { - let tstring = core::mem::replace(value.iter_mut().nth(i).unwrap(), default_tstring.clone()); + for tstring in value.iter_mut() { + let tstring = core::mem::replace(tstring, default_tstring.clone());
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/_opcode_metadata.pyis excluded by!Lib/**
📒 Files selected for processing (12)
crates/codegen/src/compile.rscrates/codegen/src/symboltable.rscrates/compiler-core/src/bytecode/instruction.rscrates/vm/src/builtins/interpolation.rscrates/vm/src/builtins/mod.rscrates/vm/src/builtins/template.rscrates/vm/src/frame.rscrates/vm/src/stdlib/ast/expression.rscrates/vm/src/stdlib/ast/pyast.rscrates/vm/src/stdlib/ast/string.rscrates/vm/src/types/zoo.rscrates/vm/src/vm/vm_ops.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style usingcargo fmtto format Rust code
Always run clippy to lint 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/types/zoo.rscrates/vm/src/stdlib/ast/expression.rscrates/codegen/src/symboltable.rscrates/vm/src/builtins/mod.rscrates/vm/src/vm/vm_ops.rscrates/vm/src/stdlib/ast/pyast.rscrates/vm/src/builtins/interpolation.rscrates/compiler-core/src/bytecode/instruction.rscrates/vm/src/builtins/template.rscrates/vm/src/frame.rscrates/codegen/src/compile.rscrates/vm/src/stdlib/ast/string.rs
🧠 Learnings (1)
📚 Learning: 2026-01-14T14:52:10.778Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.778Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/vm/src/builtins/mod.rscrates/vm/src/frame.rs
🧬 Code graph analysis (10)
crates/vm/src/types/zoo.rs (1)
crates/vm/src/builtins/template.rs (1)
iter(234-236)
crates/vm/src/stdlib/ast/expression.rs (1)
crates/vm/src/stdlib/ast/string.rs (1)
tstring_to_object(565-596)
crates/codegen/src/symboltable.rs (2)
crates/vm/src/builtins/interpolation.rs (1)
format_spec(128-130)crates/vm/src/frame.rs (1)
element(650-650)
crates/vm/src/builtins/mod.rs (1)
crates/vm/src/stdlib/sre.rs (1)
template(140-165)
crates/vm/src/vm/vm_ops.rs (2)
crates/vm/src/protocol/mapping.rs (1)
slots(108-110)crates/vm/src/protocol/sequence.rs (1)
slots(134-136)
crates/vm/src/builtins/interpolation.rs (1)
crates/vm/src/types/zoo.rs (1)
init(109-203)
crates/compiler-core/src/bytecode/instruction.rs (1)
crates/compiler-core/src/bytecode/oparg.rs (2)
from(25-27)from(68-70)
crates/vm/src/builtins/template.rs (1)
crates/vm/src/types/slot.rs (1)
sequence_downcast(1982-1984)
crates/vm/src/frame.rs (2)
crates/vm/src/builtins/template.rs (2)
interpolations(103-105)strings(98-100)crates/vm/src/builtins/interpolation.rs (4)
format_spec(128-130)expression(118-120)value(113-115)conversion(123-125)
crates/vm/src/stdlib/ast/string.rs (1)
crates/vm/src/stdlib/ast/expression.rs (16)
ast_to_object(11-53)ast_to_object(148-165)ast_to_object(191-208)ast_to_object(234-254)ast_to_object(285-302)ast_to_object(327-344)ast_to_object(370-390)ast_to_object(421-445)ast_to_object(477-491)ast_to_object(511-528)ast_to_object(554-571)ast_to_object(597-617)ast_to_object(648-666)ast_to_object(694-708)ast_to_object(728-742)ast_to_object(761-775)
🔇 Additional comments (16)
crates/vm/src/stdlib/ast/expression.rs (1)
39-39: TString conversion is now wired in cleanly.This removes the unimplemented path and routes t-strings through the dedicated converter as expected.
crates/codegen/src/symboltable.rs (1)
1444-1456: T-string interpolation scanning mirrors f-strings correctly.Nice alignment with the f-string path, including format-spec traversal.
crates/compiler-core/src/bytecode/instruction.rs (2)
267-279: Opcode definitions and decode registration are consistent.Good to see BuildTemplate/BuildInterpolation added to both the enum and custom opcode list.
Also applies to: 441-445
802-809: Stack-effect and disassembly updates look consistent.The stack deltas and disassembly names align with the new template/interpolation ops.
Also applies to: 1004-1005
crates/vm/src/builtins/interpolation.rs (1)
49-203: Interpolation constructor and protocol impls look solid.Validation, reduce, equality, hashing, and repr are consistent and well-scoped.
crates/vm/src/builtins/template.rs (1)
17-197: Template normalization, concat, and iteration logic look consistent.Constructor normalization and concat boundary handling read clean, and the iterator skips empty strings as intended.
Also applies to: 199-267, 269-293, 308-341, 343-346
crates/vm/src/stdlib/ast/string.rs (1)
366-563: TemplateStr/Interpolation AST wiring looks consistent.Nice alignment with the existing JoinedStr/FormattedValue flow.
crates/vm/src/builtins/mod.rs (1)
39-41: New interpolation/template exports are wired cleanly.Also applies to: 81-82
crates/vm/src/types/zoo.rs (1)
6-9: TypeZoo registration for template/interpolation looks complete.Also applies to: 96-98, 194-196, 252-253
crates/vm/src/vm/vm_ops.rs (1)
383-387:sequence_unchecked()is safe for non-sequence operands—it's a thin wrapper without assertions.The method simply wraps the object in a
PySequencestruct without validation. Safety is guaranteed by subsequent.slots().concat.load()checks, which returnOption<T>and are guarded byif let Some(f)conditionals. Non-sequences safely returnNonefrom.load(), preventing execution.crates/vm/src/stdlib/ast/pyast.rs (2)
935-936: LGTM! Module registration is correctly placed and follows established patterns.The new nodes are appropriately registered after
JoinedStrandFormattedValue, grouping related string expression types together.
451-463: LGTM! New AST nodes correctly implement PEP 750 template string support.The
TemplateStrandInterpolationnode definitions follow the establishedimpl_node!macro pattern and correctly mirror their f-string counterparts (JoinedStrandFormattedValue). Field definitions align with PEP 750:
TemplateStr.values: sequence of constants and interpolationsInterpolation: includesvalue,str(original expression source text),conversion, andformat_specfields, matching the PEP 750 specification exactlycrates/codegen/src/compile.rs (4)
27-33: Import updates look good.
The addedExprTString/TStringimports align with the new t-string codegen path.
6285-6287: TString expression dispatch is wired correctly.
RoutingExpr::TStringinto the new compiler path is the right integration point.
7495-7514: Stack contract and oparg encoding are correctly implemented.The BuildTemplate/BuildInterpolation stack order and bit layout match across compiler and VM:
- BuildTemplate receives [strings_tuple, interpolations_tuple] after the swap (line 7511)
- BuildInterpolation oparg encoding (conversion << 2) | has_format_spec matches VM expectations
- Conversion bits 2+ encode None (0), Str (1), Repr (2), Ascii (3); has_format_spec is bit 0
7526-7531: The suggested code references a non-existent field and will not compile.
InterpolatedStringLiteralElement(the type oflit) has only three fields:node_index,range, andvalue. It does not have aflagsfield. The flags information is stored on the parentTStringstruct, not on individual literal elements.For this change to work correctly,
compile_tstring_intowould need to be modified to:
- Accept the
tstring.flagsparameter- Use
tstring.flags(notlit.flags) when callingparse_fstring_literal_element- Update the function signature accordingly
The core concern about parity with f-string surrogate handling is reasonable, but the proposed implementation is incorrect.
Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
incomplete but enough for #6718
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.