From 751d4d8e57741ab22806f91982f59c71ecf37d9d Mon Sep 17 00:00:00 2001 From: Tomasz Gorochowik Date: Thu, 19 Mar 2020 16:44:26 -0700 Subject: [PATCH] PR #229: Allow lint configuration to be placed in files I am opening this early to ask for comments. Some general info: * The configuration from file is read after setting up the default configs, but before parsing the invocation arguments. This way the arguments can easily overwrite the configuration from the file * I think the following should be considered (all of them should be optional): * a global configuration file (e.g. `~/.verible_lint.rules`) * a local configuration file (e.g. the top directory of a project), this assumes the linter is executed from the top dir of the project * any arbitrary file, specified with an argument * If we decide to use all of the options listed above we could: * combine the global file, local file and file from a flag (in that order), or * ignore global file if a local one exists, ignore both if another one is specified with a flag * always overwrite the config from files when the same flags are specified with the `-rules` switch * for now the implementation does some string operations on the file to make the format compatible with the arguments format. The only difference is the settings can be put in separate lines to improve readability. Thanks to this we can reuse the same parsing code Other options to consider: * use a completely different format for the configuration (e.g. ini files, with a general `[rules]` section, and more rule-specific configuration) Please share your thoughts. CC @hzeller GitHub PR https://github.com/google/verible/pull/229 Copybara import of the project: - 776ca4e4fb9049602175e6ab72e7c31d950b2b42 Allow lint configuration to be placed in files by Tomasz Gorochowik Closes #229 PiperOrigin-RevId: 301922115 --- README.md | 6 + verilog/analysis/verilog_linter.cc | 19 ++++ .../analysis/verilog_linter_configuration.cc | 107 ++++++++++-------- .../analysis/verilog_linter_configuration.h | 2 + .../verilog_linter_configuration_test.cc | 82 +++++++++++--- 5 files changed, 150 insertions(+), 66 deletions(-) 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\""); }