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

Jump to bottom

Add a regex style pattern to the enum-name-style #2210

Merged
merged 2 commits into from
Aug 5, 2024
Merged

Add a regex style pattern to the enum-name-style #2210

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f6c4a1
Add a regex style pattern to the enum-name-style
sconwayaus Jun 21, 2024
fa735dd
Moving some documentation into the linter README.md.
sconwayaus Jul 10, 2024
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
4 changes: 3 additions & 1 deletion verilog/analysis/checkers/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,16 +650,18 @@ 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",
"//common/util:logging",
"//verilog/CST:type",
"//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
62 changes: 47 additions & 15 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 @@ -14,17 +14,21 @@

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

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

#include "absl/strings/match.h"
#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/util/logging.h"
#include "re2/re2.h"
#include "verilog/CST/type.h"
#include "verilog/CST/verilog_matchers.h"
#include "verilog/analysis/descriptions.h"
Expand All @@ -40,39 +44,54 @@ using verible::LintViolation;
using verible::SyntaxTreeContext;
using verible::matcher::Matcher;

static constexpr absl::string_view kMessage =
"Enum names must use lower_snake_case naming convention "
"and end with _t or _e.";
static constexpr absl::string_view style_default_regex = "[a-z_0-9]+(_t|_e)";

const LintRuleDescriptor& EnumNameStyleRule::GetDescriptor() {
EnumNameStyleRule::EnumNameStyleRule()
: style_regex_(
std::make_unique<re2::RE2>(style_default_regex, re2::RE2::Quiet)),
violation_message_(absl::StrCat(
"Enum type name does not match the naming convention ",
"defined by regex pattern: ", style_regex_->pattern())) {}

const LintRuleDescriptor &EnumNameStyleRule::GetDescriptor() {
static const LintRuleDescriptor d{
.name = "enum-name-style",
.topic = "enumerations",
.desc =
"Checks that `enum` names use lower_snake_case naming convention "
"and end with '_t' or '_e'.",
"Checks that enum type names follow a naming convention defined by "
"a RE2 regular expression.\n"
"Example common regex patterns:\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of repeating the same examples in various places using regexps, how about we add a little section in the lint README.md with regexp examples and just add a link here ?

(also here, the default regex, the one with the trailing _e or _t is not even mentioned)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem.

" 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", std::string(style_default_regex),
"A regex used to check enum type name style."}},
};
return d;
}

static const Matcher& TypedefMatcher() {
static const Matcher &TypedefMatcher() {
static const Matcher matcher(NodekTypeDeclaration());
return matcher;
}

void EnumNameStyleRule::HandleSymbol(const verible::Symbol& symbol,
const SyntaxTreeContext& context) {
void EnumNameStyleRule::HandleSymbol(const verible::Symbol &symbol,
const SyntaxTreeContext &context) {
verible::matcher::BoundSymbolManager manager;
if (TypedefMatcher().Matches(symbol, &manager)) {
// TODO: This can be changed to checking type of child (by index) when we
// have consistent shape for all kTypeDeclaration nodes.
if (!FindAllEnumTypes(symbol).empty()) {
const auto* identifier_leaf = GetIdentifierFromTypeDeclaration(symbol);
const auto *identifier_leaf = GetIdentifierFromTypeDeclaration(symbol);
const auto name = ABSL_DIE_IF_NULL(identifier_leaf)->get().text();
if (!verible::IsLowerSnakeCaseWithDigits(name) ||
!(absl::EndsWith(name, "_t") || absl::EndsWith(name, "_e"))) {
if (!RE2::FullMatch(name, *style_regex_)) {
violations_.insert(
LintViolation(identifier_leaf->get(), kMessage, context));
LintViolation(identifier_leaf->get(), violation_message_, context));
}
} else {
// Not an enum definition
Expand All @@ -81,6 +100,19 @@ void EnumNameStyleRule::HandleSymbol(const verible::Symbol& symbol,
}
}

absl::Status EnumNameStyleRule::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;

violation_message_ =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering: instead of having a validation_message_ member that we most likely never use, yet allocate and prepare should we maybe have a std::string CreateValidationMessage() {} that creates it on-the-fly if needed in the actual valdation ? It also would mean that we don't have to duplicate the code creating it in the constructor and in the Configure() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem.

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

return absl::OkStatus();
}

LintRuleStatus EnumNameStyleRule::Report() const {
return LintRuleStatus(violations_, GetDescriptor());
}
Expand Down
24 changes: 19 additions & 5 deletions verilog/analysis/checkers/enum_name_style_rule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,32 +15,46 @@
#ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_ENUM_NAME_STYLE_RULE_H_
#define VERIBLE_VERILOG_ANALYSIS_CHECKERS_ENUM_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 {

// EnumNameStyleRule checks that all enum names use
// lower_snake_case naming convention and end with "_e" or "_t".
// EnumNameStyleRule checks that all enum names follow
// a naming convention matching a regex pattern.
class EnumNameStyleRule : public verible::SyntaxTreeLintRule {
public:
using rule_type = verible::SyntaxTreeLintRule;

static const LintRuleDescriptor& GetDescriptor();
EnumNameStyleRule();

void HandleSymbol(const verible::Symbol& symbol,
const verible::SyntaxTreeContext& context) final;
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 violation_message_;
};

} // namespace analysis
Expand Down
52 changes: 52 additions & 0 deletions verilog/analysis/checkers/enum_name_style_rule_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ namespace analysis {
namespace {

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

TEST(EnumNameStyleRuleTest, ValidEnumNames) {
Expand Down Expand Up @@ -153,6 +154,57 @@ TEST(EnumNameStyleRuleTest, UncheckedCases) {
};
RunLintTestCases<VerilogAnalyzer, EnumNameStyleRule>(kTestCases);
}

TEST(EnumNameStyleRuleTest, UpperSnakeCaseTests) {
constexpr int kToken = SymbolIdentifier;
const std::initializer_list<LintTestCase> kTestCases = {
{""},
{""},
{"typedef enum BAZ_T;"},
{"typedef enum GOOD_NAME_T;"},
{"typedef enum B_A_Z_T;"},
{"typedef enum BAZ_E;"},
{"typedef enum GOOD_NAME_E;"},
{"typedef enum B_A_Z_E;"},
{"typedef enum { OneValue, TwoValue } MY_NAME_E;\nmy_name_e a_instance;"},
{"typedef enum logic [1:0] { Fir, Oak, Pine } TREE_E;\ntree_e a_tree;"},
{"typedef enum { Red=3, Green=5 } STATE_E;\nstate_e a_state;"},
{"typedef // We declare a type here"
"enum { Idle, Busy } STATUS_E;\nstatus_e a_status;"},
{"typedef enum { OneValue, TwoValue } MY_NAME_T;\nmy_name_t a_instance;"},
{"typedef enum logic [1:0] { Fir, Oak, Pine } TREE_T;\ntree_t a_tree;"},
{"typedef enum { Red=3, Green=5 } STATE_T;\nstate_t a_state;"},
{"typedef // We declare a type here"
"enum { Idle, Busy } STATUS_T;\nstatus_t a_status;"},
// Declarations inside a class
{"class foo;\n"
"typedef enum { Red=3, Green=5 } STATE_E;\n"
"state_e a_state;\n"
"endclass"},
{"class foo;\n"
"typedef enum logic [1:0] { Fir, Oak, Pine } TREE_T;\n"
"tree_t a_tree;\n"
"endclass"},
// Declarations inside a module
{"module foo;\n"
"typedef enum { Red=3, Green=5 } STATE_E;\n"
"state_e a_state;\n"
"endmodule"},
{"module foo;\n"
"typedef enum logic [1:0] { Fir, Oak, Pine } TREE_T;\n"
"tree_t a_tree;\n"
"endmodule"},
{"typedef enum ", {kToken, "HelloWorld"}, ";"},
{"typedef enum ", {kToken, "_baz"}, ";"},
{"typedef enum ", {kToken, "Bad_name"}, ";"},
{"typedef enum ", {kToken, "bad_Name"}, ";"},
{"typedef enum ", {kToken, "Bad2"}, ";"},

};
RunConfiguredLintTestCases<VerilogAnalyzer, EnumNameStyleRule>(
kTestCases, "style_regex:[A-Z_0-9]+(_T|_E)");
}

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