Skip to content

Commit

Permalink
PR #229: Allow lint configuration to be placed in files
Browse files Browse the repository at this point in the history 
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 #229

Copybara import of the project:

  - 776ca4e Allow lint configuration to be placed in files by Tomasz Gorochowik <[email protected]>

Closes #229

PiperOrigin-RevId: 301922115
  • Loading branch information
@tgorochowik @hzeller
tgorochowik authored and hzeller committed Mar 20, 2020
1 parent af5273e commit 751d4d8
Show file tree
Hide file tree
Showing 5 changed files with 150 additions and 66 deletions.
6 changes: 6 additions & 0 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,8 @@ verilog_lint: usage: verilog_lint [options] <file> [<file>...]
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;
Expand Down Expand Up @@ -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
Expand Down
19 changes: 19 additions & 0 deletions verilog/analysis/verilog_linter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down Expand Up @@ -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);
Expand Down
107 changes: 59 additions & 48 deletions verilog/analysis/verilog_linter_configuration.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string> 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;
Expand Down Expand Up @@ -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<std::string> 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
2 changes: 2 additions & 0 deletions verilog/analysis/verilog_linter_configuration.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ 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);
std::string UnparseConfiguration(const char separator) const;
};

// Pair of functions that perform stringification and destringification
Expand Down
82 changes: 64 additions & 18 deletions verilog/analysis/verilog_linter_configuration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -586,48 +586,69 @@ 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());
}

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);
}
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
}
Expand All @@ -678,17 +702,39 @@ 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);
}

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\"");
}
Expand Down

0 comments on commit 751d4d8

Please sign in to comment.