Skip to content

Commit

Permalink
Add a regex configration to the signal name style rule
Browse files Browse the repository at this point in the history
  • Loading branch information
sconwayaus committed Jun 22, 2024
1 parent 73f291d commit 56359eb
Show file tree
Hide file tree
Showing 4 changed files with 148 additions and 14 deletions.
5 changes: 4 additions & 1 deletion verilog/analysis/checkers/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -1936,8 +1936,8 @@ cc_library(
"//common/analysis:syntax-tree-lint-rule",
"//common/analysis/matcher",
"//common/analysis/matcher:bound-symbol-manager",
"//common/strings:naming-utils",
"//common/text:concrete-syntax-leaf",
"//common/text:config-utils",
"//common/text:symbol",
"//common/text:syntax-tree-context",
"//common/text:token-info",
Expand All @@ -1948,7 +1948,10 @@ cc_library(
"//verilog/CST:verilog-matchers",
"//verilog/analysis:descriptions",
"//verilog/analysis:lint-rule-registry",
"@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
55 changes: 46 additions & 9 deletions verilog/analysis/checkers/signal_name_style_rule.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,23 @@

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

#include <memory>
#include <set>
#include <string>

#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/concrete_syntax_leaf.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 "common/util/logging.h"
#include "re2/re2.h"
#include "verilog/CST/data.h"
#include "verilog/CST/net.h"
#include "verilog/CST/port.h"
Expand All @@ -43,17 +48,36 @@ using verible::LintViolation;
using verible::SyntaxTreeContext;
using verible::matcher::Matcher;

static constexpr absl::string_view kMessage =
"Signal names must use lower_snake_case naming convention.";
static std::string style_default_regex = "[a-z_0-9]+";

SignalNameStyleRule::SignalNameStyleRule() {
style_regex_ =
std::make_unique<re2::RE2>(style_default_regex, re2::RE2::Quiet);

kMessage =
absl::StrCat("Signal name does not match the naming convention ",
"defined by regex pattern: ", style_regex_->pattern());
}

const LintRuleDescriptor &SignalNameStyleRule::GetDescriptor() {
static const LintRuleDescriptor d{
.name = "signal-name-style",
.topic = "signal-conventions",
.desc =
"Checks that signal names use lower_snake_case naming convention. "
"Signals are defined as \"a net, variable, or port within a "
"SystemVerilog design\".",
"Checks that signal names conform to a naming convention defined by "
"a RE2 regular expression. Signals are defined as \"a net, variable, "
"or port within a SystemVerilog design\".\n"
"Example common regex patterns:\n"
" lower_snake_case: \"[a-z_0-9]+\"\n"
" UPPER_SNAKE_CASE: \"[A-Z_0-9]+\"\n"
" Title_Snake_Case: \"[A-Z]+[a-z0-9]*(_[A-Z0-9]+[a-z0-9]*)*\"\n"
" Sentence_snake_case: \"([A-Z0-9]+[a-z0-9]*_?)([a-z0-9]*_*)*\"\n"
" camelCase: \"([a-z0-9]+[A-Z0-9]*)+\"\n"
" PascalCaseRegexPattern: \"([A-Z0-9]+[a-z0-9]*)+\"\n"
"RE2 regular expression syntax documentation can be found at "
"https://github.com/google/re2/wiki/syntax\n",
.param = {{"style_regex", style_default_regex,
"A regex used to check signal names style."}},
};
return d;
}
Expand All @@ -79,29 +103,42 @@ void SignalNameStyleRule::HandleSymbol(const verible::Symbol &symbol,
if (PortMatcher().Matches(symbol, &manager)) {
const auto *identifier_leaf = GetIdentifierFromPortDeclaration(symbol);
const auto name = ABSL_DIE_IF_NULL(identifier_leaf)->get().text();
if (!verible::IsLowerSnakeCaseWithDigits(name)) {
if (!RE2::FullMatch(name, *style_regex_)) {
violations_.insert(
LintViolation(identifier_leaf->get(), kMessage, context));
}
} else if (NetMatcher().Matches(symbol, &manager)) {
const auto identifier_leaves = GetIdentifiersFromNetDeclaration(symbol);
for (const auto *leaf : identifier_leaves) {
const auto name = leaf->text();
if (!verible::IsLowerSnakeCaseWithDigits(name)) {
if (!RE2::FullMatch(name, *style_regex_)) {
violations_.insert(LintViolation(*leaf, kMessage, context));
}
}
} else if (DataMatcher().Matches(symbol, &manager)) {
const auto identifier_leaves = GetIdentifiersFromDataDeclaration(symbol);
for (const auto *leaf : identifier_leaves) {
const auto name = leaf->text();
if (!verible::IsLowerSnakeCaseWithDigits(name)) {
if (!RE2::FullMatch(name, *style_regex_)) {
violations_.insert(LintViolation(*leaf, kMessage, context));
}
}
}
}

absl::Status SignalNameStyleRule::Configure(absl::string_view configuration) {
using verible::config::SetRegex;
absl::Status s = verible::ParseNameValues(
configuration, {{"style_regex", SetRegex(&style_regex_)}});
if (!s.ok()) return s;

kMessage =
absl::StrCat("Signal name does not match the naming convention ",
"defined by regex pattern: ", style_regex_->pattern());

return absl::OkStatus();
}

LintRuleStatus SignalNameStyleRule::Report() const {
return LintRuleStatus(violations_, GetDescriptor());
}
Expand Down
19 changes: 17 additions & 2 deletions verilog/analysis/checkers/signal_name_style_rule.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,33 +15,48 @@
#ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_SIGNAL_NAME_STYLE_RULE_H_
#define VERIBLE_VERILOG_ANALYSIS_CHECKERS_SIGNAL_NAME_STYLE_RULE_H_

#include <memory>
#include <set>
#include <string>

#include "absl/status/status.h"
#include "absl/strings/string_view.h"
#include "common/analysis/lint_rule_status.h"
#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 {

// SignalNameStyleRule checks that signal names use lower_snake_case naming
// convention. Signals are defined as "a net, variable, or port within a
// SignalNameStyleRule checks that signal names follow
// a naming convention matching a regex pattern.
// Signals are defined as "a net, variable, or port within a
// SystemVerilog design".
class SignalNameStyleRule : public verible::SyntaxTreeLintRule {
public:
using rule_type = verible::SyntaxTreeLintRule;

SignalNameStyleRule();

static const LintRuleDescriptor &GetDescriptor();

void HandleSymbol(const verible::Symbol &symbol,
const verible::SyntaxTreeContext &context) final;

verible::LintRuleStatus Report() const final;

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> style_regex_;

std::string kMessage;
};

} // namespace analysis
Expand Down
83 changes: 81 additions & 2 deletions verilog/analysis/checkers/signal_name_style_rule_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,17 @@ namespace analysis {
namespace {

using verible::LintTestCase;
using verible::RunConfiguredLintTestCases;
using verible::RunLintTestCases;

TEST(SignalNameStyleRuleTest, ModulePortTests) {
TEST(SignalNameStyleRuleTest, DefaultTests) {
constexpr int kToken = SymbolIdentifier;
const std::initializer_list<LintTestCase> kTestCases = {
{""},
// Tests for module ports
{"module foo(input logic b_a_r); endmodule"},
{"module foo(input wire hello_world1); endmodule"},
{"module foo(wire ", {kToken, "HelloWorld"}, "); endmodule"},
{"module foo(input logic ", {kToken, "_bar"}, "); endmodule"},
{"module foo(input logic [3:0] ", {kToken, "Foo_bar"}, "); endmodule"},
{"module foo(input logic b_a_r [3:0]); endmodule"},
{"module foo(input logic [3:0] ",
Expand Down Expand Up @@ -108,6 +108,85 @@ TEST(SignalNameStyleRuleTest, ModulePortTests) {
RunLintTestCases<VerilogAnalyzer, SignalNameStyleRule>(kTestCases);
}

TEST(SignalNameStyleRuleTest, UpperSnakeCaseTests) {
constexpr int kToken = SymbolIdentifier;
const std::initializer_list<LintTestCase> kTestCases = {
{""},
// Tests for module ports
{"module foo(input logic B_A_R); endmodule"},
{"module foo(input wire HELLO_WORLD1); endmodule"},
{"module foo(wire ", {kToken, "HelloWorld"}, "); endmodule"},
{"module foo(input logic [3:0] ", {kToken, "Foo_bar"}, "); endmodule"},
{"module foo(input logic B_A_R [3:0]); endmodule"},
{"module foo(input logic [3:0] ",
{kToken, "Bar"},
", input logic ",
{kToken, "Bar2"},
" [4]); endmodule"},
{"module foo(input logic HELLO_WORLD, input BAR); endmodule"},
{"module foo(input logic HELLO_WORLD, input ",
{kToken, "b_A_r"},
"); endmodule"},
{"module foo(input logic ",
{kToken, "HelloWorld"},
", output ",
{kToken, "Bar"},
"); endmodule"},
{"module foo(input logic ",
{kToken, "hello_World"},
", wire B_A_R = 1); endmodule"},
{"module foo(input HELLO_WORLD, output B_A_R, input wire BAR2); "
"endmodule"},
{"module foo(input HELLO_WORLD, output B_A_R, input wire ",
{kToken, "Bad"},
"); "
"endmodule"},
// Tests for nets
{"module foo; wire SINGLE_NET; endmodule"},
{"module foo; wire FIRST_NET, SECOND_NET; endmodule"},
{"module foo; "
"wire ",
{kToken, "SingleNet"},
"; endmodule"},
{"module foo; "
"wire ",
{kToken, "FirstNet"},
";"
"wire SECOND_NET; endmodule"},
{"module foo; "
"wire ",
{kToken, "FirstNet"},
";"
"wire ",
{kToken, "SecondNet"},
"; endmodule"},
{"module foo; "
"wire ",
{kToken, "FirstNet"},
", ",
{kToken, "SecondNet"},
"; endmodule"},
{"module foo; wire FIRST_NET, ", {kToken, "SecondNet"}, "; endmodule"},
{"module foo; wire ", {kToken, "FirstNet"}, ", SECOND_NET; endmodule"},
// Tests for data
{"module foo; logic OKAY_NAME; endmodule"},
{"module foo; logic ", {kToken, "NotOkayName"}, "; endmodule"},
{"module foo; logic M_ULTIPLE, O_KAY, N_AMES; endmodule"},
{"module foo; "
"logic THE_MIDDLE, ",
{kToken, "NameIs"},
", NOT_OKAY; endmodule"},
{"module foo; logic A = 1, B = 0, C = 1; endmodule"},
{"module foo; logic A = 1, ", {kToken, "b"}, "= 0, C = 1; endmodule"},
{"module top; foo BAZ(0); endmodule"},
{"module top; foo BA_X(0), BA_Y(1), BA_Z(0); endmodule"},
{"module top; foo ", {kToken, "Baz"}, "(0); endmodule"},
{"module top; foo BA_X(0), ", {kToken, "BaY"}, "(1); endmodule"},
};
RunConfiguredLintTestCases<VerilogAnalyzer, SignalNameStyleRule>(
kTestCases, "style_regex:[A-Z_0-9]+");
}

} // namespace
} // namespace analysis
} // namespace verilog

0 comments on commit 56359eb

Please sign in to comment.