fn unparse_expr -> UnparseExpr::new - #6121
Conversation
Sorry, something went wrong.
WalkthroughReplaces the free function unparse::unparse_expr with the associated constructor UnparseExpr::new across the codegen. Updates compile.rs call sites accordingly. Removes the old function, adds the constructor, and keeps formatting behavior unchanged via Display on UnparseExpr. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Codegen caller
participant UE as UnparseExpr
participant Disp as Display
participant U as Unparser
Caller->>UE: UnparseExpr::new(expr, source)
Note right of UE: Stores expr/source
Caller->>Disp: to_string() on UnparseExpr
Disp->>U: Unparser::new(f, source)
U-->>Disp: write formatted expr
Disp-->>Caller: String
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type @coderabbitai help to get the list of available commands. Other keywords and placeholders
Status, Documentation and Community
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review detailsConfiguration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 CommitsReviewing files that changed from the base of the PR and between fa91df6 and 82fc735.
📒 Files selected for processing (2)- compiler/codegen/src/compile.rs (3 hunks)
- compiler/codegen/src/unparse.rs (7 hunks)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.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
Files:
- compiler/codegen/src/compile.rs
- compiler/codegen/src/unparse.rs
compiler/codegen/src/unparse.rs (2)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- new (34-36)
- new (613-615)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
compiler/codegen/src/compile.rs (3)compiler/codegen/src/unparse.rs (2)3595-3604: Call-site migration preserves behavior.
UnparseExpr::new(key, &self.source_file).to_string() is a drop-in replacement for the prior helper. No functional change.
4149-4152: Annotation unparsing change is correct.
UnparseExpr::new(...).to_string().into() properly produces Wtf8Buf.
17-18: Approve import switch—no lingering free function calls
Verified that all unparse_expr( occurrences are method definitions or self.unparse_expr calls in compiler/codegen/src/unparse.rs; no standalone free‐function invocations remain.34-36: Minor constructor style tweak — OK.
Using Self { .. } is idiomatic.
612-616: Public constructor addition — OK.
pub const fn new(...) -> Self mirrors the old free function’s const-ness and keeps lifetimes intact.
Sorry, something went wrong.
| write!(self, ": {}", UnparseExpr::new(body, self.source))?; | ||
| }) |
There was a problem hiding this comment.
⚠️ Potential issue
write!(self, ...) requires implementing fmt::Write for Unparser (compile error otherwise).
write! calls target std::fmt::Write::write_fmt, so the destination must implement fmt::Write. Unparser currently does not, which will fail to compile.
Apply this diff to implement fmt::Write and forward to the inner formatter:
@@
impl<'a, 'b, 'c> Unparser<'a, 'b, 'c> {
@@
fn write_fmt(&mut self, f: fmt::Arguments<'_>) -> fmt::Result {
self.f.write_fmt(f)
}
}
+impl<'a, 'b, 'c> fmt::Write for Unparser<'a, 'b, 'c> {
+ fn write_str(&mut self, s: &str) -> fmt::Result {
+ self.f.write_str(s)
+ }
+ fn write_char(&mut self, c: char) -> fmt::Result {
+ self.f.write_char(c)
+ }
+}Alternatively, replace each write!(self, "...", ...) with self.write_fmt(format_args!("...", ...))?, but the trait impl keeps call sites cleaner.
Also applies to: 197-199, 275-277, 285-289, 483-486, 492-494
🤖 Prompt for AI AgentsIn compiler/codegen/src/unparse.rs around lines 172-173 (also applies to 197-199, 275-277, 285-289, 483-486, 492-494): the code uses write!(self, ...) but Unparser does not implement std::fmt::Write so compilation fails; implement the fmt::Write trait for Unparser and forward its methods to the inner formatter/collector (i.e., implement fn write_str(&mut self, s: &str) -> fmt::Result and any other required fmt::Write methods by delegating to the inner formatter or its write methods), so existing write! call sites compile unchanged (alternatively replace write! calls with self.write_fmt(format_args!(...)) at each site if you prefer not to add the trait impl).
Sorry, something went wrong.
youknowone
left a comment
There was a problem hiding this comment.
The change looks good.
On the other hand, Ruff also have the corresponding part. Checking if we can share the implement may be worth
Sorry, something went wrong.
Summary by CodeRabbit
Refactor
Style