Skip to content

Commit

Permalink
linter: make constraint-name-style configurable
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
IEncinas10 committed Jan 13, 2024
1 parent a4d61b1 commit bbccb82
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 28 deletions.
4 changes: 2 additions & 2 deletions verilog/analysis/checkers/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -1467,16 +1467,16 @@ 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",
"//verilog/CST:constraints",
"//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,
)
Expand Down
48 changes: 30 additions & 18 deletions verilog/analysis/checkers/constraint_name_style_rule.cc
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -17,21 +17,19 @@
#include <set>
#include <string>

#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"
#include "verilog/CST/constraints.h"
#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 {
Expand All @@ -44,50 +42,64 @@ 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<re2::RE2>(pattern);

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());
}
Expand Down
32 changes: 25 additions & 7 deletions verilog/analysis/checkers/constraint_name_style_rule.h
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -15,34 +15,52 @@
#ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_CONSTRAINT_NAME_STYLE_RULE_H_
#define VERIBLE_VERILOG_ANALYSIS_CHECKERS_CONSTRAINT_NAME_STYLE_RULE_H_

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

#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 {

// 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<verible::LintViolation> violations_;
inline static std::unique_ptr<re2::RE2> regex =
std::make_unique<re2::RE2>(kSuffix);

std::string FormatReason() const;
};

} // namespace analysis
Expand Down
24 changes: 23 additions & 1 deletion verilog/analysis/checkers/constraint_name_style_rule_test.cc
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -29,6 +29,7 @@ namespace analysis {
namespace {

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

// Tests that ConstraintNameStyleRule correctly accepts valid names.
Expand All @@ -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; }"},
Expand All @@ -51,6 +53,26 @@ TEST(ConstraintNameStyleRuleTest, AcceptTests) {
RunLintTestCases<VerilogAnalyzer, ConstraintNameStyleRule>(kTestCases);
}

TEST(ConstraintNameStyleRuleTest, VariousPrefixTests) {
constexpr int kToken = SymbolIdentifier;
const std::initializer_list<LintTestCase> 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<VerilogAnalyzer, ConstraintNameStyleRule>(
kTestCases, "pattern:" kPrefix);
}

// Tests that ConstraintNameStyleRule rejects invalid names.
TEST(ConstraintNameStyleRuleTest, RejectTests) {
constexpr int kToken = SymbolIdentifier;
Expand Down

0 comments on commit bbccb82

Please sign in to comment.