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.

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

linter: make constraint-name-style configurable #2017

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 verible/verilog/analysis/checkers/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -1533,16 +1533,18 @@ cc_library(
"//verible/common/analysis:syntax-tree-lint-rule",
"//verible/common/analysis/matcher",
"//verible/common/analysis/matcher:bound-symbol-manager",
"//verible/common/strings:naming-utils",
"//verible/common/text:config-utils",
"//verible/common/text:symbol",
"//verible/common/text:syntax-tree-context",
"//verible/common/text:token-info",
"//verible/verilog/CST:constraints",
"//verible/verilog/CST:verilog-matchers",
"//verible/verilog/analysis:descriptions",
"//verible/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
34 changes: 23 additions & 11 deletions verible/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 @@ -15,13 +15,16 @@
#include "verible/verilog/analysis/checkers/constraint-name-style-rule.h"

#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 "re2/re2.h"
#include "verible/common/analysis/lint-rule-status.h"
#include "verible/common/analysis/matcher/bound-symbol-manager.h"
#include "verible/common/analysis/matcher/matcher.h"
#include "verible/common/strings/naming-utils.h"
#include "verible/common/text/config-utils.h"
#include "verible/common/text/symbol.h"
#include "verible/common/text/syntax-tree-context.h"
#include "verible/common/text/token-info.h"
Expand All @@ -41,20 +44,24 @@ 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() {
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.data()}},
};
return d;
}

absl::Status ConstraintNameStyleRule::Configure(
absl::string_view configuration) {
return verible::ParseNameValues(
configuration, {{"pattern", verible::config::SetRegex(&regex)}});
}

static const Matcher &ConstraintMatcher() {
static const Matcher matcher(NodekConstraintDeclaration());
return matcher;
Expand All @@ -78,13 +85,18 @@ void ConstraintNameStyleRule::HandleSymbol(const verible::Symbol &symbol,

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
27 changes: 23 additions & 4 deletions verible/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,8 +15,13 @@
#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 "absl/status/status.h"
#include "absl/strings/string_view.h"
#include "re2/re2.h"
#include "verible/common/analysis/lint-rule-status.h"
#include "verible/common/analysis/syntax-tree-lint-rule.h"
#include "verible/common/text/symbol.h"
Expand All @@ -26,22 +31,36 @@
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.
// 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();

absl::Status Configure(absl::string_view configuration) final;
void HandleSymbol(const verible::Symbol &symbol,
IEncinas10 marked this conversation as resolved.
Show resolved Hide resolved
const verible::SyntaxTreeContext &context) final;

verible::LintRuleStatus Report() const final;

std::string Pattern() const { return regex->pattern(); }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this public method (returning a string by value) needed anywhere ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It isn't, I used it to diagnose if the user-provided regex failed to parse but it is not needed anymore thanks to the SetRegex utility. Missed it, thanks!

private:
// Lower snake case, ends with `_c`
static constexpr absl::string_view kSuffix = "([a-z0-9]+_)+c";

std::set<verible::LintViolation> violations_;
std::unique_ptr<re2::RE2> regex = std::make_unique<re2::RE2>(kSuffix);

std::string FormatReason() const;
};

} // namespace analysis
Expand Down
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 @@ -27,6 +27,7 @@ namespace analysis {
namespace {

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

// Tests that ConstraintNameStyleRule correctly accepts valid names.
Expand All @@ -49,6 +50,27 @@ 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"},
};
// Lower snake case, starts with `c_`
RunConfiguredLintTestCases<VerilogAnalyzer, ConstraintNameStyleRule>(
kTestCases, "pattern:c+(_[a-z0-9]+)+");
}

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