Skip to content

Commit

Permalink
PR #306: linter: Move Flags handling closer to main
Browse files Browse the repository at this point in the history 
Cleaning up the flag accesses as promised in #237

GitHub PR #306

Copybara import of the project:

  - c07ca70 Clean up flag management for verilog_lint by Tomasz Gorochowik <[email protected]>

Closes #306

PiperOrigin-RevId: 313803455
  • Loading branch information
@tgorochowik @hzeller
tgorochowik authored and hzeller committed May 29, 2020
1 parent 0b0a4c8 commit 85f8ff8
Show file tree
Hide file tree
Showing 5 changed files with 174 additions and 64 deletions.
1 change: 1 addition & 0 deletions verilog/analysis/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ cc_library(
"//common/analysis:token_stream_lint_rule",
"//common/util:container_util",
"//common/util:enum_flags",
"//common/util:file_util",
"//common/util:logging",
"@com_google_absl//absl/status",
"@com_google_absl//absl/strings",
Expand Down
76 changes: 12 additions & 64 deletions verilog/analysis/verilog_linter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,76 +233,24 @@ std::vector<LintRuleStatus> VerilogLinter::ReportStatus(
return statuses;
}

absl::Status AppendLinterConfigurationFromFile(
LinterConfiguration* config, absl::string_view config_filename) {
// Read local configuration file
std::string content;

const absl::Status config_read_status =
verible::file::GetContents(config_filename, &content);
if (config_read_status.ok()) {
RuleBundle local_rules_bundle;
std::string error;
if (local_rules_bundle.ParseConfiguration(content, '\n', &error)) {
config->UseRuleBundle(local_rules_bundle);
} else {
LOG(WARNING) << "Unable to fully parse configuration: " << error;
}
return absl::OkStatus();
}

return config_read_status;
}

LinterConfiguration LinterConfigurationFromFlags(
absl::string_view linting_start_file) {
LinterConfiguration config;

// TODO move all of these calls to GetFlag outside of this function

// Turn on default ruleset.
const auto& ruleset = absl::GetFlag(FLAGS_ruleset);
config.UseRuleSet(ruleset);

if (FLAGS_rules_config.IsModified()) {
// Use configuration file from flag if specified
std::string config_file = absl::GetFlag(FLAGS_rules_config);

const absl::Status config_read_status =
AppendLinterConfigurationFromFile(&config, config_file);

if (!config_read_status.ok()) {
LOG(WARNING) << config_file
<< ": Unable to read rules configuration file "
<< config_read_status << std::endl;
}

} else if (absl::GetFlag(FLAGS_rules_config_search)) {
// Search upward if search is enabled and no configuration file is
// specified
static constexpr absl::string_view linter_config = ".rules.verible_lint";
std::string resolved_config_file;
if (verible::file::UpwardFileSearch(linting_start_file, linter_config,
&resolved_config_file)
.ok()) {
const absl::Status config_read_status =
AppendLinterConfigurationFromFile(&config, resolved_config_file);

if (!config_read_status.ok()) {
LOG(WARNING) << resolved_config_file
<< ": Unable to read rules configuration file "
<< config_read_status << std::endl;
}
}
const verilog::LinterOptions options = {
.ruleset = absl::GetFlag(FLAGS_ruleset),
.rules = absl::GetFlag(FLAGS_rules),
.config_file = absl::GetFlag(FLAGS_rules_config),
.config_file_is_custom = FLAGS_rules_config.IsModified(),
.rules_config_search = absl::GetFlag(FLAGS_rules_config_search),
.linting_start_file = std::string(linting_start_file),
.waiver_files = absl::GetFlag(FLAGS_waiver_files)};

absl::Status config_status = config.ConfigureFromOptions(options);
if (!config_status.ok()) {
LOG(WARNING) << "Unable to configure linter for: " << linting_start_file;
}

// Turn on rules found in config flags.
const auto& rules = absl::GetFlag(FLAGS_rules);
config.UseRuleBundle(rules);

// Apply external waivers
config.external_waivers = absl::GetFlag(FLAGS_waiver_files);

return config;
}

Expand Down
63 changes: 63 additions & 0 deletions verilog/analysis/verilog_linter_configuration.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include "common/analysis/token_stream_lint_rule.h"
#include "common/util/container_util.h"
#include "common/util/enum_flags.h"
#include "common/util/file_util.h"
#include "common/util/logging.h"
#include "verilog/analysis/default_rules.h"
#include "verilog/analysis/lint_rule_registry.h"
Expand Down Expand Up @@ -282,6 +283,68 @@ bool LinterConfiguration::operator==(const LinterConfiguration& config) const {
return ActiveRuleIds() == config.ActiveRuleIds();
}

absl::Status LinterConfiguration::AppendFromFile(
absl::string_view config_filename) {
// Read local configuration file
std::string content;

const absl::Status config_read_status =
verible::file::GetContents(config_filename, &content);
if (config_read_status.ok()) {
RuleBundle local_rules_bundle;
std::string error;
if (local_rules_bundle.ParseConfiguration(content, '\n', &error)) {
UseRuleBundle(local_rules_bundle);
} else {
LOG(WARNING) << "Unable to fully parse configuration: " << error;
}
return absl::OkStatus();
}

return config_read_status;
}

absl::Status LinterConfiguration::ConfigureFromOptions(
const LinterOptions& options) {
UseRuleSet(options.ruleset);

if (options.config_file_is_custom) {
const absl::Status config_read_status = AppendFromFile(options.config_file);

if (!config_read_status.ok()) {
LOG(WARNING) << options.config_file
<< ": Unable to read rules configuration file "
<< config_read_status << std::endl;
}

} else if (options.rules_config_search) {
// Search upward if search is enabled and no configuration file is
// specified
static constexpr absl::string_view linter_config = ".rules.verible_lint";
std::string resolved_config_file;
if (verible::file::UpwardFileSearch(options.linting_start_file,
linter_config, &resolved_config_file)
.ok()) {
const absl::Status config_read_status =
AppendFromFile(resolved_config_file);

if (!config_read_status.ok()) {
LOG(WARNING) << resolved_config_file
<< ": Unable to read rules configuration file "
<< config_read_status << std::endl;
}
}
}

// Turn on rules found in config
UseRuleBundle(options.rules);

// Apply external waivers
external_waivers = std::string(options.waiver_files);

return absl::OkStatus();
}

std::ostream& operator<<(std::ostream& stream,
const LinterConfiguration& config) {
const auto rules = config.ActiveRuleIds();
Expand Down
30 changes: 30 additions & 0 deletions verilog/analysis/verilog_linter_configuration.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,30 @@ struct ProjectPolicy {
std::string ListPathGlobs() const;
};

struct LinterOptions {
// strings, ints, bools, and unprocessed values from flags, no other derived
// information. Reasonable default values may be specified here for each
// member

// The base set of rules used by linter
const RuleSet ruleset;
// Bundle of rules to enable/disable in addition to the base set
const RuleBundle& rules;
// Path to a file with extra linter configuration, applied on top of the
// base 'ruleset' and extra 'rules'
std::string config_file;
// Indicates whether the 'config_file' is the default config file or a custom
// one
bool config_file_is_custom;
// Enables upward config file search
bool rules_config_search;
// Defines the starting point for the upward config search algorithm,
// usually set to the currently linted file
std::string linting_start_file;
// Path to the external waivers configuration file
std::string waiver_files;
};

// LinterConfiguration is used for tracking enabled lint rules
// Individual LintRules are defined LintRuleRegistry. Their names are the
// strings that they are registered under.
Expand Down Expand Up @@ -204,6 +228,12 @@ class LinterConfiguration {

bool operator!=(const LinterConfiguration& r) const { return !(*this == r); }

// Appends linter rules configuration from a file
absl::Status AppendFromFile(absl::string_view filename);

// Generates configuration forn LinterOptions
absl::Status ConfigureFromOptions(const LinterOptions& options);

private:
// map of all enabled rules
std::map<analysis::LintRuleId, RuleSetting> configuration_;
Expand Down
68 changes: 68 additions & 0 deletions verilog/analysis/verilog_linter_configuration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -775,5 +775,73 @@ TEST(RuleBundleTest, ParseRuleBundleSkipComments) {
}
}

// ConfigureFromOptions Tests
TEST(ConfigureFromOptionsTest, Basic) {
LinterConfiguration config;

LinterOptions options = {.ruleset = RuleSet::kAll,
.rules = RuleBundle(),
.config_file = "-",
.config_file_is_custom = false,
.rules_config_search = false,
.linting_start_file = "filename",
.waiver_files = "filename"};

auto status = config.ConfigureFromOptions(options);
EXPECT_TRUE(status.ok());
}

TEST(ConfigureFromOptionsTest, RulesNumber) {
LinterConfiguration config;

LinterOptions options = {.ruleset = RuleSet::kAll,
.rules = RuleBundle(),
.config_file = "-",
.config_file_is_custom = false,
.rules_config_search = false,
.linting_start_file = "filename",
.waiver_files = "filename"};

auto status = config.ConfigureFromOptions(options);
EXPECT_TRUE(status.ok());

// Should enable all rules because uses kAll ruleset and an empty rulebundle
auto expected_size = analysis::RegisteredSyntaxTreeRulesNames().size() +
analysis::RegisteredTokenStreamRulesNames().size() +
analysis::RegisteredTextStructureRulesNames().size() +
analysis::RegisteredLineRulesNames().size();
EXPECT_THAT(config.ActiveRuleIds(), SizeIs(expected_size));
}

TEST(ConfigureFromOptionsTest, RulesSelective) {
LinterConfiguration config;

RuleBundle bundle = {
{{analysis::RegisteredSyntaxTreeRulesNames()[0], {false, ""}}}};

LinterOptions options = {.ruleset = RuleSet::kAll,
.rules = bundle,
.config_file = "-",
.config_file_is_custom = false,
.rules_config_search = false,
.linting_start_file = "filename",
.waiver_files = "filename"};

auto status = config.ConfigureFromOptions(options);
EXPECT_TRUE(status.ok());

// Should enable all rules - 1 because uses kAll ruleset and a rulebundle
// with one rule disabled
auto expected_size = analysis::RegisteredSyntaxTreeRulesNames().size() +
analysis::RegisteredTokenStreamRulesNames().size() +
analysis::RegisteredTextStructureRulesNames().size() +
analysis::RegisteredLineRulesNames().size() - 1;

EXPECT_THAT(config.ActiveRuleIds(), SizeIs(expected_size));
}
// TODO: LinterOptions could be refactored to store the content
// of the configuration files. After this is made it will be possible to
// test the configuration that is applied after reading the files.

} // namespace
} // namespace verilog

0 comments on commit 85f8ff8

Please sign in to comment.