Merge pull request #901 from hzeller/expand-undersize-binary-to-oct-hex

Extend the undersized binary literal rule to also check Hex and Oct.

.github/workflows/verible-ci.yml

@@ -124,12 +124,6 @@ jobs:
        # Download complete repository + tags
        fetch-depth: 0
 
-    - name: Mount bazel cache
-      uses: actions/cache@v1
-      with:
-        path: "/home/runner/.cache/bazel"
-        key: bazel-kythe
-
     - name: Install Dependencies
       run: |
         source ./.github/settings.sh

verilog/analysis/checkers/BUILD

@@ -965,6 +965,7 @@ cc_library(
         "//common/analysis/matcher",
         "//common/analysis/matcher:bound_symbol_manager",
         "//common/text:concrete_syntax_leaf",
+        "//common/text:config_utils",
         "//common/text:symbol",
         "//common/text:syntax_tree_context",
         "//common/text:token_info",

verilog/analysis/checkers/undersized_binary_literal_rule.cc

@@ -1,4 +1,4 @@
-// Copyright 2017-2020 The Verible Authors.
+// Copyright 2017-2021 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.
@@ -26,6 +26,7 @@
 #include "common/analysis/matcher/bound_symbol_manager.h"
 #include "common/analysis/matcher/matcher.h"
 #include "common/text/concrete_syntax_leaf.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"
@@ -38,11 +39,13 @@
 namespace verilog {
 namespace analysis {
 
+using verible::down_cast;
 using verible::GetStyleGuideCitation;
 using verible::LintRuleStatus;
 using verible::LintViolation;
 using verible::SyntaxTreeContext;
 using verible::SyntaxTreeLeaf;
+using verible::SyntaxTreeNode;
 using verible::matcher::Matcher;
 
 // Register UndersizedBinaryLiteralRule
@@ -55,65 +58,108 @@ const char UndersizedBinaryLiteralRule::kTopic[] = "number-literals";
 
 std::string UndersizedBinaryLiteralRule::GetDescription(
     DescriptionType description_type) {
-  return absl::StrCat(
-      "Checks that the digits of binary literals match their declared "
-      "width. See ",
-      GetStyleGuideCitation(kTopic), ".");
+  static const std::string basic_desc = absl::StrCat(
+      "Checks that the digits of binary literals for the configured bases "
+      "match their declared width. See ",
+      GetStyleGuideCitation(kTopic), ".\n");
+
+  return absl::StrCat(basic_desc,
+                      description_type == DescriptionType::kHelpRulesFlag
+                          ? "Parameters: "
+                            "bin:true;oct:false;hex:false;"
+                          : "##### Parameters\n"
+                            "  * `bin` Default: `true`\n"
+                            "  * `oct` Default: `false`\n"
+                            "  * `hex` Default: `false`");
 }
 
 // Broadly, start by matching all number nodes with a
 // constant width and based literal.
-// TODO(fangism): If more precision is needed than what the inner matcher
-// provides, pass a more specific predicate matching function instead.
 
 static const Matcher& NumberMatcher() {
-  static const Matcher matcher(NodekNumber(
-      NumberHasConstantWidth().Bind("width"),
-      NumberHasBasedLiteral(NumberIsBinary().Bind("base"),
-                            NumberHasBinaryDigits().Bind("digits"))));
+  static const Matcher matcher(
+      NodekNumber(NumberHasConstantWidth().Bind("width"),
+                  NumberHasBasedLiteral().Bind("literal")));
   return matcher;
 }
 
 void UndersizedBinaryLiteralRule::HandleSymbol(
     const verible::Symbol& symbol, const SyntaxTreeContext& context) {
   verible::matcher::BoundSymbolManager manager;
-  if (NumberMatcher().Matches(symbol, &manager)) {
-    if (auto width_leaf = manager.GetAs<SyntaxTreeLeaf>("width")) {
-      if (auto base_leaf = manager.GetAs<SyntaxTreeLeaf>("base")) {
-        if (auto digits_leaf = manager.GetAs<SyntaxTreeLeaf>("digits")) {
-          auto width_text = width_leaf->get().text();
-          auto base_text = base_leaf->get().text();
-          auto digits_text = digits_leaf->get().text();
-          size_t width;
-          if (absl::SimpleAtoi(width_text, &width)) {
-            const BasedNumber number(base_text, digits_text);
-            CHECK(number.ok)
-                << "Expecting valid numeric literal from lexer, but got: "
-                << digits_text;
-            // Detect binary values, whose literal width is shorter than the
-            // declared width.
-            // Allow 'b0 and 'b? as an exception.
-            CHECK_EQ(number.base,
-                     'b');  // guaranteed by matching TK_BinBase
-            if (width > number.literal.length() && number.literal != "0" &&
-                number.literal != "?") {
-              violations_.insert(LintViolation(
-                  digits_leaf->get(),
-                  FormatReason(width_text, base_text, digits_text), context));
-            }
-          }  // else width is not constant, so ignore
-        }
-      }
-    }
+  if (!NumberMatcher().Matches(symbol, &manager)) return;
+  const auto width_leaf = manager.GetAs<SyntaxTreeLeaf>("width");
+  const auto literal_node = manager.GetAs<SyntaxTreeNode>("literal");
+  if (!width_leaf || !literal_node) return;
+
+  const auto width_text = width_leaf->get().text();
+  size_t width;
+  if (!absl::SimpleAtoi(width_text, &width)) return;
+
+  const auto& base_digit_part = literal_node->children();
+  auto base_leaf = down_cast<const SyntaxTreeLeaf*>(base_digit_part[0].get());
+  auto digits_leaf = down_cast<const SyntaxTreeLeaf*>(base_digit_part[1].get());
+
+  const auto base_text = base_leaf->get().text();
+  const auto digits_text = digits_leaf->get().text();
+
+  const BasedNumber number(base_text, digits_text);
+  int bits_per_digit = 1;
+  switch (number.base) {
+    case 'd':
+      return;  // Don't care about decimal values.
+    case 'b':
+      if (!check_bin_numbers_) return;
+      bits_per_digit = 1;
+      break;
+    case 'o':
+      if (!check_oct_numbers_) return;
+      bits_per_digit = 3;
+      break;
+    case 'h':
+      if (!check_hex_numbers_) return;
+      bits_per_digit = 4;
+      break;
+    default:
+      LOG(FATAL) << "Unexpected base '" << base_text << "'";  // Lexer issue ?
+  }
+
+  // Allow literals with single "0" or "?" as an exception
+  if (width > number.literal.length() * bits_per_digit &&
+      number.literal != "0" && number.literal != "?") {
+    violations_.insert(LintViolation(
+        digits_leaf->get(),
+        FormatReason(width_text, base_text, number.base, digits_text),
+        context));
   }
 }
 
 // Generate string representation of why lint error occurred at leaf
 std::string UndersizedBinaryLiteralRule::FormatReason(
-    absl::string_view width, absl::string_view base,
+    absl::string_view width, absl::string_view base_text, char base,
     absl::string_view literal) {
-  return absl::StrCat("Binary literal ", width, base, literal,
-                      " is shorter than its declared width: ", width, ".");
+  absl::string_view base_describe;
+  switch (base) {
+    case 'b':
+      base_describe = "Binary";
+      break;
+    case 'h':
+      base_describe = "Hex";
+      break;
+    case 'o':
+      base_describe = "Octal";
+      break;
+  }
+  return absl::StrCat(base_describe, " literal ", width, base_text, literal,
+                      " has less digits than expected for ", width, " bits.");
+}
+
+absl::Status UndersizedBinaryLiteralRule::Configure(
+    absl::string_view configuration) {
+  using verible::config::SetBool;
+  return verible::ParseNameValues(configuration,
+                                  {{"bin", SetBool(&check_bin_numbers_)},
+                                   {"hex", SetBool(&check_hex_numbers_)},
+                                   {"oct", SetBool(&check_oct_numbers_)}});
 }
 
 LintRuleStatus UndersizedBinaryLiteralRule::Report() const {

verilog/analysis/checkers/undersized_binary_literal_rule.h

@@ -42,19 +42,25 @@ class UndersizedBinaryLiteralRule : public verible::SyntaxTreeLintRule {
   static std::string GetDescription(DescriptionType);
 
   void HandleSymbol(const verible::Symbol& symbol,
-                    const verible::SyntaxTreeContext& context) override;
+                    const verible::SyntaxTreeContext& context) final;
 
-  verible::LintRuleStatus Report() const override;
+  verible::LintRuleStatus Report() const final;
+
+  absl::Status Configure(absl::string_view configuration) final;
 
  private:
   // Generate string representation of why lint error occurred at leaf
   static std::string FormatReason(absl::string_view width,
-                                  absl::string_view base,
+                                  absl::string_view base_text, char base,
                                   absl::string_view literal);
 
   // Link to style guide rule.
   static const char kTopic[];
 
+  bool check_bin_numbers_ = true;
+  bool check_hex_numbers_ = false;
+  bool check_oct_numbers_ = false;
+
   std::set<verible::LintViolation> violations_;
 };
 

verilog/analysis/checkers/undersized_binary_literal_rule_test.cc

@@ -1,4 +1,4 @@
-// Copyright 2017-2020 The Verible Authors.
+// Copyright 2017-2021 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.
@@ -29,9 +29,17 @@ namespace analysis {
 namespace {
 
 using verible::LintTestCase;
-using verible::RunLintTestCases;
+using verible::RunConfiguredLintTestCases;
 
-TEST(UndersizedBinaryLiteralTest, FunctionFailures) {
+TEST(UndersizedBinaryLiteralTest, ConfigurationPass) {
+  UndersizedBinaryLiteralRule rule;
+  absl::Status status;
+  EXPECT_TRUE((status = rule.Configure("")).ok()) << status.message();
+  EXPECT_TRUE((status = rule.Configure("bin:true;oct:true;hex:true")).ok())
+      << status.message();
+}
+
+TEST(UndersizedBinaryLiteralTest, TooShortBinaryNumbers) {
   constexpr int kToken = TK_BinDigits;
   const std::initializer_list<LintTestCase> kTestCases = {
       {""},
@@ -50,10 +58,7 @@ TEST(UndersizedBinaryLiteralTest, FunctionFailures) {
       {"localparam x = 2'b ", {kToken, "_1_"}, ";"},  // with underscores
       {"localparam x = 1'b0;"},
       {"localparam x = 1'b1;"},
-      {"localparam x = 2'h1;"},
-      {"localparam x = 2'habcd;"},
-      {"localparam x = 32'd20;"},
-      {"localparam x = 16'o7;"},
+      {"localparam x = 32'd20;"},  // decimal numbers not treated
       {"localparam x = 8'b 0001_1000;"},
       {"localparam x = 8'b ", {kToken, "001_1000"}, ";"},
       {"localparam x = 8'b ", {kToken, "0001_100"}, ";"},
@@ -67,7 +72,101 @@ TEST(UndersizedBinaryLiteralTest, FunctionFailures) {
       {"localparam x = 5'b", {kToken, "??"}, ";"},  // only 2 ?s for 5 bits
   };
 
-  RunLintTestCases<VerilogAnalyzer, UndersizedBinaryLiteralRule>(kTestCases);
+  RunConfiguredLintTestCases<VerilogAnalyzer, UndersizedBinaryLiteralRule>(
+      kTestCases, "bin:true");
+}
+
+TEST(UndersizedBinaryLiteralTest, BinaryNumbersConfiguredDontCare) {
+  const std::initializer_list<LintTestCase> kTestCases = {
+      {"localparam x = 0;"},
+      {"localparam x = 1;"},
+      {"localparam x = 3'b00;"},
+      {"localparam x = 32'b000;"},
+  };
+
+  RunConfiguredLintTestCases<VerilogAnalyzer, UndersizedBinaryLiteralRule>(
+      kTestCases, "bin:false");
+}
+
+TEST(UndersizedBinaryLiteralTest, TooShortHexNumbers) {
+  constexpr int kToken = TK_HexDigits;
+  const std::initializer_list<LintTestCase> kTestCases = {
+      {"localparam x = 16'h0;"},  // Exception granted for single 0 and ?
+      {"localparam x = 16'h?;"},
+
+      {"localparam x = 16'h", {kToken, "00"}, ";"},
+      {"localparam x = 16'h", {kToken, "??"}, ";"},
+
+      {"localparam x = 1'h1;"},
+      {"localparam x = 4'hf;"},
+      {"localparam x = 5'h", {kToken, "f"}, ";"},
+      {"localparam x = 5'h1f;"},
+      {"localparam x = 16'h0001;"},
+      {"localparam x = 16'h00_01;"},
+      {"localparam x = 16'h", {kToken, "001"}, ";"},
+      {"localparam x = 16'h", {kToken, "0_01"}, ";"},
+      {"localparam x = 2'habcd;"},  // Note: truncated values are ok for this
+                                    // rule
+  };
+
+  RunConfiguredLintTestCases<VerilogAnalyzer, UndersizedBinaryLiteralRule>(
+      kTestCases, "hex:true");
+}
+
+TEST(UndersizedBinaryLiteralTest, HexNumbersConfiguredDontCare) {
+  const std::initializer_list<LintTestCase> kTestCases = {
+      {"localparam x = 16'h0;"},
+      {"localparam x = 16'h?;"},
+      {"localparam x = 5'hf;"},
+      {"localparam x = 16'h001;"},
+  };
+
+  RunConfiguredLintTestCases<VerilogAnalyzer, UndersizedBinaryLiteralRule>(
+      kTestCases, "hex:false");
+}
+
+TEST(UndersizedBinaryLiteralTest, TooShortOctalNumbers) {
+  constexpr int kToken = TK_OctDigits;
+  const std::initializer_list<LintTestCase> kTestCases = {
+      {"localparam x = 12'o0;"},  // Exception granted for 0 and ?
+      {"localparam x = 12'o?;"},
+
+      {"localparam x = 12'o", {kToken, "00"}, ";"},
+      {"localparam x = 12'o", {kToken, "??"}, ";"},
+
+      {"localparam x = 1'o1;"},
+      {"localparam x = 3'o7;"},
+      {"localparam x = 8'o777;"},  // Note: truncated values are ok for this
+                                   // rule
+      {"localparam x = 9'o777;"},
+      {"localparam x = 9'o7_7_7;"},
+      {"localparam x = 9'o", {kToken, "77"}, ";"},
+      {"localparam x = 9'o", {kToken, "7_7"}, ";"},
+      {"localparam x = 4'o17;"},
+  };
+
+  RunConfiguredLintTestCases<VerilogAnalyzer, UndersizedBinaryLiteralRule>(
+      kTestCases, "oct:true");
+}
+
+TEST(UndersizedBinaryLiteralTest, OctalNumbersConfiguredDontCare) {
+  const std::initializer_list<LintTestCase> kTestCases = {
+      {"localparam x = 9'o77;"},
+      {"localparam x = 12'o77;"},
+  };
+
+  RunConfiguredLintTestCases<VerilogAnalyzer, UndersizedBinaryLiteralRule>(
+      kTestCases, "oct:false");
+}
+
+TEST(UndersizedBinaryLiteralTest, DecimalNumbersNeverCare) {
+  const std::initializer_list<LintTestCase> kTestCases = {
+      {"localparam x = 32'd42;"},
+      {"localparam x = 32'd123456789;"},
+  };
+
+  RunConfiguredLintTestCases<VerilogAnalyzer, UndersizedBinaryLiteralRule>(
+      kTestCases, "");
 }
 
 }  // namespace