Merge pull request #1835 from IEncinas10/forbid_negative_array_dim

Add new lint rule for negative array dimensions

verilog/CST/expression.cc

@@ -1,4 +1,4 @@
-// Copyright 2017-2020 The Verible Authors.
+// Copyright 2017-2023 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.
@@ -98,6 +98,29 @@ const verible::Symbol* GetConditionExpressionFalseCase(
   return GetSubtreeAsSymbol(condition_expr, NodeEnum::kConditionExpression, 4);
 }
 
+const verible::TokenInfo* GetUnaryPrefixOperator(
+    const verible::Symbol& symbol) {
+  const SyntaxTreeNode* node = symbol.Kind() == SymbolKind::kNode
+                                   ? &verible::SymbolCastToNode(symbol)
+                                   : nullptr;
+  if (!node || !MatchNodeEnumOrNull(*node, NodeEnum::kUnaryPrefixExpression)) {
+    return nullptr;
+  }
+  const verible::Symbol* leaf_symbol = node->children().front().get();
+  return &verible::down_cast<const verible::SyntaxTreeLeaf*>(leaf_symbol)
+              ->get();
+}
+
+const verible::Symbol* GetUnaryPrefixOperand(const verible::Symbol& symbol) {
+  const SyntaxTreeNode* node = symbol.Kind() == SymbolKind::kNode
+                                   ? &verible::SymbolCastToNode(symbol)
+                                   : nullptr;
+  if (!node || !MatchNodeEnumOrNull(*node, NodeEnum::kUnaryPrefixExpression)) {
+    return nullptr;
+  }
+  return node->children().back().get();
+}
+
 std::vector<verible::TreeSearchMatch> FindAllBinaryOperations(
     const verible::Symbol& root) {
   return verible::SearchSyntaxTree(root, NodekBinaryExpression());

verilog/CST/expression.h

@@ -1,4 +1,4 @@
-// Copyright 2017-2020 The Verible Authors.
+// Copyright 2017-2023 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.
@@ -81,6 +81,12 @@ const verible::Symbol* GetConditionExpressionTrueCase(const verible::Symbol&);
 // Returns the false-case expression of a kConditionExpression.
 const verible::Symbol* GetConditionExpressionFalseCase(const verible::Symbol&);
 
+// Returns the operator of a kUnaryPrefixExpression
+const verible::TokenInfo* GetUnaryPrefixOperator(const verible::Symbol&);
+
+// Returns the operand of a kUnaryPrefixExpression
+const verible::Symbol* GetUnaryPrefixOperand(const verible::Symbol&);
+
 // From binary expression operations, e.g. "a + b".
 // Associative binary operators may span more than two operands, e.g. "a+b+c".
 std::vector<verible::TreeSearchMatch> FindAllBinaryOperations(

verilog/CST/expression_test.cc

@@ -1,4 +1,4 @@
-// Copyright 2017-2020 The Verible Authors.
+// Copyright 2017-2023 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.
@@ -423,6 +423,61 @@ TEST(GetConditionExpressionFalseCaseTest, Various) {
   }
 }
 
+TEST(GetUnaryPrefixOperator, Exprs) {
+  const std::pair<const char*, const char*> kTestCases[] = {
+      {"-(2)", "-"},    {"-1", "-"},        {"&1", "&"},
+      {"666", nullptr}, {"1 + 2", nullptr}, {"!1", "!"},
+  };
+  for (auto test : kTestCases) {
+    const auto analyzer_ptr =
+        AnalyzeVerilogExpression(test.first, "<file>", kDefaultPreprocess);
+    const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree();
+    const auto tag = node->Tag();
+    EXPECT_EQ(tag.kind, verible::SymbolKind::kNode);
+    EXPECT_EQ(NodeEnum(tag.tag), NodeEnum::kExpression);
+    const verible::Symbol* last_node = DescendThroughSingletons(*node);
+
+    if (test.second) {
+      const verible::SyntaxTreeNode& unary_expr =
+          verible::SymbolCastToNode(*last_node);
+      EXPECT_EQ(NodeEnum(unary_expr.Tag().tag),
+                NodeEnum::kUnaryPrefixExpression);
+      EXPECT_EQ(test.second, GetUnaryPrefixOperator(unary_expr)->text());
+    } else {
+      EXPECT_NE(NodeEnum(last_node->Tag().tag),
+                NodeEnum::kUnaryPrefixExpression);
+      EXPECT_FALSE(GetUnaryPrefixOperand(*last_node));
+    }
+  }
+}
+
+TEST(GetUnaryPrefixOperand, Exprs) {
+  const std::pair<const char*, const char*> kTestCases[] = {
+      {"-1", ""}, {"&1", ""}, {"666", nullptr}, {"1 + 2", nullptr}, {"!1", ""},
+  };
+  for (auto test : kTestCases) {
+    const auto analyzer_ptr =
+        AnalyzeVerilogExpression(test.first, "<file>", kDefaultPreprocess);
+    const auto& node = ABSL_DIE_IF_NULL(analyzer_ptr)->SyntaxTree();
+    const auto tag = node->Tag();
+    EXPECT_EQ(tag.kind, verible::SymbolKind::kNode);
+    EXPECT_EQ(NodeEnum(tag.tag), NodeEnum::kExpression);
+    const verible::Symbol* last_node = DescendThroughSingletons(*node);
+
+    if (test.second) {
+      const verible::SyntaxTreeNode& unary_expr =
+          verible::SymbolCastToNode(*last_node);
+      EXPECT_EQ(NodeEnum(unary_expr.Tag().tag),
+                NodeEnum::kUnaryPrefixExpression);
+      EXPECT_TRUE(GetUnaryPrefixOperand(unary_expr));
+    } else {
+      EXPECT_NE(NodeEnum(last_node->Tag().tag),
+                NodeEnum::kUnaryPrefixExpression);
+      EXPECT_FALSE(GetUnaryPrefixOperand(*last_node));
+    }
+  }
+}
+
 TEST(FindAllConditionExpressionsTest, Various) {
   constexpr int kTag = 1;  // value doesn't matter
   const SyntaxTreeSearchTestCase kTestCases[] = {

verilog/analysis/checkers/BUILD

@@ -33,6 +33,7 @@ cc_library(
         ":forbidden_anonymous_structs_unions_rule",
         ":forbidden_macro_rule",
         ":forbidden_symbol_rule",
+        ":forbid_negative_array_dim",
         ":generate_label_prefix_rule",
         ":generate_label_rule",
         ":interface_name_style_rule",
@@ -1380,6 +1381,45 @@ cc_test(
     ],
 )
 
+cc_library(
+    name = "forbid_negative_array_dim",
+    srcs = ["forbid_negative_array_dim.cc"],
+    hdrs = ["forbid_negative_array_dim.h"],
+    deps = [
+        "//common/analysis:lint_rule_status",
+        "//common/analysis:syntax_tree_lint_rule",
+        "//common/analysis/matcher",
+        "//common/analysis/matcher:bound_symbol_manager",
+        "//common/text:symbol",
+        "//common/text:syntax_tree_context",
+        "//common/text:tree_utils",
+        "//common/util:logging",
+        "//verilog/CST:context_functions",
+        "//verilog/CST:dimensions",
+        "//verilog/CST:expression",
+        "//verilog/CST:verilog_matchers",
+        "//verilog/analysis:descriptions",
+        "//verilog/analysis:lint_rule_registry",
+        "@com_google_absl//absl/strings",
+    ],
+    alwayslink = 1,
+)
+
+cc_test(
+    name = "forbid_negative_array_dim_test",
+    srcs = ["forbid_negative_array_dim_test.cc"],
+    deps = [
+        ":forbid_negative_array_dim",
+        "//common/analysis:linter_test_utils",
+        "//common/analysis:syntax_tree_linter_test_utils",
+        "//common/text:symbol",
+        "//verilog/CST:verilog_nonterminals",
+        "//verilog/analysis:verilog_analyzer",
+        "//verilog/parser:verilog_token_enum",
+        "@com_google_googletest//:gtest_main",
+    ],
+)
+
 cc_library(
     name = "case_missing_default_rule",
     srcs = ["case_missing_default_rule.cc"],

verilog/analysis/checkers/forbid_negative_array_dim.cc

@@ -0,0 +1,96 @@
+// Copyright 2017-2023 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.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "verilog/analysis/checkers/forbid_negative_array_dim.h"
+
+#include <set>
+#include <string>
+
+#include "absl/strings/str_cat.h"
+#include "absl/strings/string_view.h"
+#include "common/analysis/lint_rule_status.h"
+#include "common/analysis/matcher/bound_symbol_manager.h"
+#include "common/analysis/matcher/matcher.h"
+#include "common/text/symbol.h"
+#include "common/text/syntax_tree_context.h"
+#include "common/text/tree_utils.h"
+#include "common/util/logging.h"
+#include "verilog/CST/context_functions.h"
+#include "verilog/CST/dimensions.h"
+#include "verilog/CST/expression.h"
+#include "verilog/CST/verilog_matchers.h"
+#include "verilog/analysis/descriptions.h"
+#include "verilog/analysis/lint_rule_registry.h"
+
+namespace verilog {
+namespace analysis {
+
+using verible::matcher::Matcher;
+
+// Register the lint rule
+VERILOG_REGISTER_LINT_RULE(ForbidNegativeArrayDim);
+
+static constexpr absl::string_view kMessage =
+    "Avoid using negative constant literals for array dimensions.";
+
+const LintRuleDescriptor& ForbidNegativeArrayDim::GetDescriptor() {
+  static const LintRuleDescriptor d{
+      .name = "forbid-negative-array-dim",
+      .topic = "forbid-negative-array-dim",
+      .desc = "Check for negative constant literals inside array dimensions.",
+  };
+  return d;
+}
+
+// Matches the begin node.
+static const Matcher& UnaryPrefixExprMatcher() {
+  static const Matcher matcher(NodekUnaryPrefixExpression());
+  return matcher;
+}
+
+void ForbidNegativeArrayDim::HandleSymbol(
+    const verible::Symbol& symbol, const verible::SyntaxTreeContext& context) {
+  // This only works for simple unary expressions. They can't be nested inside
+  // other expressions. This avoids false positives of the form:
+  // logic l [10+(-5):0], logic l[-(-5):0]
+  if (!context.IsInsideFirst(
+          {NodeEnum::kPackedDimensions, NodeEnum::kUnpackedDimensions},
+          {NodeEnum::kBinaryExpression, NodeEnum::kUnaryPrefixExpression})) {
+    return;
+  }
+
+  verible::matcher::BoundSymbolManager manager;
+  if (!UnaryPrefixExprMatcher().Matches(symbol, &manager)) return;
+
+  // As we've previously ensured that this symbol is a kUnaryPrefixExpression
+  // both its operator and operand are defined
+  const verible::TokenInfo* u_operator =
+      verilog::GetUnaryPrefixOperator(symbol);
+  const verible::Symbol* operand = verilog::GetUnaryPrefixOperand(symbol);
+
+  int value = 0;
+  const bool is_constant = verilog::ConstantIntegerValue(*operand, &value);
+  if (is_constant && value > 0 && u_operator->text() == "-") {
+    const verible::TokenInfo token(TK_OTHER,
+                                   verible::StringSpanOfSymbol(symbol));
+    violations_.insert(verible::LintViolation(token, kMessage, context));
+  }
+}
+
+verible::LintRuleStatus ForbidNegativeArrayDim::Report() const {
+  return verible::LintRuleStatus(violations_, GetDescriptor());
+}
+
+}  // namespace analysis
+}  // namespace verilog

verilog/analysis/checkers/forbid_negative_array_dim.h

@@ -0,0 +1,57 @@
+// Copyright 2017-2023 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.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_FORBID_NEGATIVE_ARRAY_DIM_RULE_H_
+#define VERIBLE_VERILOG_ANALYSIS_CHECKERS_FORBID_NEGATIVE_ARRAY_DIM_RULE_H_
+
+#include <set>
+#include <string>
+
+#include "common/analysis/lint_rule_status.h"
+#include "common/analysis/syntax_tree_lint_rule.h"
+#include "common/text/symbol.h"
+#include "common/text/syntax_tree_context.h"
+#include "verilog/analysis/descriptions.h"
+
+namespace verilog {
+namespace analysis {
+/*
+ * Check for negative sizes inside array dimensions. It only
+ * detects constant literals at the moment.
+ *
+ * Examples:
+ *   logic l [-1:0];
+ *   logic [-1:0] l;
+ *
+ * See forbid_negative_array_dim_test.cc for more examples.
+ */
+class ForbidNegativeArrayDim : public verible::SyntaxTreeLintRule {
+ public:
+  using rule_type = verible::SyntaxTreeLintRule;
+
+  static const LintRuleDescriptor& GetDescriptor();
+
+  void HandleSymbol(const verible::Symbol& symbol,
+                    const verible::SyntaxTreeContext& context) final;
+
+  verible::LintRuleStatus Report() const final;
+
+ private:
+  std::set<verible::LintViolation> violations_;
+};
+
+}  // namespace analysis
+}  // namespace verilog
+
+#endif  // VERIBLE_VERILOG_ANALYSIS_CHECKERS_FORBID_NEGATIVE_ARRAY_DIM_RULE_H_