Prevent blank lines between a comment and an item from moving with th…

…e item. This makes it possible to keep the file header comment at the top of the file even after multiple rounds of sorting, by adding a blank line after the header comment. PiperOrigin-RevId: 598791768
protocolbuffers · Jan 16, 2024 · 5d63926 · 5d63926
1 parent 442737f
commit 5d63926
Show file tree

Hide file tree

Showing 4 changed files with 131 additions and 11 deletions.
diff --git a/ast/ast_test.go b/ast/ast_test.go
@@ -257,8 +257,9 @@ bar: 2
 
 foo: 1
 `,
-			// The blank line is part of the node of the `foo: 1` item.
-			want: []bool{false, false},
+			// The blank line is a separate node and not part of the `foo: 1` item
+			// because it follows a comment node.
+			want: []bool{false, true, false},
 		},
 	}
 	for _, input := range inputs {

diff --git a/parser/parser.go b/parser/parser.go
@@ -580,9 +580,31 @@ func (p *parser) parse(isRoot bool) (result []*ast.Node, endPos ast.Position, er
 		if blankLines > 0 {
 			if p.config.infoLevel() {
 				p.config.infof("blankLines: %v", blankLines)
+				p.config.infof("comments: %v", comments)
+			}
+			if len(res) > 0 && res[len(res)-1].IsCommentOnly() {
+				// Blanks lines after a comment-only node are collapsed into one blank
+				// line which becomes an independent unnamed node, so that the blank
+				// line does not move when the item node below it moves for sorting.
+				//
+				// This allows users to add a blank line after a header comment as a way
+				// to avoid the header comment moving if the first item moves due to
+				// sorting.
+				res = append(res, &ast.Node{Start: startPos, PreComments: []string{""}})
+				// Each blank line is exactly 1 byte, and we need to adjust the start
+				// position for the next node to be after all the collapsed blank lines
+				// represented by the just added node.
+				startPos.Byte += uint32(blankLines)
+				startPos.Line += int32(blankLines)
+				startPos.Column = 1
+			} else {
+				// Unless the previous node is a comment-only node, we collapse the
+				// blanks lines into one blank line as the start of the comment (if any)
+				// of the next node, such that this blank line will move with the node
+				// when sorting, will be deleted with the node if targeted for deletion,
+				// etc.
+				comments = append([]string{""}, comments...)
 			}
-			// Here we collapse the leading blank lines into one blank line.
-			comments = append([]string{""}, comments...)
 		}
 
 		for p.nextInputIs('%') {
@@ -1064,7 +1086,16 @@ func sortAndFilterNodes(parent *ast.Node, nodes []*ast.Node, sortFunction NodeSo
 		}
 	}
 	if sortFunction != nil {
-		return sortFunction(parent, nodes)
+		err := sortFunction(parent, nodes)
+		if err != nil {
+			return err
+		}
+		// Sorting can sometimes cause a blank line node to be before an item node
+		// that also starts with a blank line. Given that we normally collapse
+		// sequences of multiple blank lines into a single blank line, this would
+		// make formatting not idempotent, so we run an extra scan to collapse such
+		// blank lines here.
+		collapseBlankLines(nodes)
 	}
 	return nil
 }
@@ -1088,6 +1119,31 @@ func RemoveDuplicates(nodes []*ast.Node) {
 	}
 }
 
+// Marks blank line nodes followed by a blank line deleted.
+//
+// This function ignores whether other nodes are already marked deleted or not
+// to keep the logic simpler. This may make certain blank lines be deleted or
+// not be deleted incorrectly, depending on the context of where the function is
+// called.
+//
+// See also `removeDeleted` where blank lines are handled specially.
+func collapseBlankLines(nodes []*ast.Node) {
+	for i := 0; i < len(nodes)-1; i++ {
+		nd := nodes[i]
+		if !nd.IsBlankLine() {
+			continue
+		}
+		next := nodes[i+1]
+		if len(next.PreComments) > 0 && next.PreComments[0] == "" {
+			// This marks the blank line node as deleted, whereas parsing this content
+			// would instead collapse both the blank line of the blank line node and
+			// the blank line in `next.PreComments[0]` into one blank line node, and
+			// `next.PreComments[0]` would not be a blank line.
+			nd.Deleted = true
+		}
+	}
+}
+
 func wrapStrings(nodes []*ast.Node, depth int, c Config) error {
 	if c.WrapStringsAtColumn == 0 && !c.WrapStringsAfterNewlines {
 		return nil

diff --git a/parser/parser_position_test.go b/parser/parser_position_test.go
@@ -58,7 +58,7 @@ func TestParsePositions(t *testing.T) {
 			in: mkString(
 				"# top",        // 5 bytes + newline
 				"unrelated: 1", // 12 bytes + newline
-				"",             // this already gets accounted for trackme
+				"",             // this already gets accounted for trackme (blank line without a comment before it)
 				"trackme: 4",
 			),
 			startByte: 19,
@@ -76,12 +76,23 @@ func TestParsePositions(t *testing.T) {
 			in: mkString(
 				"# top",        // 5 bytes + newline
 				"unrelated: 1", // 12 bytes + newline
-				"",             // this already gets accounted for trackme
+				"",             // this already gets accounted for trackme (blank line without a comment before it)
 				"# boo",
 				"trackme: 4",
 			),
 			startByte: 19,
 		},
+		{
+			in: mkString(
+				"# top",              // 5 bytes + newline
+				"unrelated: 1",       // 12 bytes + newline
+				"",                   // newline
+				"# detached comment", // 18 bytes + newline
+				"",                   // newline (blank line with a comment before it becomes an unnamed node, not attached to trackme)
+				"trackme: 4",
+			),
+			startByte: 40,
+		},
 		{
 			in: mkString(
 				"outer: {", // 8 bytes + newline

diff --git a/parser/parser_test.go b/parser/parser_test.go
@@ -1172,8 +1172,8 @@ presubmit: {
 presubmit: {
   check_contents: {
     # This comment is a separate node and does not move when the fields are sorted.
-    operation: ADD
 
+    operation: ADD
     operation: EDIT
   }
 }
@@ -1354,7 +1354,6 @@ message: { id: "a" }
 
 # field a
 field: "a"
-
 # field b
 field: "b"
 message: { id: "a" }
@@ -1364,7 +1363,6 @@ message: { id: "b" }
 
 # field a
 field: "a"
-
 # field b
 field: "b"
 message: { id: "a" }
@@ -1388,7 +1386,6 @@ field: "b"
 
 # field a
 field: "a"
-
 # field c
 field: "c"
 
@@ -1594,6 +1591,61 @@ func TestParserConfigs(t *testing.T) {
   # Should remain below
   auto_reviewers: "reviewerA"
 }
+`,
+	}, {
+		name: "SortRepeatedFieldsBySubfieldWithHeaderComment",
+		in: `# proto-file: path/to/my/file.proto
+# proto-message: my.package.MyMessage
+#
+# My comments here.
+foo: {
+  bar: EDIT
+}
+foo: {
+  bar: ADD
+}
+`,
+		config: Config{SortRepeatedFieldsBySubfield: []string{"bar"}},
+		// We always attach the comment to the item - there is no special support
+		// for header comments. See the EmptyLineDetachesComment test case below for
+		// a workaround.
+		out: `foo: {
+  bar: ADD
+}
+# proto-file: path/to/my/file.proto
+# proto-message: my.package.MyMessage
+#
+# My comments here.
+foo: {
+  bar: EDIT
+}
+`,
+	}, {
+		name: "SortRepeatedFieldsBySubfieldWithHeaderComment_EmptyLineDetachesComment",
+		in: `# proto-file: path/to/my/file.proto
+# proto-message: my.package.MyMessage
+#
+# My comments here.
+
+foo: {
+  bar: EDIT
+}
+foo: {
+  bar: ADD
+}
+`,
+		config: Config{SortRepeatedFieldsBySubfield: []string{"bar"}},
+		out: `# proto-file: path/to/my/file.proto
+# proto-message: my.package.MyMessage
+#
+# My comments here.
+
+foo: {
+  bar: ADD
+}
+foo: {
+  bar: EDIT
+}
 `,
 	}, {
 		name: "SortNamedFieldByMultipleSubfieldContents",