From 84dd7db9c367e4aeeb2fbfc156a1f760787871e6 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 20 Jun 2023 08:32:15 +0200 Subject: [PATCH] Fix ArgWithDefault comments handling --- Cargo.lock | 12 +++---- Cargo.toml | 10 +++--- crates/ruff_python_ast/src/node.rs | 2 +- .../test/fixtures/ruff/statement/function.py | 7 ++++ crates/ruff_python_formatter/src/cli.rs | 2 +- .../src/comments/placement.rs | 9 ++--- ...sitional_arguments_slash_on_same_line.snap | 6 ++-- ...on_positional_arguments_with_defaults.snap | 12 +++---- ...sts__positional_argument_only_comment.snap | 4 +-- ...t_only_comment_without_following_node.snap | 2 +- ...l_argument_only_leading_comma_comment.snap | 4 +-- ...comments__tests__trailing_after_comma.snap | 2 +- .../src/comments/visitor.rs | 7 ++++ crates/ruff_python_formatter/src/lib.rs | 11 ++++--- .../src/other/arg_with_default.rs | 5 ++- .../src/other/arguments.rs | 33 +++++-------------- ...ts__ruff_test__statement__function_py.snap | 14 ++++++++ 17 files changed, 76 insertions(+), 66 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 62ddd695153124..67bb6f749d6763 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2105,7 +2105,7 @@ dependencies = [ [[package]] name = "ruff_text_size" version = "0.0.0" -source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=8d74eee75031b68d2204219963fae54a3f31a394#8d74eee75031b68d2204219963fae54a3f31a394" +source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=db0735c95186c5daf5dfa626b724687a3a2285de#db0735c95186c5daf5dfa626b724687a3a2285de" dependencies = [ "schemars", "serde", @@ -2183,7 +2183,7 @@ dependencies = [ [[package]] name = "rustpython-ast" version = "0.2.0" -source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=8d74eee75031b68d2204219963fae54a3f31a394#8d74eee75031b68d2204219963fae54a3f31a394" +source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=db0735c95186c5daf5dfa626b724687a3a2285de#db0735c95186c5daf5dfa626b724687a3a2285de" dependencies = [ "is-macro", "num-bigint", @@ -2194,7 +2194,7 @@ dependencies = [ [[package]] name = "rustpython-format" version = "0.2.0" -source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=8d74eee75031b68d2204219963fae54a3f31a394#8d74eee75031b68d2204219963fae54a3f31a394" +source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=db0735c95186c5daf5dfa626b724687a3a2285de#db0735c95186c5daf5dfa626b724687a3a2285de" dependencies = [ "bitflags 2.3.1", "itertools", @@ -2206,7 +2206,7 @@ dependencies = [ [[package]] name = "rustpython-literal" version = "0.2.0" -source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=8d74eee75031b68d2204219963fae54a3f31a394#8d74eee75031b68d2204219963fae54a3f31a394" +source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=db0735c95186c5daf5dfa626b724687a3a2285de#db0735c95186c5daf5dfa626b724687a3a2285de" dependencies = [ "hexf-parse", "is-macro", @@ -2218,7 +2218,7 @@ dependencies = [ [[package]] name = "rustpython-parser" version = "0.2.0" -source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=8d74eee75031b68d2204219963fae54a3f31a394#8d74eee75031b68d2204219963fae54a3f31a394" +source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=db0735c95186c5daf5dfa626b724687a3a2285de#db0735c95186c5daf5dfa626b724687a3a2285de" dependencies = [ "anyhow", "is-macro", @@ -2241,7 +2241,7 @@ dependencies = [ [[package]] name = "rustpython-parser-core" version = "0.2.0" -source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=8d74eee75031b68d2204219963fae54a3f31a394#8d74eee75031b68d2204219963fae54a3f31a394" +source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=db0735c95186c5daf5dfa626b724687a3a2285de#db0735c95186c5daf5dfa626b724687a3a2285de" dependencies = [ "is-macro", "memchr", diff --git a/Cargo.toml b/Cargo.toml index b1bd3edc84e9ff..1127e8317ff1a8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -36,11 +36,11 @@ proc-macro2 = { version = "1.0.51" } quote = { version = "1.0.23" } regex = { version = "1.7.1" } rustc-hash = { version = "1.1.0" } -ruff_text_size = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "8d74eee75031b68d2204219963fae54a3f31a394" } -rustpython-ast = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "8d74eee75031b68d2204219963fae54a3f31a394" , default-features = false, features = ["all-nodes-with-ranges", "num-bigint"]} -rustpython-format = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "8d74eee75031b68d2204219963fae54a3f31a394", default-features = false, features = ["num-bigint"] } -rustpython-literal = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "8d74eee75031b68d2204219963fae54a3f31a394" } -rustpython-parser = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "8d74eee75031b68d2204219963fae54a3f31a394" , default-features = false, features = ["full-lexer", "all-nodes-with-ranges", "num-bigint"] } +ruff_text_size = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "db0735c95186c5daf5dfa626b724687a3a2285de" } +rustpython-ast = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "db0735c95186c5daf5dfa626b724687a3a2285de" , default-features = false, features = ["all-nodes-with-ranges", "num-bigint"]} +rustpython-format = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "db0735c95186c5daf5dfa626b724687a3a2285de", default-features = false, features = ["num-bigint"] } +rustpython-literal = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "db0735c95186c5daf5dfa626b724687a3a2285de" } +rustpython-parser = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "db0735c95186c5daf5dfa626b724687a3a2285de" , default-features = false, features = ["full-lexer", "all-nodes-with-ranges", "num-bigint"] } schemars = { version = "0.8.12" } serde = { version = "1.0.152", features = ["derive"] } serde_json = { version = "1.0.93" } diff --git a/crates/ruff_python_ast/src/node.rs b/crates/ruff_python_ast/src/node.rs index b9a6654753c68e..9928fa5d2aa5c4 100644 --- a/crates/ruff_python_ast/src/node.rs +++ b/crates/ruff_python_ast/src/node.rs @@ -3680,7 +3680,7 @@ impl AnyNodeRef<'_> { /// Compares two any node refs by their pointers (referential equality). pub fn ptr_eq(self, other: AnyNodeRef) -> bool { - self.as_ptr().eq(&other.as_ptr()) + self.as_ptr().eq(&other.as_ptr()) && self.kind() == other.kind() } /// Returns the node's [`kind`](NodeKind) that has no data associated and is [`Copy`]. diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/function.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/function.py index 8b540fe646d3dd..674b9ef43e06a8 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/function.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/function.py @@ -90,3 +90,10 @@ def f2(): # Regression test for https://github.com/python/cpython/blob/7199584ac8632eab57612f595a7162ab8d2ebbc0/Lib/warnings.py#L513 def f(arg1=1, *, kwonlyarg1, kwonlyarg2=2): pass + + +# Regression test for https://github.com/astral-sh/ruff/issues/5176#issuecomment-1598171989 +def foo( + b=3 + 2 # comment +): + ... diff --git a/crates/ruff_python_formatter/src/cli.rs b/crates/ruff_python_formatter/src/cli.rs index f605e6640768a9..8a277ece242b03 100644 --- a/crates/ruff_python_formatter/src/cli.rs +++ b/crates/ruff_python_formatter/src/cli.rs @@ -63,7 +63,7 @@ pub fn format_and_debug_print(input: &str, cli: &Cli) -> Result { } if cli.print_comments { println!( - "{:?}", + "{:#?}", formatted.context().comments().debug(SourceCode::new(input)) ); } diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index 2bfe71690a5c60..e6c04dafd8675c 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -622,13 +622,8 @@ fn handle_positional_only_arguments_separator_comment<'a>( return CommentPlacement::Default(comment); }; - let is_last_positional_argument = are_same_optional(last_argument_or_default, arguments.posonlyargs.last()) - // If the preceding node is the default for the last positional argument - // ```python - // def test(a=10, /, b): pass - // ``` - || are_same_optional(last_argument_or_default, arguments - .posonlyargs.last().and_then(|arg| arg.default.as_deref())); + let is_last_positional_argument = + are_same_optional(last_argument_or_default, arguments.posonlyargs.last()); if !is_last_positional_argument { return CommentPlacement::Default(comment); diff --git a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__non_positional_arguments_slash_on_same_line.snap b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__non_positional_arguments_slash_on_same_line.snap index ab41a810a3bf55..6764fffaaaba81 100644 --- a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__non_positional_arguments_slash_on_same_line.snap +++ b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__non_positional_arguments_slash_on_same_line.snap @@ -19,9 +19,9 @@ expression: comments.debug(test_case.source_code) "trailing": [], }, Node { - kind: Arg, - range: 90..91, - source: `b`, + kind: ArgWithDefault, + range: 90..94, + source: `b=20`, }: { "leading": [ SourceComment { diff --git a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__non_positional_arguments_with_defaults.snap b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__non_positional_arguments_with_defaults.snap index a4fde5b30d0c22..6fa4a9c9d03703 100644 --- a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__non_positional_arguments_with_defaults.snap +++ b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__non_positional_arguments_with_defaults.snap @@ -24,9 +24,9 @@ expression: comments.debug(test_case.source_code) "trailing": [], }, Node { - kind: ExprConstant, - range: 17..19, - source: `10`, + kind: ArgWithDefault, + range: 15..19, + source: `a=10`, }: { "leading": [], "dangling": [], @@ -39,9 +39,9 @@ expression: comments.debug(test_case.source_code) ], }, Node { - kind: Arg, - range: 173..174, - source: `b`, + kind: ArgWithDefault, + range: 173..177, + source: `b=20`, }: { "leading": [ SourceComment { diff --git a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__positional_argument_only_comment.snap b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__positional_argument_only_comment.snap index ff26fa729232ad..aa1f34eb1f1534 100644 --- a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__positional_argument_only_comment.snap +++ b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__positional_argument_only_comment.snap @@ -24,7 +24,7 @@ expression: comments.debug(test_case.source_code) "trailing": [], }, Node { - kind: Arg, + kind: ArgWithDefault, range: 15..16, source: `a`, }: { @@ -39,7 +39,7 @@ expression: comments.debug(test_case.source_code) ], }, Node { - kind: Arg, + kind: ArgWithDefault, range: 166..167, source: `b`, }: { diff --git a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__positional_argument_only_comment_without_following_node.snap b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__positional_argument_only_comment_without_following_node.snap index 2996b7bee1413a..2c93c46b007b76 100644 --- a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__positional_argument_only_comment_without_following_node.snap +++ b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__positional_argument_only_comment_without_following_node.snap @@ -24,7 +24,7 @@ expression: comments.debug(test_case.source_code) "trailing": [], }, Node { - kind: Arg, + kind: ArgWithDefault, range: 15..16, source: `a`, }: { diff --git a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__positional_argument_only_leading_comma_comment.snap b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__positional_argument_only_leading_comma_comment.snap index ff26fa729232ad..aa1f34eb1f1534 100644 --- a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__positional_argument_only_leading_comma_comment.snap +++ b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__positional_argument_only_leading_comma_comment.snap @@ -24,7 +24,7 @@ expression: comments.debug(test_case.source_code) "trailing": [], }, Node { - kind: Arg, + kind: ArgWithDefault, range: 15..16, source: `a`, }: { @@ -39,7 +39,7 @@ expression: comments.debug(test_case.source_code) ], }, Node { - kind: Arg, + kind: ArgWithDefault, range: 166..167, source: `b`, }: { diff --git a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__trailing_after_comma.snap b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__trailing_after_comma.snap index 030b63f38b2af6..5a2c9222079321 100644 --- a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__trailing_after_comma.snap +++ b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__trailing_after_comma.snap @@ -4,7 +4,7 @@ expression: comments.debug(test_case.source_code) --- { Node { - kind: Arg, + kind: ArgWithDefault, range: 15..16, source: `a`, }: { diff --git a/crates/ruff_python_formatter/src/comments/visitor.rs b/crates/ruff_python_formatter/src/comments/visitor.rs index 26850b409c15a4..55ce3f42fcb5eb 100644 --- a/crates/ruff_python_formatter/src/comments/visitor.rs +++ b/crates/ruff_python_formatter/src/comments/visitor.rs @@ -236,6 +236,13 @@ impl<'ast> PreorderVisitor<'ast> for CommentsVisitor<'ast> { self.finish_node(arg); } + fn visit_arg_with_default(&mut self, arg_with_default: &'ast ArgWithDefault) { + if self.start_node(arg_with_default).is_traverse() { + walk_arg_with_default(self, arg_with_default); + } + self.finish_node(arg_with_default); + } + fn visit_keyword(&mut self, keyword: &'ast Keyword) { if self.start_node(keyword).is_traverse() { walk_keyword(self, keyword); diff --git a/crates/ruff_python_formatter/src/lib.rs b/crates/ruff_python_formatter/src/lib.rs index 6ad37c5665c5d3..f09dcde000e8b2 100644 --- a/crates/ruff_python_formatter/src/lib.rs +++ b/crates/ruff_python_formatter/src/lib.rs @@ -412,10 +412,12 @@ Formatted twice: #[test] fn quick_test() { let src = r#" -def test(): ... -# Comment -def with_leading_comment(): ... +def foo( + b=3 + + 2 # comment +): + ... "#; // Tokenize once let mut tokens = Vec::new(); @@ -437,10 +439,11 @@ def with_leading_comment(): ... // Uncomment the `dbg` to print the IR. // Use `dbg_write!(f, []) instead of `write!(f, [])` in your formatting code to print some IR // inside of a `Format` implementation + // use ruff_formatter::FormatContext; // dbg!(formatted // .document() // .display(formatted.context().source_code())); - + // // dbg!(formatted // .context() // .comments() diff --git a/crates/ruff_python_formatter/src/other/arg_with_default.rs b/crates/ruff_python_formatter/src/other/arg_with_default.rs index 2dc9993407adca..c6badb551a002b 100644 --- a/crates/ruff_python_formatter/src/other/arg_with_default.rs +++ b/crates/ruff_python_formatter/src/other/arg_with_default.rs @@ -1,6 +1,5 @@ -use rustpython_parser::ast::ArgWithDefault; - use ruff_formatter::write; +use rustpython_parser::ast::ArgWithDefault; use crate::prelude::*; use crate::FormatNodeRule; @@ -20,7 +19,7 @@ impl FormatNodeRule for FormatArgWithDefault { if let Some(default) = default { let space = def.annotation.is_some().then_some(space()); - write!(f, [space, text("="), space, default.format()])?; + write!(f, [space, text("="), space, group(&default.format())])?; } Ok(()) diff --git a/crates/ruff_python_formatter/src/other/arguments.rs b/crates/ruff_python_formatter/src/other/arguments.rs index 63cc7859b94b29..ae1a08686c2055 100644 --- a/crates/ruff_python_formatter/src/other/arguments.rs +++ b/crates/ruff_python_formatter/src/other/arguments.rs @@ -34,14 +34,9 @@ impl FormatNodeRule for FormatArguments { let mut last_node: Option = None; for arg_with_default in posonlyargs { - joiner.entry(&arg_with_default.into_format()); - - last_node = Some( - arg_with_default - .default - .as_deref() - .map_or_else(|| (&arg_with_default.def).into(), AnyNodeRef::from), - ); + joiner.entry(&arg_with_default.format()); + + last_node = Some(arg_with_default.into()); } if !posonlyargs.is_empty() { @@ -49,14 +44,9 @@ impl FormatNodeRule for FormatArguments { } for arg_with_default in args { - joiner.entry(&arg_with_default.into_format()); - - last_node = Some( - arg_with_default - .default - .as_deref() - .map_or_else(|| (&arg_with_default.def).into(), AnyNodeRef::from), - ); + joiner.entry(&arg_with_default.format()); + + last_node = Some(arg_with_default.into()); } // kw only args need either a `*args` ahead of them capturing all var args or a `*` @@ -74,14 +64,9 @@ impl FormatNodeRule for FormatArguments { } for arg_with_default in kwonlyargs { - joiner.entry(&arg_with_default.into_format()); - - last_node = Some( - arg_with_default - .default - .as_deref() - .map_or_else(|| (&arg_with_default.def).into(), AnyNodeRef::from), - ); + joiner.entry(&arg_with_default.format()); + + last_node = Some(arg_with_default.into()); } if let Some(kwarg) = kwarg { diff --git a/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__ruff_test__statement__function_py.snap b/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__ruff_test__statement__function_py.snap index ad063ab680e168..222c32b4a4b0a3 100644 --- a/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__ruff_test__statement__function_py.snap +++ b/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__ruff_test__statement__function_py.snap @@ -96,6 +96,13 @@ else: # Regression test for https://github.com/python/cpython/blob/7199584ac8632eab57612f595a7162ab8d2ebbc0/Lib/warnings.py#L513 def f(arg1=1, *, kwonlyarg1, kwonlyarg2=2): pass + + +# Regression test for https://github.com/astral-sh/ruff/issues/5176#issuecomment-1598171989 +def foo( + b=3 + 2 # comment +): + ... ``` @@ -223,6 +230,13 @@ else: # Regression test for https://github.com/python/cpython/blob/7199584ac8632eab57612f595a7162ab8d2ebbc0/Lib/warnings.py#L513 def f(arg1=1, *, kwonlyarg1, kwonlyarg2=2): pass + + +# Regression test for https://github.com/astral-sh/ruff/issues/5176#issuecomment-1598171989 +def foo( + b=3 + 2, # comment +): + ... ```