Fix formatter non-convergence for multi-line macro sums

verible/verilog/analysis/verilog-equivalence.cc

@@ -167,11 +167,21 @@ DiffStatus LexicallyEquivalent(
   DiffStatus diff_status = DiffStatus::kEquivalent;
   auto recursive_comparator = [&](const TokenSequence::const_iterator l,
                                   const TokenSequence::const_iterator r) {
+    // Some token enums differ only by surrounding whitespace (e.g. whether a
+    // macro or ')' ends a line). Treat those pairs as matching enums when the
+    // spelling is unchanged so FormatEquivalent tolerates re-wrapping.
+    const bool whitespace_dependent_enum_match =
+        ((l->token_enum() == verilog_tokentype::MacroCallCloseToEndLine &&
+          r->text() == ")") ||
+         (r->token_enum() == verilog_tokentype::MacroCallCloseToEndLine &&
+          l->text() == ")") ||
+         ((l->token_enum() == verilog_tokentype::MacroIdentifier ||
+           l->token_enum() == verilog_tokentype::MacroIdItem) &&
+          (r->token_enum() == verilog_tokentype::MacroIdentifier ||
+           r->token_enum() == verilog_tokentype::MacroIdItem) &&
+          l->text() == r->text()));
     if (l->token_enum() != r->token_enum() &&
-        !((l->token_enum() == verilog_tokentype::MacroCallCloseToEndLine &&
-           r->text() == ")") ||
-          (r->token_enum() == verilog_tokentype::MacroCallCloseToEndLine &&
-           l->text() == ")"))) {
+        !whitespace_dependent_enum_match) {
       if (errstream != nullptr) {
         *errstream << "Mismatched token enums.  got: ";
         token_printer(*l, *errstream);
@@ -258,6 +268,16 @@ DiffStatus FormatEquivalent(std::string_view left, std::string_view right,
              (r.text() == ")"))) {
           return true;
         }
+        // MacroIdentifier vs MacroIdItem depends only on whether the macro
+        // ends the line (see POST_MACRO_ID in verilog.lex). Spelling-equal
+        // macros are format-equivalent across that reclassification.
+        if ((l.token_enum() == verilog_tokentype::MacroIdentifier ||
+             l.token_enum() == verilog_tokentype::MacroIdItem) &&
+            (r.token_enum() == verilog_tokentype::MacroIdentifier ||
+             r.token_enum() == verilog_tokentype::MacroIdItem) &&
+            l.text() == r.text()) {
+          return true;
+        }
         return l.EquivalentWithoutLocation(r);
       },
       errstream);

verible/verilog/analysis/verilog-equivalence_test.cc

@@ -255,6 +255,21 @@ TEST(FormatEquivalentTest, EquivalenceOfRightParen) {
   }
 }
 
+// MacroIdentifier vs MacroIdItem depends on whether the macro ends the line.
+TEST(FormatEquivalentTest, EquivalenceOfMacroIdentifierAndMacroIdItem) {
+  const char *kSameSpelling[] = {
+      "assign x = f(`TOKEN);\n",
+      "assign x = f(\n`TOKEN\n);\n",
+  };
+  ExpectCompareWithErrstream(FormatEquivalent, DiffStatus::kEquivalent,
+                             kSameSpelling[0], kSameSpelling[1]);
+
+  // Different macro names remain different.
+  ExpectCompareWithErrstream(FormatEquivalent, DiffStatus::kDifferent,
+                             "assign x = f(`TOKEN);\n",
+                             "assign x = f(`OTHER);\n");
+}
+
 TEST(FormatEquivalentTest, DiagnosticMismatch) {
   const char *kTestCases[] = {
       "module foo;\n",

verible/verilog/formatting/formatter_test.cc

@@ -19216,6 +19216,50 @@ TEST(FormatterEndToEndTest,
   }
 }
 
+// Regression for https://github.com/chipsalliance/verible/issues/2547:
+// A localparam initialized to a sum of long macros must converge: infix `+`
+// stays with the following operand so re-format does not oscillate between
+// `+\n`MACRO` and `+ `MACRO`.
+TEST(FormatterEndToEndTest, LongMacroSumLocalparamConverges) {
+  static constexpr FormatterTestCase kTestCases[] = {
+      {"module m;\n"
+       "  localparam N =\n"
+       "      `MACRO_GEN3_SCRAMBLE_LFSR_REGOUT\n"
+       "      + `MACRO_GEN3_SCRAMBLE_REGIN\n"
+       "      + `MACRO_GEN3_SCRAMBLE_REGOUT\n"
+       "      ;\n"
+       "endmodule\n",
+       "module m;\n"
+       "  localparam N =\n"
+       "  `MACRO_GEN3_SCRAMBLE_LFSR_REGOUT\n"
+       "  + `MACRO_GEN3_SCRAMBLE_REGIN\n"
+       "  + `MACRO_GEN3_SCRAMBLE_REGOUT;\n"
+       "endmodule\n"},
+      // Already in pass-1 form must stay stable.
+      {"module m;\n"
+       "  localparam N =\n"
+       "  `MACRO_GEN3_SCRAMBLE_LFSR_REGOUT\n"
+       "  + `MACRO_GEN3_SCRAMBLE_REGIN\n"
+       "  + `MACRO_GEN3_SCRAMBLE_REGOUT;\n"
+       "endmodule\n",
+       "module m;\n"
+       "  localparam N =\n"
+       "  `MACRO_GEN3_SCRAMBLE_LFSR_REGOUT\n"
+       "  + `MACRO_GEN3_SCRAMBLE_REGIN\n"
+       "  + `MACRO_GEN3_SCRAMBLE_REGOUT;\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

verible/verilog/formatting/tree-unwrapper.cc

@@ -1637,6 +1637,53 @@ static void AttachSeparatorsToListElementPartitions(
   }
 }
 
+// True when a leaf partition contains only a binary/infix operator (and
+// optional comments/attributes). Used to normalize wrapping of expressions
+// like `A + B + C` so operators always stay with the following operand.
+static bool PartitionIsInfixOperatorOnly(const TokenPartitionTree &partition) {
+  if (!is_leaf(partition)) return false;
+  const auto tokens = partition.Value().TokensRange();
+  if (tokens.empty()) return false;
+
+  const verible::PreFormatToken *op = nullptr;
+  for (const auto &token : tokens) {
+    switch (token.TokenEnum()) {
+      case verilog_tokentype::TK_COMMENT_BLOCK:
+      case verilog_tokentype::TK_EOL_COMMENT:
+      case verilog_tokentype::TK_ATTRIBUTE:
+        break;
+      default:
+        if (GetFormatTokenType(static_cast<verilog_tokentype>(
+                token.TokenEnum())) != FormatTokenType::binary_operator ||
+            op != nullptr) {
+          return false;
+        }
+        op = &token;
+        break;
+    }
+  }
+  return op != nullptr;
+}
+
+// Always attach infix-operator-only partitions to the following operand.
+// Attachment based on original newlines is unstable for macro sums:
+//   `A\n+\n`B  vs  `A\n+ `B  produce different partition shapes and oscillate
+// under re-format (GitHub issue 2547).
+static void AttachInfixOperatorsToFollowingOperands(
+    TokenPartitionTree *partition) {
+  // Iterate by index; merges invalidate sibling pointers.
+  for (int i = 0; i < static_cast<int>(partition->Children().size()); ++i) {
+    auto &child = partition->Children()[i];
+    if (!PartitionIsInfixOperatorOnly(child)) continue;
+    if (NextLeaf(child) == nullptr) continue;
+    VLOG(4) << "Attaching infix operator partition to following operand:\n"
+            << child;
+    verible::MergeLeafIntoNextLeaf(&child);
+    // Children shifted; re-check current index.
+    --i;
+  }
+}
+
 static void AttachTrailingSemicolonToPreviousPartition(
     TokenPartitionTree *partition) {
   // TODO(mglb): Replace this function with
@@ -3111,6 +3158,7 @@ void TreeUnwrapper::ReshapeTokenPartitions(
     case NodeEnum::kParamDeclaration: {
       AttachTrailingSemicolonToPreviousPartition(&partition);
       AttachOpeningBraceToDeclarationsAssignmentOperator(&partition);
+      AttachInfixOperatorsToFollowingOperands(&partition);
       break;
     }