From f9359f826e88a2d564ecfb10d28d1e76f32baf7a Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Sun, 23 Jun 2024 17:31:42 +1000 Subject: [PATCH 1/2] Adding regex configuration to the parameter-name-style rule --- verilog/analysis/checkers/BUILD | 2 +- .../checkers/parameter_name_style_rule.cc | 119 ++++++++++-------- .../checkers/parameter_name_style_rule.h | 30 ++--- .../parameter_name_style_rule_test.cc | 38 ++++-- verilog/tools/lint/BUILD | 4 +- 5 files changed, 107 insertions(+), 86 deletions(-) diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index b90f2fe1e..2c6563a87 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -1668,7 +1668,6 @@ cc_library( "//common/analysis:syntax-tree-lint-rule", "//common/analysis/matcher", "//common/analysis/matcher:bound-symbol-manager", - "//common/strings:naming-utils", "//common/text:config-utils", "//common/text:symbol", "//common/text:syntax-tree-context", @@ -1681,6 +1680,7 @@ cc_library( "@com_google_absl//absl/status", "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:string_view", + "@com_googlesource_code_re2//:re2", ], alwayslink = 1, ) diff --git a/verilog/analysis/checkers/parameter_name_style_rule.cc b/verilog/analysis/checkers/parameter_name_style_rule.cc index 1c6cbedc5..cf65ef23f 100644 --- a/verilog/analysis/checkers/parameter_name_style_rule.cc +++ b/verilog/analysis/checkers/parameter_name_style_rule.cc @@ -14,11 +14,9 @@ #include "verilog/analysis/checkers/parameter_name_style_rule.h" -#include +#include #include #include -#include -#include #include "absl/status/status.h" #include "absl/strings/str_cat.h" @@ -26,11 +24,11 @@ #include "common/analysis/lint_rule_status.h" #include "common/analysis/matcher/bound_symbol_manager.h" #include "common/analysis/matcher/matcher.h" -#include "common/strings/naming_utils.h" #include "common/text/config_utils.h" #include "common/text/symbol.h" #include "common/text/syntax_tree_context.h" #include "common/text/token_info.h" +#include "re2/re2.h" #include "verilog/CST/parameters.h" #include "verilog/CST/verilog_matchers.h" #include "verilog/analysis/descriptions.h" @@ -40,27 +38,47 @@ namespace verilog { namespace analysis { +VERILOG_REGISTER_LINT_RULE(ParameterNameStyleRule); + using verible::LintRuleStatus; using verible::LintViolation; using verible::SyntaxTreeContext; using Matcher = verible::matcher::Matcher; -// Register ParameterNameStyleRule. -VERILOG_REGISTER_LINT_RULE(ParameterNameStyleRule); +// PascalCase, may end in _[0-9]+ +static constexpr absl::string_view localparam_default_regex = + "([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?"; + +// PascalCase (may end in _[0-9]+) or UPPER_SNAKE_CASE +static constexpr absl::string_view parameter_default_regex = + "(([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?)|([A-Z_0-9]+)"; + +ParameterNameStyleRule::ParameterNameStyleRule() + : localparam_style_regex_(std::make_unique( + localparam_default_regex, re2::RE2::Quiet)), + parameter_style_regex_(std::make_unique(parameter_default_regex, + re2::RE2::Quiet)) {} const LintRuleDescriptor &ParameterNameStyleRule::GetDescriptor() { static const LintRuleDescriptor d{ .name = "parameter-name-style", .topic = "constants", .desc = - "Checks that non-type parameter and localparam names follow at least " - "one of the naming conventions from a choice of " - "CamelCase and ALL_CAPS, ORed together with the pipe-symbol(|). " - "Empty configuration: no style enforcement.", - .param = {{"localparam_style", "CamelCase", "Style of localparam name"}, - {"parameter_style", "CamelCase|ALL_CAPS", - "Style of parameter names"}}, + "Checks that parameter and localparm names conform to a naming " + "convention defined by RE2 regular expressions. The default regex " + "pattern for boht localparam and parameter names is PascalCase with " + "an optional _digit suffix. Parameters may also be UPPER_SNAKE_CASE. " + "Refer " + "to " + "https://github.com/chipsalliance/verible/tree/master/verilog/tools/" + "lint#readme for more detail on verible regex patterns.", + .param = {{"localparam_style_regex", + std::string(localparam_default_regex), + "A regex used to check localparam name style."}, + {"parameter_style_regex", std::string(parameter_default_regex), + "A regex used to check parameter name style."}}, }; + return d; } @@ -69,21 +87,16 @@ static const Matcher &ParamDeclMatcher() { return matcher; } -std::string ParameterNameStyleRule::ViolationMsg(absl::string_view symbol_type, - uint32_t allowed_bitmap) { - // TODO(hzeller): there are multiple places in this file referring to the - // same string representations of these options. - static constexpr std::pair kBitNames[] = { - {kUpperCamelCase, "CamelCase"}, {kAllCaps, "ALL_CAPS"}}; - std::string bit_list; - for (const auto &b : kBitNames) { - if (allowed_bitmap & b.first) { - if (!bit_list.empty()) bit_list.append(" or "); - bit_list.append(b.second); - } - } - return absl::StrCat("Non-type ", symbol_type, " names must be styled with ", - bit_list); +std::string ParameterNameStyleRule::CreateLocalparamViolationMessage() { + return absl::StrCat( + "Localparam name does not match the naming convention ", + "defined by regex pattern: ", localparam_style_regex_->pattern()); +} + +std::string ParameterNameStyleRule::CreateParameterViolationMessage() { + return absl::StrCat( + "Parameter name does not match the naming convention ", + "defined by regex pattern: ", parameter_style_regex_->pattern()); } void ParameterNameStyleRule::HandleSymbol(const verible::Symbol &symbol, @@ -92,28 +105,29 @@ void ParameterNameStyleRule::HandleSymbol(const verible::Symbol &symbol, if (ParamDeclMatcher().Matches(symbol, &manager)) { if (IsParamTypeDeclaration(symbol)) return; - const auto param_decl_token = GetParamKeyword(symbol); + const verilog_tokentype param_decl_token = GetParamKeyword(symbol); auto identifiers = GetAllParameterNameTokens(symbol); for (const auto *id : identifiers) { - const auto param_name = id->text(); - uint32_t observed_style = 0; - if (verible::IsUpperCamelCaseWithDigits(param_name)) { - observed_style |= kUpperCamelCase; - } - if (verible::IsNameAllCapsUnderscoresDigits(param_name)) { - observed_style |= kAllCaps; - } - if (param_decl_token == TK_localparam && localparam_allowed_style_ && - (observed_style & localparam_allowed_style_) == 0) { - violations_.insert(LintViolation( - *id, ViolationMsg("localparam", localparam_allowed_style_), - context)); - } else if (param_decl_token == TK_parameter && parameter_allowed_style_ && - (observed_style & parameter_allowed_style_) == 0) { - violations_.insert(LintViolation( - *id, ViolationMsg("parameter", parameter_allowed_style_), context)); + const auto name = id->text(); + switch (param_decl_token) { + case TK_localparam: + if (!RE2::FullMatch(name, *localparam_style_regex_)) { + violations_.insert(LintViolation( + *id, CreateLocalparamViolationMessage(), context)); + } + break; + + case TK_parameter: + if (!RE2::FullMatch(name, *parameter_style_regex_)) { + violations_.insert( + LintViolation(*id, CreateParameterViolationMessage(), context)); + } + break; + + default: + break; } } } @@ -121,14 +135,13 @@ void ParameterNameStyleRule::HandleSymbol(const verible::Symbol &symbol, absl::Status ParameterNameStyleRule::Configure( absl::string_view configuration) { - // TODO(issue #133) include bitmap choices in generated documentation. - static const std::vector choices = { - "CamelCase", "ALL_CAPS"}; // same sequence as enum StyleChoicesBits - using verible::config::SetNamedBits; - return verible::ParseNameValues( + using verible::config::SetRegex; + absl::Status s = verible::ParseNameValues( configuration, - {{"localparam_style", SetNamedBits(&localparam_allowed_style_, choices)}, - {"parameter_style", SetNamedBits(¶meter_allowed_style_, choices)}}); + {{"localparam_style_regex", SetRegex(&localparam_style_regex_)}, + {"parameter_style_regex", SetRegex(¶meter_style_regex_)}}); + + return s; } LintRuleStatus ParameterNameStyleRule::Report() const { diff --git a/verilog/analysis/checkers/parameter_name_style_rule.h b/verilog/analysis/checkers/parameter_name_style_rule.h index 548dc2be3..f0d4340b0 100644 --- a/verilog/analysis/checkers/parameter_name_style_rule.h +++ b/verilog/analysis/checkers/parameter_name_style_rule.h @@ -15,7 +15,7 @@ #ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_PARAMETER_NAME_STYLE_RULE_H_ #define VERIBLE_VERILOG_ANALYSIS_CHECKERS_PARAMETER_NAME_STYLE_RULE_H_ -#include +#include #include #include @@ -25,42 +25,38 @@ #include "common/analysis/syntax_tree_lint_rule.h" #include "common/text/symbol.h" #include "common/text/syntax_tree_context.h" +#include "re2/re2.h" #include "verilog/analysis/descriptions.h" namespace verilog { namespace analysis { // ParameterNameStyleRule checks that each non-type parameter/localparam -// follows the correct naming convention. -// parameter should follow UpperCamelCase (preferred) or ALL_CAPS. -// localparam should follow UpperCamelCase. +// follows the correct naming convention matching a regex pattern. class ParameterNameStyleRule : public verible::SyntaxTreeLintRule { public: using rule_type = verible::SyntaxTreeLintRule; + ParameterNameStyleRule(); + static const LintRuleDescriptor &GetDescriptor(); - absl::Status Configure(absl::string_view configuration) final; + std::string CreateLocalparamViolationMessage(); + std::string CreateParameterViolationMessage(); void HandleSymbol(const verible::Symbol &symbol, const verible::SyntaxTreeContext &context) final; verible::LintRuleStatus Report() const final; - private: - // Format diagnostic message. - static std::string ViolationMsg(absl::string_view symbol_type, - uint32_t allowed_bitmap); - - enum StyleChoicesBits { - kUpperCamelCase = (1 << 0), - kAllCaps = (1 << 1), - }; - - uint32_t localparam_allowed_style_ = kUpperCamelCase; - uint32_t parameter_allowed_style_ = kUpperCamelCase | kAllCaps; + absl::Status Configure(absl::string_view configuration) final; + private: std::set violations_; + + // A regex to check the style against + std::unique_ptr localparam_style_regex_; + std::unique_ptr parameter_style_regex_; }; } // namespace analysis diff --git a/verilog/analysis/checkers/parameter_name_style_rule_test.cc b/verilog/analysis/checkers/parameter_name_style_rule_test.cc index 1160ed231..7774ed608 100644 --- a/verilog/analysis/checkers/parameter_name_style_rule_test.cc +++ b/verilog/analysis/checkers/parameter_name_style_rule_test.cc @@ -119,7 +119,8 @@ TEST(ParameterNameStyleRuleTest, ConfigurationStyleParameterAssignment) { // Make sure configuration parameter name matches corresponding style - { // parameter:CamelCase, localparam:ALL_CAPS + { // parameter:([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)? (PascalCase), + // localparam:[A-Z_0-9]+ (UPPER_SNAKE_CASE) const std::initializer_list kTestCases = { {"package a; parameter int FooBar = 1; endpackage"}, {"package a; parameter int ", {kToken, "FOO_BAR"}, " = 1; endpackage"}, @@ -127,9 +128,12 @@ TEST(ParameterNameStyleRuleTest, ConfigurationStyleParameterAssignment) { {"module a; localparam int FOO_BAR = 1; endmodule"}, }; RunConfiguredLintTestCases( - kTestCases, "parameter_style:CamelCase;localparam_style:ALL_CAPS"); + kTestCases, + "parameter_style_regex:([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?;localparam_" + "style_regex:[A-Z_0-9]+"); } - { // parameter:ALL_CAPS, localparam:CamelCase + { // parameter:[A-Z_0-9]+ (UPPER_SNAKE_CASE), + // localparam:([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)? (PascalCase) const std::initializer_list kTestCases = { {"package a; parameter int ", {kToken, "FooBar"}, " = 1; endpackage"}, {"package a; parameter int FOO_BAR = 1; endpackage"}, @@ -137,13 +141,16 @@ TEST(ParameterNameStyleRuleTest, ConfigurationStyleParameterAssignment) { {"module a; localparam int ", {kToken, "FOO_BAR"}, " = 1; endmodule"}, }; RunConfiguredLintTestCases( - kTestCases, "parameter_style:ALL_CAPS;localparam_style:CamelCase"); + kTestCases, + "parameter_style_regex:[A-Z_0-9]+;localparam_style_regex:([A-Z0-9]+[a-" + "z0-9]*)+(_[0-9]+)?"); } } TEST(ParameterNameStyleRuleTest, ConfigurationFlavorCombinations) { constexpr int kToken = SymbolIdentifier; - { // CamelCase|ALL_CAPS configured + { // (([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?)|([A-Z_0-9]+) + // (PascalCase|UPPER_SNAKE_CASE) configured const std::initializer_list kTestCases = { // Single-letter uppercase matches both styles: {"package a; parameter int N = 1; endpackage"}, @@ -161,10 +168,11 @@ TEST(ParameterNameStyleRuleTest, ConfigurationFlavorCombinations) { }; RunConfiguredLintTestCases( kTestCases, - "parameter_style:CamelCase|ALL_CAPS;" - "localparam_style:CamelCase|ALL_CAPS"); + "parameter_style_regex:(([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?)|([A-Z_0-9]+);" + "localparam_style_regex:(([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?)|([A-Z_0-9]+" + ")"); } - { // CamelCase configured + { // ([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)? configured (PascalCase) const std::initializer_list kTestCases = { {"package a; parameter int N = 1; endpackage"}, {"package a; parameter int ", {kToken, "no_style"}, " = 1; endpackage"}, @@ -178,9 +186,11 @@ TEST(ParameterNameStyleRuleTest, ConfigurationFlavorCombinations) { {"module a; localparam int ", {kToken, "FOO_BAR"}, " = 1; endmodule"}, }; RunConfiguredLintTestCases( - kTestCases, "parameter_style:CamelCase;localparam_style:CamelCase"); + kTestCases, + "parameter_style_regex:([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?;localparam_" + "style_regex:([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?"); } - { // ALL_CAPS configured + { // ([A-Z_0-9]+) configured (UPPER_SNAKE_CASE) const std::initializer_list kTestCases = { {"package a; parameter int N = 1; endpackage"}, {"package a; parameter int ", {kToken, "no_style"}, " = 1; endpackage"}, @@ -194,9 +204,11 @@ TEST(ParameterNameStyleRuleTest, ConfigurationFlavorCombinations) { {"module a; localparam int FOO_BAR = 1; endmodule"}, }; RunConfiguredLintTestCases( - kTestCases, "parameter_style:ALL_CAPS;localparam_style:ALL_CAPS"); + kTestCases, + "parameter_style_regex:([A-Z_0-9]+);localparam_style_regex:([A-Z_0-9]+" + ")"); } - { // No styles configured + { // No styles enforcement (.*) const std::initializer_list kTestCases = { {"package a; parameter int N = 1; endpackage"}, {"package a; parameter int no_style = 1; endpackage"}, @@ -210,7 +222,7 @@ TEST(ParameterNameStyleRuleTest, ConfigurationFlavorCombinations) { {"module a; localparam int FOO_BAR = 1; endmodule"}, }; RunConfiguredLintTestCases( - kTestCases, "parameter_style:;localparam_style:"); + kTestCases, "parameter_style_regex:.*;localparam_style_regex:.*"); } } diff --git a/verilog/tools/lint/BUILD b/verilog/tools/lint/BUILD index 75e2ca46a..03b38df59 100644 --- a/verilog/tools/lint/BUILD +++ b/verilog/tools/lint/BUILD @@ -84,8 +84,8 @@ _linter_test_configs = [ ("package-filename", "package_filename_pkg", True), ("packed-dimensions-range-ordering", "packed_dimensions", True), ("parameter-name-style", "localparam_name_style", True), - ("parameter-name-style=localparam_style:ALL_CAPS", "localparam_name_style_all_caps", False), - ("parameter-name-style=localparam_style:CamelCase", "localparam_name_style_camel_case", True), + ("parameter-name-style=localparam_style_regex:[A-Z_0-9]+", "localparam_name_style_all_caps", False), + ("parameter-name-style=localparam_style_regex:\\([A-Z0-9]+[a-z0-9]*\\)+\\(_[0-9]+\\)?", "localparam_name_style_camel_case", True), ("parameter-name-style", "parameter_name_style", True), ("parameter-type-name-style", "parameter_type_name_style", False), ("parameter-type-name-style", "localparam_type_name_style", False), From 23d76bee19332a856a2488a0d5d273768cd0eb6b Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Thu, 8 Aug 2024 23:43:12 +1000 Subject: [PATCH 2/2] Adding back the original configuration to avoid breaking users setups. The rule now forms a single regex using the original configuration values and a user regex. --- verilog/analysis/checkers/BUILD | 3 + .../checkers/parameter_name_style_rule.cc | 130 ++++++++-- .../checkers/parameter_name_style_rule.h | 21 +- .../parameter_name_style_rule_test.cc | 224 ++++++++++++++++-- verilog/tools/lint/BUILD | 4 +- 5 files changed, 335 insertions(+), 47 deletions(-) diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index 2c6563a87..cd7d4e7de 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -1694,6 +1694,9 @@ cc_test( "//common/analysis:syntax-tree-linter-test-utils", "//verilog/analysis:verilog-analyzer", "//verilog/parser:verilog-token-enum", + "@com_google_absl//absl/status", + "@com_google_absl//absl/strings", + "@com_google_absl//absl/strings:string_view", "@com_google_googletest//:gtest", "@com_google_googletest//:gtest_main", ], diff --git a/verilog/analysis/checkers/parameter_name_style_rule.cc b/verilog/analysis/checkers/parameter_name_style_rule.cc index cf65ef23f..f9f24481e 100644 --- a/verilog/analysis/checkers/parameter_name_style_rule.cc +++ b/verilog/analysis/checkers/parameter_name_style_rule.cc @@ -14,9 +14,11 @@ #include "verilog/analysis/checkers/parameter_name_style_rule.h" +#include #include #include #include +#include #include "absl/status/status.h" #include "absl/strings/str_cat.h" @@ -45,19 +47,21 @@ using verible::LintViolation; using verible::SyntaxTreeContext; using Matcher = verible::matcher::Matcher; -// PascalCase, may end in _[0-9]+ -static constexpr absl::string_view localparam_default_regex = +// Upper Camel Case (may end in _[0-9]+) +static constexpr absl::string_view kUpperCamelCaseRegex = "([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?"; +// ALL_CAPS +static constexpr absl::string_view kAllCapsRegex = "[A-Z_0-9]+"; -// PascalCase (may end in _[0-9]+) or UPPER_SNAKE_CASE -static constexpr absl::string_view parameter_default_regex = - "(([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?)|([A-Z_0-9]+)"; +static constexpr absl::string_view kLocalparamDefaultRegex = ""; +static constexpr absl::string_view kParameterDefaultRegex = ""; ParameterNameStyleRule::ParameterNameStyleRule() : localparam_style_regex_(std::make_unique( - localparam_default_regex, re2::RE2::Quiet)), - parameter_style_regex_(std::make_unique(parameter_default_regex, - re2::RE2::Quiet)) {} + absl::StrCat("(", kUpperCamelCaseRegex, ")"), re2::RE2::Quiet)), + parameter_style_regex_(std::make_unique( + absl::StrCat("(", kUpperCamelCaseRegex, ")|(", kAllCapsRegex, ")"), + re2::RE2::Quiet)) {} const LintRuleDescriptor &ParameterNameStyleRule::GetDescriptor() { static const LintRuleDescriptor d{ @@ -65,20 +69,19 @@ const LintRuleDescriptor &ParameterNameStyleRule::GetDescriptor() { .topic = "constants", .desc = "Checks that parameter and localparm names conform to a naming " - "convention defined by RE2 regular expressions. The default regex " - "pattern for boht localparam and parameter names is PascalCase with " - "an optional _digit suffix. Parameters may also be UPPER_SNAKE_CASE. " - "Refer " - "to " + "convention based on a choice of 'CamelCase', 'ALL_CAPS' and a user " + "defined regex ORed together. Empty configurtaion: no style " + "enforcement. Refer to " "https://github.com/chipsalliance/verible/tree/master/verilog/tools/" "lint#readme for more detail on verible regex patterns.", - .param = {{"localparam_style_regex", - std::string(localparam_default_regex), + .param = {{"localparam_style", "CamelCase", "Style of localparam names"}, + {"parameter_style", "CamelCase|ALL_CAPS", + "Style of parameter names."}, + {"localparam_style_regex", std::string(kLocalparamDefaultRegex), "A regex used to check localparam name style."}, - {"parameter_style_regex", std::string(parameter_default_regex), + {"parameter_style_regex", std::string(kParameterDefaultRegex), "A regex used to check parameter name style."}}, }; - return d; } @@ -133,14 +136,103 @@ void ParameterNameStyleRule::HandleSymbol(const verible::Symbol &symbol, } } +absl::Status ParameterNameStyleRule::AppendRegex( + std::unique_ptr *rule_regex, absl::string_view regex_str) { + if (*rule_regex == nullptr) { + *rule_regex = std::make_unique(absl::StrCat("(", regex_str, ")"), + re2::RE2::Quiet); + } else { + *rule_regex = std::make_unique( + absl::StrCat((*rule_regex)->pattern(), "|(", regex_str, ")"), + re2::RE2::Quiet); + } + + if ((*rule_regex)->ok()) { + return absl::OkStatus(); + } + + std::string error_msg = absl::StrCat("Failed to parse regular expression: ", + (*rule_regex)->error()); + return absl::InvalidArgumentError(error_msg); +} + +absl::Status ParameterNameStyleRule::ConfigureRegex( + std::unique_ptr *rule_regex, uint32_t config_style, + std::unique_ptr *config_style_regex) { + absl::Status s; + + int config_style_regex_length = (*config_style_regex)->pattern().length(); + + // If no rule is set, no style enforcement + if (config_style == 0 && config_style_regex_length == 0) { + // Set the regex pattern to match everything + *rule_regex = std::make_unique(".*", re2::RE2::Quiet); + return absl::OkStatus(); + } + + // Clear rule_regex + *rule_regex = nullptr; + + // Append UpperCamelCase regex to rule_regex (if enabled) + if (config_style & kUpperCamelCase) { + s = AppendRegex(rule_regex, kUpperCamelCaseRegex); + if (!s.ok()) { + return s; + } + } + + // Append ALL_CAPS regex to rule_regex (if enabled) + if (config_style & kAllCaps) { + s = AppendRegex(rule_regex, kAllCapsRegex); + if (!s.ok()) { + return s; + } + } + + // Append regex from config_style_regex (if provided by the user) + if (config_style_regex_length > 0) { + s = AppendRegex(rule_regex, (*config_style_regex)->pattern()); + } + + return s; +} + absl::Status ParameterNameStyleRule::Configure( absl::string_view configuration) { + // same sequence as enum StyleChoicesBits + static const std::vector choices = {"CamelCase", + "ALL_CAPS"}; + + uint32_t localparam_style = kUpperCamelCase; + uint32_t parameter_style = kUpperCamelCase | kAllCaps; + std::unique_ptr localparam_style_regex = + std::make_unique(kLocalparamDefaultRegex, re2::RE2::Quiet); + std::unique_ptr parameter_style_regex = + std::make_unique(kParameterDefaultRegex, re2::RE2::Quiet); + + using verible::config::SetNamedBits; using verible::config::SetRegex; + absl::Status s = verible::ParseNameValues( configuration, - {{"localparam_style_regex", SetRegex(&localparam_style_regex_)}, - {"parameter_style_regex", SetRegex(¶meter_style_regex_)}}); + {{"localparam_style", SetNamedBits(&localparam_style, choices)}, + {"parameter_style", SetNamedBits(¶meter_style, choices)}, + {"localparam_style_regex", SetRegex(&localparam_style_regex)}, + {"parameter_style_regex", SetRegex(¶meter_style_regex)}}); + + if (!s.ok()) { + return s; + } + + // Form a regex to use based on *_style, and *_style_regex + s = ConfigureRegex(&localparam_style_regex_, localparam_style, + &localparam_style_regex); + if (!s.ok()) { + return s; + } + s = ConfigureRegex(¶meter_style_regex_, parameter_style, + ¶meter_style_regex); return s; } diff --git a/verilog/analysis/checkers/parameter_name_style_rule.h b/verilog/analysis/checkers/parameter_name_style_rule.h index f0d4340b0..4fe4be90c 100644 --- a/verilog/analysis/checkers/parameter_name_style_rule.h +++ b/verilog/analysis/checkers/parameter_name_style_rule.h @@ -15,6 +15,7 @@ #ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_PARAMETER_NAME_STYLE_RULE_H_ #define VERIBLE_VERILOG_ANALYSIS_CHECKERS_PARAMETER_NAME_STYLE_RULE_H_ +#include #include #include #include @@ -51,10 +52,28 @@ class ParameterNameStyleRule : public verible::SyntaxTreeLintRule { absl::Status Configure(absl::string_view configuration) final; + const RE2 *localparam_style_regex() const { + return localparam_style_regex_.get(); + } + const RE2 *parameter_style_regex() const { + return parameter_style_regex_.get(); + } + private: + absl::Status AppendRegex(std::unique_ptr *rule_regex, + absl::string_view regex_str); + absl::Status ConfigureRegex(std::unique_ptr *rule_regex, + uint32_t config_style, + std::unique_ptr *config_style_regex); + + enum StyleChoicesBits { + kUpperCamelCase = (1 << 0), + kAllCaps = (1 << 1), + }; + std::set violations_; - // A regex to check the style against + // Regex's to check the style against std::unique_ptr localparam_style_regex_; std::unique_ptr parameter_style_regex_; }; diff --git a/verilog/analysis/checkers/parameter_name_style_rule_test.cc b/verilog/analysis/checkers/parameter_name_style_rule_test.cc index 7774ed608..248ae9984 100644 --- a/verilog/analysis/checkers/parameter_name_style_rule_test.cc +++ b/verilog/analysis/checkers/parameter_name_style_rule_test.cc @@ -15,7 +15,11 @@ #include "verilog/analysis/checkers/parameter_name_style_rule.h" #include +#include +#include "absl/status/status.h" +#include "absl/strings/str_cat.h" +#include "absl/strings/string_view.h" #include "common/analysis/linter_test_utils.h" #include "common/analysis/syntax_tree_linter_test_utils.h" #include "gtest/gtest.h" @@ -39,6 +43,7 @@ TEST(ParameterNameStyleRuleTest, AcceptTests) { {"module foo; localparam type Bar_1 = 1; endmodule"}, {"module foo; localparam Bar = 1; endmodule"}, {"module foo; localparam int Bar = 1; endmodule"}, + {"module foo; localparam int P = 1; endmodule"}, {"module foo; parameter int HelloWorld = 1; endmodule"}, {"module foo #(parameter int HelloWorld_1 = 1); endmodule"}, {"module foo #(parameter type Foo); endmodule"}, @@ -53,6 +58,7 @@ TEST(ParameterNameStyleRuleTest, AcceptTests) { {"parameter int Foo = 1;"}, {"parameter type FooBar;"}, {"parameter Foo = 1;"}, + {"parameter P = 1;"}, // Make sure parameter type triggers no violation {"module foo; localparam type Bar_Hello_1 = 1; endmodule"}, @@ -119,8 +125,7 @@ TEST(ParameterNameStyleRuleTest, ConfigurationStyleParameterAssignment) { // Make sure configuration parameter name matches corresponding style - { // parameter:([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)? (PascalCase), - // localparam:[A-Z_0-9]+ (UPPER_SNAKE_CASE) + { // parameter:CamelCase, localparam:ALL_CAPS const std::initializer_list kTestCases = { {"package a; parameter int FooBar = 1; endpackage"}, {"package a; parameter int ", {kToken, "FOO_BAR"}, " = 1; endpackage"}, @@ -128,12 +133,9 @@ TEST(ParameterNameStyleRuleTest, ConfigurationStyleParameterAssignment) { {"module a; localparam int FOO_BAR = 1; endmodule"}, }; RunConfiguredLintTestCases( - kTestCases, - "parameter_style_regex:([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?;localparam_" - "style_regex:[A-Z_0-9]+"); + kTestCases, "parameter_style:CamelCase;localparam_style:ALL_CAPS"); } - { // parameter:[A-Z_0-9]+ (UPPER_SNAKE_CASE), - // localparam:([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)? (PascalCase) + { // parameter:ALL_CAPS, localparam:CamelCase const std::initializer_list kTestCases = { {"package a; parameter int ", {kToken, "FooBar"}, " = 1; endpackage"}, {"package a; parameter int FOO_BAR = 1; endpackage"}, @@ -141,16 +143,13 @@ TEST(ParameterNameStyleRuleTest, ConfigurationStyleParameterAssignment) { {"module a; localparam int ", {kToken, "FOO_BAR"}, " = 1; endmodule"}, }; RunConfiguredLintTestCases( - kTestCases, - "parameter_style_regex:[A-Z_0-9]+;localparam_style_regex:([A-Z0-9]+[a-" - "z0-9]*)+(_[0-9]+)?"); + kTestCases, "parameter_style:ALL_CAPS;localparam_style:CamelCase"); } } TEST(ParameterNameStyleRuleTest, ConfigurationFlavorCombinations) { constexpr int kToken = SymbolIdentifier; - { // (([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?)|([A-Z_0-9]+) - // (PascalCase|UPPER_SNAKE_CASE) configured + { // CamelCase|ALL_CAPS configured const std::initializer_list kTestCases = { // Single-letter uppercase matches both styles: {"package a; parameter int N = 1; endpackage"}, @@ -168,11 +167,10 @@ TEST(ParameterNameStyleRuleTest, ConfigurationFlavorCombinations) { }; RunConfiguredLintTestCases( kTestCases, - "parameter_style_regex:(([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?)|([A-Z_0-9]+);" - "localparam_style_regex:(([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?)|([A-Z_0-9]+" - ")"); + "parameter_style:CamelCase|ALL_CAPS;" + "localparam_style:CamelCase|ALL_CAPS"); } - { // ([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)? configured (PascalCase) + { // CamelCase configured const std::initializer_list kTestCases = { {"package a; parameter int N = 1; endpackage"}, {"package a; parameter int ", {kToken, "no_style"}, " = 1; endpackage"}, @@ -186,11 +184,9 @@ TEST(ParameterNameStyleRuleTest, ConfigurationFlavorCombinations) { {"module a; localparam int ", {kToken, "FOO_BAR"}, " = 1; endmodule"}, }; RunConfiguredLintTestCases( - kTestCases, - "parameter_style_regex:([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?;localparam_" - "style_regex:([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?"); + kTestCases, "parameter_style:CamelCase;localparam_style:CamelCase"); } - { // ([A-Z_0-9]+) configured (UPPER_SNAKE_CASE) + { // ALL_CAPS configured const std::initializer_list kTestCases = { {"package a; parameter int N = 1; endpackage"}, {"package a; parameter int ", {kToken, "no_style"}, " = 1; endpackage"}, @@ -204,11 +200,9 @@ TEST(ParameterNameStyleRuleTest, ConfigurationFlavorCombinations) { {"module a; localparam int FOO_BAR = 1; endmodule"}, }; RunConfiguredLintTestCases( - kTestCases, - "parameter_style_regex:([A-Z_0-9]+);localparam_style_regex:([A-Z_0-9]+" - ")"); + kTestCases, "parameter_style:ALL_CAPS;localparam_style:ALL_CAPS"); } - { // No styles enforcement (.*) + { // No styles configured const std::initializer_list kTestCases = { {"package a; parameter int N = 1; endpackage"}, {"package a; parameter int no_style = 1; endpackage"}, @@ -222,10 +216,190 @@ TEST(ParameterNameStyleRuleTest, ConfigurationFlavorCombinations) { {"module a; localparam int FOO_BAR = 1; endmodule"}, }; RunConfiguredLintTestCases( - kTestCases, "parameter_style_regex:.*;localparam_style_regex:.*"); + kTestCases, "parameter_style:;localparam_style:"); } } +TEST(ParameterNameStyleRuleTest, ConfigurationPass) { + ParameterNameStyleRule rule; + absl::Status status; + + // Upper Camel Case (may end in _[0-9]+) + constexpr absl::string_view kUpperCamelCaseRegex = + "(([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?)"; + // ALL_CAPS + constexpr absl::string_view kAllCapsRegex = "([A-Z_0-9]+)"; + + const std::string default_localparam_regex = + std::string(kUpperCamelCaseRegex); + const std::string default_parameter_regex = + absl::StrCat(kUpperCamelCaseRegex, "|", kAllCapsRegex); + + // Default Config + EXPECT_TRUE((status = rule.Configure("")).ok()) << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), default_localparam_regex); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), default_parameter_regex); + + // Set to ALL_CAPS using *_style + EXPECT_TRUE((status = rule.Configure("localparam_style:ALL_CAPS")).ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), kAllCapsRegex); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), default_parameter_regex); + + EXPECT_TRUE((status = rule.Configure("parameter_style:ALL_CAPS")).ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), default_localparam_regex); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), kAllCapsRegex); + + // Set to CamelCase using *_style + EXPECT_TRUE((status = rule.Configure("localparam_style:CamelCase")).ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), kUpperCamelCaseRegex); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), default_parameter_regex); + + EXPECT_TRUE((status = rule.Configure("parameter_style:CamelCase")).ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), default_localparam_regex); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), kUpperCamelCaseRegex); + + // Set to ALL_CAPS | CamelCase using *_style + EXPECT_TRUE( + (status = rule.Configure("localparam_style:ALL_CAPS|CamelCase")).ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), + absl::StrCat(kUpperCamelCaseRegex, "|", kAllCapsRegex)); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), default_parameter_regex); + + EXPECT_TRUE( + (status = rule.Configure("parameter_style:ALL_CAPS|CamelCase")).ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), default_localparam_regex); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), + absl::StrCat(kUpperCamelCaseRegex, "|", kAllCapsRegex)); + + // Set *_style_regex without setting *_style, expect default configuration and + // regex + EXPECT_TRUE((status = rule.Configure("localparam_style_regex:[a-z_]+")).ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), + absl::StrCat(default_localparam_regex, "|([a-z_]+)")); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), default_parameter_regex); + + EXPECT_TRUE((status = rule.Configure("parameter_style_regex:[a-z_]+")).ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), default_localparam_regex); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), + absl::StrCat(default_parameter_regex, "|([a-z_]+)")); + + // Set using just a regex pattern + EXPECT_TRUE((status = rule.Configure( + "localparam_style:;localparam_style_regex:[a-z_]+")) + .ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), "([a-z_]+)"); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), default_parameter_regex); + + EXPECT_TRUE((status = rule.Configure( + "parameter_style:;parameter_style_regex:[a-z_]+")) + .ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), default_localparam_regex); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), "([a-z_]+)"); + + // Set ALL_CAPS and user regex + EXPECT_TRUE((status = rule.Configure( + "localparam_style:ALL_CAPS;localparam_style_regex:[a-z_]+")) + .ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), + absl::StrCat(kAllCapsRegex, "|([a-z_]+)")); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), default_parameter_regex); + + EXPECT_TRUE((status = rule.Configure( + "parameter_style:ALL_CAPS;parameter_style_regex:[a-z_]+")) + .ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), default_localparam_regex); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), + absl::StrCat(kAllCapsRegex, "|([a-z_]+)")); + + // Set CamelCase and user regex + EXPECT_TRUE((status = rule.Configure( + "localparam_style:CamelCase;localparam_style_regex:[a-z_]+")) + .ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), + absl::StrCat(kUpperCamelCaseRegex, "|([a-z_]+)")); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), default_parameter_regex); + + EXPECT_TRUE((status = rule.Configure( + "parameter_style:CamelCase;parameter_style_regex:[a-z_]+")) + .ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), default_localparam_regex); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), + absl::StrCat(kUpperCamelCaseRegex, "|([a-z_]+)")); + + // Set CamelCase|All_CAPS and set user regex (everything) + EXPECT_TRUE((status = rule.Configure("localparam_style:CamelCase|ALL_CAPS;" + "localparam_style_regex:[a-z_]+")) + .ok()) + << status.message(); + EXPECT_EQ( + rule.localparam_style_regex()->pattern(), + absl::StrCat(kUpperCamelCaseRegex, "|", kAllCapsRegex, "|([a-z_]+)")); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), default_parameter_regex); + + EXPECT_TRUE( + (status = rule.Configure( + "parameter_style:CamelCase|ALL_CAPS;parameter_style_regex:[a-z_]+")) + .ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), default_localparam_regex); + EXPECT_EQ( + rule.parameter_style_regex()->pattern(), + absl::StrCat(kUpperCamelCaseRegex, "|", kAllCapsRegex, "|([a-z_]+)")); + + // Test *_style with regex and not setting *_style_regex. The rule should not + // violate any style + EXPECT_TRUE((status = rule.Configure("localparam_style:")).ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), ".*"); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), default_parameter_regex); + + EXPECT_TRUE((status = rule.Configure("parameter_style:")).ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), default_localparam_regex); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), ".*"); + + // test empty *style (empty string) and setting *_style_regex with empty + // string + EXPECT_TRUE( + (status = rule.Configure("localparam_style:;localparam_style_regex:")) + .ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), ".*"); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), default_parameter_regex); + + EXPECT_TRUE( + (status = rule.Configure("parameter_style:;parameter_style_regex:")).ok()) + << status.message(); + EXPECT_EQ(rule.localparam_style_regex()->pattern(), default_localparam_regex); + EXPECT_EQ(rule.parameter_style_regex()->pattern(), ".*"); + + // FIXME: test invalid regex + EXPECT_FALSE((status = rule.Configure("localparam_style_regex:[a-z")).ok()) + << status.message(); + EXPECT_FALSE((status = rule.Configure("parameter_style_regex:[a-z")).ok()) + << status.message(); + + // FIXME: test invalid style + EXPECT_FALSE((status = rule.Configure("localparam_style:invalid_style")).ok()) + << status.message(); + EXPECT_FALSE((status = rule.Configure("parameter_style:invalid_style")).ok()) + << status.message(); +} + } // namespace } // namespace analysis } // namespace verilog diff --git a/verilog/tools/lint/BUILD b/verilog/tools/lint/BUILD index 03b38df59..75e2ca46a 100644 --- a/verilog/tools/lint/BUILD +++ b/verilog/tools/lint/BUILD @@ -84,8 +84,8 @@ _linter_test_configs = [ ("package-filename", "package_filename_pkg", True), ("packed-dimensions-range-ordering", "packed_dimensions", True), ("parameter-name-style", "localparam_name_style", True), - ("parameter-name-style=localparam_style_regex:[A-Z_0-9]+", "localparam_name_style_all_caps", False), - ("parameter-name-style=localparam_style_regex:\\([A-Z0-9]+[a-z0-9]*\\)+\\(_[0-9]+\\)?", "localparam_name_style_camel_case", True), + ("parameter-name-style=localparam_style:ALL_CAPS", "localparam_name_style_all_caps", False), + ("parameter-name-style=localparam_style:CamelCase", "localparam_name_style_camel_case", True), ("parameter-name-style", "parameter_name_style", True), ("parameter-type-name-style", "parameter_type_name_style", False), ("parameter-type-name-style", "localparam_type_name_style", False),