Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Naming conventions with regex #737

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Naming conventions with regex #737

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9bc3f14
implementing the name validation with regex for the struct_union rule
Apr 1, 2021
a29fdb9
adding regex name matcher to interface name rule and struct_union_rule
Apr 2, 2021
7e62480
adding regex to parameter_type_name_style rule, adjusting old test cases
Apr 2, 2021
4685110
adding regex name match to constraint_name_style rule
Apr 2, 2021
e431100
adding regex name option check to enum_name_style rule
Apr 2, 2021
8c77635
adding descriptions to rules
Apr 2, 2021
240d2cf
chnaging std::optional to absl::optional, adding exceptions, formatin…
Apr 9, 2021
291adb5
updating descriptions, removing typos
Apr 16, 2021
85e3596
formatting code
Apr 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions verilog/analysis/checkers/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,20 +651,23 @@ cc_library(
name = "enum_name_style_rule",
srcs = ["enum_name_style_rule.cc"],
hdrs = ["enum_name_style_rule.h"],
copts = ["-fexceptions"],
deps = [
"//common/analysis:citation",
"//common/analysis:lint_rule_status",
"//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",
"//verilog/CST:type",
"//verilog/CST:verilog_matchers",
"//verilog/analysis:descriptions",
"//verilog/analysis:lint_rule_registry",
"@com_google_absl//absl/strings",
"@com_google_absl//absl/types:optional",
],
alwayslink = 1,
)
Expand All @@ -681,6 +684,7 @@ cc_test(
"//verilog/analysis:verilog_analyzer",
"//verilog/parser:verilog_token_enum",
"@com_google_googletest//:gtest_main",
"@com_google_absl//absl/strings",
],
)

Expand Down Expand Up @@ -1411,13 +1415,15 @@ cc_library(
name = "constraint_name_style_rule",
srcs = ["constraint_name_style_rule.cc"],
hdrs = ["constraint_name_style_rule.h"],
copts = ["-fexceptions"],
deps = [
"//common/analysis:citation",
"//common/analysis:lint_rule_status",
"//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",
"//common/text:token_info",
Expand All @@ -1427,6 +1433,7 @@ cc_library(
"//verilog/analysis:lint_rule_registry",
"//verilog/parser:verilog_token_enum",
"@com_google_absl//absl/strings",
"@com_google_absl//absl/types:optional",
],
alwayslink = 1,
)
Expand All @@ -1443,6 +1450,7 @@ cc_test(
"//verilog/analysis:verilog_analyzer",
"//verilog/parser:verilog_token_enum",
"@com_google_googletest//:gtest_main",
"@com_google_absl//absl/strings",
],
)

Expand Down Expand Up @@ -1558,6 +1566,7 @@ cc_library(
name = "parameter_type_name_style_rule",
srcs = ["parameter_type_name_style_rule.cc"],
hdrs = ["parameter_type_name_style_rule.h"],
copts = ["-fexceptions"],
deps = [
"//common/analysis:citation",
"//common/analysis:lint_rule_status",
Expand All @@ -1568,12 +1577,14 @@ cc_library(
"//common/text:symbol",
"//common/text:syntax_tree_context",
"//common/text:token_info",
"//common/text:config_utils",
"//verilog/CST:parameters",
"//verilog/CST:verilog_matchers",
"//verilog/analysis:descriptions",
"//verilog/analysis:lint_rule_registry",
"//verilog/parser:verilog_token_enum",
"@com_google_absl//absl/strings",
"@com_google_absl//absl/types:optional",
],
alwayslink = 1,
)
Expand All @@ -1590,6 +1601,7 @@ cc_test(
"//verilog/analysis:verilog_analyzer",
"//verilog/parser:verilog_token_enum",
"@com_google_googletest//:gtest_main",
"@com_google_absl//absl/strings",
],
)

Expand Down Expand Up @@ -1815,6 +1827,7 @@ cc_library(
"//verilog/analysis:descriptions",
"//verilog/analysis:lint_rule_registry",
"@com_google_absl//absl/strings",
"@com_google_absl//absl/types:optional",
],
alwayslink = 1,
)
Expand All @@ -1831,13 +1844,15 @@ cc_test(
"//verilog/analysis:verilog_analyzer",
"//verilog/parser:verilog_token_enum",
"@com_google_googletest//:gtest_main",
"@com_google_absl//absl/strings",
],
)

cc_library(
name = "struct_union_name_style_rule",
srcs = ["struct_union_name_style_rule.cc"],
hdrs = ["struct_union_name_style_rule.h"],
copts = ["-fexceptions"],
deps = [
"//common/analysis:citation",
"//common/analysis:lint_rule_status",
Expand All @@ -1853,6 +1868,7 @@ cc_library(
"//verilog/analysis:descriptions",
"//verilog/analysis:lint_rule_registry",
"@com_google_absl//absl/strings",
"@com_google_absl//absl/types:optional",
],
alwayslink = 1,
)
Expand All @@ -1877,6 +1893,7 @@ cc_library(
name = "interface_name_style_rule",
srcs = ["interface_name_style_rule.cc"],
hdrs = ["interface_name_style_rule.h"],
copts = ["-fexceptions"],
deps = [
"//common/analysis:citation",
"//common/analysis:lint_rule_status",
Expand All @@ -1886,6 +1903,7 @@ cc_library(
"//common/strings:naming_utils",
"//common/text:symbol",
"//common/text:syntax_tree_context",
"//common/text:config_utils",
"//verilog/CST:module",
"//verilog/CST:type",
"//verilog/CST:verilog_matchers",
Expand Down
38 changes: 37 additions & 1 deletion verilog/analysis/checkers/constraint_name_style_rule.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#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"
Expand Down Expand Up @@ -55,10 +56,18 @@ const char ConstraintNameStyleRule::kMessage[] =

std::string ConstraintNameStyleRule::GetDescription(
DescriptionType description_type) {
return absl::StrCat(
static std::string basic_desc = absl::StrCat(
"Check that constraint names follow the lower_snake_case convention and"
" end with _c. See ",
GetStyleGuideCitation(kTopic), ".");
if (description_type == DescriptionType::kHelpRulesFlag) {
return absl::StrCat(basic_desc, "Paramteres: name_regex:regex rule");
} else {
return absl::StrCat(
basic_desc,
"\n##### Parameters\n"
"* `name_regex` (The regex rule validating the names. Default: Empty)");
}
}

static const Matcher& ConstraintMatcher() {
Expand All @@ -82,13 +91,40 @@ void ConstraintNameStyleRule::HandleSymbol(const verible::Symbol& symbol,
GetSymbolIdentifierFromConstraintDeclaration(symbol);

const auto constraint_name = identifier_token.text();
if (name_regex_.has_value()) {
if (!std::regex_match(std::string(constraint_name), *name_regex_)) {
violations_.insert(LintViolation(identifier_token,
"Regex rule does not match", context));
}
return;
}

if (!verible::IsLowerSnakeCaseWithDigits(constraint_name) ||
!absl::EndsWith(constraint_name, "_c"))
violations_.insert(LintViolation(identifier_token, kMessage, context));
}
}

absl::Status ConstraintNameStyleRule::Configure(
absl::string_view configuration) {
using verible::config::SetString;
std::string name_regex;
auto status = verible::ParseNameValues(
configuration, {{"name_regex", SetString(&name_regex)}});

if (!status.ok()) return status;

if (!name_regex.empty()) {
try {
name_regex_ = name_regex;
} catch (const std::regex_error& e) {
return absl::Status(absl::StatusCode::kInvalidArgument,
"Invalid regex specified");
}
}
return absl::OkStatus();
}

LintRuleStatus ConstraintNameStyleRule::Report() const {
return LintRuleStatus(violations_, Name(), GetStyleGuideCitation(kTopic));
}
Expand Down
6 changes: 6 additions & 0 deletions verilog/analysis/checkers/constraint_name_style_rule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@
#ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_CONSTRAINT_NAME_STYLE_RULE_H_
#define VERIBLE_VERILOG_ANALYSIS_CHECKERS_CONSTRAINT_NAME_STYLE_RULE_H_

#include <regex>
#include <set>
#include <string>

#include "absl/types/optional.h"
#include "common/analysis/lint_rule_status.h"
#include "common/analysis/syntax_tree_lint_rule.h"
#include "common/text/symbol.h"
Expand All @@ -42,6 +44,8 @@ class ConstraintNameStyleRule : public verible::SyntaxTreeLintRule {
void HandleSymbol(const verible::Symbol& symbol,
const verible::SyntaxTreeContext& context) override;

absl::Status Configure(absl::string_view configuration) override;

verible::LintRuleStatus Report() const override;

private:
Expand All @@ -51,6 +55,8 @@ class ConstraintNameStyleRule : public verible::SyntaxTreeLintRule {
// Diagnostic message for constraint name violations.
static const char kMessage[];

absl::optional<std::regex> name_regex_;

std::set<verible::LintViolation> violations_;
};

Expand Down
58 changes: 58 additions & 0 deletions verilog/analysis/checkers/constraint_name_style_rule_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include <initializer_list>

#include "absl/strings/match.h"
#include "common/analysis/linter_test_utils.h"
#include "common/analysis/syntax_tree_linter_test_utils.h"
#include "common/text/symbol.h"
Expand All @@ -29,8 +30,65 @@ namespace analysis {
namespace {

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

TEST(ConstraintNameStyleRuleTest, ConfigurationPass) {
ConstraintNameStyleRule rule;
absl::Status status;
EXPECT_TRUE((status = rule.Configure("name_regex:[a-z]")).ok())
<< status.message();
}

TEST(ConstraintNameStyleRuleTest, ConfigurationFail) {
ConstraintNameStyleRule rule;
absl::Status status;
EXPECT_FALSE((status = rule.Configure("bad_name_regex:")).ok())
<< status.message();

EXPECT_FALSE((status = rule.Configure("name_regex:[a-z")).ok())
<< status.message();
EXPECT_TRUE(absl::StrContains(status.message(), "Invalid regex specified"));
}

TEST(ConstraintNameStyleRuleTestConfiguredRegex,
ValidInterfaceDeclarationNames) {
const absl::string_view regex = "name_regex:.*_i";
const std::initializer_list<LintTestCase> kTestCases = {
{""},
{"module foo; logic a; endmodule"},
{"class foo; logic a; endclass"},
{"class foo; rand logic a; constraint _foo_i { a < 16; } endclass"},
{"class foo; rand logic a; constraint foo_i { a < 16; } endclass"},
{"class foo; rand logic a; constraint bar_i { a >= 16; } endclass"},
{"class foo; rand logic a; constraint foo_bar_i { a == 16; } endclass"},
{"class foo; rand logic a; constraint foo2_i { a == 16; } endclass"},
{"class foo; rand logic a; constraint foo_2_bar_i { a == 16; } endclass"},

/* Ignore out of line definitions */
{"constraint classname::constraint_i { a <= b; }"},
{"constraint classname::MY_CONSTRAINT { a <= b; }"},
{"constraint classname::MyConstraint { a <= b; }"},
};
RunConfiguredLintTestCases<VerilogAnalyzer, ConstraintNameStyleRule>(
kTestCases, regex);
}

TEST(ConstraintNameStyleRuleTestConfiguredRegex, RejectTests) {
const absl::string_view regex = "name_regex:[a-zA-Z]*_i";
constexpr int kToken = SymbolIdentifier;
const std::initializer_list<LintTestCase> kTestCases = {
{"class foo; rand logic a; constraint ",
{kToken, "foo_c"},
" { a == 16; } endclass"},
{"class foo; rand logic a; constraint ",
{kToken, "foo12_i"},
" { a == 16; } endclass"},
};
RunConfiguredLintTestCases<VerilogAnalyzer, ConstraintNameStyleRule>(
kTestCases, regex);
}

// Tests that ConstraintNameStyleRule correctly accepts valid names.
TEST(ConstraintNameStyleRuleTest, AcceptTests) {
const std::initializer_list<LintTestCase> kTestCases = {
Expand Down
44 changes: 40 additions & 4 deletions verilog/analysis/checkers/enum_name_style_rule.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#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 "verilog/CST/type.h"
Expand All @@ -49,10 +50,19 @@ const char EnumNameStyleRule::kMessage[] =

std::string EnumNameStyleRule::GetDescription(
DescriptionType description_type) {
return absl::StrCat("Checks that ", Codify("enum", description_type),
" names use lower_snake_case naming convention"
" and end with '_t' or '_e'. See ",
GetStyleGuideCitation(kTopic), ".");
static std::string basic_desc =
absl::StrCat("Checks that ", Codify("enum", description_type),
" names use lower_snake_case naming convention"
" and end with '_t' or '_e'. See ",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.. and also the additional optional regular expression.

Otherwise the explanation looks a bit confusing to the user: tests for_t and _e ... and then there is a parameter with regular expressions - it is unclear from seeing that if that is replacing the _t, _e rule or augmenting it.

Similar in the other rules you added regexp: we should update the documentation to include the optional regexp capability.

Copy link
Contributor Author

@pawelsag pawelsag Apr 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so I changed the description as suggested, but I change the phrase to: or also the additional optional regular expression., cause we can use either regex or the original rule, but when regex is enabled we do not validate the old rules.

GetStyleGuideCitation(kTopic), ".");
if (description_type == DescriptionType::kHelpRulesFlag) {
return absl::StrCat(basic_desc, "Paramteres: name_regex:regex rule");
} else {
return absl::StrCat(
basic_desc,
"\n##### Parameters\n"
"* `name_regex` (The regex rule validating the names. Default: Empty)");
}
}

static const Matcher& TypedefMatcher() {
Expand All @@ -69,6 +79,13 @@ void EnumNameStyleRule::HandleSymbol(const verible::Symbol& symbol,
if (!FindAllEnumTypes(symbol).empty()) {
const auto* identifier_leaf = GetIdentifierFromTypeDeclaration(symbol);
const auto name = ABSL_DIE_IF_NULL(identifier_leaf)->get().text();
if (name_regex_.has_value()) {
if (!std::regex_match(std::string(name), *name_regex_)) {
violations_.insert(LintViolation(
*identifier_leaf, "Regex rule does not match", context));
}
return;
}
if (!verible::IsLowerSnakeCaseWithDigits(name) ||
!(absl::EndsWith(name, "_t") || absl::EndsWith(name, "_e"))) {
violations_.insert(
Expand All @@ -81,6 +98,25 @@ void EnumNameStyleRule::HandleSymbol(const verible::Symbol& symbol,
}
}

absl::Status EnumNameStyleRule::Configure(absl::string_view configuration) {
using verible::config::SetString;
std::string name_regex;
auto status = verible::ParseNameValues(
configuration, {{"name_regex", SetString(&name_regex)}});

if (!status.ok()) return status;

if (!name_regex.empty()) {
try {
name_regex_ = name_regex;
} catch (const std::regex_error& e) {
return absl::Status(absl::StatusCode::kInvalidArgument,
"Invalid regex specified");
}
}
return absl::OkStatus();
}

LintRuleStatus EnumNameStyleRule::Report() const {
return LintRuleStatus(violations_, Name(), GetStyleGuideCitation(kTopic));
}
Expand Down
Loading