From 44f5a3504ca16869794837e2da548f1d7b7f782e Mon Sep 17 00:00:00 2001 From: Wojciech Sipak Date: Fri, 10 Sep 2021 20:46:33 +0200 Subject: [PATCH] Add lint rule aliasing --- .../undersized_binary_literal_rule.cc | 16 ++++- .../checkers/undersized_binary_literal_rule.h | 2 + verilog/analysis/default_rules.h | 2 +- verilog/analysis/descriptions.h | 10 +++ verilog/analysis/lint_rule_registry.cc | 72 ++++++++++++++++++- verilog/analysis/lint_rule_registry.h | 52 ++++++++++++-- verilog/analysis/verilog_linter.cc | 16 +++++ .../analysis/verilog_linter_configuration.cc | 27 ++++++- 8 files changed, 187 insertions(+), 10 deletions(-) diff --git a/verilog/analysis/checkers/undersized_binary_literal_rule.cc b/verilog/analysis/checkers/undersized_binary_literal_rule.cc index fab381cdd3..17467f87ec 100644 --- a/verilog/analysis/checkers/undersized_binary_literal_rule.cc +++ b/verilog/analysis/checkers/undersized_binary_literal_rule.cc @@ -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 " @@ -73,6 +73,20 @@ const LintRuleDescriptor& UndersizedBinaryLiteralRule::GetDescriptor() { return d; } +const std::vector& +UndersizedBinaryLiteralRule::GetAliasDescriptors() { + static const std::vector 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. diff --git a/verilog/analysis/checkers/undersized_binary_literal_rule.h b/verilog/analysis/checkers/undersized_binary_literal_rule.h index c357863ee7..56e49db77b 100644 --- a/verilog/analysis/checkers/undersized_binary_literal_rule.h +++ b/verilog/analysis/checkers/undersized_binary_literal_rule.h @@ -38,6 +38,8 @@ class UndersizedBinaryLiteralRule : public verible::SyntaxTreeLintRule { static const LintRuleDescriptor& GetDescriptor(); + static const std::vector& GetAliasDescriptors(); + void HandleSymbol(const verible::Symbol& symbol, const verible::SyntaxTreeContext& context) final; diff --git a/verilog/analysis/default_rules.h b/verilog/analysis/default_rules.h index dcde27f9b3..5cac5662d6 100644 --- a/verilog/analysis/default_rules.h +++ b/verilog/analysis/default_rules.h @@ -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", diff --git a/verilog/analysis/descriptions.h b/verilog/analysis/descriptions.h index 75181bac6a..6e5c0e2bfd 100644 --- a/verilog/analysis/descriptions.h +++ b/verilog/analysis/descriptions.h @@ -36,6 +36,7 @@ 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 @@ -43,6 +44,15 @@ struct LintRuleDescriptor { std::vector 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> + param_defaults; // List of param values set by the alias +}; + } // namespace analysis } // namespace verilog diff --git a/verilog/analysis/lint_rule_registry.cc b/verilog/analysis/lint_rule_registry.cc index 5966dce851..f7f5a7deea 100644 --- a/verilog/analysis/lint_rule_registry.cc +++ b/verilog/analysis/lint_rule_registry.cc @@ -16,6 +16,7 @@ #include #include +#include #include #include "absl/container/node_hash_map.h" @@ -38,6 +39,21 @@ using verible::TokenStreamLintRule; using verible::container::FindOrNull; namespace { + +absl::node_hash_map* GetLintRuleAliases() { + // maps aliases to the original names of rules + static auto* aliases = new absl::node_hash_map(); + return aliases; +} + +absl::node_hash_map* +GetLintRuleAliasDescriptors() { + // maps rule name to a function that returns descriptors of its aliases + static auto* desc = + new absl::node_hash_map(); + return desc; +} + // Used to export function local static pointer to avoid global variables template absl::node_hash_map>* GetLintRuleRegistry() { @@ -112,11 +128,55 @@ class LintRuleRegistry { } // namespace +std::set GetLintRuleAliases(LintRuleId rule_name) { + std::set 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 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 LintRuleRegisterer::LintRuleRegisterer( const LintDescriptionFun& descriptor, - const LintRuleGeneratorFun& creator) { + const LintRuleGeneratorFun& creator, + const LintAliasDescriptionsFun alias_descriptors) { LintRuleRegistry::Register(descriptor, creator); + + if (!alias_descriptors) return; + + // map rule name with the function that returns a vector of alias descriptions + GetLintRuleAliasDescriptors()->insert( + std::pair(descriptor().name, + alias_descriptors)); + + const std::vector descrs = alias_descriptors(); + for (auto const& descr : descrs) { + // map every alias of this rule to the name of the rule + GetLintRuleAliases()->insert( + std::pair(descr.name, descriptor().name)); + } } bool IsRegisteredLintRule(const LintRuleId& rule_name) { @@ -180,6 +240,16 @@ std::set 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; diff --git a/verilog/analysis/lint_rule_registry.h b/verilog/analysis/lint_rule_registry.h index dbeb66afc5..9a8066857f 100644 --- a/verilog/analysis/lint_rule_registry.h +++ b/verilog/analysis/lint_rule_registry.h @@ -52,6 +52,8 @@ class LintRuleRegisterer; template using LintRuleGeneratorFun = std::function()>; using LintDescriptionFun = std::function; +using LintAliasDescriptionsFun = + const std::vector& (*)(); template struct LintRuleInfo { @@ -67,6 +69,28 @@ struct LintRuleDefaultConfig { using LintRuleDescriptionsMap = std::map; +// Class that uses SFINAE to conditionally get T::GetAliasDescriptors function. +template +class GetAliasDescriptorsFuncOrNullptr { + template + static constexpr LintAliasDescriptionsFun get( + decltype(U::GetAliasDescriptors)*) { + return U::GetAliasDescriptors; + } + template + static constexpr LintAliasDescriptionsFun get(...) { + return nullptr; + } + + public: + // Pointer to T::GetAliasDescriptors if it's implemented, nullptr otherwise. + static constexpr LintAliasDescriptionsFun value = get(nullptr); +}; + +template +inline constexpr LintAliasDescriptionsFun GetAliasDescriptorsFunc = + GetAliasDescriptorsFuncOrNullptr::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. @@ -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##__registerer(class_name::GetDescriptor, []() { \ - return std::unique_ptr(new class_name()); \ - }); +#define VERILOG_REGISTER_LINT_RULE(class_name) \ + static verilog::analysis::LintRuleRegisterer \ + __##class_name##__registerer( \ + class_name::GetDescriptor, \ + []() { \ + return std::unique_ptr(new class_name()); \ + }, \ + verilog::analysis::GetAliasDescriptorsFunc); // Static objects of type LintRuleRegisterer are used to register concrete // parsers in LintRuleRegistry. Users are expected to create these objects @@ -111,7 +138,8 @@ template class LintRuleRegisterer { public: LintRuleRegisterer(const LintDescriptionFun& descriptor, - const LintRuleGeneratorFun& creator); + const LintRuleGeneratorFun& creator, + LintAliasDescriptionsFun alias_descriptors); }; // Returns true if rule_name refers to a known lint rule. @@ -150,6 +178,18 @@ std::unique_ptr CreateTextStructureLintRule( // this set, because their lifetime is guaranteed by the registration process. std::set GetAllRegisteredLintRuleNames(); +// Returns a set of aliases that were registered for given rule name. +std::set 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(); diff --git a/verilog/analysis/verilog_linter.cc b/verilog/analysis/verilog_linter.cc index 0bcf7a765c..8c1a7bb11b 100644 --- a/verilog/analysis/verilog_linter.cc +++ b/verilog/analysis/verilog_linter.cc @@ -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( @@ -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 diff --git a/verilog/analysis/verilog_linter_configuration.cc b/verilog/analysis/verilog_linter_configuration.cc index 53b18e7227..44780e7289 100644 --- a/verilog/analysis/verilog_linter_configuration.cc +++ b/verilog/analysis/verilog_linter_configuration.cc @@ -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); @@ -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 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;