Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Don't mistake a following if for an elif #5296

Merged
merged 2 commits into from
Jun 23, 2023
Merged

Don't mistake a following if for an elif #5296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b963c7f
Fix formatter panic with comment after parenthesized dict value
konstin Jun 22, 2023
97b027a
Don't mistake a following if for an elif
konstin Jun 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/dict.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,11 @@
C: 0.1 * (10.0 / 12),
D: 0.1 * (10.0 / 12),
}

# Regression test for formatter panic with comment after parenthesized dict value
# Originally found in https://github.com/bolucat/Firefox/blob/636a717ef025c16434997dc89e42351ef740ee6b/testing/marionette/client/marionette_driver/geckoinstance.py#L109
a = {
Copy link
Member

Choose a reason for hiding this comment

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

This fix seems unrelated to if/elif right?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's already merged: https://github.com/astral-sh/ruff/pull/5293/files#diff-eb676c05c3588ddfa4f15b679292a4418e3b3d1bc5e17ce2ad9e150ff128097aR53

Copy link
Member Author

Choose a reason for hiding this comment

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

just confirmed, it's also not part of the merge commit

1: (2),
# comment
3: True,
}
9 changes: 9 additions & 0 deletions crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,12 @@
pass
# Don't drop this comment 3
x = 3

# Regression test for a following if that could get confused for an elif
# Originally found in https://github.com/gradio-app/gradio/blob/1570b94a02d23d051ae137e0063974fd8a48b34e/gradio/external.py#L478
if True:
pass
else: # Comment
if False:
pass
pass
93 changes: 61 additions & 32 deletions crates/ruff_python_formatter/src/comments/placement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,38 +335,37 @@ fn handle_in_between_bodies_end_of_line_comment<'a>(
return CommentPlacement::Default(comment);
}

if !locator.contains_line_break(TextRange::new(preceding.end(), comment.slice().start())) {
// Trailing comment of the preceding statement
if locator.contains_line_break(TextRange::new(preceding.end(), comment.slice().start())) {
// The `elif` or except handlers have their own body to which we can attach the trailing comment
// ```python
// while test:
// a # comment
// else:
// if test:
// a
// elif c: # comment
// b
// ```
if preceding.is_node_with_body() {
// We can't set this as a trailing comment of the function declaration because it
// will then move behind the function block instead of sticking with the pass
if following.is_except_handler() {
return CommentPlacement::trailing(following, comment);
} else if following.is_stmt_if() {
// We have to exclude for following if statements that are not elif by checking the
// indentation
// ```python
// if True:
// def f():
// pass # a
// else:
// pass
// else: # Comment
// if False:
// pass
// pass
// ```
CommentPlacement::Default(comment)
} else {
CommentPlacement::trailing(preceding, comment)
let base_if_indent =
Copy link
Member

Choose a reason for hiding this comment

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

Just an idea. It may be annoying to do because of all the type narrowing that's necessary and you would need to identify the right parent body but I thought it may be worth mentioning. The way to normally detect whether it is an elif or not... oh no.... It is not. These two programs have the same AST

if True:
    pass
elif False:
    pass
If(
    StmtIf {
        range: 0..38,
        test: Constant(
            ExprConstant {
                range: 3..7,
                value: Bool(
                    true,
                ),
                kind: None,
            },
        ),
        body: [
            Pass(
                StmtPass {
                    range: 13..17,
                },
            ),
        ],
        orelse: [
            If(
                StmtIf {
                    range: 18..38,
                    test: Constant(
                        ExprConstant {
                            range: 23..28,
                            value: Bool(
                                false,
                            ),
                            kind: None,
                        },
                    ),
                    body: [
                        Pass(
                            StmtPass {
                                range: 34..38,
                            },
                        ),
                    ],
                    orelse: [],
                },
            ),
        ],
    },
),

if True:
    pass
else:
    if False:
        pass
If(
        StmtIf {
            range: 41..91,
            test: Constant(
                ExprConstant {
                    range: 44..48,
                    value: Bool(
                        true,
                    ),
                    kind: None,
                },
            ),
            body: [
                Pass(
                    StmtPass {
                        range: 54..58,
                    },
                ),
            ],
            orelse: [
                If(
                    StmtIf {
                        range: 69..91,
                        test: Constant(
                            ExprConstant {
                                range: 72..77,
                                value: Bool(
                                    false,
                                ),
                                kind: None,
                            },
                        ),
                        body: [
                            Pass(
                                StmtPass {
                                    range: 87..91,
                                },
                            ),
                        ],
                        orelse: [],
                    },
                ),
            ],
        },
    ),

whitespace::indentation_at_offset(locator, following.range().start());
let maybe_elif_indent = whitespace::indentation_at_offset(
locator,
comment.enclosing_node().range().start(),
);
if base_if_indent == maybe_elif_indent {
return CommentPlacement::trailing(following, comment);
}
}
} else if following.is_stmt_if() || following.is_except_handler() {
// The `elif` or except handlers have their own body to which we can attach the trailing comment
// ```python
// if test:
// a
// elif c: # comment
// b
// ```
CommentPlacement::trailing(following, comment)
} else {
// There are no bodies for the "else" branch and other bodies that are represented as a `Vec<Stmt>`.
// This means, there's no good place to attach the comments to.
// Make this a dangling comments and manually format the comment in
Expand All @@ -381,6 +380,28 @@ fn handle_in_between_bodies_end_of_line_comment<'a>(
// print("nooop")
// ```
CommentPlacement::dangling(comment.enclosing_node(), comment)
} else {
// Trailing comment of the preceding statement
// ```python
// while test:
// a # comment
// else:
// b
// ```
if preceding.is_node_with_body() {
// We can't set this as a trailing comment of the function declaration because it
// will then move behind the function block instead of sticking with the pass
// ```python
// if True:
// def f():
// pass # a
// else:
// pass
// ```
CommentPlacement::Default(comment)
} else {
CommentPlacement::trailing(preceding, comment)
}
}
} else {
CommentPlacement::Default(comment)
Expand Down Expand Up @@ -991,14 +1012,22 @@ fn handle_dict_unpacking_comment<'a>(
.skip_trivia();

// we start from the preceding node but we skip its token
if let Some(first) = tokens.next() {
debug_assert!(matches!(
first,
Token {
kind: TokenKind::LBrace | TokenKind::Comma | TokenKind::Colon,
..
}
));
for token in tokens.by_ref() {
// Skip closing parentheses that are not part of the node range
if token.kind == TokenKind::RParen {
continue;
}
debug_assert!(
matches!(
token,
Token {
kind: TokenKind::LBrace | TokenKind::Comma | TokenKind::Colon,
..
}
),
"{token:?}",
);
break;
}

// if the remaining tokens from the previous node is exactly `**`,
Expand Down
16 changes: 16 additions & 0 deletions ...formatter/src/snapshots/ruff_python_formatter__tests__ruff_test__expression__dict_py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,14 @@ mapping = {
C: 0.1 * (10.0 / 12),
D: 0.1 * (10.0 / 12),
}

# Regression test for formatter panic with comment after parenthesized dict value
# Originally found in https://github.com/bolucat/Firefox/blob/636a717ef025c16434997dc89e42351ef740ee6b/testing/marionette/client/marionette_driver/geckoinstance.py#L109
a = {
1: (2),
# comment
3: True,
}
```


Expand Down Expand Up @@ -112,6 +120,14 @@ mapping = {
C: 0.1 * (10.0 / 12),
D: 0.1 * (10.0 / 12),
}

# Regression test for formatter panic with comment after parenthesized dict value
# Originally found in https://github.com/bolucat/Firefox/blob/636a717ef025c16434997dc89e42351ef740ee6b/testing/marionette/client/marionette_driver/geckoinstance.py#L109
a = {
1: (2),
# comment
3: True,
}
```


18 changes: 18 additions & 0 deletions ...on_formatter/src/snapshots/ruff_python_formatter__tests__ruff_test__statement__if_py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,15 @@ else:
pass
# Don't drop this comment 3
x = 3

# Regression test for a following if that could get confused for an elif
# Originally found in https://github.com/gradio-app/gradio/blob/1570b94a02d23d051ae137e0063974fd8a48b34e/gradio/external.py#L478
if True:
pass
else: # Comment
if False:
pass
pass
```


Expand Down Expand Up @@ -137,6 +146,15 @@ else:
pass
# Don't drop this comment 3
x = 3

# Regression test for a following if that could get confused for an elif
# Originally found in https://github.com/gradio-app/gradio/blob/1570b94a02d23d051ae137e0063974fd8a48b34e/gradio/external.py#L478
if True:
pass
else: # Comment
if False:
pass
pass
```