-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Use ruff for Expr unparsing
#6124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe PR integrates Ruff's Python code generation and parser dependencies into the RustPython codegen compiler and refactors unparse functionality by replacing external Changes
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
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
🔇 Additional comments (1)
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. Comment |
compiler/codegen/src/compile.rs
Outdated
|
|
||
| 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 |
There was a problem hiding this comment.
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
There was a problem hiding this 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_parserIt’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 endingsUnconditionally 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 equivalenceSwitching 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.
⛔ Files ignored due to path filters (12)
Cargo.lockis excluded by!**/*.lockLib/test/test_future_stmt/badsyntax_future.pyis excluded by!Lib/**Lib/test/test_future_stmt/badsyntax_future3.pyis excluded by!Lib/**Lib/test/test_future_stmt/badsyntax_future4.pyis excluded by!Lib/**Lib/test/test_future_stmt/badsyntax_future5.pyis excluded by!Lib/**Lib/test/test_future_stmt/badsyntax_future6.pyis excluded by!Lib/**Lib/test/test_future_stmt/badsyntax_future7.pyis excluded by!Lib/**Lib/test/test_future_stmt/badsyntax_future8.pyis excluded by!Lib/**Lib/test/test_future_stmt/badsyntax_future9.pyis excluded by!Lib/**Lib/test/test_future_stmt/import_nested_scope_twice.pyis excluded by!Lib/**Lib/test/test_future_stmt/nested_scope.pyis excluded by!Lib/**Lib/test/test_future_stmt/test_future.pyis 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 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
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.tomlcompiler/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: LGTMThese additions align with the new unparse path in compile.rs.
Cargo.toml (1)
165-165: Workspace adds ruff_python_codegen: LGTMKeeps all ruff crates pinned to the same tag; good for API coherence.
compiler/codegen/src/compile.rs (2)
33-33: Import of LineEnding: LGTMNeeded for Generator construction.
4160-4163: Future annotations stringification parityUsing 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.
Lib/test/test_typing.py
Outdated
| def barfoo2(x: CT): ... | ||
| self.assertIs(get_type_hints(barfoo2, globals(), locals())['x'], CT) | ||
|
|
||
| @unittest.expectedFailure # TODO: RUSTPYTHON; 'List[list["C2"]]' != "List[list['C2']]" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
|
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 |
youknowone
left a comment
There was a problem hiding this 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!
There's regression:/ It generates tuples without I forgot to point that out before, I think it's better to revert the changes and discuss it |
This reverts commit 0fb7d0f.
This reverts commit 0fb7d0f.
This reverts commit 153d0ee.
Follow up on #6121 (review)
Summary by CodeRabbit