Merge pull request chipsalliance#2210 from sconwayaus/enum_name_style…

…_regex Add a regex style pattern to the enum-name-style
hzeller · Aug 5, 2024 · 1cf4a4d · 1cf4a4d
2 parents 361afaa + fa735dd
commit 1cf4a4d
Show file tree

Hide file tree

Showing 7 changed files with 139 additions and 28 deletions.
diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD
@@ -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,
 )

diff --git a/verilog/analysis/checkers/README.md b/verilog/analysis/checkers/README.md
@@ -12,4 +12,4 @@ User documentation for the lint rules is generated dynamically, and can be found
 at https://chipsalliance.github.io/verible/verilog_lint.html, or by running
 `verible-verilog-lint --help_rules` for text or `--generate_markdown`.
 
-[Linter user documentation can be found here](../../tool/lint).
+[Linter user documentation can be found here](../../tools/lint).
diff --git a/verilog/analysis/checkers/enum_name_style_rule.cc b/verilog/analysis/checkers/enum_name_style_rule.cc
@@ -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"
@@ -40,39 +44,51 @@ 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)) {}
+
+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. The default regex pattern expects "
+          "\"lower_snake_case\" with either a \"_t\" or \"_e\" suffix. Refer "
+          "to "
+          "https://github.com/chipsalliance/verible/tree/master/verilog/tools/"
+          "lint#readme for more detail on verible regex patterns.",
+      .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) {
+std::string EnumNameStyleRule::CreateViolationMessage() {
+  return absl::StrCat("Enum type name does not match the naming convention ",
+                      "defined by regex pattern: ", style_regex_->pattern());
+}
+
+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"))) {
-        violations_.insert(
-            LintViolation(identifier_leaf->get(), kMessage, context));
+      if (!RE2::FullMatch(name, *style_regex_)) {
+        violations_.insert(LintViolation(identifier_leaf->get(),
+                                         CreateViolationMessage(), context));
       }
     } else {
       // Not an enum definition
@@ -81,6 +97,13 @@ 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_)}});
+  return s;
+}
+
 LintRuleStatus EnumNameStyleRule::Report() const {
   return LintRuleStatus(violations_, GetDescriptor());
 }

diff --git a/verilog/analysis/checkers/enum_name_style_rule.h b/verilog/analysis/checkers/enum_name_style_rule.h
@@ -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();
+
+  std::string CreateViolationMessage();
+
+  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_;
 };
 
 }  // namespace analysis

diff --git a/verilog/analysis/checkers/enum_name_style_rule_test.cc b/verilog/analysis/checkers/enum_name_style_rule_test.cc
@@ -27,6 +27,7 @@ namespace analysis {
 namespace {
 
 using verible::LintTestCase;
+using verible::RunConfiguredLintTestCases;
 using verible::RunLintTestCases;
 
 TEST(EnumNameStyleRuleTest, ValidEnumNames) {
@@ -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
diff --git a/verilog/analysis/checkers/interface_name_style_rule.cc b/verilog/analysis/checkers/interface_name_style_rule.cc
@@ -45,7 +45,7 @@ static constexpr absl::string_view kMessage =
     "Interface names must use lower_snake_case naming convention "
     "and end with _if.";
 
-const LintRuleDescriptor& InterfaceNameStyleRule::GetDescriptor() {
+const LintRuleDescriptor &InterfaceNameStyleRule::GetDescriptor() {
   static const LintRuleDescriptor d{
       .name = "interface-name-style",
       .topic = "interface-conventions",
@@ -56,16 +56,16 @@ const LintRuleDescriptor& InterfaceNameStyleRule::GetDescriptor() {
   return d;
 }
 
-static const Matcher& InterfaceMatcher() {
+static const Matcher &InterfaceMatcher() {
   static const Matcher matcher(NodekInterfaceDeclaration());
   return matcher;
 }
 
-void InterfaceNameStyleRule::HandleSymbol(const verible::Symbol& symbol,
-                                          const SyntaxTreeContext& context) {
+void InterfaceNameStyleRule::HandleSymbol(const verible::Symbol &symbol,
+                                          const SyntaxTreeContext &context) {
   verible::matcher::BoundSymbolManager manager;
   absl::string_view name;
-  const verible::TokenInfo* identifier_token;
+  const verible::TokenInfo *identifier_token;
   if (InterfaceMatcher().Matches(symbol, &manager)) {
     identifier_token = GetInterfaceNameToken(symbol);
     name = identifier_token->text();

diff --git a/verilog/tools/lint/README.md b/verilog/tools/lint/README.md
@@ -157,6 +157,26 @@ Additionally, the `--rules_config` flag can be used to read configuration stored
 in a file. The syntax is the same as above, except the rules can be also
 separated with the newline character.
 
+### Regular Expressions
+Some lint rules allow users to use a regular expression to configure the rule. Verible utilises the RE2 regular expression library and full syntax documentation can be found at https://github.com/google/re2/wiki/syntax.
+
+```bash
+verible-verilog-lint --rules="enum-name-style=style_regex:[a-z_0-9]+(_t|_e)" ...
+```
+
+For naming style rules, the regex can be kept simple and not have to worry about 
+the beginning starting with a digit as this is already taken care of by the 
+lexer/parser. Below are some common naming style regex patterns that can be used.
+
+| Naming Style           | Regex Expression                           |
+|------------------------|--------------------------------------------|
+| lower\_snake\_case     | [a-z\_0-9]+                                |
+| UPPER\_SNAKE\_CASE     | [A-Z\_0-9]+                                |
+| Title\_Snake\_Case     | [A-Z]+[a-z0-9]\*(\_[A-Z0-9]+[a-z0-9]\*)\*  |
+| Sentence\_snake\_case  | ([A-Z0-9]+[a-z0-9]\*\_?)([a-z0-9]\*\_\*)\* |
+| camelCase              | ([a-z0-9]+[A-Z0-9]\*)+                     |
+| PascalCase             | ([A-Z0-9]+[a-z0-9]\*)+                     |
+
 ## Waiving Lint Violations {#lint-waiver}
 
 ### In-file waiver comments