From 7332d1bdffbeebf382fb36704d0ee036699ad8cf Mon Sep 17 00:00:00 2001 From: Ignacio Encinas Date: Sat, 23 Sep 2023 22:26:00 +0200 Subject: [PATCH] linter: make constraint-name-style configurable Up until now, constraint-name-style rule required constraint names to end in '_c'. This makes it configurable, so that users can provide a regular expression to specify what name style they want. The default behaviour is still the same. --- verilog/analysis/checkers/BUILD | 4 +- .../checkers/constraint_name_style_rule.cc | 55 +++++++++++++------ .../checkers/constraint_name_style_rule.h | 32 ++++++++--- .../constraint_name_style_rule_test.cc | 24 +++++++- 4 files changed, 87 insertions(+), 28 deletions(-) diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index 77984f5d5b..4088ae291d 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -1467,7 +1467,7 @@ 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/text:token-info", @@ -1475,8 +1475,8 @@ cc_library( "//verilog/CST:verilog-matchers", "//verilog/analysis:descriptions", "//verilog/analysis:lint-rule-registry", - "//verilog/parser:verilog-token-enum", "@com_google_absl//absl/strings", + "@com_googlesource_code_re2//:re2", ], alwayslink = 1, ) diff --git a/verilog/analysis/checkers/constraint_name_style_rule.cc b/verilog/analysis/checkers/constraint_name_style_rule.cc index abc1688f1f..baba0f1fa0 100644 --- a/verilog/analysis/checkers/constraint_name_style_rule.cc +++ b/verilog/analysis/checkers/constraint_name_style_rule.cc @@ -1,4 +1,4 @@ -// Copyright 2017-2020 The Verible Authors. +// Copyright 2017-2023 The Verible Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -17,13 +17,12 @@ #include #include -#include "absl/strings/match.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" @@ -31,7 +30,6 @@ #include "verilog/CST/verilog_matchers.h" #include "verilog/analysis/descriptions.h" #include "verilog/analysis/lint_rule_registry.h" -#include "verilog/parser/verilog_token_enum.h" namespace verilog { namespace analysis { @@ -44,50 +42,71 @@ using verible::matcher::Matcher; // Register ConstraintNameStyleRule. VERILOG_REGISTER_LINT_RULE(ConstraintNameStyleRule); -static constexpr absl::string_view kMessage = - "Constraint names must by styled with lower_snake_case and end with _c."; - -const LintRuleDescriptor& ConstraintNameStyleRule::GetDescriptor() { +const LintRuleDescriptor &ConstraintNameStyleRule::GetDescriptor() { static const LintRuleDescriptor d{ .name = "constraint-name-style", .topic = "constraints", .desc = - "Check that constraint names follow the lower_snake_case " - "convention and end with _c.", + "Check that constraint names follow the required name style " + "specified by a regular expression.", + .param = {{"pattern", kSuffix}}, }; return d; } -static const Matcher& ConstraintMatcher() { +absl::Status ConstraintNameStyleRule::Configure( + absl::string_view configuration) { + std::string pattern = kSuffix; + absl::Status status = verible::ParseNameValues( + configuration, {{"pattern", verible::config::SetString(&pattern)}}); + + regex = std::make_unique(pattern, re2::RE2::Quiet); + if (!regex->ok()) { + std::cerr << "[ERR] Error parsing pattern " << std::quoted(pattern) << ": " + << regex->error() + << ". Falling back to the default configuration: " + << std::quoted(kSuffix); + regex = std::make_unique(kSuffix); + } + + return status; +} + +static const Matcher &ConstraintMatcher() { static const Matcher matcher(NodekConstraintDeclaration()); return matcher; } -void ConstraintNameStyleRule::HandleSymbol(const verible::Symbol& symbol, - const SyntaxTreeContext& context) { +void ConstraintNameStyleRule::HandleSymbol(const verible::Symbol &symbol, + const SyntaxTreeContext &context) { verible::matcher::BoundSymbolManager manager; if (ConstraintMatcher().Matches(symbol, &manager)) { // Since an out-of-line definition is always followed by a forward // declaration somewhere else (in this case inside a class), we can just - // ignore all out-of-line definitions to avoid duplicate lint errors on + // ignore all out-of-line definitions to avoid duplicate lint errors on // the same name. if (IsOutOfLineConstraintDefinition(symbol)) { return; } - const auto* identifier_token = + const auto *identifier_token = GetSymbolIdentifierFromConstraintDeclaration(symbol); if (!identifier_token) return; const absl::string_view constraint_name = identifier_token->text(); - if (!verible::IsLowerSnakeCaseWithDigits(constraint_name) || - !absl::EndsWith(constraint_name, "_c")) { - violations_.insert(LintViolation(*identifier_token, kMessage, context)); + if (!RE2::FullMatch(constraint_name, *regex)) { + violations_.insert( + LintViolation(*identifier_token, FormatReason(), context)); } } } +std::string ConstraintNameStyleRule::FormatReason() const { + return absl::StrCat("Constraint names must obey the following regex: ", + regex->pattern()); +} + LintRuleStatus ConstraintNameStyleRule::Report() const { return LintRuleStatus(violations_, GetDescriptor()); } diff --git a/verilog/analysis/checkers/constraint_name_style_rule.h b/verilog/analysis/checkers/constraint_name_style_rule.h index c24ac8c0f6..f333719b2e 100644 --- a/verilog/analysis/checkers/constraint_name_style_rule.h +++ b/verilog/analysis/checkers/constraint_name_style_rule.h @@ -1,4 +1,4 @@ -// Copyright 2017-2020 The Verible Authors. +// Copyright 2017-2023 The Verible Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -15,6 +15,7 @@ #ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_CONSTRAINT_NAME_STYLE_RULE_H_ #define VERIBLE_VERILOG_ANALYSIS_CHECKERS_CONSTRAINT_NAME_STYLE_RULE_H_ +#include #include #include @@ -22,27 +23,44 @@ #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 { -// ConstraintNameStyleRule check each constraint name follows the correct naming -// convention. -// The constraints should be named with lower_snake_case and end with _c. +// Lower snake case, ends with `_c` +#define kSuffix "([a-z0-9]+_)+c" + +// Lower snake case, starts with `c_` +#define kPrefix "c+(_[a-z0-9]+)+" + +// ConstraintNameStyleRule checks that each constraint name follows the +// specified naming convention. +// +// This convention is set by providing a regular expression to be matched +// against. +// +// The default, `kSuffix` checks that the name is written in lower_snake_case +// and ends with `_c` class ConstraintNameStyleRule : public verible::SyntaxTreeLintRule { public: using rule_type = verible::SyntaxTreeLintRule; - static const LintRuleDescriptor& GetDescriptor(); + static const LintRuleDescriptor &GetDescriptor(); - void HandleSymbol(const verible::Symbol& symbol, - const verible::SyntaxTreeContext& context) final; + absl::Status Configure(absl::string_view configuration) final; + void HandleSymbol(const verible::Symbol &symbol, + const verible::SyntaxTreeContext &context) final; verible::LintRuleStatus Report() const final; private: std::set violations_; + inline static std::unique_ptr regex = + std::make_unique(kSuffix); + + std::string FormatReason() const; }; } // namespace analysis diff --git a/verilog/analysis/checkers/constraint_name_style_rule_test.cc b/verilog/analysis/checkers/constraint_name_style_rule_test.cc index 6256b363ef..2f911fac48 100644 --- a/verilog/analysis/checkers/constraint_name_style_rule_test.cc +++ b/verilog/analysis/checkers/constraint_name_style_rule_test.cc @@ -1,4 +1,4 @@ -// Copyright 2017-2020 The Verible Authors. +// Copyright 2017-2023 The Verible Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -29,6 +29,7 @@ namespace analysis { namespace { using verible::LintTestCase; +using verible::RunConfiguredLintTestCases; using verible::RunLintTestCases; // Tests that ConstraintNameStyleRule correctly accepts valid names. @@ -42,6 +43,7 @@ TEST(ConstraintNameStyleRuleTest, AcceptTests) { {"class foo; rand logic a; constraint foo_bar_c { a == 16; } endclass"}, {"class foo; rand logic a; constraint foo2_c { a == 16; } endclass"}, {"class foo; rand logic a; constraint foo_2_bar_c { a == 16; } endclass"}, + {"class foo; rand logic a; constraint a_c { a == 16; } endclass"}, /* Ignore out of line definitions */ {"constraint classname::constraint_c { a <= b; }"}, @@ -51,6 +53,26 @@ TEST(ConstraintNameStyleRuleTest, AcceptTests) { RunLintTestCases(kTestCases); } +TEST(ConstraintNameStyleRuleTest, VariousPrefixTests) { + constexpr int kToken = SymbolIdentifier; + const std::initializer_list kTestCases = { + {"class foo; rand logic a; constraint c_foo { a == 16; } endclass"}, + {"class foo; rand logic a; constraint c_a { a == 16; } endclass"}, + {"class foo; rand logic a; constraint c_foo_bar { a == 16; } endclass"}, + {"class foo; rand logic a; constraint ", + {kToken, "c_"}, + " { a == 16; } endclass"}, + {"class foo; rand logic a; constraint ", + {kToken, "no_suffix"}, + " { a == 16; } endclass"}, + {"class foo; rand logic a; constraint ", + {kToken, "suffix_ok_but_we_want_prefix_c"}, + " { a == 16; } endclass"}, + }; + RunConfiguredLintTestCases( + kTestCases, "pattern:" kPrefix); +} + // Tests that ConstraintNameStyleRule rejects invalid names. TEST(ConstraintNameStyleRuleTest, RejectTests) { constexpr int kToken = SymbolIdentifier;