diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index f723853cd..fdbacba89 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -1673,7 +1673,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", @@ -1686,6 +1685,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, ) @@ -1699,6 +1699,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 1c6cbedc5..f9f24481e 100644 --- a/verilog/analysis/checkers/parameter_name_style_rule.cc +++ b/verilog/analysis/checkers/parameter_name_style_rule.cc @@ -15,9 +15,9 @@ #include "verilog/analysis/checkers/parameter_name_style_rule.h" #include +#include #include #include -#include #include #include "absl/status/status.h" @@ -26,11 +26,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,26 +40,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); +// 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]+"; + +static constexpr absl::string_view kLocalparamDefaultRegex = ""; +static constexpr absl::string_view kParameterDefaultRegex = ""; + +ParameterNameStyleRule::ParameterNameStyleRule() + : localparam_style_regex_(std::make_unique( + 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{ .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"}, + "Checks that parameter and localparm names conform to a naming " + "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", "CamelCase", "Style of localparam names"}, {"parameter_style", "CamelCase|ALL_CAPS", - "Style of parameter names"}}, + "Style of parameter names."}, + {"localparam_style_regex", std::string(kLocalparamDefaultRegex), + "A regex used to check localparam name style."}, + {"parameter_style_regex", std::string(kParameterDefaultRegex), + "A regex used to check parameter name style."}}, }; return d; } @@ -69,21 +90,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,43 +108,132 @@ 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; } } } } +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) { - // TODO(issue #133) include bitmap choices in generated documentation. - static const std::vector choices = { - "CamelCase", "ALL_CAPS"}; // same sequence as enum StyleChoicesBits + // 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; - 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", 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; } 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..4fe4be90c 100644 --- a/verilog/analysis/checkers/parameter_name_style_rule.h +++ b/verilog/analysis/checkers/parameter_name_style_rule.h @@ -16,6 +16,7 @@ #define VERIBLE_VERILOG_ANALYSIS_CHECKERS_PARAMETER_NAME_STYLE_RULE_H_ #include +#include #include #include @@ -25,42 +26,56 @@ #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; + 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: - // Format diagnostic message. - static std::string ViolationMsg(absl::string_view symbol_type, - uint32_t allowed_bitmap); + 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), }; - uint32_t localparam_allowed_style_ = kUpperCamelCase; - uint32_t parameter_allowed_style_ = kUpperCamelCase | kAllCaps; - std::set violations_; + + // Regex's 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..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"}, @@ -214,6 +220,186 @@ TEST(ParameterNameStyleRuleTest, ConfigurationFlavorCombinations) { } } +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