nitpicks #5886
nitpicks #5886
Conversation
WalkthroughThis update applies widespread stylistic and structural refactoring across multiple modules. The primary changes include converting suitable methods to Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VM
participant IOClass
User->>VM: Call IO method (e.g., readable, writable)
VM->>IOClass: Invoke const fn readable/writable/seekable
IOClass-->>VM: Return boolean result (at compile time if possible)
VM-->>User: Return result
Possibly related PRs
Suggested reviewers
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vm/src/stdlib/ast/constant.rs (1)
175-236: Consider preserving string/bytes prefixes when round-trippingWhile not introduced in this diff, the current
ast_from_objectpath always setsprefix: StringLiteralPrefix::Emptyfor strings/bytes, losing information (u,b,ur, etc.).
If full fidelity is a goal, grab the original token’s prefix here (or surface it later).
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
derive-impl/src/pyclass.rs(10 hunks)vm/src/readline.rs(3 hunks)vm/src/stdlib/ast.rs(3 hunks)vm/src/stdlib/ast/constant.rs(5 hunks)vm/src/stdlib/ast/exception.rs(1 hunks)vm/src/stdlib/ast/expression.rs(46 hunks)vm/src/stdlib/ast/module.rs(8 hunks)vm/src/stdlib/ast/node.rs(2 hunks)vm/src/stdlib/ast/operator.rs(4 hunks)vm/src/stdlib/ast/other.rs(4 hunks)vm/src/stdlib/ast/parameter.rs(3 hunks)vm/src/stdlib/ast/pattern.rs(17 hunks)vm/src/stdlib/io.rs(52 hunks)vm/src/stdlib/marshal.rs(4 hunks)vm/src/stdlib/posix.rs(9 hunks)vm/src/stdlib/sre.rs(7 hunks)vm/src/stdlib/symtable.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.rs`: Follow the default rustfmt code style (`cargo fmt` to format) Always ...
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your 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
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
vm/src/readline.rsvm/src/stdlib/ast/module.rsvm/src/stdlib/ast/operator.rsvm/src/stdlib/ast/other.rsvm/src/stdlib/ast/parameter.rsvm/src/stdlib/ast/pattern.rsvm/src/stdlib/ast/exception.rsvm/src/stdlib/ast/expression.rsvm/src/stdlib/ast/node.rsvm/src/stdlib/marshal.rsderive-impl/src/pyclass.rsvm/src/stdlib/posix.rsvm/src/stdlib/ast.rsvm/src/stdlib/sre.rsvm/src/stdlib/symtable.rsvm/src/stdlib/ast/constant.rsvm/src/stdlib/io.rs
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Follow the default rustfmt code style (`cargo fmt` to format)
vm/src/readline.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
vm/src/stdlib/ast/module.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
vm/src/stdlib/ast/pattern.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
vm/src/stdlib/ast/expression.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
derive-impl/src/pyclass.rs (2)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
vm/src/stdlib/posix.rs (4)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
Learnt from: moreal
PR: RustPython/RustPython#5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Learnt from: moreal
PR: RustPython/RustPython#5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
vm/src/stdlib/ast.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
vm/src/stdlib/sre.rs (4)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Learnt from: moreal
PR: RustPython/RustPython#5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Learnt from: moreal
PR: RustPython/RustPython#5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
vm/src/stdlib/symtable.rs (3)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Learnt from: moreal
PR: RustPython/RustPython#5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Learnt from: moreal
PR: RustPython/RustPython#5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
vm/src/stdlib/io.rs (3)
undefined
<retrieved_learning>
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
</retrieved_learning>
<retrieved_learning>
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
</retrieved_learning>
<retrieved_learning>
Learnt from: moreal
PR: #5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
</retrieved_learning>
🧬 Code Graph Analysis (3)
vm/src/stdlib/ast/operator.rs (2)
vm/src/class.rs (1)
static_type(22-26)vm/src/stdlib/ast/node.rs (10)
ast_from_object(6-10)ast_from_object(29-35)ast_from_object(43-49)ast_from_object(64-75)ast_from_object(85-93)ast_to_object(5-5)ast_to_object(19-27)ast_to_object(39-41)ast_to_object(57-62)ast_to_object(81-83)
vm/src/stdlib/ast/exception.rs (1)
vm/src/stdlib/ast/expression.rs (16)
ast_from_object(51-139)ast_from_object(158-176)ast_from_object(199-217)ast_from_object(243-266)ast_from_object(284-302)ast_from_object(325-343)ast_from_object(369-392)ast_from_object(419-443)ast_from_object(459-472)ast_from_object(495-513)ast_from_object(536-554)ast_from_object(580-603)ast_from_object(627-647)ast_from_object(663-676)ast_from_object(693-704)ast_from_object(721-734)
vm/src/stdlib/ast/expression.rs (4)
vm/src/stdlib/ast/constant.rs (10)
number_literal_to_object(330-342)string_literal_to_object(344-356)bytes_literal_to_object(358-367)boolean_literal_to_object(369-377)none_literal_to_object(379-387)ellipsis_literal_to_object(389-397)ast_from_object(120-135)ast_from_object(168-243)ast_to_object(100-118)ast_to_object(139-166)vm/src/stdlib/ast/string.rs (7)
fstring_to_object(342-354)ast_from_object(231-245)ast_from_object(262-281)ast_from_object(318-339)ast_to_object(216-230)ast_to_object(256-261)ast_to_object(294-317)vm/src/stdlib/ast/exception.rs (4)
ast_from_object(10-28)ast_from_object(56-75)ast_to_object(5-9)ast_to_object(32-54)vm/src/class.rs (1)
static_type(22-26)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run tests under miri
🔇 Additional comments (55)
vm/src/readline.rs (3)
32-32: LGTM: Good use ofSelfshorthand.Using
Selfinstead of the explicit type name follows Rust best practices for constructor methods.
89-89: LGTM: Consistent use ofSelfshorthand.This change maintains consistency with the stylistic improvements throughout the codebase.
139-139: LGTM: Proper use ofSelfin tuple struct constructor.The
Selfshorthand correctly applies to tuple struct construction as well.vm/src/stdlib/ast/operator.rs (4)
7-8: LGTM: Excellent use ofSelffor enum variants.Using
Self::AndandSelf::Orinstead of the full enum path improves readability and follows Rust idioms for enum variant references within impl blocks.Also applies to: 23-25
39-51: LGTM: Consistent enum variant refactoring.The systematic replacement of
ruff::Operator::*withSelf::*throughout both theast_to_objectandast_from_objectmethods improves code conciseness while maintaining functionality.Also applies to: 66-90
104-107: LGTM: ProperSelfusage for UnaryOp variants.The changes maintain consistency with the refactoring pattern applied to other enum implementations in this file.
Also applies to: 122-128
142-151: LGTM: Complete and consistent CmpOp refactoring.All comparison operator variants correctly use the
Self::shorthand, completing the stylistic improvements for this enum'sNodetrait implementation.Also applies to: 166-184
derive-impl/src/pyclass.rs (3)
45-45: LGTM: Improved readability with blank line.Adding a blank line after the type definition improves code structure and readability.
1220-1220: LGTM: Consistent formatting improvements.The added blank lines after method implementations enhance code readability and provide better visual separation between different trait implementations.
Also applies to: 1230-1230, 1250-1250, 1344-1344, 1397-1397
1281-1282: LGTM: Cleaner string literals with raw strings.Converting to raw string literals (
r#""#) improves readability for strings containing quotes and escape sequences, making the error messages clearer to maintain.Also applies to: 1293-1294, 1412-1413, 1424-1425
vm/src/stdlib/posix.rs (6)
394-394: LGTM! String allocation optimization approved.Replacing
String::from()with a string literal eliminates unnecessary heap allocation for the error message, improving performance.
402-402: LGTM! String allocation optimization approved.Replacing
String::from()with a string literal eliminates unnecessary heap allocation for the error message, improving performance.
591-591: LGTM! Readability improvements approved.Adding blank lines after function definitions improves code readability and follows Rust formatting conventions.
Also applies to: 607-607, 608-608, 609-609, 619-619, 624-624, 625-625, 629-629, 633-633, 642-642
670-674: LGTM! UsingSelfimproves maintainability.Using
Selfinstead of the explicit type nameSchedParamis a Rust best practice that improves code maintainability and reduces refactoring overhead.
710-718: LGTM! UsingSelfimproves maintainability.Using
Selfinstead of the explicit type nameSchedParamis a Rust best practice that improves code maintainability and reduces refactoring overhead.
1767-1767: LGTM! String allocation optimization approved.Replacing
.to_owned()with a string literal eliminates unnecessary heap allocation for the error message, improving performance.vm/src/stdlib/sre.rs (7)
33-35: LGTM!const fnannotation approved.Adding
const fntogetcodesize()enables compile-time evaluation of this simple constant-returning function, which is a good optimization.
39-39: LGTM! Improved readability with range pattern matching.Using
(b'a' as i32..=b'z' as i32).contains(&ch)is more idiomatic and readable than manual range checks, while maintaining the same logic.
409-411: LGTM!const fnannotation approved.Adding
const fnto thegroups()getter method enables compile-time evaluation of this simple field access, which is appropriate for a const getter.
641-643: LGTM!const fnannotations approved.Adding
const fnto these simple getter methods (pos(),endpos(),lastindex()) enables compile-time evaluation and is appropriate for these const accessors.Also applies to: 646-648, 651-657
724-724: LGTM! String allocation optimization approved.Replacing
.to_owned()with a string literal eliminates unnecessary heap allocation for the error message, improving performance.
749-749: LGTM! String allocation optimization approved.Replacing
.to_owned()with a string literal eliminates unnecessary heap allocation for the error message, improving performance.
36-36: LGTM! Readability improvements approved.Adding blank lines improves code readability and follows Rust formatting conventions. The trailing newline at the end of the file is also a good practice.
Also applies to: 41-41, 47-47, 52-52, 355-355, 402-402, 407-407, 412-412, 658-658, 664-664, 669-669, 674-674, 687-687, 692-692
vm/src/stdlib/symtable.rs (1)
62-64: LGTM! Appropriate const fn conversions.These method conversions to
const fnare well-suited as they only return primitive values, boolean flags, or simple field access without side effects. This enables compile-time evaluation and aligns with Rust's const-correctness principles.Also applies to: 67-69, 134-136, 178-180, 188-190, 199-201, 204-206, 209-211, 214-216, 219-221, 224-226, 229-231
vm/src/stdlib/ast/other.rs (2)
58-58: LGTM! Good use of Self shorthand.Using
Self { expression, range }instead of the explicit struct name improves maintainability and follows Rust conventions.
81-81: LGTM! Improved code readability.The added blank lines improve separation between methods and enhance code readability.
Also applies to: 100-100, 127-127
vm/src/stdlib/ast/node.rs (2)
11-11: LGTM! Improved trait method separation.The added blank line enhances readability by providing better visual separation between trait methods.
48-48: LGTM! More idiomatic Self usage.Using
Self::newinstead ofBox::newis more idiomatic and maintains consistency with similar patterns throughout the codebase.vm/src/stdlib/ast/parameter.rs (1)
46-46: LGTM! Enhanced code readability.The added blank lines improve visual separation between methods and enhance overall code readability.
Also applies to: 99-99, 130-130
vm/src/stdlib/ast/exception.rs (2)
55-55: LGTM! Consistent formatting improvement.The added blank line maintains consistency with the formatting improvements applied across the AST module and enhances method separation.
61-61: LGTM! Consistent Self shorthand usage.Using
Self {instead of the explicit struct name aligns with the broader refactoring pattern applied across the AST module and improves maintainability.vm/src/stdlib/ast/module.rs (2)
67-67: Excellent use ofSelffor improved readability.The replacement of explicit type names like
ruff::ModModulewithSelffollows Rust best practices and makes the code more concise and maintainable.Also applies to: 96-96, 149-149, 185-185, 211-211
36-36: Good addition of blank lines for better code organization.The blank lines after method implementations improve readability by clearly separating different implementation blocks.
Also applies to: 63-63, 90-90, 129-129, 145-145, 159-159, 205-205
vm/src/stdlib/ast.rs (2)
58-58: Improved string literal clarity with raw strings.Using raw string literals
r#""#instead of theformat!macro makes the error messages more readable and eliminates the need for escaping quotes.Also applies to: 79-79
110-110: Excellent conversion toconst fn.Making
get_one_indexedaconst fnenables compile-time evaluation, which can improve performance and follows modern Rust practices for simple accessor methods.vm/src/stdlib/ast/pattern.rs (2)
53-60: Consistent use ofSelfimproves code maintainability.The systematic replacement of
ruff::Pattern::withSelf::in pattern matching makes the code more concise and easier to refactor while maintaining the same functionality.Also applies to: 70-76, 82-88, 94-100, 106-112
24-24: Improved code organization with blank lines.The addition of blank lines after method implementations enhances readability by clearly separating different implementation blocks.
Also applies to: 48-48, 141-141, 157-157, 177-177, 193-193, 207-207, 227-227, 243-243, 269-269, 293-293, 322-322, 430-430, 444-444, 464-464, 481-481
vm/src/stdlib/ast/expression.rs (2)
10-44: Comprehensive use ofSelfenhances code consistency.The systematic replacement of explicit
ruff::Expr::variant names withSelf::throughout the expression matching and construction code improves readability and follows Rust conventions. This makes the code more maintainable and consistent with the broader codebase refactoring.Also applies to: 58-60, 62-64, 66-68, 70-72, 74-78, 80-82, 88-92, 94-96, 98-102, 104-106, 108-112, 114-118, 120-122, 124-126, 128-130, 147-149, 167-171
50-50: Well-organized code with improved readability.The addition of blank lines after method implementations creates clear visual separation between different AST node implementations, improving code organization and readability.
Also applies to: 141-141, 157-157, 178-178, 198-198, 219-219, 242-242, 268-268, 304-304, 324-324, 345-345, 368-368, 394-394, 418-418, 445-445, 459-459, 474-474, 494-494, 515-515, 535-535, 556-556, 579-579, 605-605, 626-626, 649-649, 678-678, 692-692, 706-706, 720-720, 736-736, 763-763, 795-795, 826-826, 854-854, 877-877, 903-903, 951-951, 987-987, 1002-1002, 1019-1019, 1035-1035, 1056-1056, 1077-1077, 1099-1099, 1122-1122, 1142-1142, 1159-1159, 1180-1180, 1205-1205, 1236-1236
vm/src/stdlib/marshal.rs (3)
33-33: Good addition of associated type annotations.Adding the
type ConstantBagdeclarations improves trait clarity and explicitly documents the type relationships in the marshal implementations.Also applies to: 130-130
135-135: Improved code organization with consistent spacing.The addition of blank lines between method implementations enhances readability and creates clear visual separation between different functionality.
Also applies to: 139-139, 143-143, 147-147, 151-151, 155-155, 159-159, 163-163, 167-167, 172-172, 176-176, 183-183, 195-195, 203-203, 215-215
184-194: More explicit implementations improve code clarity.The updated
make_setandmake_frozensetimplementations are more explicit about their operations, making the code easier to understand while maintaining the same functionality.Also applies to: 196-202
vm/src/stdlib/ast/constant.rs (4)
24-37: 👍 Const-ifying the numeric constructors looks good
new_intandnew_floatcontain only field initialisation, so they are safely promotable toconst fn.
No functional or compile-time issues spotted.
38-43: 👍new_complexis also a safeconst fnSame rationale as above – just a plain struct literal.
52-71: 👍 Remaining trivial constructors are fine
new_bool,new_none, andnew_ellipsisare straightforward and safe asconst fn.
141-166: Enum shorthand improves readabilitySwitching to
Self::Variantremoves noise without altering logic.vm/src/stdlib/io.rs (9)
255-257: Good use ofconst fnfor the constructorMaking
newa const function is appropriate here since it only performs field initialization.
1908-1914: Appropriate use ofconst fnfor constant methodsBoth methods return compile-time constants, making them ideal candidates for const functions.
2047-2058: Good refactoring to useSelfUsing
Selfinstead of the explicit type name improves maintainability and follows Rust best practices.
2219-2219: Consistent improvements withSelfandconst fnGood use of
Selfin the constructor and appropriateconst fnfor the simple getter method.Also applies to: 2260-2265
2737-2746: Well-implemented getter methodsThe new getters follow the established pattern, properly acquiring locks and delegating or returning cloned values as appropriate.
2897-2939: Well-designed helper struct for string slicingThe
SlicedStrstruct is well-implemented with appropriate use ofconst fnand#[inline]attributes for performance optimization.
3493-3498: Consistent refactoring in StringIOGood use of
Selfin the constructor and appropriateconst fnfor methods returning constants.Also applies to: 3515-3527
3846-3857: Good const fn and Self usageThe
rawmodemethod is ideal forconst fnas it only performs pattern matching on compile-time values. TheSelf::usage in the enum improves maintainability.Also applies to: 3870-3879
4154-4160: Consistent implementation of writable() methodThe new
writable()method follows the same pattern asreadable()and theSelf::usage improves code maintainability.Also applies to: 4439-4442
| pub(super) const fn new_bytes(value: Box<[u8]>, range: TextRange) -> Self { | ||
| Self { | ||
| range, | ||
| value: ConstantLiteral::Bytes(value), | ||
| } | ||
| } |
There was a problem hiding this comment.
Mismatch between new_bytes signature and its call-sites
new_bytes still expects a Box<[u8]>, but bytes_literal_to_object (l. 363-366, unchanged) passes a Vec<u8>.
This doesn’t compile on stable Rust – there’s no implicit conversion.
Either change the constructor to accept impl Into<Box<[u8]>> (cannot be const yet because trait calls aren’t const-stable) or convert at the call-site:
-let c = Constant::new_bytes(bytes.copied().collect(), range);
+let c = Constant::new_bytes(bytes.copied().collect::<Vec<u8>>().into(), range);Failing to fix this will break the build.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub(super) const fn new_bytes(value: Box<[u8]>, range: TextRange) -> Self { | |
| Self { | |
| range, | |
| value: ConstantLiteral::Bytes(value), | |
| } | |
| } | |
| // in bytes_literal_to_object (around line 363) | |
| - let c = Constant::new_bytes(bytes.copied().collect(), range); | |
| + let c = Constant::new_bytes(bytes.copied().collect::<Vec<u8>>().into(), range); |
🤖 Prompt for AI Agents
In vm/src/stdlib/ast/constant.rs around lines 45 to 50, the new_bytes function
currently accepts a Box<[u8]>, but its call-sites pass a Vec<u8>, causing a
compilation error. To fix this, either change new_bytes to accept a Vec<u8> and
convert it to Box<[u8]> inside the function, or convert the Vec<u8> to Box<[u8]>
at the call-sites before calling new_bytes. Since trait calls are not
const-stable, avoid using impl Into<Box<[u8]>> in the const fn signature.
| @@ -44,7 +45,7 @@ impl<T: Node> Node for Box<T> { | |||
| source_code: &SourceCodeOwned, | |||
| object: PyObjectRef, | |||
| ) -> PyResult<Self> { | |||
| T::ast_from_object(vm, source_code, object).map(Box::new) | |||
| T::ast_from_object(vm, source_code, object).map(Self::new) | |||
There was a problem hiding this comment.
This is logically right but lookins so unfamiliar 😲
There was a problem hiding this comment.
It can be reverted, just say the word:)
Summary by CodeRabbit
New Features
Refactor
Style
Bug Fixes