Skip to content

Commit

Permalink
Adding regex configuration to the parameter-name-style rule
Browse files Browse the repository at this point in the history
  • Loading branch information
@sconwayaus
sconwayaus committed Jul 10, 2024
1 parent 1d393d4 commit f9359f8
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 86 deletions.
2 changes: 1 addition & 1 deletion verilog/analysis/checkers/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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,
)
Expand Down
119 changes: 66 additions & 53 deletions verilog/analysis/checkers/parameter_name_style_rule.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,21 @@

#include "verilog/analysis/checkers/parameter_name_style_rule.h"

#include <cstdint>
#include <memory>
#include <set>
#include <string>
#include <utility>
#include <vector>

#include "absl/status/status.h"
#include "absl/strings/str_cat.h"
#include "absl/strings/string_view.h"
#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"
Expand All @@ -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<re2::RE2>(
localparam_default_regex, re2::RE2::Quiet)),
parameter_style_regex_(std::make_unique<re2::RE2>(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;
}

Expand All @@ -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<uint32_t, const char *> 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,
Expand All @@ -92,43 +105,43 @@ 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::Configure(
absl::string_view configuration) {
// TODO(issue #133) include bitmap choices in generated documentation.
static const std::vector<absl::string_view> 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(&parameter_allowed_style_, choices)}});
{{"localparam_style_regex", SetRegex(&localparam_style_regex_)},
{"parameter_style_regex", SetRegex(&parameter_style_regex_)}});

return s;
}

LintRuleStatus ParameterNameStyleRule::Report() const {
Expand Down
30 changes: 13 additions & 17 deletions verilog/analysis/checkers/parameter_name_style_rule.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <cstdint>
#include <memory>
#include <set>
#include <string>

Expand All @@ -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<verible::LintViolation> violations_;

// A regex to check the style against
std::unique_ptr<re2::RE2> localparam_style_regex_;
std::unique_ptr<re2::RE2> parameter_style_regex_;
};

} // namespace analysis
Expand Down
38 changes: 25 additions & 13 deletions verilog/analysis/checkers/parameter_name_style_rule_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,31 +119,38 @@ 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<LintTestCase> kTestCases = {
{"package a; parameter int FooBar = 1; endpackage"},
{"package a; parameter int ", {kToken, "FOO_BAR"}, " = 1; endpackage"},
{"module a; localparam int ", {kToken, "FooBar"}, " = 1; endmodule"},
{"module a; localparam int FOO_BAR = 1; endmodule"},
};
RunConfiguredLintTestCases<VerilogAnalyzer, ParameterNameStyleRule>(
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<LintTestCase> kTestCases = {
{"package a; parameter int ", {kToken, "FooBar"}, " = 1; endpackage"},
{"package a; parameter int FOO_BAR = 1; endpackage"},
{"module a; localparam int FooBar = 1; endmodule"},
{"module a; localparam int ", {kToken, "FOO_BAR"}, " = 1; endmodule"},
};
RunConfiguredLintTestCases<VerilogAnalyzer, ParameterNameStyleRule>(
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<LintTestCase> kTestCases = {
// Single-letter uppercase matches both styles:
{"package a; parameter int N = 1; endpackage"},
Expand All @@ -161,10 +168,11 @@ TEST(ParameterNameStyleRuleTest, ConfigurationFlavorCombinations) {
};
RunConfiguredLintTestCases<VerilogAnalyzer, ParameterNameStyleRule>(
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<LintTestCase> kTestCases = {
{"package a; parameter int N = 1; endpackage"},
{"package a; parameter int ", {kToken, "no_style"}, " = 1; endpackage"},
Expand All @@ -178,9 +186,11 @@ TEST(ParameterNameStyleRuleTest, ConfigurationFlavorCombinations) {
{"module a; localparam int ", {kToken, "FOO_BAR"}, " = 1; endmodule"},
};
RunConfiguredLintTestCases<VerilogAnalyzer, ParameterNameStyleRule>(
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<LintTestCase> kTestCases = {
{"package a; parameter int N = 1; endpackage"},
{"package a; parameter int ", {kToken, "no_style"}, " = 1; endpackage"},
Expand All @@ -194,9 +204,11 @@ TEST(ParameterNameStyleRuleTest, ConfigurationFlavorCombinations) {
{"module a; localparam int FOO_BAR = 1; endmodule"},
};
RunConfiguredLintTestCases<VerilogAnalyzer, ParameterNameStyleRule>(
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<LintTestCase> kTestCases = {
{"package a; parameter int N = 1; endpackage"},
{"package a; parameter int no_style = 1; endpackage"},
Expand All @@ -210,7 +222,7 @@ TEST(ParameterNameStyleRuleTest, ConfigurationFlavorCombinations) {
{"module a; localparam int FOO_BAR = 1; endmodule"},
};
RunConfiguredLintTestCases<VerilogAnalyzer, ParameterNameStyleRule>(
kTestCases, "parameter_style:;localparam_style:");
kTestCases, "parameter_style_regex:.*;localparam_style_regex:.*");
}
}

Expand Down
4 changes: 2 additions & 2 deletions verilog/tools/lint/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down

0 comments on commit f9359f8

Please sign in to comment.