diff --git a/README.md b/README.md index d3949e8f6..fa7d9b47c 100644 --- a/README.md +++ b/README.md @@ -304,6 +304,8 @@ verilog_lint: usage: verilog_lint [options] [...] Flags from verilog/analysis/verilog_linter.cc: --rules (List of lint rules to enable. Prefix a rule name with '-' to disable it.); default: ; + --rules_config (Path to lint rules configuration file.); + default: ".rules.verible_lint"; --ruleset ([default|all|none], the base set of rules used by linter); default: default; @@ -337,6 +339,10 @@ disables the [`no-tabs`][lint-rule-list_no-tabs] rule. verilog_lint --rules=enum-name-style,line-length=length:80,-no-tabs ... ``` +Additionally, the `--rules_config` flag can be used to read configuration stored +in a file. The syntax is the same as above, except the rules can be also +separated with the newline character. + #### Waiving Lint Violations {#lint-waiver} In the rare circumstance where a line needs to be waived from a particular lint diff --git a/verilog/analysis/verilog_linter.cc b/verilog/analysis/verilog_linter.cc index 9cb449b3e..c476ba0c9 100644 --- a/verilog/analysis/verilog_linter.cc +++ b/verilog/analysis/verilog_linter.cc @@ -55,6 +55,8 @@ ABSL_FLAG(verilog::RuleBundle, rules, {}, "List of lint rules to enable. " "Prefix a rule name with '-' to disable it."); +ABSL_FLAG(std::string, rules_config, ".rules.verible_lint", + "Path to lint rules configuration file."); ABSL_FLAG(verilog::RuleSet, ruleset, verilog::RuleSet::kDefault, "[default|all|none], the base set of rules used by linter"); @@ -210,6 +212,23 @@ LinterConfiguration LinterConfigurationFromFlags() { const auto& ruleset = absl::GetFlag(FLAGS_ruleset); config.UseRuleSet(ruleset); + // Read local configuration file + std::string content; + if (verible::file::GetContents(absl::GetFlag(FLAGS_rules_config), &content)) { + RuleBundle local_rules_bundle; + std::string error; + if (local_rules_bundle.ParseConfiguration(content, &error)) { + config.UseRuleBundle(local_rules_bundle); + } else { + LOG(ERROR) << "Unable to fully parse configuration: " << error + << std::endl; + } + } else if (FLAGS_rules_config.IsModified()) { + // If flag is modified and we were unable to open the file, report that + LOG(WARNING) << "Unable to open rules configuration file: " + << absl::GetFlag(FLAGS_rules_config) << std::endl; + } + // Turn on rules found in config flags. const auto& rules = absl::GetFlag(FLAGS_rules); config.UseRuleBundle(rules); diff --git a/verilog/analysis/verilog_linter_configuration.cc b/verilog/analysis/verilog_linter_configuration.cc index 179d5fdc3..cebb019a3 100644 --- a/verilog/analysis/verilog_linter_configuration.cc +++ b/verilog/analysis/verilog_linter_configuration.cc @@ -83,6 +83,63 @@ std::string ProjectPolicy::ListPathGlobs() const { }); } +bool RuleBundle::ParseConfiguration(absl::string_view text, + 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())) { + // If prefix is '-', the rule is disabled. + // Note that part is guaranteed to be at least one character because + // of absl::SkipEmpty() + const bool prefix_minus = (part[0] == '-'); + + RuleSetting setting = {!prefix_minus, ""}; + + const auto rule_name_with_config = part.substr(prefix_minus ? 1 : 0); + + // Independent of the enabled-ness: extract a configuration string + // if there is any assignment. + const auto equals_pos = rule_name_with_config.find('='); + if (equals_pos != absl::string_view::npos) { + const auto config = rule_name_with_config.substr(equals_pos + 1); + setting.configuration.assign(config.data(), config.size()); + } + const auto rule_name = rule_name_with_config.substr(0, equals_pos); + const auto rule_name_set = analysis::GetAllRegisteredLintRuleNames(); + const auto rule_iter = rule_name_set.find(rule_name); + + // Check if text is a valid lint rule. + if (rule_iter == rule_name_set.end()) { + *error = absl::StrCat("invalid flag \"", rule_name, "\""); + return false; + } else { + // Map keys must use canonical registered string_views for guaranteed + // lifetime, not just any string-equivalent copy. + rules[*rule_iter] = setting; + } + } + + return true; +} + +// Parse and unparse for RuleBundle (for commandlineflags) +std::string RuleBundle::UnparseConfiguration(const char separator) const { + std::vector switches; + for (const auto& rule : rules) { + switches.push_back(absl::StrCat( + // If rule is set off, prepend "-" + rule.second.enabled ? "" : "-", rule.first, + // If we have a configuration, append assignment. + rule.second.configuration.empty() ? "" : "=", + rule.second.configuration)); + } + // Concatenates all of rules into text. + return absl::StrJoin(switches.rbegin(), switches.rend(), + std::string(1, separator)); +} + bool LinterConfiguration::RuleIsOn(const analysis::LintRuleId& rule) const { const auto* entry = FindOrNull(configuration_, rule); if (entry == nullptr) return false; @@ -245,59 +302,13 @@ bool AbslParseFlag(absl::string_view text, RuleSet* rules, std::string* error) { return EnumMapParseFlag(*flag_map, text, rules, error); } -// Parse and unparse for RuleBundle (for commandlineflags) std::string AbslUnparseFlag(const RuleBundle& bundle) { - std::vector switches; - for (const auto& rule : bundle.rules) { - switches.push_back(absl::StrCat( - // If rule is set off, prepend "-" - rule.second.enabled ? "" : "-", rule.first, - // If we have a configuration, append assignment. - rule.second.configuration.empty() ? "" : "=", - rule.second.configuration)); - } - // Concatenates all of rules into text. - return absl::StrJoin(switches.rbegin(), switches.rend(), ","); + return bundle.UnparseConfiguration(','); } bool AbslParseFlag(absl::string_view text, RuleBundle* bundle, std::string* error) { - // Clear the vector to overwrite any existing value. - bundle->rules.clear(); - - for (absl::string_view part : absl::StrSplit(text, ',', absl::SkipEmpty())) { - // If prefix is '-', the rule is disabled. - // Note that part is guaranteed to be at least one character because - // of absl::SkipEmpty() - const bool prefix_minus = (part[0] == '-'); - - RuleSetting setting = {!prefix_minus, ""}; - - const auto rule_name_with_config = part.substr(prefix_minus ? 1 : 0); - - // Independent of the enabled-ness: extract a configuration string - // if there is any assignment. - const auto equals_pos = rule_name_with_config.find('='); - if (equals_pos != absl::string_view::npos) { - const auto config = rule_name_with_config.substr(equals_pos + 1); - setting.configuration.assign(config.data(), config.size()); - } - const auto rule_name = rule_name_with_config.substr(0, equals_pos); - const auto rule_name_set = analysis::GetAllRegisteredLintRuleNames(); - const auto rule_iter = rule_name_set.find(rule_name); - - // Check if text is a valid lint rule. - if (rule_iter == rule_name_set.end()) { - *error = absl::StrCat("invalid flag \"", rule_name, "\""); - return false; - } else { - // Map keys must use canonical registered string_views for guaranteed - // lifetime, not just any string-equivalent copy. - bundle->rules[*rule_iter] = setting; - } - } - - return true; + return bundle->ParseConfiguration(text, error); } } // namespace verilog diff --git a/verilog/analysis/verilog_linter_configuration.h b/verilog/analysis/verilog_linter_configuration.h index 87b05e91d..86ef7a8d8 100644 --- a/verilog/analysis/verilog_linter_configuration.h +++ b/verilog/analysis/verilog_linter_configuration.h @@ -61,6 +61,8 @@ bool AbslParseFlag(absl::string_view text, RuleSet* rules, std::string* error); // equivalent) for the lifetime guarantee. struct RuleBundle { std::map rules; + bool ParseConfiguration(absl::string_view text, std::string* error); + std::string UnparseConfiguration(const char separator) const; }; // Pair of functions that perform stringification and destringification diff --git a/verilog/analysis/verilog_linter_configuration_test.cc b/verilog/analysis/verilog_linter_configuration_test.cc index 1db635127..b761ca891 100644 --- a/verilog/analysis/verilog_linter_configuration_test.cc +++ b/verilog/analysis/verilog_linter_configuration_test.cc @@ -586,38 +586,58 @@ TEST(RuleSetTest, UnparseRuleSetSuccess) { // TEST(RuleBundleTest, UnparseRuleBundleSeveral) { RuleBundle bundle = {{{"flag1", {true, ""}}, {"flag2", {true, ""}}}}; - std::string expected = "flag2,flag1"; - std::string result = AbslUnparseFlag(bundle); - EXPECT_EQ(result, expected); + std::string expected_comma = "flag2,flag1"; + std::string expected_newline = "flag2\nflag1"; + + std::string result_comma = bundle.UnparseConfiguration(','); + EXPECT_EQ(result_comma, expected_comma); + + std::string result_newline = bundle.UnparseConfiguration('\n'); + EXPECT_EQ(result_newline, expected_newline); } TEST(RuleBundleTest, UnparseRuleBundleSeveralTurnOff) { RuleBundle bundle = {{{"flag1", {false, ""}}, {"flag2", {true, ""}}}}; - std::string expected = "flag2,-flag1"; - std::string result = AbslUnparseFlag(bundle); - EXPECT_EQ(result, expected); + std::string expected_comma = "flag2,-flag1"; + std::string expected_newline = "flag2\n-flag1"; + + std::string result_comma = bundle.UnparseConfiguration(','); + EXPECT_EQ(result_comma, expected_comma); + + std::string result_newline = bundle.UnparseConfiguration('\n'); + EXPECT_EQ(result_newline, expected_newline); } TEST(RuleBundleTest, UnparseRuleBundleSeveralConfiguration) { RuleBundle bundle = {{{"flag1", {false, "foo"}}, {"flag2", {true, "bar"}}}}; - std::string expected = "flag2=bar,-flag1=foo"; - std::string result = AbslUnparseFlag(bundle); - EXPECT_EQ(result, expected); + std::string expected_comma = "flag2=bar,-flag1=foo"; + std::string expected_newline = "flag2=bar\n-flag1=foo"; + + std::string result_comma = bundle.UnparseConfiguration(','); + EXPECT_EQ(result_comma, expected_comma); + + std::string result_newline = bundle.UnparseConfiguration('\n'); + EXPECT_EQ(result_newline, expected_newline); } TEST(RuleBundleTest, UnparseRuleBundleEmpty) { RuleBundle bundle = {}; std::string expected = ""; - std::string result = AbslUnparseFlag(bundle); - EXPECT_EQ(result, expected); + + std::string result_comma = bundle.UnparseConfiguration(','); + EXPECT_EQ(result_comma, expected); + + std::string result_newline = bundle.UnparseConfiguration('\n'); + EXPECT_EQ(result_newline, expected); } TEST(RuleBundleTest, ParseRuleBundleEmpty) { std::string text = ""; RuleBundle bundle; std::string error; - bool success = AbslParseFlag(text, &bundle, &error); + bool success = bundle.ParseConfiguration(text, &error); EXPECT_TRUE(success); + EXPECT_TRUE(error.empty()); EXPECT_TRUE(bundle.rules.empty()); } @@ -625,9 +645,10 @@ TEST(RuleBundleTest, ParseRuleBundleAcceptSeveral) { std::string text = "test-rule-1,test-rule-2"; RuleBundle bundle; std::string error; - bool success = AbslParseFlag(text, &bundle, &error); + bool success = bundle.ParseConfiguration(text, &error); ASSERT_TRUE(success); ASSERT_THAT(bundle.rules, SizeIs(2)); + EXPECT_TRUE(error.empty()); EXPECT_TRUE(bundle.rules["test-rule-1"].enabled); EXPECT_TRUE(bundle.rules["test-rule-2"].enabled); } @@ -636,9 +657,10 @@ 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 = AbslParseFlag(text, &bundle, &error); + bool success = bundle.ParseConfiguration(text, &error); ASSERT_TRUE(success); ASSERT_THAT(bundle.rules, SizeIs(4)); + EXPECT_TRUE(error.empty()); EXPECT_TRUE(bundle.rules["test-rule-1"].enabled); EXPECT_EQ("foo", bundle.rules["test-rule-1"].configuration); @@ -657,7 +679,8 @@ TEST(RuleBundleTest, ParseRuleBundleAcceptOne) { std::string text = "test-rule-1"; RuleBundle bundle; std::string error; - bool success = AbslParseFlag(text, &bundle, &error); + bool success = bundle.ParseConfiguration(text, &error); + EXPECT_TRUE(error.empty()); ASSERT_TRUE(success); ASSERT_THAT(bundle.rules, SizeIs(1)); EXPECT_TRUE(bundle.rules["test-rule-1"].enabled); @@ -667,9 +690,10 @@ TEST(RuleBundleTest, ParseRuleBundleAcceptSeveralTurnOff) { std::string text = "test-rule-1,-test-rule-2"; RuleBundle bundle; std::string error; - bool success = AbslParseFlag(text, &bundle, &error); + bool success = bundle.ParseConfiguration(text, &error); ASSERT_TRUE(success); ASSERT_THAT(bundle.rules, SizeIs(2)); + EXPECT_TRUE(error.empty()); EXPECT_TRUE(bundle.rules["test-rule-1"].enabled); EXPECT_FALSE(bundle.rules["test-rule-2"].enabled); } @@ -678,9 +702,10 @@ TEST(RuleBundleTest, ParseRuleBundleAcceptOneTurnOff) { std::string text = "-test-rule-1"; RuleBundle bundle; std::string error; - bool success = AbslParseFlag(text, &bundle, &error); + bool success = bundle.ParseConfiguration(text, &error); ASSERT_TRUE(success); ASSERT_THAT(bundle.rules, SizeIs(1)); + EXPECT_TRUE(error.empty()); EXPECT_FALSE(bundle.rules["test-rule-1"].enabled); } @@ -688,7 +713,28 @@ TEST(RuleBundleTest, ParseRuleBundleReject) { std::string text = "test-rule-1,bad-flag"; RuleBundle bundle; std::string error; - bool success = AbslParseFlag(text, &bundle, &error); + bool success = bundle.ParseConfiguration(text, &error); + EXPECT_FALSE(success); + EXPECT_EQ(error, "invalid flag \"bad-flag\""); +} + +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); + ASSERT_THAT(bundle.rules, SizeIs(2)); + EXPECT_TRUE(error.empty()); + EXPECT_TRUE(bundle.rules["test-rule-1"].enabled); + EXPECT_FALSE(bundle.rules["test-rule-2"].enabled); +} + +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); EXPECT_FALSE(success); EXPECT_EQ(error, "invalid flag \"bad-flag\""); }