Skip to content

Conversation

@ShaharNaveh
Copy link
Collaborator

@ShaharNaveh ShaharNaveh commented Sep 3, 2025

Follow up on #6121 (review)

Summary by CodeRabbit

  • Chores
    • Updated workspace dependencies to include new Python code generation and parsing libraries.
    • Refactored internal code generation utilities for improved maintainability.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 3, 2025

Walkthrough

The PR integrates Ruff's Python code generation and parser dependencies into the RustPython codegen compiler and refactors unparse functionality by replacing external UnparseExpr usage with an internal helper function that leverages Ruff's Generator directly.

Changes

Cohort / File(s) Change Summary
Dependency Integration
Cargo.toml, compiler/codegen/Cargo.toml
Added ruff_python_codegen to workspace dependencies and as a direct dependency in codegen, alongside ruff_python_parser and ruff_source_file
Unparse Refactoring
compiler/codegen/src/compile.rs
Introduced private unparse_expr() helper function using Ruff's Generator; replaced two call sites using UnparseExpr::new(...).to_string() with the new helper; added LineEnding import
Module Cleanup
compiler/codegen/src/lib.rs
Removed mod unparse; declaration

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

The changes involve straightforward dependency additions and localized unparse function refactoring with consistent replacement patterns. Logic density is low and the edits follow predictable patterns.

Possibly related PRs

  • fn unparse_expr -> UnparseExpr::new #6121: Directly related through opposing changes to the same unparse API and call sites in compiler/codegen/src/compile.rs (this PR removes external UnparseExpr usage, while that PR introduced it).

Suggested reviewers

  • youknowone

Poem

🐰 A rabbit's joyful hop and bound,
As Ruff's codegen code is found!
We pluck the UnparseExpr away,
And weave new helpers in its play,
Deps aligned—our builds now say hooray! 🌟

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 title "Use ruff for Expr unparsing" directly and accurately describes the primary objective of this pull request. The changes comprehensively replace the existing Expr unparsing logic with ruff-based unparsing, including adding ruff dependencies (ruff_python_codegen, ruff_python_parser), implementing a new ruff-based unparse_expr helper function, and removing the old unparse module. The title is concise, clear, and specific enough that a developer scanning the repository history would immediately understand that this PR integrates ruff as the unparsing solution for expressions, which aligns perfectly with the stated PR objective of replacing the existing unparsing logic with ruff-based unparsing to achieve code reduction.
✨ 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 87dd525 and 4b80a5d.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • Lib/test/test_typing.py is excluded by !Lib/**
📒 Files selected for processing (3)
  • Cargo.toml (1 hunks)
  • compiler/codegen/src/compile.rs (4 hunks)
  • compiler/codegen/src/lib.rs (0 hunks)
💤 Files with no reviewable changes (1)
  • compiler/codegen/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • compiler/codegen/src/compile.rs
⏰ 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). (11)
  • GitHub Check: Check Rust code with rustfmt and clippy
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Ensure compilation on various targets
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Run tests under miri
  • GitHub Check: Check the WASM package and demo
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
🔇 Additional comments (1)
Cargo.toml (1)

162-162: Workspace dependency added correctly.

The new ruff_python_codegen dependency is properly declared with consistent versioning (tag 0.14.1) and sourcing alongside other ruff workspace dependencies. The syntax and placement are correct.

The PR context indicates test regressions were observed with this integration. Before merging, please verify that:

  1. All test failures related to the unparsing refactor have been resolved or are documented as expected behavioral changes
  2. The test suite passes in CI/CD

If regressions remain, they should be addressed in the implementation files (e.g., compiler/codegen/src/compile.rs mentioned in the AI summary) rather than here.


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.

@ShaharNaveh ShaharNaveh changed the title Use ruff for Expr nparsing Use ruff for Expr unparsing Sep 3, 2025

fn unparse_expr(expr: &Expr) -> String {
// Hack, because we can't do `ruff_python_codegen::Indentation::default()`
// https://github.com/astral-sh/ruff/pull/20216
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that it was already merged astral-sh/ruff#20216 (that was fast 😅 )

I'll wait for it be released, and I wanted to upgrade ruff anyway so it will be a good excuse to do so

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
compiler/codegen/Cargo.toml (1)

34-34: Remove redundant dev-dependency on ruff_python_parser

It’s already a normal dependency due to compile-time usage; duplicating it in dev-deps is unnecessary.

 [dev-dependencies]
-ruff_python_parser = { workspace = true }
 insta = { workspace = true }
compiler/codegen/src/compile.rs (2)

150-162: Avoid reparsing for every unparse and stabilize line endings

Unconditionally reparsing "x = 1" on every call is unnecessary. Cache the computed indentation with OnceLock and force LF for reproducible output across platforms.

-fn unparse_expr(expr: &Expr) -> String {
-    // Hack, because we can't do `ruff_python_codegen::Indentation::default()`
-    // https://github.com/astral-sh/ruff/pull/20216
-    let indentation = {
-        let contents = r"x = 1";
-        let module = ruff_python_parser::parse_module(contents).unwrap();
-        let stylist = ruff_python_codegen::Stylist::from_tokens(module.tokens(), contents);
-        stylist.indentation().clone()
-    };
-
-    ruff_python_codegen::Generator::new(&indentation, LineEnding::default()).expr(expr)
-}
+fn unparse_expr(expr: &Expr) -> String {
+    static INDENT: std::sync::OnceLock<ruff_python_codegen::Indentation> =
+        std::sync::OnceLock::new();
+    let indentation = INDENT.get_or_init(|| {
+        // Hack, until Indentation::default() is available (see ruff PR #20216)
+        let contents = "x = 1";
+        let module = ruff_python_parser::parse_module(contents)
+            .expect("parse_module('x = 1') should never fail");
+        let stylist = ruff_python_codegen::Stylist::from_tokens(module.tokens(), contents);
+        stylist.indentation().clone()
+    });
+    ruff_python_codegen::Generator::new(indentation, LineEnding::Lf).expr(expr)
+}

3608-3616: Duplicate-key detection now relies on Ruff unparse; add coverage for semantic equivalence

Switching to Ruff’s generator likely normalizes literals (e.g., 1_0 vs 10, different string quotes). Please add/adjust tests to ensure duplicates are rejected based on semantic equality, not just textual quirks.

Would you like me to add regression tests covering numbers with underscores, different string quote styles, and mixed-case hex literals?

📜 Review details

Configuration 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 056795e and 87dd525.

⛔ Files ignored due to path filters (12)
  • Cargo.lock is excluded by !**/*.lock
  • Lib/test/test_future_stmt/badsyntax_future.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/import_nested_scope_twice.py is excluded by !Lib/**
  • Lib/test/test_future_stmt/nested_scope.py is excluded by !Lib/**
  • Lib/test/test_future_stmt/test_future.py is excluded by !Lib/**
📒 Files selected for processing (5)
  • Cargo.toml (1 hunks)
  • compiler/codegen/Cargo.toml (1 hunks)
  • compiler/codegen/src/compile.rs (4 hunks)
  • compiler/codegen/src/lib.rs (0 hunks)
  • compiler/codegen/src/unparse.rs (0 hunks)
💤 Files with no reviewable changes (2)
  • compiler/codegen/src/lib.rs
  • compiler/codegen/src/unparse.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 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
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.py : Use ruff for linting Python code
📚 Learning: 2025-07-19T03:16:56.511Z
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.py : Use ruff for linting Python code

Applied to files:

  • Cargo.toml
  • compiler/codegen/Cargo.toml
⏰ 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 (windows-latest)
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Check the WASM package and demo
  • GitHub Check: Check Rust code with rustfmt and clippy
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Ensure compilation on various targets
🔇 Additional comments (4)
compiler/codegen/Cargo.toml (1)

16-18: Ruff parser/codegen/source_file deps: LGTM

These additions align with the new unparse path in compile.rs.

Cargo.toml (1)

165-165: Workspace adds ruff_python_codegen: LGTM

Keeps all ruff crates pinned to the same tag; good for API coherence.

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

33-33: Import of LineEnding: LGTM

Needed for Generator construction.


4160-4163: Future annotations stringification parity

Using Ruff for annotation strings is fine; please verify parity with CPython across edge cases (unions, generics, nested subscripts, parentheses, and starred PEP 646 forms) to avoid subtle mismatches in annotations.

I can draft insta tests comparing against CPython’s unparse for a corpus of annotations if helpful.

def barfoo2(x: CT): ...
self.assertIs(get_type_hints(barfoo2, globals(), locals())['x'], CT)

@unittest.expectedFailure # TODO: RUSTPYTHON; 'List[list["C2"]]' != "List[list['C2']]"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any option to change the quote style?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No:/ not from the Generator directly, I think that this needs to be fixed upstream (over Ruff's side)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once Ruff forked rustpython-parser, we patched unparse to decide quote by option. That might be removed during refactoring. I have no idea if Ruff maintainers are interested in adding feature only for downstream user. Probably asking if possible be worth. Otherwise fork? but in easier form to sync.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youknowone It will supported in the next ruff release.
astral-sh/ruff#20434

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you so much!

@ShaharNaveh
Copy link
Collaborator Author

ShaharNaveh commented Sep 4, 2025

ngl, I have mixed feelings about this PR. on one hand it's a major code reduction, and on the other hand there is a regression when it comes to the test cases

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Less code, no regression. Great!

@youknowone youknowone merged commit 0fb7d0f into RustPython:main Oct 22, 2025
12 checks passed
@ShaharNaveh
Copy link
Collaborator Author

Less code, no regression. Great!

There's regression:/

It generates tuples without () so what used to be (1, 2) is now 1, 2.

I forgot to point that out before, I think it's better to revert the changes and discuss it

ShaharNaveh added a commit to ShaharNaveh/RustPython that referenced this pull request Oct 22, 2025
youknowone pushed a commit that referenced this pull request Oct 22, 2025
ShaharNaveh added a commit to ShaharNaveh/RustPython that referenced this pull request Oct 22, 2025
@ShaharNaveh ShaharNaveh deleted the ruff-unparse branch November 3, 2025 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants