Merge branch 'main' into ed/bufcheckRunnerProviderConfig

CHANGELOG.md

@@ -2,7 +2,9 @@
 
 ## [Unreleased]
 
-- No changes yet.
+- Fix bugs in `buf format` where trailing comments on commas in message literals were not properly
+  propagated to the formatted proto, empty message literals were not properly indented, and
+  compound strings in options added an extra newline before trailing commas.
 
 ## [v1.41.0] - 2024-09-11
 

private/buf/bufformat/formatter.go

@@ -450,7 +450,9 @@ func (f *formatter) writeOption(optionNode *ast.OptionNode) {
 	}
 
 	if node, ok := optionNode.Val.(*ast.CompoundStringLiteralNode); ok {
-		f.writeCompoundStringLiteralIndent(node)
+		// We write the compound string literal node and end in-line to handle any commas for
+		// the option.
+		f.writeCompoundStringLiteralIndentEndInline(node)
 		return
 	}
 	f.writeInline(optionNode.Val)
@@ -595,9 +597,23 @@ func (f *formatter) writeMessageLiteral(messageLiteralNode *ast.MessageLiteralNo
 			f.writeMessageLiteralElements(messageLiteralNode)
 		}
 	}
+	closeNode := messageLiteralClose(messageLiteralNode)
+	// In the case where we are writing a compact message literal, we need to check if there
+	// are any trailing comments on the message literal that needs to be handled. The trailing
+	// comments may end up on the message literal node itself rather than the closing bracket
+	// in the case where a message literal is an element of a message literal, and trailing
+	// comments on the separator are attached to message literal.
+	messageLiteralTrailingComments := f.nodeInfo(messageLiteralNode).TrailingComments()
+	// Similar to how we handle the trailing comments on separators, we check if there are
+	// any existing trailing comments on the closing bracket. If not, then we attach the trailing
+	// comments of the message literal node to the closing bracket. Skip this if the closing
+	// bracket already has trailing comments.
+	if messageLiteralTrailingComments.Len() > 0 && f.nodeInfo(closeNode).TrailingComments().Len() == 0 {
+		f.setTrailingComments(closeNode, messageLiteralTrailingComments)
+	}
 	f.writeCompositeValueBody(
 		messageLiteralOpen(messageLiteralNode),
-		messageLiteralClose(messageLiteralNode),
+		closeNode,
 		elementWriterFunc,
 	)
 }
@@ -621,9 +637,23 @@ func (f *formatter) writeMessageLiteralForArray(
 	if lastElement {
 		closeWriter = f.writeBodyEnd
 	}
+	closeNode := messageLiteralClose(messageLiteralNode)
+	// In the case where we are writing a compact message literal, we need to check if there
+	// are any trailing comments on the message literal that needs to be handled. The trailing
+	// comments may end up on the message literal node itself rather than the closing bracket
+	// in the case where a message literal is an element of a message literal, and trailing
+	// comments on the separator are attached to message literal.
+	messageLiteralTrailingComments := f.nodeInfo(messageLiteralNode).TrailingComments()
+	// Similar to how we handle the trailing comments on separators, we check if there are
+	// any existing trailing comments on the closing bracket. If not, then we attach the trailing
+	// comments of the message literal node to the closing bracket. Skip this if the closing
+	// bracket already has trailing comments.
+	if messageLiteralTrailingComments.Len() > 0 && f.nodeInfo(closeNode).TrailingComments().Len() == 0 {
+		f.setTrailingComments(closeNode, messageLiteralTrailingComments)
+	}
 	f.writeBody(
 		messageLiteralOpen(messageLiteralNode),
-		messageLiteralClose(messageLiteralNode),
+		closeNode,
 		elementWriterFunc,
 		f.writeOpenBracePrefixForArray,
 		closeWriter,
@@ -634,38 +664,60 @@ func (f *formatter) maybeWriteCompactMessageLiteral(
 	messageLiteralNode *ast.MessageLiteralNode,
 	inArrayLiteral bool,
 ) bool {
-	if len(messageLiteralNode.Elements) == 0 || len(messageLiteralNode.Elements) > 1 ||
+	// We only want to write a compact message literal for a message literal with either 0 or
+	// 1 elements, and if there are no interior comments, and the element, if present, is not
+	// a message or array literal.
+	if len(messageLiteralNode.Elements) > 1 ||
 		f.hasInteriorComments(messageLiteralNode.Children()...) ||
 		messageLiteralHasNestedMessageOrArray(messageLiteralNode) {
 		return false
 	}
+
 	// messages with a single scalar field and no comments can be
 	// printed all on one line
 	openNode := messageLiteralOpen(messageLiteralNode)
 	closeNode := messageLiteralClose(messageLiteralNode)
+
+	// In the case where we are writing a compact message literal, we need to check if there
+	// are any trailing comments on the message literal that needs to be handled. The trailing
+	// comments may end up on the message literal node itself rather than the closing bracket
+	// in the case where a message literal is an element of a message literal, and trailing
+	// comments on the separator are attached to message literal.
+	messageLiteralTrailingComments := f.nodeInfo(messageLiteralNode).TrailingComments()
+	// Similar to how we handle the trailing comments on separators, we check if there are
+	// any existing trailing comments on the closing bracket. If not, then we attach the trailing
+	// comments of the message literal node to the closing bracket. Skip this if the closing
+	// bracket already has trailing comments.
+	if messageLiteralTrailingComments.Len() > 0 && f.nodeInfo(closeNode).TrailingComments().Len() == 0 {
+		f.setTrailingComments(closeNode, messageLiteralTrailingComments)
+	}
+
 	if inArrayLiteral {
 		f.Indent(openNode)
 	}
 	f.writeInline(openNode)
-	fieldNode := messageLiteralNode.Elements[0]
-	f.writeInline(fieldNode.Name)
-	if fieldNode.Sep != nil {
-		f.writeInline(fieldNode.Sep)
-	} else {
-		f.WriteString(":")
-	}
-	f.Space()
-	if messageLiteralNode.Seps[0] != nil {
-		// We are dropping the optional trailing separator. If it had
-		// trailing comments and the value does not, move the separator's
-		// trailing comment to the value.
-		sepTrailingComments := f.nodeInfo(messageLiteralNode.Seps[0]).TrailingComments()
-		if sepTrailingComments.Len() > 0 &&
-			f.nodeInfo(fieldNode.Val).TrailingComments().Len() == 0 {
-			f.setTrailingComments(fieldNode.Val, sepTrailingComments)
+	// We check if our compact message has one element, if so, write that value as compact.
+	if len(messageLiteralNode.Elements) == 1 {
+		fieldNode := messageLiteralNode.Elements[0]
+		f.writeInline(fieldNode.Name)
+		if fieldNode.Sep != nil {
+			f.writeInline(fieldNode.Sep)
+		} else {
+			f.WriteString(":")
 		}
+		f.Space()
+		if messageLiteralNode.Seps[0] != nil {
+			// We are dropping the optional trailing separator. If it had
+			// trailing comments and the value does not, move the separator's
+			// trailing comment to the value.
+			sepTrailingComments := f.nodeInfo(messageLiteralNode.Seps[0]).TrailingComments()
+			if sepTrailingComments.Len() > 0 &&
+				f.nodeInfo(fieldNode.Val).TrailingComments().Len() == 0 {
+				f.setTrailingComments(fieldNode.Val, sepTrailingComments)
+			}
+		}
+		f.writeInline(fieldNode.Val)
 	}
-	f.writeInline(fieldNode.Val)
 	f.writeInline(closeNode)
 	return true
 }
@@ -1248,6 +1300,19 @@ func (f *formatter) hasInteriorComments(nodes ...ast.Node) bool {
 //	  "bar"
 //	]
 func (f *formatter) writeArrayLiteral(arrayLiteralNode *ast.ArrayLiteralNode) {
+	// We need to check if there are any trailing comments on the array literal itself that
+	// needs to be handled. The trailing comments may end up on the array literal node itself
+	// rather than the closing bracket in the case where an array literal is an element of a
+	// message literal, and trailing comments on the separator are attached to array literal.
+	arrayLiteralTrailingComments := f.nodeInfo(arrayLiteralNode).TrailingComments()
+	// Similar to how we handle trailing comments on separators, we check if there are any
+	// existing trailing comments on the closing bracket. If not, then we attach the trailing
+	// comments of the array literal node to the closing bracket. Skip this if the closing
+	// bracket already has trailing comments.
+	if arrayLiteralTrailingComments.Len() > 0 && f.nodeInfo(arrayLiteralNode.CloseBracket).TrailingComments().Len() == 0 {
+		f.setTrailingComments(arrayLiteralNode.CloseBracket, arrayLiteralTrailingComments)
+	}
+
 	if len(arrayLiteralNode.Elements) == 1 &&
 		!f.hasInteriorComments(arrayLiteralNode.Children()...) &&
 		!arrayLiteralHasNestedMessageOrArray(arrayLiteralNode) {
@@ -1282,6 +1347,7 @@ func (f *formatter) writeArrayLiteral(arrayLiteralNode *ast.ArrayLiteralNode) {
 			}
 		}
 	}
+
 	f.writeCompositeValueBody(
 		arrayLiteralNode.OpenBracket,
 		arrayLiteralNode.CloseBracket,

private/buf/bufformat/formatter_test.go

@@ -60,13 +60,13 @@ func testFormatProto2(t *testing.T) {
 
 func testFormatProto3(t *testing.T) {
 	testFormatNoDiff(t, "testdata/proto3/all/v1")
+	testFormatNoDiff(t, "testdata/proto3/block/v1")
 	testFormatNoDiff(t, "testdata/proto3/file/v1")
 	testFormatNoDiff(t, "testdata/proto3/header/v1")
 	testFormatNoDiff(t, "testdata/proto3/literal/v1")
 	testFormatNoDiff(t, "testdata/proto3/oneof/v1")
 	testFormatNoDiff(t, "testdata/proto3/range/v1")
 	testFormatNoDiff(t, "testdata/proto3/service/v1")
-	testFormatNoDiff(t, "testdata/proto3/block/v1")
 }
 
 func testFormatNoDiff(t *testing.T, path string) {

private/buf/bufformat/testdata/proto3/literal/v1/literal_comments.golden.proto

@@ -24,6 +24,36 @@ message Foo {
       /* Two */
       "is a"
       /* Three */
-      "compound string" // Trailing
+      "compound string", // Trailing
+    (custom.message_literal_field_option) = /* Before */ {
+      /* interior comment */
+      rule: "rule"
+    }, // Trailing
+    (custom.message_literal_no_interior_comment_field_option) = /* Before */ {rule: "rule"}, // Trailing
+    (custom.nested_message_literal_field_option) = {
+      // Comment on nested message
+      nested: {
+        nested_nested: {
+          /* interior comment on nested_nested message */
+          inner_most: "value"
+        }
+      } /* Trailing */
+      // Before
+      other_nested: /* Before */ {once: "value"} /* Trailing */
+    },
+    (custom.message_literal_with_array_literals) = {
+      additional_rules: [] /* comment on , */
+      additional_rules: [] /* comment on [ */
+      additional_rules: [] /* multi-line comment on , */
+      additional_rules: [
+        /* Before */ {}, // Trailing
+        /* Before */ {rule: "child"}, /* child node */
+        {
+          rule: "another_child"
+          additional_rules: [] /* Trailing */
+        }
+      ]
+      rule: "parent" // but this one is
+    }
   ];
 }

private/buf/bufformat/testdata/proto3/literal/v1/literal_comments.proto

@@ -18,6 +18,30 @@ message Foo {
     (custom.sfixed64_field_option) = - /* Finally */ 6464 /* After */, // Trailing
     (custom.bool_field_option) = /* Before */ true /* After */ , // Trailing
     (custom.bytes_field_option) = /* Before */ "bytes" /* After */ , // Trailing
-    (custom.string_field_option) = /* One */  "this"   /* Two */ "is a"  /* Three */ "compound string" // Trailing
+    (custom.string_field_option) = /* One */  "this"   /* Two */ "is a"  /* Three */ "compound string", // Trailing
+    (custom.message_literal_field_option) = /* Before */ { /* interior comment */  rule: "rule"}, // Trailing
+    (custom.message_literal_no_interior_comment_field_option) = /* Before */ {rule: "rule"}, // Trailing
+    (custom.nested_message_literal_field_option) = {
+      // Comment on nested message
+      nested: {
+        nested_nested: { /* interior comment on nested_nested message */ inner_most: "value" }
+      }, // Trailing
+      // Before
+      other_nested: /* Before */ { once: "value" } // Trailing
+    },
+    (custom.message_literal_with_array_literals) = {
+       additional_rules: [], // comment on ,
+       additional_rules: [] // comment on [
+       additional_rules: [], /* multi-line comment on , */
+       additional_rules: [
+        /* Before */ {}, // Trailing
+        /* Before */ { rule: "child" }, /* child node */
+        {
+          rule: "another_child",
+          additional_rules: [] // Trailing
+        }
+      ]
+      rule: "parent" // but this one is
+    }
   ];
 }