Skip to content

Commit

Permalink
Add lint rule aliasing
Browse files Browse the repository at this point in the history
  • Loading branch information
wsipak committed Oct 4, 2021
1 parent cb60b63 commit 44f5a35
Show file tree
Hide file tree
Showing 8 changed files with 187 additions and 10 deletions.
16 changes: 15 additions & 1 deletion verilog/analysis/checkers/undersized_binary_literal_rule.cc
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ VERILOG_REGISTER_LINT_RULE(UndersizedBinaryLiteralRule);

const LintRuleDescriptor& UndersizedBinaryLiteralRule::GetDescriptor() {
static const LintRuleDescriptor d{
.name = "undersized-binary-literal",
.name = "undersized-numeric-literal",
.topic = "number-literals",
.desc =
"Checks that the digits of binary literals for the configured "
Expand All @@ -73,6 +73,20 @@ const LintRuleDescriptor& UndersizedBinaryLiteralRule::GetDescriptor() {
return d;
}

const std::vector<LintRuleAliasDescriptor>&
UndersizedBinaryLiteralRule::GetAliasDescriptors() {
static const std::vector<LintRuleAliasDescriptor> d{
{
.name = "undersized-binary-literal",
.param_defaults =
{
{"bin", "true"},
},
},
};
return d;
}

// Broadly, start by matching all number nodes with a
// constant width and based literal.

Expand Down
2 changes: 2 additions & 0 deletions verilog/analysis/checkers/undersized_binary_literal_rule.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ class UndersizedBinaryLiteralRule : public verible::SyntaxTreeLintRule {

static const LintRuleDescriptor& GetDescriptor();

static const std::vector<LintRuleAliasDescriptor>& GetAliasDescriptors();

void HandleSymbol(const verible::Symbol& symbol,
const verible::SyntaxTreeContext& context) final;

Expand Down
2 changes: 1 addition & 1 deletion verilog/analysis/default_rules.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ constexpr const char* kDefaultRuleSet[] = {
"no-tabs",
"posix-eof",
"line-length",
"undersized-binary-literal",
"undersized-numeric-literal",
"explicit-function-lifetime",
"explicit-function-task-parameter-type",
"explicit-task-lifetime",
Expand Down
10 changes: 10 additions & 0 deletions verilog/analysis/descriptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,23 @@ struct LintConfigParameterDescriptor {
};

struct LintRuleDescriptor {
// TODO(wsip) use this data in help messages. Add/remove fields if needed
LintRuleId name; // ID/name of the rule.
absl::string_view topic; // section in style-guide
absl::string_view dv_topic; // section in design verification style-guide
std::string desc; // Detailed description.
std::vector<LintConfigParameterDescriptor> param;
};

struct LintRuleAliasDescriptor {
LintRuleId name; // ID/name of the alias.
absl::string_view topic; // section in style-guide
absl::string_view dv_topic; // section in design verification style-guide
std::string desc; // Detailed description
std::vector<std::pair<absl::string_view, absl::string_view>>
param_defaults; // List of param values set by the alias
};

} // namespace analysis
} // namespace verilog

Expand Down
72 changes: 71 additions & 1 deletion verilog/analysis/lint_rule_registry.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include <memory>
#include <string>
#include <utility>
#include <vector>

#include "absl/container/node_hash_map.h"
Expand All @@ -38,6 +39,21 @@ using verible::TokenStreamLintRule;
using verible::container::FindOrNull;

namespace {

absl::node_hash_map<LintRuleId, LintRuleId>* GetLintRuleAliases() {
// maps aliases to the original names of rules
static auto* aliases = new absl::node_hash_map<LintRuleId, LintRuleId>();
return aliases;
}

absl::node_hash_map<LintRuleId, LintAliasDescriptionsFun>*
GetLintRuleAliasDescriptors() {
// maps rule name to a function that returns descriptors of its aliases
static auto* desc =
new absl::node_hash_map<LintRuleId, LintAliasDescriptionsFun>();
return desc;
}

// Used to export function local static pointer to avoid global variables
template <typename RuleType>
absl::node_hash_map<LintRuleId, LintRuleInfo<RuleType>>* GetLintRuleRegistry() {
Expand Down Expand Up @@ -112,11 +128,55 @@ class LintRuleRegistry {

} // namespace

std::set<LintRuleId> GetLintRuleAliases(LintRuleId rule_name) {
std::set<LintRuleId> result;

for (auto const& alias : *GetLintRuleAliases()) {
if (alias.second == rule_name) result.insert(alias.first);
}
return result;
}

LintRuleAliasDescriptor GetLintRuleAliasDescriptor(LintRuleId rule_name,
LintRuleId alias) {
const auto* descriptors = GetLintRuleAliasDescriptors();
const auto target = descriptors->find(rule_name);

CHECK(target != descriptors->end());
LintAliasDescriptionsFun desc_fun = target->second;
// desc_fun() returns a reference to a vector of descriptors of aliases
std::vector<LintRuleAliasDescriptor> alias_descriptors = desc_fun();
size_t i = 0;
for (; i != alias_descriptors.size(); i++) {
if (alias_descriptors[i].name == alias) {
return alias_descriptors[i];
}
}
LOG(FATAL) << "caller of " << __FUNCTION__
<< "shall make sure that the alias belongs to the rule";
abort();
}

template <typename RuleType>
LintRuleRegisterer<RuleType>::LintRuleRegisterer(
const LintDescriptionFun& descriptor,
const LintRuleGeneratorFun<RuleType>& creator) {
const LintRuleGeneratorFun<RuleType>& creator,
const LintAliasDescriptionsFun alias_descriptors) {
LintRuleRegistry<RuleType>::Register(descriptor, creator);

if (!alias_descriptors) return;

// map rule name with the function that returns a vector of alias descriptions
GetLintRuleAliasDescriptors()->insert(
std::pair<LintRuleId, LintAliasDescriptionsFun>(descriptor().name,
alias_descriptors));

const std::vector<LintRuleAliasDescriptor> descrs = alias_descriptors();
for (auto const& descr : descrs) {
// map every alias of this rule to the name of the rule
GetLintRuleAliases()->insert(
std::pair<LintRuleId, LintRuleId>(descr.name, descriptor().name));
}
}

bool IsRegisteredLintRule(const LintRuleId& rule_name) {
Expand Down Expand Up @@ -180,6 +240,16 @@ std::set<LintRuleId> GetAllRegisteredLintRuleNames() {
return result;
}

LintRuleId TranslateAliasIfExists(const LintRuleId alias) {
const auto* aliases = GetLintRuleAliases();
const auto target = aliases->find(alias);
if (target != aliases->end()) {
return target->second;
} else {
return alias;
}
}

LintRuleDescriptionsMap GetAllRuleDescriptions() {
// Map that will hold the information to print about each rule.
LintRuleDescriptionsMap res;
Expand Down
52 changes: 46 additions & 6 deletions verilog/analysis/lint_rule_registry.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ class LintRuleRegisterer;
template <typename RuleType>
using LintRuleGeneratorFun = std::function<std::unique_ptr<RuleType>()>;
using LintDescriptionFun = std::function<LintRuleDescriptor()>;
using LintAliasDescriptionsFun =
const std::vector<LintRuleAliasDescriptor>& (*)();

template <typename RuleType>
struct LintRuleInfo {
Expand All @@ -67,6 +69,28 @@ struct LintRuleDefaultConfig {
using LintRuleDescriptionsMap =
std::map<LintRuleId, LintRuleDefaultConfig, verible::StringViewCompare>;

// Class that uses SFINAE to conditionally get T::GetAliasDescriptors function.
template <class T>
class GetAliasDescriptorsFuncOrNullptr {
template <typename U>
static constexpr LintAliasDescriptionsFun get(
decltype(U::GetAliasDescriptors)*) {
return U::GetAliasDescriptors;
}
template <typename U>
static constexpr LintAliasDescriptionsFun get(...) {
return nullptr;
}

public:
// Pointer to T::GetAliasDescriptors if it's implemented, nullptr otherwise.
static constexpr LintAliasDescriptionsFun value = get<T>(nullptr);
};

template <class T>
inline constexpr LintAliasDescriptionsFun GetAliasDescriptorsFunc =
GetAliasDescriptorsFuncOrNullptr<T>::value;

// Helper macro to register a LintRule with LintRuleRegistry. In order to have
// a global registry, some static initialization is needed. This macros
// centralizes this unsafe code into one place in order to prevent mistakes.
Expand Down Expand Up @@ -98,11 +122,14 @@ using LintRuleDescriptionsMap =
//
// TODO(hzeller): once the class does not contain a state, extract the name
// and description from the instance to avoid weird static initialization.
#define VERILOG_REGISTER_LINT_RULE(class_name) \
static verilog::analysis::LintRuleRegisterer<class_name::rule_type> \
__##class_name##__registerer(class_name::GetDescriptor, []() { \
return std::unique_ptr<class_name::rule_type>(new class_name()); \
});
#define VERILOG_REGISTER_LINT_RULE(class_name) \
static verilog::analysis::LintRuleRegisterer<class_name::rule_type> \
__##class_name##__registerer( \
class_name::GetDescriptor, \
[]() { \
return std::unique_ptr<class_name::rule_type>(new class_name()); \
}, \
verilog::analysis::GetAliasDescriptorsFunc<class_name>);

// Static objects of type LintRuleRegisterer are used to register concrete
// parsers in LintRuleRegistry. Users are expected to create these objects
Expand All @@ -111,7 +138,8 @@ template <typename RuleType>
class LintRuleRegisterer {
public:
LintRuleRegisterer(const LintDescriptionFun& descriptor,
const LintRuleGeneratorFun<RuleType>& creator);
const LintRuleGeneratorFun<RuleType>& creator,
LintAliasDescriptionsFun alias_descriptors);
};

// Returns true if rule_name refers to a known lint rule.
Expand Down Expand Up @@ -150,6 +178,18 @@ std::unique_ptr<verible::TextStructureLintRule> CreateTextStructureLintRule(
// this set, because their lifetime is guaranteed by the registration process.
std::set<LintRuleId> GetAllRegisteredLintRuleNames();

// Returns a set of aliases that were registered for given rule name.
std::set<LintRuleId> GetLintRuleAliases(LintRuleId rule_name);

// Returns a descriptor of a specific alias of a rule.
// The caller shall check that the alias belongs to this rule.
LintRuleAliasDescriptor GetLintRuleAliasDescriptor(LintRuleId rule_name,
LintRuleId alias);

// Returns a translated name of lint rule using a registered alias.
// If such an alias doesn't exist, returns the input alias.
LintRuleId TranslateAliasIfExists(LintRuleId alias);

// Returns a map mapping each rule to a struct of information about the rule to
// print.
LintRuleDescriptionsMap GetAllRuleDescriptions();
Expand Down
16 changes: 16 additions & 0 deletions verilog/analysis/verilog_linter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,7 @@ absl::Status PrintRuleInfo(std::ostream* os,
constexpr int kParamIndent = kRuleWidth + 4;
constexpr char kFill = ' ';

rule_name = analysis::TranslateAliasIfExists(rule_name);
const auto it = rule_map.find(rule_name);
if (it == rule_map.end())
return absl::NotFoundError(absl::StrCat(
Expand All @@ -574,6 +575,21 @@ absl::Status PrintRuleInfo(std::ostream* os,
}
}

const auto aliases = analysis::GetLintRuleAliases(rule_name);
if (!aliases.empty()) {
*os << std::left << std::setw(kRuleWidth) << std::setfill(kFill) << " "
<< "Alias" << (aliases.size() > 1 ? "es" : "") << ":\n";
for (const auto& alias : aliases) {
const auto descr = analysis::GetLintRuleAliasDescriptor(rule_name, alias);
*os << std::left << std::setw(kParamIndent) << std::setfill(kFill) << " "
<< "* `" << alias << "` sets parameters:";
for (const auto& param : descr.param_defaults) {
*os << " " << param.first << ":" << param.second << ";";
}
*os << "\n";
}
}

// Print default enabled.
*os << std::left << std::setw(kRuleWidth) << std::setfill(kFill) << " "
<< "Enabled by default: " << std::boolalpha << it->second.default_enabled
Expand Down
27 changes: 26 additions & 1 deletion verilog/analysis/verilog_linter_configuration.cc
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,8 @@ bool RuleBundle::ParseConfiguration(absl::string_view text, char separator,
const auto config = rule_name_with_config.substr(equals_pos + 1);
setting.configuration.assign(config.data(), config.size());
}
const auto rule_name = rule_name_with_config.substr(0, equals_pos);
const auto raw_name = rule_name_with_config.substr(0, equals_pos);
const auto rule_name = analysis::TranslateAliasIfExists(raw_name);
const auto rule_name_set = analysis::GetAllRegisteredLintRuleNames();
const auto rule_iter = rule_name_set.find(rule_name);

Expand All @@ -139,6 +140,30 @@ bool RuleBundle::ParseConfiguration(absl::string_view text, char separator,
*error = absl::StrCat("invalid flag \"", rule_name, "\"");
return false;
} else {
// check if it's an alias
if (raw_name != rule_name) {
auto alias_descriptor =
analysis::GetLintRuleAliasDescriptor(rule_name, raw_name);
VLOG(2) << raw_name << " is an alias of " << rule_name;

if (setting.enabled) {
// apply alias defaults, only if we're enabling the rule
std::vector<std::string> params;
for (auto const& param : alias_descriptor.param_defaults) {
// get the default parameters from the alias descriptor
params.push_back(absl::StrCat(param.first, ":", param.second));
}
std::string defaults = absl::StrJoin(params, ";");
VLOG(2) << "configuration from alias defaults: " << defaults;

// use the defaults first, then the commandline arguments
// join them with "," only if both have contents
setting.configuration = absl::StrJoin(
{defaults, setting.configuration},
(defaults.empty() || setting.configuration.empty()) ? "" : ";");
}
}

// Map keys must use canonical registered string_views for guaranteed
// lifetime, not just any string-equivalent copy.
rules[*rule_iter] = setting;
Expand Down

0 comments on commit 44f5a35

Please sign in to comment.