Skip to content

Commit

Permalink
Don't reorder parameters in function calls (#7268)
Browse files Browse the repository at this point in the history
## Summary

In `f(*args, a=b, *args2, **kwargs)` the args (`*args`, `*args2`) and
keywords (`a=b`, `**kwargs`) are interleaved, which we previously didn't
handle.

Fixes #6498

**main**

| project | similarity index | total files | changed files |

|--------------|------------------:|------------------:|------------------:|
| cpython | 0.76083 | 1789 | 1632 |
| **django** | 0.99966 | 2760 | 58 |
| transformers | 0.99930 | 2587 | 447 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99983 | 3496 | 18 |
| warehouse | 0.99825 | 648 | 22 |
| zulip | 0.99950 | 1437 | 27 |

**PR**

| project | similarity index | total files | changed files |

|--------------|------------------:|------------------:|------------------:|
| cpython | 0.76083 | 1789 | 1632 |
| **django** | 0.99967 | 2760 | 53 |
| transformers | 0.99930 | 2587 | 447 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99983 | 3496 | 18 |
| warehouse | 0.99825 | 648 | 22 |
| zulip | 0.99950 | 1437 | 27 |


## Test Plan

New fixtures
  • Loading branch information
konstin authored Sep 13, 2023
1 parent 56440ad commit f4c7bff
Show file tree
Hide file tree
Showing 6 changed files with 203 additions and 129 deletions.
2 changes: 1 addition & 1 deletion crates/ruff_python_ast/src/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ pub fn walk_format_spec<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, format_spe
pub fn walk_arguments<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, arguments: &'a Arguments) {
// Note that the there might be keywords before the last arg, e.g. in
// f(*args, a=2, *args2, **kwargs)`, but we follow Python in evaluating first `args` and then
// `keywords`. See also [Arguments::arguments_as_declared`].
// `keywords`. See also [Arguments::arguments_source_order`].
for arg in &arguments.args {
visitor.visit_expr(arg);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,3 +242,26 @@ def f(*args, **kwargs):
# comment
1
))

args = [2]
args2 = [3]
kwargs = {"4": 5}

# https://github.com/astral-sh/ruff/issues/6498
f(a=1, *args, **kwargs)
f(*args, a=1, **kwargs)
f(*args, a=1, *args2, **kwargs)
f( # a
* # b
args
# c
, # d
a=1,
# e
* # f
args2
# g
** # h
kwargs,
)

18 changes: 7 additions & 11 deletions crates/ruff_python_formatter/src/comments/placement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ pub(super) fn place_comment<'a>(
/// ):
/// ...
/// ```
/// The parentheses enclose `True`, but the range of `True`doesn't include the `# comment`.
/// The parentheses enclose `True`, but the range of `True` doesn't include the `# comment`.
///
/// Default handling can get parenthesized comments wrong in a number of ways. For example, the
/// comment here is marked (by default) as a trailing comment of `x`, when it should be a leading
Expand Down Expand Up @@ -120,10 +120,8 @@ fn handle_parenthesized_comment<'a>(
// For now, we _can_ assert, but to do so, we stop lexing when we hit a token that precedes an
// identifier.
if comment.line_position().is_end_of_line() {
let tokenizer = SimpleTokenizer::new(
locator.contents(),
TextRange::new(preceding.end(), comment.start()),
);
let range = TextRange::new(preceding.end(), comment.start());
let tokenizer = SimpleTokenizer::new(locator.contents(), range);
if tokenizer
.skip_trivia()
.take_while(|token| {
Expand All @@ -136,7 +134,7 @@ fn handle_parenthesized_comment<'a>(
debug_assert!(
!matches!(token.kind, SimpleTokenKind::Bogus),
"Unexpected token between nodes: `{:?}`",
locator.slice(TextRange::new(preceding.end(), comment.start()),)
locator.slice(range)
);

token.kind() == SimpleTokenKind::LParen
Expand All @@ -145,10 +143,8 @@ fn handle_parenthesized_comment<'a>(
return CommentPlacement::leading(following, comment);
}
} else {
let tokenizer = SimpleTokenizer::new(
locator.contents(),
TextRange::new(comment.end(), following.start()),
);
let range = TextRange::new(comment.end(), following.start());
let tokenizer = SimpleTokenizer::new(locator.contents(), range);
if tokenizer
.skip_trivia()
.take_while(|token| {
Expand All @@ -161,7 +157,7 @@ fn handle_parenthesized_comment<'a>(
debug_assert!(
!matches!(token.kind, SimpleTokenKind::Bogus),
"Unexpected token between nodes: `{:?}`",
locator.slice(TextRange::new(comment.end(), following.start()))
locator.slice(range)
);
token.kind() == SimpleTokenKind::RParen
})
Expand Down
39 changes: 23 additions & 16 deletions crates/ruff_python_formatter/src/other/arguments.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use ruff_formatter::write;
use ruff_python_ast::node::AstNode;
use ruff_python_ast::{Arguments, Expr};
use ruff_python_ast::{ArgOrKeyword, Arguments, Expr};
use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer};
use ruff_text_size::{Ranged, TextRange, TextSize};

Expand All @@ -14,24 +13,29 @@ pub struct FormatArguments;

impl FormatNodeRule<Arguments> for FormatArguments {
fn fmt_fields(&self, item: &Arguments, f: &mut PyFormatter) -> FormatResult<()> {
let Arguments {
range,
args,
keywords,
} = item;
// We have a case with `f()` without any argument, which is a special case because we can
// have a comment with no node attachment inside:
// ```python
// f(
// # This call has a dangling comment.
// )
// ```
if item.args.is_empty() && item.keywords.is_empty() {
if args.is_empty() && keywords.is_empty() {
let comments = f.context().comments().clone();
let dangling = comments.dangling(item);
return write!(f, [empty_parenthesized("(", dangling, ")")]);
}

let all_arguments = format_with(|f: &mut PyFormatter| {
let source = f.context().source();
let mut joiner = f.join_comma_separated(item.end());
match item.args.as_slice() {
[arg] if item.keywords.is_empty() => {
let mut joiner = f.join_comma_separated(range.end());
match args.as_slice() {
[arg] if keywords.is_empty() => {
match arg {
Expr::GeneratorExp(generator_exp) => joiner.entry(
generator_exp,
Expand All @@ -41,7 +45,7 @@ impl FormatNodeRule<Arguments> for FormatArguments {
),
other => {
let parentheses =
if is_single_argument_parenthesized(arg, item.end(), source) {
if is_single_argument_parenthesized(arg, range.end(), source) {
Parentheses::Always
} else {
// Note: no need to handle opening-parenthesis comments, since
Expand All @@ -53,14 +57,17 @@ impl FormatNodeRule<Arguments> for FormatArguments {
}
};
}
args => {
joiner
.entries(
// We have the parentheses from the call so the item never need any
args.iter()
.map(|arg| (arg, arg.format().with_options(Parentheses::Preserve))),
)
.nodes(item.keywords.iter());
_ => {
for arg_or_keyword in item.arguments_source_order() {
match arg_or_keyword {
ArgOrKeyword::Arg(arg) => {
joiner.entry(arg, &arg.format());
}
ArgOrKeyword::Keyword(keyword) => {
joiner.entry(keyword, &keyword.format());
}
}
}
}
}

Expand All @@ -76,7 +83,7 @@ impl FormatNodeRule<Arguments> for FormatArguments {
// c,
// )
let comments = f.context().comments().clone();
let dangling_comments = comments.dangling(item.as_any_node_ref());
let dangling_comments = comments.dangling(item);

write!(
f,
Expand Down
Loading

0 comments on commit f4c7bff

Please sign in to comment.