From 33b246f1cf07c1ca2046d5a90f5c6591ac0df87c Mon Sep 17 00:00:00 2001 From: Ignacio Encinas Date: Sat, 16 Sep 2023 15:36:42 +0200 Subject: [PATCH 1/2] linter: configuration: Ignore incorrect rules Change the behaviour after observing an incorrect rule in a configuration file. Previously: error out and go back to the default configuration Now: ignore the incorrect rule and parse the rest of the file --- .../analysis/verilog_linter_configuration.cc | 31 ++++++++++--------- .../analysis/verilog_linter_configuration.h | 9 ++++++ .../verilog_linter_configuration_test.cc | 4 +-- 3 files changed, 27 insertions(+), 17 deletions(-) diff --git a/verilog/analysis/verilog_linter_configuration.cc b/verilog/analysis/verilog_linter_configuration.cc index 04f0b687b..d3498f8ed 100644 --- a/verilog/analysis/verilog_linter_configuration.cc +++ b/verilog/analysis/verilog_linter_configuration.cc @@ -92,6 +92,7 @@ bool RuleBundle::ParseConfiguration(absl::string_view text, char separator, // Clear the vector to overwrite any existing value. rules.clear(); + bool parsed_correctly = true; for (absl::string_view part : absl::StrSplit(text, separator, absl::SkipEmpty())) { if (separator == '\n') { @@ -108,9 +109,8 @@ bool RuleBundle::ParseConfiguration(absl::string_view text, char separator, part = absl::StripAsciiWhitespace(part); while (!part.empty() && part[part.size() - 1] == ',') { // Not fatal, just report - if (!error->empty()) error->append("\n"); - error->append(absl::StrCat( - "Ignoring stray comma at end of configuration `", part, "`")); + absl::StrAppend(error, error->empty() ? "" : "\n", kStrayCommaWarning, + " `", part, "`"); part = part.substr(0, part.size() - 1); } @@ -119,8 +119,8 @@ bool RuleBundle::ParseConfiguration(absl::string_view text, char separator, // '+' to enable rule. // Note that part is guaranteed to be at least one character because // of absl::SkipEmpty() - const bool has_prefix = (part[0] == '+' || part[0] == '-'); const bool prefix_minus = (part[0] == '-'); + const bool has_prefix = (part[0] == '+' || prefix_minus); RuleSetting setting = {!prefix_minus, ""}; @@ -148,15 +148,19 @@ bool RuleBundle::ParseConfiguration(absl::string_view text, char separator, // Check if text is a valid lint rule. if (rule_iter == rule_name_set.end()) { - *error = absl::StrCat("invalid flag \"", rule_name, "\""); - return false; + absl::StrAppend(error, error->empty() ? "" : "\n", kInvalidFlagMessage, + " \"", rule_name, "\""); + // If the rule doesn't exist just ignore it. Take note of this information + // so it can be reported but keep parsing configuration. + parsed_correctly = false; + continue; } // Map keys must use canonical registered string_views for guaranteed // lifetime, not just any string-equivalent copy. rules[*rule_iter] = setting; } - return true; + return parsed_correctly; } // Parse and unparse for RuleBundle (for commandlineflags) @@ -312,15 +316,12 @@ absl::Status LinterConfiguration::AppendFromFile( if (config_or.ok()) { RuleBundle local_rules_bundle; std::string error; - if (local_rules_bundle.ParseConfiguration(*config_or, '\n', &error)) { - if (!error.empty()) { - std::cerr << "Warnings in parse configuration: " << error << std::endl; - } - UseRuleBundle(local_rules_bundle); - } else { - std::cerr << "Unable to fully parse configuration: " << error - << std::endl; + local_rules_bundle.ParseConfiguration(*config_or, '\n', &error); + // Log warnings and errors + if (!error.empty()) { + std::cerr << error; } + UseRuleBundle(local_rules_bundle); return absl::OkStatus(); } diff --git a/verilog/analysis/verilog_linter_configuration.h b/verilog/analysis/verilog_linter_configuration.h index dc4a90a83..471da65e9 100644 --- a/verilog/analysis/verilog_linter_configuration.h +++ b/verilog/analysis/verilog_linter_configuration.h @@ -38,6 +38,15 @@ struct RuleSetting { std::string configuration; }; +// Error to be shown when an invalid flag is +// encountered while parsing a configuration +inline constexpr absl::string_view kInvalidFlagMessage = "[ERR] Invalid flag"; + +// Warning to be shown when an stray comma is +// encountered while parsing a configuration +inline constexpr absl::string_view kStrayCommaWarning = + "[WARN] Ignoring stray comma at the end of configuration"; + // Enum denoting a ruleset // kNone no rules are enabled // kDefault default ruleset is enabled diff --git a/verilog/analysis/verilog_linter_configuration_test.cc b/verilog/analysis/verilog_linter_configuration_test.cc index be2e2c9bf..daaee687e 100644 --- a/verilog/analysis/verilog_linter_configuration_test.cc +++ b/verilog/analysis/verilog_linter_configuration_test.cc @@ -768,7 +768,7 @@ TEST(RuleBundleTest, ParseRuleBundleReject) { std::string error; bool success = bundle.ParseConfiguration(text, ',', &error); EXPECT_FALSE(success); - EXPECT_EQ(error, "invalid flag \"bad-flag\""); + EXPECT_EQ(error, absl::StrCat(kInvalidFlagMessage, " \"bad-flag\"")); } TEST(RuleBundleTest, ParseRuleBundleAcceptMultiline) { @@ -789,7 +789,7 @@ TEST(RuleBundleTest, ParseRuleBundleRejectMultiline) { std::string error; bool success = bundle.ParseConfiguration(text, '\n', &error); EXPECT_FALSE(success); - EXPECT_EQ(error, "invalid flag \"bad-flag\""); + EXPECT_EQ(error, absl::StrCat(kInvalidFlagMessage, " \"bad-flag\"")); } TEST(RuleBundleTest, ParseRuleBundleSkipComments) { From fcd6f998136487382318b6dfabe4a35a95c4fbb9 Mon Sep 17 00:00:00 2001 From: Ignacio Encinas Date: Wed, 27 Dec 2023 15:01:48 +0100 Subject: [PATCH 2/2] fixup! linter: configuration: Ignore incorrect rules --- verilog/analysis/verilog_linter_configuration.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/verilog/analysis/verilog_linter_configuration.cc b/verilog/analysis/verilog_linter_configuration.cc index d3498f8ed..15ecb3000 100644 --- a/verilog/analysis/verilog_linter_configuration.cc +++ b/verilog/analysis/verilog_linter_configuration.cc @@ -319,7 +319,8 @@ absl::Status LinterConfiguration::AppendFromFile( local_rules_bundle.ParseConfiguration(*config_or, '\n', &error); // Log warnings and errors if (!error.empty()) { - std::cerr << error; + std::cerr << "Using a partial version from " << config_filename + << ". Found the following issues: " << error; } UseRuleBundle(local_rules_bundle); return absl::OkStatus();