Merge pull request #2577 from kbrunham-intel/fix/2352

Fix formatter spacing around path separators in macro arguments

verible/verilog/formatting/formatter_issue_regression_test.cc

diff --git a/verible/verilog/formatting/formatter_issue_regression_test.cc b/verible/verilog/formatting/formatter_issue_regression_test.cc
index 4dcbad5..ae6d475 100644
--- a/verible/verilog/formatting/formatter_issue_regression_test.cc
+++ b/verible/verilog/formatting/formatter_issue_regression_test.cc
@@ -224,6 +224,34 @@
   }
 }
 
+// Regression for https://github.com/chipsalliance/verible/issues/2352:
+// '/' between identifiers in a macro argument is a path separator and must
+// not be spaced as a division operator (that breaks compiles).
+TEST(FormatterEndToEndTest, MacroArgPathSeparatorsKeepNoSpace) {
+  static constexpr FormatterTestCase kTestCases[] = {
+      {// Original issue sample
+       "`PROJECT_INCLUDE(`PATH_MY_MODULE/src/config_class.sv)\n",
+       "`PROJECT_INCLUDE(`PATH_MY_MODULE/src/config_class.sv)\n"},
+      {// Extra spaces around '/' are removed in macro args
+       "`PROJECT_INCLUDE(`PATH_MY_MODULE / src / config_class.sv)\n",
+       "`PROJECT_INCLUDE(`PATH_MY_MODULE/src/config_class.sv)\n"},
+      {// Nested directories
+       "`INCLUDE(foo/bar/baz.svh)\n", "`INCLUDE(foo/bar/baz.svh)\n"},
+      {// Path as a later argument
+       "`LOAD(cfg, `ROOT/hw/ip/file.sv)\n",
+       "`LOAD(cfg, `ROOT/hw/ip/file.sv)\n"},
+      {// Division between identifiers outside macros still gets spaces
+       "module m;\n"
+       "  assign x = a/b;\n"
+       "endmodule\n",
+       "module m;\n"
+       "  assign x = a / b;\n"
+       "endmodule\n"},
+  };
+  FormatStyle style;  // default column_limit (100)
+  RunFormatterTestCases(style, kTestCases);
+}
+
 // Regression for https://github.com/chipsalliance/verible/issues/2008
 // (also https://github.com/chipsalliance/verible/issues/2474 and
 // https://github.com/chipsalliance/verible/issues/2063):

verible/verilog/formatting/token-annotator.cc

diff --git a/verible/verilog/formatting/token-annotator.cc b/verible/verilog/formatting/token-annotator.cc
index 29fe61e..47a8669 100644
--- a/verible/verilog/formatting/token-annotator.cc
+++ b/verible/verilog/formatting/token-annotator.cc
@@ -120,6 +120,29 @@
       {NodeEnum::kConditionExpression});  // exclude
 }
 
+// '/' between identifiers inside a macro argument is a filesystem path
+// (e.g. `INCLUDE(`PATH/src/file.sv)), not a division operator (issue #2352).
+static bool InMacroArgumentContext(const SyntaxTreeContext &context) {
+  return context.IsInsideFirst({NodeEnum::kMacroArgList, NodeEnum::kMacroCall,
+                                NodeEnum::kMacroGenericItem},
+                               {});
+}
+
+static bool IsPathSeparatorSlashInMacroArg(
+    const PreFormatToken &left, const PreFormatToken &right,
+    const SyntaxTreeContext &left_context,
+    const SyntaxTreeContext &right_context) {
+  if (left.TokenEnum() != '/' && right.TokenEnum() != '/') return false;
+  const bool identifier_on_other_side =
+      (left.TokenEnum() == '/' &&
+       right.format_token_enum == FormatTokenType::identifier) ||
+      (right.TokenEnum() == '/' &&
+       left.format_token_enum == FormatTokenType::identifier);
+  if (!identifier_on_other_side) return false;
+  return InMacroArgumentContext(left_context) ||
+         InMacroArgumentContext(right_context);
+}
+
 static bool IsAnySemicolon(const PreFormatToken &ftoken) {
   // These are just syntactically disambiguated versions of ';'.
   return ftoken.TokenEnum() == ';' ||
@@ -228,6 +251,10 @@
   // Consider assignment operators in the same class as binary operators.
   if (left.format_token_enum == FormatTokenType::binary_operator ||
       right.format_token_enum == FormatTokenType::binary_operator) {
+    if (IsPathSeparatorSlashInMacroArg(left, right, left_context,
+                                       right_context)) {
+      return {0, "No space around '/' path separators in macro arguments"};
+    }
     // Inside [], allows 0 or 1 spaces, and symmetrize.
     // TODO(fangism): make this behavior configurable
     if (right.format_token_enum == FormatTokenType::binary_operator &&

verible/verilog/formatting/token-annotator_test.cc

diff --git a/verible/verilog/formatting/token-annotator_test.cc b/verible/verilog/formatting/token-annotator_test.cc
index c0f7e86..df27619 100644
--- a/verible/verilog/formatting/token-annotator_test.cc
+++ b/verible/verilog/formatting/token-annotator_test.cc
@@ -4788,6 +4788,48 @@
           {/* unspecified context */},
           {1, SpacingOptions::kUndecided},
       },
+      // '/' between identifiers is a path separator in macro args (#2352),
+      // but remains a binary operator in other contexts.
+      {
+          DefaultStyle,
+          {verilog_tokentype::MacroIdentifier, "`PATH"},
+          {'/', "/"},
+          {NodeEnum::kMacroArgList, NodeEnum::kMacroCall},
+          {NodeEnum::kMacroArgList, NodeEnum::kMacroCall},
+          {0, SpacingOptions::kUndecided},
+      },
+      {
+          DefaultStyle,
+          {'/', "/"},
+          {verilog_tokentype::SymbolIdentifier, "src"},
+          {NodeEnum::kMacroArgList, NodeEnum::kMacroCall},
+          {NodeEnum::kMacroArgList, NodeEnum::kMacroCall},
+          {0, SpacingOptions::kUndecided},
+      },
+      {
+          DefaultStyle,
+          {verilog_tokentype::SymbolIdentifier, "src"},
+          {'/', "/"},
+          {NodeEnum::kMacroArgList, NodeEnum::kMacroCall},
+          {NodeEnum::kMacroArgList, NodeEnum::kMacroCall},
+          {0, SpacingOptions::kUndecided},
+      },
+      {
+          DefaultStyle,
+          {verilog_tokentype::SymbolIdentifier, "a"},
+          {'/', "/"},
+          {/* expression, not a macro argument */},
+          {/* expression, not a macro argument */},
+          {1, SpacingOptions::kUndecided},
+      },
+      {
+          DefaultStyle,
+          {'/', "/"},
+          {verilog_tokentype::SymbolIdentifier, "b"},
+          {/* expression, not a macro argument */},
+          {/* expression, not a macro argument */},
+          {1, SpacingOptions::kUndecided},
+      },
   };
   int test_index = 0;
   for (const auto &test_case : kTestCases) {