diff --git a/.github/workflows/verible-ci.yml b/.github/workflows/verible-ci.yml index 2512fae6b..333b809cd 100644 --- a/.github/workflows/verible-ci.yml +++ b/.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 diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index d10874622..c66e05926 100644 --- a/verilog/analysis/checkers/BUILD +++ b/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", diff --git a/verilog/analysis/checkers/undersized_binary_literal_rule.cc b/verilog/analysis/checkers/undersized_binary_literal_rule.cc index 5076501b6..1062b7287 100644 --- a/verilog/analysis/checkers/undersized_binary_literal_rule.cc +++ b/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("width")) { - if (auto base_leaf = manager.GetAs("base")) { - if (auto digits_leaf = manager.GetAs("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("width"); + const auto literal_node = manager.GetAs("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(base_digit_part[0].get()); + auto digits_leaf = down_cast(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 { diff --git a/verilog/analysis/checkers/undersized_binary_literal_rule.h b/verilog/analysis/checkers/undersized_binary_literal_rule.h index e1f320b6f..f52e4c0ec 100644 --- a/verilog/analysis/checkers/undersized_binary_literal_rule.h +++ b/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 violations_; }; diff --git a/verilog/analysis/checkers/undersized_binary_literal_rule_test.cc b/verilog/analysis/checkers/undersized_binary_literal_rule_test.cc index eb24a21b4..c974ee354 100644 --- a/verilog/analysis/checkers/undersized_binary_literal_rule_test.cc +++ b/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 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(kTestCases); + RunConfiguredLintTestCases( + kTestCases, "bin:true"); +} + +TEST(UndersizedBinaryLiteralTest, BinaryNumbersConfiguredDontCare) { + const std::initializer_list kTestCases = { + {"localparam x = 0;"}, + {"localparam x = 1;"}, + {"localparam x = 3'b00;"}, + {"localparam x = 32'b000;"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "bin:false"); +} + +TEST(UndersizedBinaryLiteralTest, TooShortHexNumbers) { + constexpr int kToken = TK_HexDigits; + const std::initializer_list 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( + kTestCases, "hex:true"); +} + +TEST(UndersizedBinaryLiteralTest, HexNumbersConfiguredDontCare) { + const std::initializer_list kTestCases = { + {"localparam x = 16'h0;"}, + {"localparam x = 16'h?;"}, + {"localparam x = 5'hf;"}, + {"localparam x = 16'h001;"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "hex:false"); +} + +TEST(UndersizedBinaryLiteralTest, TooShortOctalNumbers) { + constexpr int kToken = TK_OctDigits; + const std::initializer_list 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( + kTestCases, "oct:true"); +} + +TEST(UndersizedBinaryLiteralTest, OctalNumbersConfiguredDontCare) { + const std::initializer_list kTestCases = { + {"localparam x = 9'o77;"}, + {"localparam x = 12'o77;"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "oct:false"); +} + +TEST(UndersizedBinaryLiteralTest, DecimalNumbersNeverCare) { + const std::initializer_list kTestCases = { + {"localparam x = 32'd42;"}, + {"localparam x = 32'd123456789;"}, + }; + + RunConfiguredLintTestCases( + kTestCases, ""); } } // namespace