Fix formatter non-convergence for continuation EOL comments

verible/verilog/formatting/formatter.cc

@@ -26,7 +26,6 @@
 #include <string_view>
 #include <vector>
 
-#include "absl/base/attributes.h"
 #include "absl/log/die_if_null.h"
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
@@ -50,7 +49,6 @@
 #include "verible/common/util/expandable-tree-view.h"
 #include "verible/common/util/interval-set.h"
 #include "verible/common/util/interval.h"
-#include "verible/common/util/iterator-range.h"
 #include "verible/common/util/logging.h"
 #include "verible/common/util/spacer.h"
 #include "verible/common/util/tree-operations.h"
@@ -743,31 +741,19 @@ class ContinuationCommentAligner {
     return column;
   }
 
-  static void AdjustColumnUsingTokenSpacing(
-      const verible::FormattedToken &token, int *column) {
-    switch (token.before.action) {
-      case verible::SpacingDecision::kPreserve: {
-        if (token.before.preserved_space_start !=
-            verible::string_view_null_iterator()) {
-          *column += token.OriginalLeadingSpaces().length();
-        } else {
-          *column += token.before.spaces;
-        }
-        break;
-      }
-      case verible::SpacingDecision::kWrap:
-        *column = 0;
-        ABSL_FALLTHROUGH_INTENDED;
-      case verible::SpacingDecision::kAlign:
-      case verible::SpacingDecision::kAppend:
-        *column += token.before.spaces;
-        break;
-    }
-  }
-
   static int CalculateEolCommentColumn(const verible::FormattedExcerpt &line) {
+    // Compute the starting column of the trailing EOL comment the same way
+    // FormattedExcerpt::FormattedText emits spaces, including:
+    //   * wrap indents (SpacingDecision::kWrap), and
+    //   * preserved leading whitespace that may contain newlines (common when
+    //     an original line break is kept). Counting those newlines as width
+    //     made continuation comments land on the wrong column and fail to
+    //     converge on re-format (GitHub issue 2542).
+    if (line.Tokens().empty()) return 0;
+
     int column = 0;
-    const auto &front = line.Tokens().front();
+    const auto &tokens = line.Tokens();
+    const auto &front = tokens.front();
 
     if (front.before.action != verible::SpacingDecision::kPreserve) {
       column += line.IndentationSpaces();
@@ -777,12 +763,39 @@ class ContinuationCommentAligner {
     }
     column += front.token->text().length();
 
-    for (const auto &ftoken : verible::make_range(line.Tokens().begin() + 1,
-                                                  line.Tokens().end() - 1)) {
-      AdjustColumnUsingTokenSpacing(ftoken, &column);
-      column += ftoken.token->text().length();
+    for (size_t i = 1; i < tokens.size(); ++i) {
+      const auto &ftoken = tokens[i];
+      switch (ftoken.before.action) {
+        case verible::SpacingDecision::kPreserve: {
+          if (ftoken.before.preserved_space_start !=
+              verible::string_view_null_iterator()) {
+            const std::string_view leading = ftoken.OriginalLeadingSpaces();
+            const auto last_nl = leading.find_last_of('\n');
+            if (last_nl == std::string_view::npos) {
+              column += leading.length();
+            } else {
+              column = static_cast<int>(leading.length() - last_nl - 1);
+            }
+          } else {
+            column += ftoken.before.spaces;
+          }
+          break;
+        }
+        case verible::SpacingDecision::kWrap:
+          // Newline then only the wrap indent (same as FormattedToken emit).
+          column = ftoken.before.spaces;
+          break;
+        case verible::SpacingDecision::kAlign:
+        case verible::SpacingDecision::kAppend:
+          column += ftoken.before.spaces;
+          break;
+      }
+      // Do not add the last token's length: that is the EOL comment whose
+      // starting column we want.
+      if (i + 1 < tokens.size()) {
+        column += ftoken.token->text().length();
+      }
     }
-    AdjustColumnUsingTokenSpacing(line.Tokens().back(), &column);
 
     CHECK_GE(column, 0);
     return column;

verible/verilog/formatting/formatter_test.cc

@@ -19216,6 +19216,50 @@ TEST(FormatterEndToEndTest,
   }
 }
 
+// Regression for https://github.com/chipsalliance/verible/issues/2542:
+// Continuation EOL comments after a wrapped assign must keep a stable column
+// across re-format (convergence).
+TEST(FormatterEndToEndTest, ContinuationCommentAfterWrappedAssignConverges) {
+  static constexpr FormatterTestCase kTestCases[] = {
+      {// Comments originally column-aligned after a wrapped assign
+       "module m;\n"
+       "  assign status_ur = !(status_sc || status_ca ||\n"
+       "    status_crs);      // Completions with a Reserved Completion\n"
+       "                      // Status value are treated as UR\n"
+       "endmodule\n",
+       "module m;\n"
+       "  assign status_ur =\n"
+       "      !(status_sc || status_ca || status_crs);  // Completions with a "
+       "Reserved Completion\n"
+       "                                                // Status value are "
+       "treated as UR\n"
+       "endmodule\n"},
+      {// Previously mis-aligned continuation is not treated as a continuation
+       // (column delta > 1) and must still converge
+       "module m;\n"
+       "  assign status_ur = !(status_sc || status_ca ||\n"
+       "    status_crs);      // Completions with a Reserved Completion\n"
+       "                                                                       "
+       "// Status value are treated as UR\n"
+       "endmodule\n",
+       "module m;\n"
+       "  assign status_ur =\n"
+       "      !(status_sc || status_ca || status_crs);  // Completions with a "
+       "Reserved Completion\n"
+       "  // Status value are treated as UR\n"
+       "endmodule\n"},
+  };
+  FormatStyle style;  // default column_limit (100)
+  for (const auto &test_case : kTestCases) {
+    VLOG(1) << "code-to-format:\n" << test_case.input << "<EOF>";
+    std::ostringstream stream;
+    const auto status =
+        FormatVerilog(test_case.input, "<filename>", style, stream);
+    EXPECT_OK(status) << status.message();
+    EXPECT_EQ(stream.str(), test_case.expected) << "code:\n" << test_case.input;
+  }
+}
+
 }  // namespace
 }  // namespace formatter
 }  // namespace verilog