nitpicks - #5886
Conversation
Sorry, something went wrong.
WalkthroughThis update applies widespread stylistic and structural refactoring across multiple modules. The primary changes include converting suitable methods to const fn, switching enum and struct constructions to use Self, simplifying string literal usage, and enhancing trait signatures for improved code clarity and const-correctness. Several blank lines are added for readability, and trait interfaces in the IO subsystem are expanded. 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. ❤️ Share 🪧 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
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)vm/src/stdlib/ast/constant.rs (1)📜 Review details175-236: Consider preserving string/bytes prefixes when round-tripping
While not introduced in this diff, the current ast_from_object path always sets prefix: StringLiteralPrefix::Empty for 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).
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between fa7af0e and da95cca.
📒 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)
**/*.rs: Follow the default rustfmt code style (cargo fmt to 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.rs
- vm/src/stdlib/ast/module.rs
- vm/src/stdlib/ast/operator.rs
- vm/src/stdlib/ast/other.rs
- vm/src/stdlib/ast/parameter.rs
- vm/src/stdlib/ast/pattern.rs
- vm/src/stdlib/ast/exception.rs
- vm/src/stdlib/ast/expression.rs
- vm/src/stdlib/ast/node.rs
- vm/src/stdlib/marshal.rs
- derive-impl/src/pyclass.rs
- vm/src/stdlib/posix.rs
- vm/src/stdlib/ast.rs
- vm/src/stdlib/sre.rs
- vm/src/stdlib/symtable.rs
- vm/src/stdlib/ast/constant.rs
- vm/src/stdlib/io.rs
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)
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 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: 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: 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 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
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 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.
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.
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>
vm/src/class.rs (1)vm/src/stdlib/ast/exception.rs (1)vm/src/stdlib/ast/node.rs (10)
- static_type (22-26)
- 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/expression.rs (16)vm/src/stdlib/ast/expression.rs (4)
- 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/constant.rs (10)⏰ Context from checks skipped due to timeout of 90000ms (11)vm/src/stdlib/ast/string.rs (7)
- 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/exception.rs (4)
- 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/class.rs (1)
- ast_from_object (10-28)
- ast_from_object (56-75)
- ast_to_object (5-9)
- ast_to_object (32-54)
- static_type (22-26)
- 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
vm/src/readline.rs (3)32-32: LGTM: Good use of Self shorthand.
Using Self instead of the explicit type name follows Rust best practices for constructor methods.
89-89: LGTM: Consistent use of Self shorthand.
This change maintains consistency with the stylistic improvements throughout the codebase.
139-139: LGTM: Proper use of Self in tuple struct constructor.
The Self shorthand correctly applies to tuple struct construction as well.
vm/src/stdlib/ast/operator.rs (4)7-8: LGTM: Excellent use of Self for enum variants.
Using Self::And and Self::Or instead 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::* with Self::* throughout both the ast_to_object and ast_from_object methods improves code conciseness while maintaining functionality.
Also applies to: 66-90
104-107: LGTM: Proper Self usage 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's Node trait 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! Using Self improves maintainability.
Using Self instead of the explicit type name SchedParam is a Rust best practice that improves code maintainability and reduces refactoring overhead.
710-718: LGTM! Using Self improves maintainability.
Using Self instead of the explicit type name SchedParam is 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 fn annotation approved.
Adding const fn to getcodesize() 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 fn annotation approved.
Adding const fn to the groups() getter method enables compile-time evaluation of this simple field access, which is appropriate for a const getter.
641-643: LGTM! const fn annotations approved.
Adding const fn to 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 fn are 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::new instead of Box::new is 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 of Self for improved readability.
The replacement of explicit type names like ruff::ModModule with Self follows 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 the format! macro makes the error messages more readable and eliminates the need for escaping quotes.
Also applies to: 79-79
110-110: Excellent conversion to const fn.
Making get_one_indexed a const fn enables 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 of Self improves code maintainability.
The systematic replacement of ruff::Pattern:: with Self:: 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 of Self enhances code consistency.
The systematic replacement of explicit ruff::Expr:: variant names with Self:: 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 ConstantBag declarations 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_set and make_frozenset implementations 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_int and new_float contain only field initialisation, so they are safely promotable to const fn.
No functional or compile-time issues spotted.
38-43: 👍 new_complex is also a safe const fn
Same rationale as above – just a plain struct literal.
52-71: 👍 Remaining trivial constructors are fine
new_bool, new_none, and new_ellipsis are straightforward and safe as const fn.
141-166: Enum shorthand improves readability
Switching to Self::Variant removes noise without altering logic.
vm/src/stdlib/io.rs (9)255-257: Good use of const fn for the constructor
Making new a const function is appropriate here since it only performs field initialization.
1908-1914: Appropriate use of const fn for constant methods
Both methods return compile-time constants, making them ideal candidates for const functions.
2047-2058: Good refactoring to use Self
Using Self instead of the explicit type name improves maintainability and follows Rust best practices.
2219-2219: Consistent improvements with Self and const fn
Good use of Self in the constructor and appropriate const fn for the simple getter method.
Also applies to: 2260-2265
2737-2746: Well-implemented getter methods
The 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 slicing
The SlicedStr struct is well-implemented with appropriate use of const fn and #[inline] attributes for performance optimization.
3493-3498: Consistent refactoring in StringIO
Good use of Self in the constructor and appropriate const fn for methods returning constants.
Also applies to: 3515-3527
3846-3857: Good const fn and Self usage
The rawmode method is ideal for const fn as it only performs pattern matching on compile-time values. The Self:: usage in the enum improves maintainability.
Also applies to: 3870-3879
4154-4160: Consistent implementation of writable() method
The new writable() method follows the same pattern as readable() and the Self:: usage improves code maintainability.
Also applies to: 4439-4442
Sorry, something went wrong.
| 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.
⚠️ Potential issue
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); |
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.
Sorry, something went wrong.
| 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 😲
Sorry, something went wrong.
There was a problem hiding this comment.
It can be reverted, just say the word:)
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Refactor
Style
Bug Fixes