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.

Sign up for GitHub

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

Allow rule aliasing #935

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Allow rule aliasing #935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a07fcfa
Add lint rule aliasing
wsipak Sep 10, 2021
b284fbf
Add tests for rule aliases
wsipak Sep 15, 2021
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
16 changes: 15 additions & 1 deletion verilog/analysis/checkers/undersized_binary_literal_rule.cc
View file
Open in desktop
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() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably rename the whole class to be UndersizedNumericLiteralRule, also the naming of the header and test.

This should probably be a separate PR after this is done, so that we don't have too many changes at once.

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 "
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the 'numeric' literal, we should now actually switch all bin, oct and hex to 'true', while the undersized-binary-literal would have bin=true and oct=false, hex=false. That way we can have the advantage of having a good easy-to-discover al lthe features-rule and the limited to what the old binary rule to not surprise existing users-rule.

This would be also the typical application of the aliases: rules evolve and will get more features, but somtimes we don't want to expose these features in a rule that had a very specific name (like in this case where 'binary' kindof indicates that it only deals with one type of number).

So let's have here all features turned on and in the binary rule the ones not bin turned off.

Expand All @@ -73,6 +73,20 @@ const LintRuleDescriptor& UndersizedBinaryLiteralRule::GetDescriptor() {
return d;
}

const std::vector<LintRuleAliasDescriptor>&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of having a separate function GetAliasDescriptors() (which then requires some SFINAE-magic), I suggest to add a new field std::vector<LintRuleAliasDescriptor> aliases at the end of our struct LintRuleDescriptor.
That way, everything can be brace-initialized more compactly in one go in the descriptor and we require less support code for the alias rules as we can just look through the existing descriptor if there are aliases.

(Recently I cleaned up the rules that had multiple static functions for their descriptions to just return one descriptor which made things simpler; I'd like to avoid it getting more compilcated again)

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
View file
Open in desktop
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
View file
Open in desktop
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
6 changes: 6 additions & 0 deletions verilog/analysis/descriptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ struct LintRuleDescriptor {
std::vector<LintConfigParameterDescriptor> param;
};

struct LintRuleAliasDescriptor {
LintRuleId name; // ID/name of the alias.
std::vector<std::pair<absl::string_view, absl::string_view>>
Copy link
Collaborator

Choose a reason for hiding this comment

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

std::pairs have somewhat arbitrary first, second members, so it is hard to read. Almost always it is better to define a struct for that with a good name. Adds a few lines, but also makes it much more readable in every place it is used.

Maybe we can re-use the name/value thing we have going on in LintRuleConfigParameterDescriptor, but decompose it to only use the part with name/value in the alias while the base rule has also description.

So have a little hierarchy (usually, I'd be in favor of composition instead of inheritance, but we already have a bunch of descriptors defined that all would need changing; it is not too bad

struct LintConfigParameterNameValue {
  absl::string_view name;
  std::string default_value;
};

// The descriptor also has a human readable description of this parameter
struct LintConfigParameterDescriptor : public LintConfigParameterNameValue {
  std::string description;
};

Then, replace the pair with our new name/value struct:

struct LintRuleAliasDescriptor {
  LintRuleId name;  // ID/name of the alias.                                                                                                                                                                  
  std::vector<LintConfigParameterNameValue>  param_defaults;  // List of param values set by the alias                                                                                                                                               
};

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
View file
Open in desktop
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
View file
Open in desktop
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
33 changes: 33 additions & 0 deletions verilog/analysis/lint_rule_registry_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,19 @@ class TreeRule1 : public TreeRuleBase {
};
return d;
}
static const std::vector<LintRuleAliasDescriptor>& GetAliasDescriptors() {
static const std::vector<LintRuleAliasDescriptor> d{
{
.name = "rule1-alias1",
.param_defaults = {{"test_param1", "false"}},
},
{
.name = "rule1-alias2",
.param_defaults = {{"test_param1", "false"}},
},
};
return d;
}
};

class TreeRule2 : public TreeRuleBase {
Expand Down Expand Up @@ -240,6 +253,26 @@ TEST(LintRuleRegistryTest, ContainsTextRuleTrue) {
EXPECT_TRUE(IsRegisteredLintRule("text-rule-1"));
}

// Verifies that the alias can be translated to a registered rule name.
TEST(LintRuleRegistryTest, CanBeAliasedTrue) {
EXPECT_TRUE(IsRegisteredLintRule(TranslateAliasIfExists("rule1-alias1")));
EXPECT_TRUE(IsRegisteredLintRule(TranslateAliasIfExists("rule1-alias2")));
}

// Verifies that an invalid alias doesn't translate to a registered rule name
TEST(LintRuleRegistryTest, TranslateInvalidAlias) {
EXPECT_FALSE(IsRegisteredLintRule(TranslateAliasIfExists("invalid-alias")));
}

// Verifies that aliases match the right rule
TEST(LintRuleRegistryTest, AliasedCorrectly) {
std::set<LintRuleId> aliases = GetLintRuleAliases("test-rule-1");
EXPECT_NE(aliases.find("rule1-alias1"), aliases.end());
EXPECT_NE(aliases.find("rule1-alias2"), aliases.end());
EXPECT_EQ(TranslateAliasIfExists("rule1-alias1"), "test-rule-1");
EXPECT_EQ(TranslateAliasIfExists("rule1-alias2"), "test-rule-1");
}

// Verifies that a nonexistent text-structure-based rule yields a nullptr.
TEST(LintRuleRegistryTest, CreateTextLintRuleInvalid) {
EXPECT_EQ(CreateTextStructureLintRule("invalid-id"), nullptr);
Expand Down
16 changes: 16 additions & 0 deletions verilog/analysis/verilog_linter.cc
View file
Open in desktop
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
View file
Open in desktop
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
Loading