This repository was archived by the owner on Jul 27, 2023. It is now read-only.
Repository navigation
Refactor if statement, introduce nodes for elif and else - #22
Merged
Merged
Conversation
Member
Author
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
konstin
marked this pull request as ready for review
July 1, 2023 19:59
MichaReiser
approved these changes
Jul 3, 2023
MichaReiser
left a comment
MichaReiser
left a comment
Member
There was a problem hiding this comment.
Can you run the parser benchmarks locally to get an idea of the runtime impact of the change?
Comment on lines
+634
to
+681
| impl Node for ElifElseClause { | ||
| const NAME: &'static str = "ElifElse"; | ||
| const FIELD_NAMES: &'static [&'static str] = &["test", "body"]; | ||
| } |
Member
There was a problem hiding this comment.
Unrelated to your change (and this PR): I think we can remove this. It was added for the Pyo3 support which I believe we removed too?
konstin
force-pushed
the
refactor_if_statement
branch
from
July 18, 2023 08:05
5a6e396 to
6a13314
Compare
konstin
added a commit
to astral-sh/ruff
that referenced
this pull request
Jul 18, 2023
## 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
astral-sh/RustPython-Parser#20
* MH: Review and merge or move later in stack
astral-sh/RustPython-Parser#21
* MH: Review and approve
astral-sh/RustPython-Parser#22
* MH: Review and approve formatter PR
#5459
* CM: Review and approve linter PR
#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 astral-sh/RustPython-Parser#22
* Create tag in the parser, update linter+formatter PR
* Merge linter+formatter PR #5459
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
evanrittenhouse
pushed a commit
to evanrittenhouse/ruff
that referenced
this pull request
Jul 19, 2023
## 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
astral-sh/RustPython-Parser#20
* MH: Review and merge or move later in stack
astral-sh/RustPython-Parser#21
* MH: Review and approve
astral-sh/RustPython-Parser#22
* MH: Review and approve formatter PR
astral-sh#5459
* CM: Review and approve linter PR
astral-sh#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 astral-sh/RustPython-Parser#22
* Create tag in the parser, update linter+formatter PR
* Merge linter+formatter PR astral-sh#5459
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
konstin
added a commit
to astral-sh/ruff
that referenced
this pull request
Jul 19, 2023
## 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
astral-sh/RustPython-Parser#20
* MH: Review and merge or move later in stack
astral-sh/RustPython-Parser#21
* MH: Review and approve
astral-sh/RustPython-Parser#22
* MH: Review and approve formatter PR
#5459
* CM: Review and approve linter PR
#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 astral-sh/RustPython-Parser#22
* Create tag in the parser, update linter+formatter PR
* Merge linter+formatter PR #5459
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The old if layout couldn't differentiate between an else block with a single if statement and an elif statement. Additionally we getting rid of the recursion in favor of a single if struct with a vec of elif/else children. This is accompanied by a big refactoring in ruff which removes a bunch of TODOs and false negatives.