Leave long-line wrapped sections of text preserved.

Line wrap optimization and penalty tuning are the weakest links of the formatter.
Attempt to do-less-harm and leave enabled uncontroversial formatting changes.

This still allows indentation at the start of such token ranges to occur, so
there is still a risk of line-length overflow.

PiperOrigin-RevId: 326726800

verilog/formatting/format_style.h

@@ -49,6 +49,13 @@ struct FormatStyle : public verible::BasicFormatStyle {
   //   and promote the control for this into an enum: {off, compact, align}
   bool format_module_instantiations = true;
 
+  // At this time line wrap optimization is problematic and risks ruining
+  // otherwise reasonable code.  When set to false, this switch will make the
+  // formatter give-up and leave code as-is in cases where it would otherwise
+  // attempt to do line wrap optimization.  By doing nothing in those cases, we
+  // reduce the risk of harming already decent code.
+  bool try_wrap_long_lines = true;
+
   // TODO(fangism): introduce the following knobs:
   //
   // Unless forced by previous line, starting a line with a comma is

verilog/formatting/formatter.cc

@@ -269,10 +269,11 @@ Status FormatVerilog(absl::string_view text, absl::string_view filename,
 
 // Decided at each node in UnwrappedLine partition tree whether or not
 // it should be expanded or unexpanded.
-static void DeterminePartitionExpansion(partition_node_type* node,
-                                        absl::string_view full_text,
-                                        const ByteOffsetSet& disabled_ranges,
-                                        const FormatStyle& style) {
+static void DeterminePartitionExpansion(
+    partition_node_type* node,
+    std::vector<verible::PreFormatToken>* preformatted_tokens,
+    absl::string_view full_text, const ByteOffsetSet& disabled_ranges,
+    const FormatStyle& style) {
   auto& node_view = node->Value();
   const auto& children = node->Children();
 
@@ -345,8 +346,21 @@ static void DeterminePartitionExpansion(partition_node_type* node,
         VLOG(3) << "Fits, un-expanding.";
         node_view.Unexpand();
       } else {
-        VLOG(3) << "Does not fit, expanding.";
-        node_view.Expand();
+        if (style.try_wrap_long_lines) {
+          VLOG(3) << "Does not fit, expanding.";
+          node_view.Expand();
+        } else {
+          // give-up early and preserve original spacing
+          VLOG(3) << "Does not fit, preserving.";
+          node_view.Unexpand();
+          const auto range = node_view.Value().TokensRange();
+          const ByteOffsetSet new_disable_range{
+              {range.front().token->left(full_text) + 1,
+               // +1 allows left indentation to be adjusted
+               range.back().token->right(full_text)}};
+          verible::PreserveSpacesOnDisabledTokenRanges(
+              preformatted_tokens, new_disable_range, full_text);
+        }
       }
     }
   }
@@ -355,6 +369,7 @@ static void DeterminePartitionExpansion(partition_node_type* node,
 // Produce a worklist of independently formattable UnwrappedLines.
 static std::vector<UnwrappedLine> MakeUnwrappedLinesWorklist(
     const TokenPartitionTree& format_tokens_partitions,
+    std::vector<verible::PreFormatToken>* preformatted_tokens,
     absl::string_view full_text, const ByteOffsetSet& disabled_ranges,
     const FormatStyle& style) {
   // Initialize a tree view that treats partitions as fully-expanded.
@@ -365,8 +380,10 @@ static std::vector<UnwrappedLine> MakeUnwrappedLinesWorklist(
   // Post-order traversal: if a child doesn't 'fit' and needs to be expanded,
   // so must all of its parents (and transitively, ancestors).
   format_tokens_partition_view.ApplyPostOrder(
-      [&full_text, &disabled_ranges, &style](partition_node_type& node) {
-        DeterminePartitionExpansion(&node, full_text, disabled_ranges, style);
+      [&full_text, &disabled_ranges, &style,
+       preformatted_tokens](partition_node_type& node) {
+        DeterminePartitionExpansion(&node, preformatted_tokens, full_text,
+                                    disabled_ranges, style);
       });
 
   // Remove trailing blank lines.
@@ -533,7 +550,8 @@ Status Formatter::Format(const ExecutionControl& control) {
 
   // Produce sequence of independently operable UnwrappedLines.
   const auto unwrapped_lines = MakeUnwrappedLinesWorklist(
-      *format_tokens_partitions, full_text, disabled_ranges_, style_);
+      *format_tokens_partitions, &unwrapper_data.preformatted_tokens, full_text,
+      disabled_ranges_, style_);
 
   // For each UnwrappedLine: minimize total penalty of wrap/break decisions.
   // TODO(fangism): This could be parallelized if results are written

verilog/formatting/formatter_test.cc

@@ -5697,6 +5697,51 @@ TEST(FormatterEndToEndTest, DisableModuleInstantiations) {
   }
 }
 
+TEST(FormatterEndToEndTest, DisableTryWrapLongLines) {
+  const std::initializer_list<FormatterTestCase> kTestCases = {
+      {"", ""},
+      {"\n", "\n"},
+      {"\n\n", "\n\n"},
+      {"module  m  ;\t\n"
+       "  endmodule\n",
+       "module m;\n"
+       "endmodule\n"},
+      {"module  m(   ) ;\n"
+       "  endmodule\n",
+       "module m ();\n"
+       "endmodule\n"},
+      {"module  m(   ) ;\n"
+       "initial assign a = b;\n"
+       "  endmodule\n",
+       "module m ();\n"
+       "  initial assign a = b;\n"
+       "endmodule\n"},
+      {"module  m(   ) ;\n"
+       "initial assign a = {never +gonna +give +you +up,\n"  // over 40 columns,
+                                                             // give up
+       "never + gonna +Let +you +down};\n"
+       "  endmodule\n",
+       "module m ();\n"
+       "  initial assign a = {never +gonna +give +you +up,\n"
+       "never + gonna +Let +you +down};\n"
+       "endmodule\n"},
+  };
+  FormatStyle style;
+  style.column_limit = 40;
+  style.indentation_spaces = 2;
+  style.wrap_spaces = 4;
+  style.try_wrap_long_lines = false;
+  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);
+    // Require these test cases to be valid.
+    EXPECT_OK(status) << status.message();
+    EXPECT_EQ(stream.str(), test_case.expected) << "code:\n" << test_case.input;
+  }
+}
+
 struct SelectLinesTestCase {
   absl::string_view input;
   LineNumberSet lines;  // explicit set of lines to enable formatting

verilog/formatting/tree_unwrapper.cc

@@ -665,18 +665,11 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions(
     case NodeEnum::kGenerateRegion:
     case NodeEnum::kCaseGenerateConstruct:
     case NodeEnum::kLoopGenerateConstruct:
-    case NodeEnum::kClassDeclaration:
     case NodeEnum::kClassConstructor:
     case NodeEnum::kPackageImportDeclaration:
     // TODO(fangism): case NodeEnum::kDPIExportItem:
     case NodeEnum::kPreprocessorInclude:
     case NodeEnum::kPreprocessorUndef:
-    case NodeEnum::kModuleDeclaration:
-    case NodeEnum::kProgramDeclaration:
-    case NodeEnum::kPackageDeclaration:
-    case NodeEnum::kInterfaceDeclaration:
-    case NodeEnum::kFunctionDeclaration:
-    case NodeEnum::kTaskDeclaration:
     case NodeEnum::kTFPortDeclaration:
     case NodeEnum::kTypeDeclaration:
     case NodeEnum::kForwardDeclaration:
@@ -752,6 +745,13 @@ void TreeUnwrapper::SetIndentationsAndCreatePartitions(
 
       // The following cases will always expand into their constituent
       // partitions:
+    case NodeEnum::kModuleDeclaration:
+    case NodeEnum::kProgramDeclaration:
+    case NodeEnum::kPackageDeclaration:
+    case NodeEnum::kInterfaceDeclaration:
+    case NodeEnum::kFunctionDeclaration:
+    case NodeEnum::kTaskDeclaration:
+    case NodeEnum::kClassDeclaration:
     case NodeEnum::kClassHeader:
     case NodeEnum::kBegin:
     case NodeEnum::kEnd:

verilog/tools/formatter/verilog_format.cc

@@ -124,6 +124,10 @@ ABSL_FLAG(bool, format_module_port_declarations, true,
 ABSL_FLAG(bool, format_module_instantiations, true,
           "If true, format module instantiations (data declarations), "
           "else leave them unformatted.  This is a short-term workaround.");
+ABSL_FLAG(bool, try_wrap_long_lines, false,
+          "If true, let the formatter attempt to optimize line wrapping "
+          "decisions where wrapping is needed, else leave them unformatted.  "
+          "This is a short-term measure to reduce risk-of-harm.");
 
 static std::ostream& FileMsg(absl::string_view filename) {
   std::cerr << filename << ": ";
@@ -181,6 +185,7 @@ static bool formatOneFile(absl::string_view filename,
         absl::GetFlag(FLAGS_format_module_port_declarations);
     format_style.format_module_instantiations =
         absl::GetFlag(FLAGS_format_module_instantiations);
+    format_style.try_wrap_long_lines = absl::GetFlag(FLAGS_try_wrap_long_lines);
   }
 
   std::ostringstream stream;