mirrors/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-07-25 14:03:51 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Refactor StmtIf: Formatter and Linter (#5459)

Browse source 
## Summary

Previously, `StmtIf` was defined recursively as
```rust
pub struct StmtIf {
    pub range: TextRange,
    pub test: Box<Expr>,
    pub body: Vec<Stmt>,
    pub orelse: Vec<Stmt>,
}
```
Every `elif` was represented as an `orelse` with a single `StmtIf`. This
means that this representation couldn't differentiate between
```python
if cond1:
    x = 1
else:
    if cond2:
        x = 2
```
and 
```python
if cond1:
    x = 1
elif cond2:
    x = 2
```
It also makes many checks harder than they need to be because we have to
recurse just to iterate over an entire if-elif-else and because we're
lacking nodes and ranges on the `elif` and `else` branches.

We change the representation to a flat

```rust
pub struct StmtIf {
    pub range: TextRange,
    pub test: Box<Expr>,
    pub body: Vec<Stmt>,
    pub elif_else_clauses: Vec<ElifElseClause>,
}

pub struct ElifElseClause {
    pub range: TextRange,
    pub test: Option<Expr>,
    pub body: Vec<Stmt>,
}
```
where `test: Some(_)` represents an `elif` and `test: None` an else.

This representation is different tradeoff, e.g. we need to allocate the
`Vec<ElifElseClause>`, the `elif`s are now different than the `if`s
(which matters in rules where want to check both `if`s and `elif`s) and
the type system doesn't guarantee that the `test: None` else is actually
last. We're also now a bit more inconsistent since all other `else`,
those from `for`, `while` and `try`, still don't have nodes. With the
new representation some things became easier, e.g. finding the `elif`
token (we can use the start of the `ElifElseClause`) and formatting
comments for if-elif-else (no more dangling comments splitting, we only
have to insert the dangling comment after the colon manually and set
`leading_alternate_branch_comments`, everything else is taken of by
having nodes for each branch and the usual placement.rs fixups).

## Merge Plan

This PR requires coordination between the parser repo and the main ruff
repo. I've split the ruff part, into two stacked PRs which have to be
merged together (only the second one fixes all tests), the first for the
formatter to be reviewed by @michareiser and the second for the linter
to be reviewed by @charliermarsh.

* MH: Review and merge
https://github.com/astral-sh/RustPython-Parser/pull/20
* MH: Review and merge or move later in stack
https://github.com/astral-sh/RustPython-Parser/pull/21
* MH: Review and approve
https://github.com/astral-sh/RustPython-Parser/pull/22
* MH: Review and approve formatter PR
https://github.com/astral-sh/ruff/pull/5459
* CM: Review and approve linter PR
https://github.com/astral-sh/ruff/pull/5460
* Merge linter PR in formatter PR, fix ecosystem checks (ecosystem
checks can't run on the formatter PR and won't run on the linter PR, so
we need to merge them first)
 * Merge https://github.com/astral-sh/RustPython-Parser/pull/22
 * Create tag in the parser, update linter+formatter PR
 * Merge linter+formatter PR https://github.com/astral-sh/ruff/pull/5459

---------

Co-authored-by: Micha Reiser <micha@reiser.io>
This commit is contained in:
konsti 2023-07-18 13:40:15 +02:00 • committed by GitHub
parent 167b9356fa
commit 730e6b2b4c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
82 changed files with 2333 additions and 2009 deletions
Download patch file Download diff file Expand all files Collapse all files

1
crates/ruff_python_formatter/src/statement/mod.rs
View file

@ -64,6 +64,7 @@ impl FormatRule<Stmt, PyFormatContext<'_>> for FormatStmt {
Stmt::Pass(x) => x.format().fmt(f),
Stmt::Break(x) => x.format().fmt(f),
Stmt::Continue(x) => x.format().fmt(f),
Stmt::TypeAlias(_) => todo!(),
}
}
}

1
crates/ruff_python_formatter/src/statement/stmt_class_def.rs
View file

@ -18,6 +18,7 @@ impl FormatNodeRule<StmtClassDef> for FormatStmtClassDef {
bases,
keywords,
body,
type_params: _,
decorator_list,
} = item;

181
crates/ruff_python_formatter/src/statement/stmt_if.rs
View file

@ -1,91 +1,43 @@
use crate::comments::{leading_alternate_branch_comments, trailing_comments, SourceComment};
use crate::comments::{leading_alternate_branch_comments, trailing_comments};
use crate::expression::maybe_parenthesize_expression;
use crate::expression::parentheses::Parenthesize;
use crate::prelude::*;
use crate::FormatNodeRule;
use ruff_formatter::{write, FormatError};
use rustpython_parser::ast::{Ranged, Stmt, StmtIf, Suite};
use ruff_formatter::write;
use ruff_python_ast::node::AnyNodeRef;
use rustpython_parser::ast::{ElifElseClause, StmtIf};
#[derive(Default)]
pub struct FormatStmtIf;
impl FormatNodeRule<StmtIf> for FormatStmtIf {
fn fmt_fields(&self, item: &StmtIf, f: &mut PyFormatter) -> FormatResult<()> {
let StmtIf {
range: _,
test,
body,
elif_else_clauses,
} = item;
let comments = f.context().comments().clone();
let trailing_colon_comment = comments.dangling_comments(item);
let mut current = IfOrElIf::If(item);
let mut else_comments: &[SourceComment];
let mut last_node_of_previous_body = None;
write!(
f,
[
text("if"),
space(),
maybe_parenthesize_expression(test, item, Parenthesize::IfBreaks),
text(":"),
trailing_comments(trailing_colon_comment),
block_indent(&body.format())
]
)?;
loop {
let current_statement = current.statement();
let StmtIf {
test, body, orelse, ..
} = current_statement;
let first_statement = body.first().ok_or(FormatError::SyntaxError)?;
let trailing = comments.dangling_comments(current_statement);
let trailing_if_comments_end = trailing
.partition_point(|comment| comment.slice().start() < first_statement.start());
let (if_trailing_comments, trailing_alternate_comments) =
trailing.split_at(trailing_if_comments_end);
if current.is_elif() {
let elif_leading = comments.leading_comments(current_statement);
// Manually format the leading comments because the formatting bypasses `NodeRule::fmt`
write!(
f,
[
leading_alternate_branch_comments(elif_leading, last_node_of_previous_body),
source_position(current_statement.start())
]
)?;
}
write!(
f,
[
text(current.keyword()),
space(),
maybe_parenthesize_expression(test, current_statement, Parenthesize::IfBreaks),
text(":"),
trailing_comments(if_trailing_comments),
block_indent(&body.format())
]
)?;
// RustPython models `elif` by setting the body to a single `if` statement. The `orelse`
// of the most inner `if` statement then becomes the `else` of the whole `if` chain.
// That's why it's necessary to take the comments here from the most inner `elif`.
else_comments = trailing_alternate_comments;
last_node_of_previous_body = body.last();
if let Some(elif) = else_if(orelse) {
current = elif;
} else {
break;
}
}
let orelse = &current.statement().orelse;
if !orelse.is_empty() {
// Leading comments are always own line comments
let leading_else_comments_end =
else_comments.partition_point(|comment| comment.line_position().is_own_line());
let (else_leading, else_trailing) = else_comments.split_at(leading_else_comments_end);
write!(
f,
[
leading_alternate_branch_comments(else_leading, last_node_of_previous_body),
text("else:"),
trailing_comments(else_trailing),
block_indent(&orelse.format())
]
)?;
let mut last_node = body.last().unwrap().into();
for clause in elif_else_clauses {
format_elif_else_clause(clause, f, Some(last_node))?;
last_node = clause.body.last().unwrap().into();
}
Ok(())
@ -97,35 +49,56 @@ impl FormatNodeRule<StmtIf> for FormatStmtIf {
}
}
fn else_if(or_else: &Suite) -> Option<IfOrElIf> {
if let [Stmt::If(if_stmt)] = or_else.as_slice() {
Some(IfOrElIf::ElIf(if_stmt))
/// Note that this implementation misses the leading newlines before the leading comments because
/// it does not have access to the last node of the previous branch. The `StmtIf` therefore doesn't
/// call this but `format_elif_else_clause` directly.
#[derive(Default)]
pub struct FormatElifElseClause;
impl FormatNodeRule<ElifElseClause> for FormatElifElseClause {
fn fmt_fields(&self, item: &ElifElseClause, f: &mut PyFormatter) -> FormatResult<()> {
format_elif_else_clause(item, f, None)
}
}
/// Extracted so we can implement `FormatElifElseClause` but also pass in `last_node` from
/// `FormatStmtIf`
fn format_elif_else_clause(
item: &ElifElseClause,
f: &mut PyFormatter,
last_node: Option<AnyNodeRef>,
) -> FormatResult<()> {
let ElifElseClause {
range: _,
test,
body,
} = item;
let comments = f.context().comments().clone();
let trailing_colon_comment = comments.dangling_comments(item);
let leading_comments = comments.leading_comments(item);
leading_alternate_branch_comments(leading_comments, last_node).fmt(f)?;
if let Some(test) = test {
write!(
f,
[
text("elif"),
space(),
maybe_parenthesize_expression(test, item, Parenthesize::IfBreaks),
]
)?;
} else {
None
}
}
enum IfOrElIf<'a> {
If(&'a StmtIf),
ElIf(&'a StmtIf),
}
impl<'a> IfOrElIf<'a> {
const fn statement(&self) -> &'a StmtIf {
match self {
IfOrElIf::If(statement) => statement,
IfOrElIf::ElIf(statement) => statement,
}
}
const fn keyword(&self) -> &'static str {
match self {
IfOrElIf::If(_) => "if",
IfOrElIf::ElIf(_) => "elif",
}
}
const fn is_elif(&self) -> bool {
matches!(self, IfOrElIf::ElIf(_))
text("else").fmt(f)?;
}
write!(
f,
[
text(":"),
trailing_comments(trailing_colon_comment),
block_indent(&body.format())
]
)
}