Fix formatter non-convergence for multi-line macro sums Always attach infix-operator-only partitions to the following operand in localparam expressions, and treat spelling-equal MacroIdentifier and MacroIdItem as format-equivalent, so long `MACRO + MACRO` sums no longer oscillate under re-format (issue #2547).
diff --git a/verible/verilog/analysis/verilog-equivalence.cc b/verible/verilog/analysis/verilog-equivalence.cc index f8a47d0..026c197 100644 --- a/verible/verilog/analysis/verilog-equivalence.cc +++ b/verible/verilog/analysis/verilog-equivalence.cc
@@ -167,11 +167,21 @@ 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 @@ (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);
diff --git a/verible/verilog/analysis/verilog-equivalence_test.cc b/verible/verilog/analysis/verilog-equivalence_test.cc index 90b0ee1..1cb9436 100644 --- a/verible/verilog/analysis/verilog-equivalence_test.cc +++ b/verible/verilog/analysis/verilog-equivalence_test.cc
@@ -255,6 +255,21 @@ } } +// 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",
diff --git a/verible/verilog/formatting/formatter_test.cc b/verible/verilog/formatting/formatter_test.cc index e885d74..551491f 100644 --- a/verible/verilog/formatting/formatter_test.cc +++ b/verible/verilog/formatting/formatter_test.cc
@@ -19216,6 +19216,50 @@ } } +// 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
diff --git a/verible/verilog/formatting/tree-unwrapper.cc b/verible/verilog/formatting/tree-unwrapper.cc index 15a0082..7b6733f 100644 --- a/verible/verilog/formatting/tree-unwrapper.cc +++ b/verible/verilog/formatting/tree-unwrapper.cc
@@ -1637,6 +1637,53 @@ } } +// 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 @@ case NodeEnum::kParamDeclaration: { AttachTrailingSemicolonToPreviousPartition(&partition); AttachOpeningBraceToDeclarationsAssignmentOperator(&partition); + AttachInfixOperatorsToFollowingOperands(&partition); break; }