Preserve comments on the final function parameter (#1519)
Buildifier currently moves a trailing comment on the final function
parameter past the closing `)` and any return annotation. For example:
```python
def f(
x, # @unused
) -> int:
return 1
```
becomes:
```python
def f(
x) -> int: # @unused
return 1
```
Since #1488, these two spellings have different AST attachments: the
first comment belongs to `x`, while the second belongs to the function
header's colon. #1518 intentionally interprets an `@unused` comment on
the header as applying to the final parameter, preserving the
longstanding behavior of existing files. However, buildifier should not
convert the preferred parameter-attached form into that legacy
header-attached form. Keeping the forms distinguishable lets users write
the clearer form today and leaves room to change or deprecate the legacy
interpretation in the future.
This PR keeps signatures with parameter suffix comments multiline and
emits a pending final parameter comment before the closing parenthesis:
```python
def f(
x # @unused
) -> int:
return 1
```
Both forms remain supported, with `@unused` applying only to the final
parameter:
```python
# Preferred: the comment is attached to y.
def f(
x,
y # @unused
):
pass
# Legacy compatibility: a header comment applies to y, not x.
def g(x, y): # @unused
pass
```
The lexer previously treated a comment following a line containing only
`):` as a body comment. That exception was added in #276 before
block-header comments had their own AST node. Remove it so comments
visibly written after the colon remain attached to the header, including
multiline `def`, `if`, and `for` headers.
Finally, flush queued end-of-line comments before reducing the
indentation margin. This keeps continuation comments on a final
parameter aligned with that parameter and makes formatting idempotent.
The `unused-variable` documentation now recommends the
parameter-attached form and documents the header-attached form as
backward compatibility behavior.diff --git a/WARNINGS.md b/WARNINGS.md
index 0dfb4da..331cc95 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -1495,7 +1495,8 @@
The line can often be safely removed.
If you want to keep the variable, you can disable the warning by adding a
-comment `# @unused`.
+comment `# @unused`. For a function parameter, place the comment on the
+parameter itself.
```python
x = [1, 2] # @unused
@@ -1503,11 +1504,20 @@
# @unused
def f(
x,
- y, # @unused
+ y # @unused
):
pass
```
+For backward compatibility, an `# @unused` comment at the end of a function
+header applies to its final parameter. New code should prefer attaching the
+comment directly to the parameter as shown above.
+
+```python
+def f(x, y): # @unused applies to y
+ pass
+```
+
If an unused variable is used for partially unpacking tuples, just prefix
its name with an underscore to suppress the warning:
diff --git a/build/lex.go b/build/lex.go
index 12b1afd..6fb76f4 100644
--- a/build/lex.go
+++ b/build/lex.go
@@ -447,16 +447,12 @@
// Find the last \n before this # and see if it's all
// spaces from there to here.
// If it's a suffix comment but the last non-space symbol before
- // it is one of (, [, or {, or it's a suffix comment to "):"
- // (e.g. trailing closing bracket or a function definition),
- // treat it as a line comment that should be
+ // it is one of (, [, or {, treat it as a line comment that should be
// put inside the corresponding block.
i := bytes.LastIndex(in.complete[:in.pos.Byte], []byte("\n"))
prefix := bytes.TrimSpace(in.complete[i+1 : in.pos.Byte])
- prefix = bytes.Replace(prefix, []byte{' '}, []byte{}, -1)
isSuffix := true
if len(prefix) == 0 ||
- (len(prefix) == 2 && prefix[0] == ')' && prefix[1] == ':') ||
prefix[len(prefix)-1] == '[' ||
prefix[len(prefix)-1] == '(' ||
prefix[len(prefix)-1] == '{' {
diff --git a/build/print.go b/build/print.go
index 3744875..875bff1 100644
--- a/build/print.go
+++ b/build/print.go
@@ -137,24 +137,31 @@
// in brackets of some kind, use breakline instead.
func (p *printer) newline() {
p.needsNewLine = false
- if len(p.comment) > 0 {
- p.prints(" ")
- for i, com := range p.comment {
- if i > 0 {
- p.trim()
- p.printc('\n')
- p.spaces(p.margin)
- }
- p.prints(strings.TrimSpace(com.Token))
- }
- p.comment = p.comment[:0]
- }
-
+ p.flushComments()
p.trim()
p.printc('\n')
p.spaces(p.margin)
}
+// flushComments prints the queued end-of-line comments: the first one at the
+// end of the current line, any further ones on their own lines at the current
+// margin.
+func (p *printer) flushComments() {
+ if len(p.comment) == 0 {
+ return
+ }
+ p.prints(" ")
+ for i, com := range p.comment {
+ if i > 0 {
+ p.trim()
+ p.printc('\n')
+ p.spaces(p.margin)
+ }
+ p.prints(strings.TrimSpace(com.Token))
+ }
+ p.comment = p.comment[:0]
+}
+
// softNewline postpones a call to newline to the next call of p.newlineIfNeeded()
// If softNewline is called several times, just one newline is printed.
// Usecase: if there are several nested blocks ending at the same time, for instance
@@ -1033,7 +1040,7 @@
// If there are line comments, use multiline
// so we can print the comments before the closing bracket.
for _, x := range *list {
- if len(x.Comment().Before) > 0 || (len(x.Comment().Suffix) > 0 && mode != modeDef) {
+ if len(x.Comment().Before) > 0 || len(x.Comment().Suffix) > 0 {
return false
}
}
@@ -1171,9 +1178,14 @@
printedFinalComments = true
}
}
+ // In modeDef, keep the closing bracket on the same line unless doing so
+ // would move a parameter's comment outside the parameter list.
+ closeOnNewLine := mode != modeDef || printedFinalComments || len(p.comment) > 0
+ // Flush the last element's end-of-line comments before dedenting so that
+ // any further comments line up with the element, not the closing bracket.
+ p.flushComments()
p.margin -= indentation
- // in modeDef print the closing bracket on the same line
- if mode != modeDef || printedFinalComments {
+ if closeOnNewLine {
p.newline()
}
}
diff --git a/build/print_test.go b/build/print_test.go
index d86f6d3..b0b4a59 100644
--- a/build/print_test.go
+++ b/build/print_test.go
@@ -528,6 +528,88 @@
return nil
}
+// TestPrintDefParameterAndHeaderComments checks that comments on the final
+// parameter and comments on the function header remain distinct after
+// formatting and reparsing.
+func TestPrintDefParameterAndHeaderComments(t *testing.T) {
+ for _, param := range []string{"x", "x = None", "x: int", "x: int = 0", "*args", "**kwargs"} {
+ for _, returnType := range []string{"", " -> int"} {
+ for _, headerComment := range []string{"", " # header"} {
+ for _, beforeParam := range []string{"", "\n "} {
+ input := "def f(" + beforeParam + param + ", # @unused\n)" + returnType + ":" + headerComment + "\n pass\n"
+ want := "def f(\n " + param + " # @unused\n)" + returnType + ":" + headerComment + "\n pass\n"
+ t.Run(input, func(t *testing.T) {
+ f, err := ParseBzl("test.bzl", []byte(input))
+ if err != nil {
+ t.Fatal(err)
+ }
+ got := Format(f)
+ if string(got) != want {
+ testutils.Tdiff(t, []byte(want), got)
+ }
+
+ // Formatting must not turn a parameter comment into a header comment.
+ reparsed, err := ParseBzl("test.bzl", got)
+ if err != nil {
+ t.Fatal(err)
+ }
+ def := reparsed.Stmt[0].(*DefStmt)
+ if comments := def.Params[0].Comment().Suffix; len(comments) != 1 || comments[0].Token != "# @unused" {
+ t.Errorf("parameter suffix comments = %v, want # @unused", comments)
+ }
+ comments := def.ColonPos.Comment().Suffix
+ if headerComment == "" && len(comments) != 0 {
+ t.Errorf("unexpected header suffix comments: %v", comments)
+ } else if headerComment != "" && (len(comments) != 1 || comments[0].Token != "# header") {
+ t.Errorf("header suffix comments = %v, want # header", comments)
+ }
+ if reformatted := Format(reparsed); !bytes.Equal(got, reformatted) {
+ t.Error("formatting is not idempotent")
+ testutils.Tdiff(t, got, reformatted)
+ }
+ })
+ }
+ }
+ }
+ }
+}
+
+// TestPrintMultipleEndOfLineComments checks that when the last parameter
+// carries more than one end-of-line comment, the additional comments are
+// aligned with the parameter rather than with the closing parenthesis, so that
+// formatting is idempotent.
+func TestPrintMultipleEndOfLineComments(t *testing.T) {
+ tests := []struct {
+ input string
+ want string
+ }{
+ {
+ input: "def f(x: # type\n int # param\n):\n pass\n",
+ want: "def f(\n x: int # type\n # param\n):\n pass\n",
+ },
+ }
+ for _, tc := range tests {
+ t.Run(tc.input, func(t *testing.T) {
+ f, err := ParseBzl("test.bzl", []byte(tc.input))
+ if err != nil {
+ t.Fatal(err)
+ }
+ got := Format(f)
+ if string(got) != tc.want {
+ testutils.Tdiff(t, []byte(tc.want), got)
+ }
+ reparsed, err := ParseBzl("test.bzl", got)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if reformatted := Format(reparsed); !bytes.Equal(got, reformatted) {
+ t.Error("formatting is not idempotent")
+ testutils.Tdiff(t, got, reformatted)
+ }
+ })
+ }
+}
+
func TestPrintTypeExprForceMultiLine(t *testing.T) {
tests := []struct {
name string
diff --git a/build/testdata/051.build.golden b/build/testdata/051.build.golden
index 7d9f186..0897631 100644
--- a/build/testdata/051.build.golden
+++ b/build/testdata/051.build.golden
@@ -29,8 +29,8 @@
def g(
a,
s,
- d): # this is d
- # this is function definition
+ d # this is d
+ ): # this is function definition
ccc
# Misalingned comments
diff --git a/build/testdata/051.bzl.golden b/build/testdata/051.bzl.golden
index ff4be4b..51b22a7 100644
--- a/build/testdata/051.bzl.golden
+++ b/build/testdata/051.bzl.golden
@@ -26,8 +26,8 @@
def g(
a,
s,
- d): # this is d
- # this is function definition
+ d # this is d
+ ): # this is function definition
ccc
# Misalingned comments
diff --git a/build/testdata/056.golden b/build/testdata/056.golden
index 81b814f..f8588aa 100644
--- a/build/testdata/056.golden
+++ b/build/testdata/056.golden
@@ -2,7 +2,6 @@
def function(
x # 1
# 2
-):
- # 3
+): # 3
# 4
pass
diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto
index e24c95f..d983a52 100644
--- a/warn/docs/warnings.textproto
+++ b/warn/docs/warnings.textproto
@@ -1091,17 +1091,25 @@
"```\n\n"
"The line can often be safely removed.\n\n"
"If you want to keep the variable, you can disable the warning by adding a\n"
- "comment `# @unused`.\n\n"
+ "comment `# @unused`. For a function parameter, place the comment on the\n"
+ "parameter itself.\n\n"
"```python\n"
"x = [1, 2] # @unused\n"
"\n"
"# @unused\n"
"def f(\n"
" x,\n"
- " y, # @unused\n"
+ " y # @unused\n"
"):\n"
" pass\n"
"```\n\n"
+ "For backward compatibility, an `# @unused` comment at the end of a function\n"
+ "header applies to its final parameter. New code should prefer attaching the\n"
+ "comment directly to the parameter as shown above.\n\n"
+ "```python\n"
+ "def f(x, y): # @unused applies to y\n"
+ " pass\n"
+ "```\n\n"
"If an unused variable is used for partially unpacking tuples, just prefix\n"
"its name with an underscore to suppress the warning:\n\n"
"```python\n"
diff --git a/warn/warn_control_flow_test.go b/warn/warn_control_flow_test.go
index a721ced..6b8ebcb 100644
--- a/warn/warn_control_flow_test.go
+++ b/warn/warn_control_flow_test.go
@@ -16,7 +16,11 @@
package warn
-import "testing"
+import (
+ "testing"
+
+ "github.com/bazel-contrib/buildtools/v10/build"
+)
func TestMissingReturnValueWarning(t *testing.T) {
// empty return
@@ -690,6 +694,26 @@
checkFindings(t, "unused-variable", `
def foo(
+ x,
+): # @unused
+ pass
+
+def bar(
+ x,
+ y,
+) -> int: # @unused
+ pass
+
+foo()
+bar()
+`,
+ []string{
+ ":7: Variable \"x\" is unused.",
+ },
+ scopeEverywhere)
+
+ checkFindings(t, "unused-variable", `
+def foo(
name,
x):
pass
@@ -947,6 +971,99 @@
scopeEverywhere)
}
+func TestUnusedVariableFunctionParameterCommentForms(t *testing.T) {
+ tests := []struct {
+ name string
+ input string
+ want string
+ commentOnParameter bool
+ }{
+ {
+ name: "preferred parameter comment",
+ input: `def f(
+ x,
+ y, # @unused
+):
+ pass
+
+f()
+`,
+ want: `def f(
+ x,
+ y # @unused
+):
+ pass
+
+f()
+`,
+ commentOnParameter: true,
+ },
+ {
+ name: "legacy header comment",
+ input: `def f(
+ x,
+ y,
+): # @unused
+ pass
+
+f()
+`,
+ want: `def f(
+ x,
+ y): # @unused
+ pass
+
+f()
+`,
+ },
+ }
+
+ for _, tc := range tests {
+ t.Run(tc.name, func(t *testing.T) {
+ file, err := build.ParseBzl("test.bzl", []byte(tc.input))
+ if err != nil {
+ t.Fatal(err)
+ }
+ formatted := build.Format(file)
+ if string(formatted) != tc.want {
+ t.Fatalf("formatted output:\n%s\nwant:\n%s", formatted, tc.want)
+ }
+
+ // The preferred and legacy forms intentionally have the same lint
+ // semantics but remain distinguishable in the AST. This lets users
+ // adopt the parameter-attached form without buildifier changing it
+ // back to the legacy header-attached form.
+ reparsed, err := build.ParseBzl("test.bzl", formatted)
+ if err != nil {
+ t.Fatal(err)
+ }
+ def := reparsed.Stmt[0].(*build.DefStmt)
+ parameterComments := def.Params[1].Comment().Suffix
+ headerComments := def.ColonPos.Comment().Suffix
+ if tc.commentOnParameter {
+ if len(parameterComments) != 1 || parameterComments[0].Token != "# @unused" {
+ t.Errorf("parameter suffix comments = %v, want # @unused", parameterComments)
+ }
+ if len(headerComments) != 0 {
+ t.Errorf("unexpected header suffix comments: %v", headerComments)
+ }
+ } else {
+ if len(parameterComments) != 0 {
+ t.Errorf("unexpected parameter suffix comments: %v", parameterComments)
+ }
+ if len(headerComments) != 1 || headerComments[0].Token != "# @unused" {
+ t.Errorf("header suffix comments = %v, want # @unused", headerComments)
+ }
+ }
+
+ // In both forms, @unused applies only to the final parameter y.
+ checkFindings(t, "unused-variable", string(formatted), []string{
+ `:2: Variable "x" is unused.`,
+ }, scopeEverywhere)
+ })
+ }
+}
+
func TestRedefinedVariable(t *testing.T) {
checkFindings(t, "redefined-variable", `
x = "old_value"
