From f9359f826e88a2d564ecfb10d28d1e76f32baf7a Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Sun, 23 Jun 2024 17:31:42 +1000 Subject: [PATCH] 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),