Add a simple way to ignore #-comments in lint configuration files.

Lint rules can be configured by flag, in which they are a comma-separated
list of rules. In a configuration file, we have multiple lines with
one rule configured per line. In such a file, it is desirable to allow
for comments. This allows for common end-of-line hash comments.

While at it, allow whitespace before and after rules.

Fixes #274

PiperOrigin-RevId: 309871733

verilog/analysis/verilog_linter.cc

@@ -218,7 +218,7 @@ LinterConfiguration LinterConfigurationFromFlags() {
   if (verible::file::GetContents(absl::GetFlag(FLAGS_rules_config), &content)) {
     RuleBundle local_rules_bundle;
     std::string error;
-    if (local_rules_bundle.ParseConfiguration(content, &error)) {
+    if (local_rules_bundle.ParseConfiguration(content, '\n', &error)) {
       config.UseRuleBundle(local_rules_bundle);
     } else {
       LOG(ERROR) << "Unable to fully parse configuration: " << error

verilog/analysis/verilog_linter_configuration.cc

@@ -83,13 +83,26 @@ std::string ProjectPolicy::ListPathGlobs() const {
                        });
 }
 
-bool RuleBundle::ParseConfiguration(absl::string_view text,
+bool RuleBundle::ParseConfiguration(absl::string_view text, char separator,
                                     std::string* error) {
   // Clear the vector to overwrite any existing value.
   rules.clear();
 
   for (absl::string_view part :
-       absl::StrSplit(text, absl::ByAnyChar(",\n"), absl::SkipEmpty())) {
+       absl::StrSplit(text, separator, absl::SkipEmpty())) {
+    if (separator == '\n') {
+      // In configuration files, we can ignore #-comments
+      // TODO(hzeller): this will fall short if in the configuration string
+      // we expect rules that accept a #-character for some reason. In that
+      // case we need to expand this to parse out 'within # quotes' parts.
+      // ... then this will finally become a more complex lexer.
+      const auto comment_pos = part.find('#');
+      if (comment_pos != absl::string_view::npos) {
+        part = part.substr(0, comment_pos);
+      }
+    }
+    part = absl::StripAsciiWhitespace(part);
+    if (part.empty()) continue;
     // If prefix is '-', the rule is disabled. For symmetry, we also allow
     // '+' to enable rule.
     // Note that part is guaranteed to be at least one character because
@@ -310,7 +323,7 @@ std::string AbslUnparseFlag(const RuleBundle& bundle) {
 
 bool AbslParseFlag(absl::string_view text, RuleBundle* bundle,
                    std::string* error) {
-  return bundle->ParseConfiguration(text, error);
+  return bundle->ParseConfiguration(text, ',', error);
 }
 
 }  // namespace verilog

verilog/analysis/verilog_linter_configuration.h

@@ -61,7 +61,10 @@ bool AbslParseFlag(absl::string_view text, RuleSet* rules, std::string* error);
 // equivalent) for the lifetime guarantee.
 struct RuleBundle {
   std::map<absl::string_view, RuleSetting> rules;
-  bool ParseConfiguration(absl::string_view text, std::string* error);
+  // Parse configuration from input. Separator between rules is 'separator',
+  // typically that would be a comma or newline.
+  bool ParseConfiguration(absl::string_view text, char separator,
+                          std::string* error);
   std::string UnparseConfiguration(const char separator) const;
 };
 

verilog/analysis/verilog_linter_configuration_test.cc

@@ -635,8 +635,8 @@ TEST(RuleBundleTest, ParseRuleBundleEmpty) {
   std::string text = "";
   RuleBundle bundle;
   std::string error;
-  bool success = bundle.ParseConfiguration(text, &error);
-  EXPECT_TRUE(success);
+  bool success = bundle.ParseConfiguration(text, ',', &error);
+  EXPECT_TRUE(success) << error;
   EXPECT_TRUE(error.empty());
   EXPECT_TRUE(bundle.rules.empty());
 }
@@ -646,8 +646,8 @@ TEST(RuleBundleTest, ParseRuleBundleAcceptSeveral) {
   std::string text = "test-rule-1,test-rule-2,+test-rule-3";
   RuleBundle bundle;
   std::string error;
-  bool success = bundle.ParseConfiguration(text, &error);
-  ASSERT_TRUE(success);
+  bool success = bundle.ParseConfiguration(text, ',', &error);
+  ASSERT_TRUE(success) << error;
   ASSERT_THAT(bundle.rules, SizeIs(3));
   EXPECT_TRUE(error.empty());
   EXPECT_TRUE(bundle.rules["test-rule-1"].enabled);
@@ -659,8 +659,8 @@ TEST(RuleBundleTest, ParseRuleBundleAcceptConfiguration) {
   auto text = "test-rule-1=foo,test-rule-2=,test-rule-3,-test-rule-4=bar";
   RuleBundle bundle;
   std::string error;
-  bool success = bundle.ParseConfiguration(text, &error);
-  ASSERT_TRUE(success);
+  bool success = bundle.ParseConfiguration(text, ',', &error);
+  ASSERT_TRUE(success) << error;
   ASSERT_THAT(bundle.rules, SizeIs(4));
   EXPECT_TRUE(error.empty());
 
@@ -681,19 +681,32 @@ TEST(RuleBundleTest, ParseRuleBundleAcceptOne) {
   std::string text = "test-rule-1";
   RuleBundle bundle;
   std::string error;
-  bool success = bundle.ParseConfiguration(text, &error);
+  bool success = bundle.ParseConfiguration(text, ',', &error);
   EXPECT_TRUE(error.empty());
-  ASSERT_TRUE(success);
+  ASSERT_TRUE(success) << error;
   ASSERT_THAT(bundle.rules, SizeIs(1));
   EXPECT_TRUE(bundle.rules["test-rule-1"].enabled);
 }
 
+TEST(RuleBundleTest, ParseRuleWhitespaceAroundAllowed) {
+  std::string text = "\t test-rule-1 \t, +test-rule-2=foo:bar \t";
+  RuleBundle bundle;
+  std::string error;
+  bool success = bundle.ParseConfiguration(text, ',', &error);
+  EXPECT_TRUE(error.empty());
+  ASSERT_TRUE(success) << error;
+  ASSERT_THAT(bundle.rules, SizeIs(2));
+  EXPECT_TRUE(bundle.rules["test-rule-1"].enabled);
+  EXPECT_TRUE(bundle.rules["test-rule-2"].enabled);
+  EXPECT_EQ("foo:bar", bundle.rules["test-rule-2"].configuration);
+}
+
 TEST(RuleBundleTest, ParseRuleBundleAcceptSeveralTurnOff) {
   std::string text = "test-rule-1,-test-rule-2";
   RuleBundle bundle;
   std::string error;
-  bool success = bundle.ParseConfiguration(text, &error);
-  ASSERT_TRUE(success);
+  bool success = bundle.ParseConfiguration(text, ',', &error);
+  ASSERT_TRUE(success) << error;
   ASSERT_THAT(bundle.rules, SizeIs(2));
   EXPECT_TRUE(error.empty());
   EXPECT_TRUE(bundle.rules["test-rule-1"].enabled);
@@ -704,8 +717,8 @@ TEST(RuleBundleTest, ParseRuleBundleAcceptOneTurnOff) {
   std::string text = "-test-rule-1";
   RuleBundle bundle;
   std::string error;
-  bool success = bundle.ParseConfiguration(text, &error);
-  ASSERT_TRUE(success);
+  bool success = bundle.ParseConfiguration(text, ',', &error);
+  ASSERT_TRUE(success) << error;
   ASSERT_THAT(bundle.rules, SizeIs(1));
   EXPECT_TRUE(error.empty());
   EXPECT_FALSE(bundle.rules["test-rule-1"].enabled);
@@ -715,7 +728,7 @@ TEST(RuleBundleTest, ParseRuleBundleReject) {
   std::string text = "test-rule-1,bad-flag";
   RuleBundle bundle;
   std::string error;
-  bool success = bundle.ParseConfiguration(text, &error);
+  bool success = bundle.ParseConfiguration(text, ',', &error);
   EXPECT_FALSE(success);
   EXPECT_EQ(error, "invalid flag \"bad-flag\"");
 }
@@ -724,8 +737,8 @@ TEST(RuleBundleTest, ParseRuleBundleAcceptMultiline) {
   std::string text = "test-rule-1\n-test-rule-2";
   RuleBundle bundle;
   std::string error;
-  bool success = bundle.ParseConfiguration(text, &error);
-  ASSERT_TRUE(success);
+  bool success = bundle.ParseConfiguration(text, '\n', &error);
+  ASSERT_TRUE(success) << error;
   ASSERT_THAT(bundle.rules, SizeIs(2));
   EXPECT_TRUE(error.empty());
   EXPECT_TRUE(bundle.rules["test-rule-1"].enabled);
@@ -736,10 +749,31 @@ TEST(RuleBundleTest, ParseRuleBundleRejectMultiline) {
   std::string text = "test-rule-1\nbad-flag\n-test-rule-2";
   RuleBundle bundle;
   std::string error;
-  bool success = bundle.ParseConfiguration(text, &error);
+  bool success = bundle.ParseConfiguration(text, '\n', &error);
   EXPECT_FALSE(success);
   EXPECT_EQ(error, "invalid flag \"bad-flag\"");
 }
 
+TEST(RuleBundleTest, ParseRuleBundleSkipComments) {
+  const std::string text =
+      "    # some comment after whitespace\n"
+      "# more comment\n"
+      "test-rule-1\n"
+      "-test-rule-2  # some comment\n"
+      "+test-rule-3=bar:baz  # config-comment\n";
+  {
+    RuleBundle bundle;
+    std::string error;
+    bool success = bundle.ParseConfiguration(text, '\n', &error);
+    ASSERT_TRUE(success) << error;
+    ASSERT_THAT(bundle.rules, SizeIs(3));
+    EXPECT_TRUE(error.empty());
+    EXPECT_TRUE(bundle.rules["test-rule-1"].enabled);
+    EXPECT_FALSE(bundle.rules["test-rule-2"].enabled);
+    EXPECT_TRUE(bundle.rules["test-rule-3"].enabled);
+    EXPECT_EQ("bar:baz", bundle.rules["test-rule-3"].configuration);
+  }
+}
+
 }  // namespace
 }  // namespace verilog