Revert "Use ruff for `Expr` unparsing" by ShaharNaveh · Pull Request #6206 · RustPython/RustPython

youknowone

Walkthrough

This PR removes workspace and package dependencies on external ruff crates (ruff_python_codegen and ruff_source_file), replaces a local unparse_expr helper with an external UnparseExpr implementation imported from a new internal unparse module, and cleans up unused imports.

Changes

Cohort / File(s)	Summary
Workspace dependency cleanup `Cargo.toml`	Removed ruff_python_codegen from workspace dependencies
Package dependency pruning `compiler/codegen/Cargo.toml`	Removed ruff_python_parser, ruff_python_codegen, and ruff_source_file from dependencies
Codegen unparse refactoring `compiler/codegen/src/compile.rs`	Removed private unparse_expr helper function; added UnparseExpr import; replaced unparse_expr calls with UnparseExpr::new().to_string() at two call sites; removed unused LineEnding import
Module structure `compiler/codegen/src/lib.rs`	Added private unparse module declaration

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

The changes involve heterogeneous edits across dependency manifests and source code refactoring. While individual changes are straightforward, understanding the rationale for dependency removal and verifying that the UnparseExpr replacement maintains functional equivalence across both call sites requires moderate attention.

Possibly related PRs

fn unparse_expr -> UnparseExpr::new #6121 — Directly related; both PRs replace the free function unparse_expr with UnparseExpr::new at identical call sites in compiler/codegen/src/compile.rs.
Use ruff for Expr unparsing #6124 — Related; modifies the same Expr unparsing surface but in the opposite direction, adding ruff_python_codegen and local unparse_expr.
Export ruff_source_file types in rustpython_compiler_core #6020 — Related; also removes ruff_source_file from compiler/codegen/Cargo.toml dependencies.

Suggested reviewers

youknowone

Poem

🐰 A codegen hop, dependencies drop,
unparse_expr we bid adieu,
UnparseExpr now unpacks the view,
cleaner modules help us skip,
workspace pruned, refactored trip! 🌿

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "Revert "Use ruff for `Expr` unparsing"" directly and accurately describes the main change in the changeset. The modifications across multiple files—including removal of ruff-related dependencies (ruff_python_codegen, ruff_python_parser, ruff_source_file) from Cargo.toml files and refactoring of the expression unparsing logic in compile.rs—are all consistent with reverting a previous change that integrated ruff for expression unparsing. The title is concise, specific, and clearly communicates the primary intent to a developer scanning the history.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f22aed2 and de5d207.

⛔ Files ignored due to path filters (12)

Cargo.lock is excluded by !**/*.lock
Lib/test/test_future_stmt/badsyntax_future10.py is excluded by !Lib/**
Lib/test/test_future_stmt/badsyntax_future3.py is excluded by !Lib/**
Lib/test/test_future_stmt/badsyntax_future4.py is excluded by !Lib/**
Lib/test/test_future_stmt/badsyntax_future5.py is excluded by !Lib/**
Lib/test/test_future_stmt/badsyntax_future6.py is excluded by !Lib/**
Lib/test/test_future_stmt/badsyntax_future7.py is excluded by !Lib/**
Lib/test/test_future_stmt/badsyntax_future8.py is excluded by !Lib/**
Lib/test/test_future_stmt/badsyntax_future9.py is excluded by !Lib/**
Lib/test/test_future_stmt/future_test1.py is excluded by !Lib/**
Lib/test/test_future_stmt/future_test2.py is excluded by !Lib/**
Lib/test/test_future_stmt/test_future.py is excluded by !Lib/**

📒 Files selected for processing (4)

Cargo.toml (0 hunks)
compiler/codegen/Cargo.toml (0 hunks)
compiler/codegen/src/compile.rs (3 hunks)
compiler/codegen/src/lib.rs (1 hunks)

💤 Files with no reviewable changes (2)

Cargo.toml
compiler/codegen/Cargo.toml

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Format Rust code with the default rustfmt style (run cargo fmt)
Run clippy and fix any warnings or lints introduced by your changes
Follow Rust best practices for error handling and memory management

Files:

compiler/codegen/src/lib.rs
compiler/codegen/src/compile.rs

🧬 Code graph analysis (2)

compiler/codegen/src/lib.rs (1)

Lib/ast.py (1)

unparse (1815-1817)

compiler/codegen/src/compile.rs (2)

compiler/codegen/src/unparse.rs (2)

new (34-36)

new (637-639)

compiler/codegen/src/symboltable.rs (3)

new (61-74)

new (175-182)

new (630-640)

⏰ 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)

GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Check Rust code with rustfmt and clippy
GitHub Check: Check the WASM package and demo
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Ensure compilation on various targets

🔇 Additional comments (4)

compiler/codegen/src/lib.rs (1)

16-16: LGTM! Module declaration is correctly placed.

The private unparse module declaration follows Rust conventions and integrates cleanly with the existing module structure.

compiler/codegen/src/compile.rs (3)

17-17: LGTM! Import correctly added.

The UnparseExpr import from the internal unparse module is properly integrated into the existing crate-local imports.

3612-3612: LGTM! Correct usage for mapping pattern keys.

The unparsing of literal keys is correctly implemented. The temporary UnparseExpr is immediately converted to a string with no lifetime concerns, and the usage is properly guarded by the literal type check at line 3610.

4166-4169: LGTM! Correct implementation for future annotations.

The unparsing of annotations when future_annotations is enabled is correctly implemented. The pattern is safe and aligns with PEP 563 requirements to store annotations as strings. The temporary UnparseExpr is immediately converted with no lifetime issues.

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

_{Comment @coderabbitai help to get the list of available commands and usage tips.}