Add new lint rule for negative array dimensions #1835

IEncinas10 · 2023-03-26T12:31:14Z

Attempt to fix #382. I'm sure the code could use some improvements, so happy to accomodate any feedback!

Edit:

Sorry for the several updates, after making the pull request I realized that there are some corner cases (parenthesis, ...) so I tried to fix some of those. It still won't pick up things like +(-5) but I can't see an easy way to fix that.

codecov-commenter · 2023-03-26T12:50:03Z

Codecov Report

Patch coverage: 97.22% and project coverage change: +0.01 🎉

Comparison is base (bb28b0b) 93.08% compared to head (ab991cf) 93.09%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1835      +/-   ##
==========================================
+ Coverage   93.08%   93.09%   +0.01%     
==========================================
  Files         353      354       +1     
  Lines       25656    25723      +67     
==========================================
+ Hits        23881    23946      +65     
- Misses       1775     1777       +2

Impacted Files	Coverage Δ
verilog/CST/expression.h	`100.00% <ø> (ø)`
verilog/CST/expression.cc	`95.89% <93.33%> (-0.67%)`	⬇️
...log/analysis/checkers/forbid_negative_array_dim.cc	`100.00% <100.00%> (ø)`

... and 8 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

fangism

Thank you for contributing this!

fangism · 2023-03-31T17:50:16Z

verilog/analysis/checkers/forbid_negative_array_dim.cc

+  if (UnaryPrefixExprMatcher().Matches(symbol, &manager)) {
+    const auto& node = SymbolCastToNode(symbol);
+    const auto& children = node.children();
+    const auto& leaf_symbol = *children[0].get();


I recommend defining a named function for anything that indexes into the list of childre.
e.g. add a GetUnaryPrefixExprOperator to https://cs.opensource.google/verible/verible/+/master:verilog/CST/expression.h

Incidentally, what I'm requesting here is aligned with #159

Went ahead and added the suggested functions. Let me know if they're ok or if you think they could use some more work.

fangism · 2023-03-31T17:50:57Z

verilog/analysis/checkers/forbid_negative_array_dim.cc

+    if (children.size() <= 1) return;
+
+    int value = -1;
+    const auto& child_node = children[1];


Similar suggestion, maybe call this GetUnaryPrefixExprOperand in https://cs.opensource.google/verible/verible/+/master:verilog/CST/expression.h

fangism · 2023-03-31T17:51:39Z

verilog/analysis/checkers/forbid_negative_array_dim.cc

+    const auto& child_node = children[1];
+    const bool is_constant = ConstantIntegerValue(*child_node.get(), &value);
+
+    const verible::TokenInfo token(TK_OTHER,


This can be declared inside the following if body, because it is only used there.

fangism · 2023-03-31T17:52:32Z

verilog/analysis/checkers/forbid_negative_array_dim.h

+namespace verilog {
+namespace analysis {
+
+class ForbidNegativeArrayDim : public verible::SyntaxTreeLintRule {


Recommend documenting what this class does here, with some simple examples.

fangism · 2023-03-31T17:52:59Z

verilog/analysis/checkers/forbid_negative_array_dim.h

@@ -0,0 +1,48 @@
+// Copyright 2017-2020 The Verible Authors.


might as well make this 2023 (other files too)

Done! Updated also expression.cc and expression.h files

fangism · 2023-03-31T17:56:02Z

verilog/analysis/checkers/forbid_negative_array_dim_test.cc

+      {"logic l [10+(-5):0];"},
+      {"logic l [(", {kScalar, "-5"}, "):0];"},
+      {"logic l [-(-5):0];"},
+  };


Suggested cases:

foo_type [a:b] name [c:d]

declaration inside module ports

declaration inside module body

declaration inside class

local declaration inside function

Add auxiliary functions for kUnaryExpressions Add more test cases Update copyright

Add auxiliary functions for kUnaryExpressions Add more test cases Update copyright for modified files

hzeller · 2023-03-31T21:32:22Z

verilog/CST/expression.cc

@@ -98,6 +98,19 @@ const verible::Symbol* GetConditionExpressionFalseCase(
  return GetSubtreeAsSymbol(condition_expr, NodeEnum::kConditionExpression, 4);
 }

+const verible::TokenInfo* GetUnaryPrefixOperator(
+    const verible::Symbol& symbol) {
+  const auto& children = verible::SymbolCastToNode(symbol).children();


if symbol does not happen to be a node, then this will crash. You probably want to return nullptr in case symbol.Kind() is not a node.
Or you could test that it is a kUnaryPrefixExpression, which is a better, more specific, test.

(Also add a test for it to see that other nodes than kUnaryPrefixExpressions always return nullptr).

Added the checks in both functions, thanks pointing it out.

hzeller · 2023-03-31T21:33:47Z

verilog/CST/expression.cc

+const verible::TokenInfo* GetUnaryPrefixOperator(
+    const verible::Symbol& symbol) {
+  const auto& children = verible::SymbolCastToNode(symbol).children();
+  const auto leaf_symbol = children.front().get();


In general, we try to avoid using auto unless whatever comes back is a huge templated type which does not add any benefit for the reader. So use the type that is returned.

Got it, thanks! Used auto because I saw it in use and thought that was the general style, looks much better now.

hzeller · 2023-03-31T21:34:29Z

verilog/CST/expression.cc

+    const verible::Symbol& symbol) {
+  const auto& children = verible::SymbolCastToNode(symbol).children();
+  const auto leaf_symbol = children.front().get();
+  return &verible::down_cast<const verible::SyntaxTreeLeaf*>(leaf_symbol)


Do we know that leaf_symbol we get is a SyntaxTreeLeaf so that we can safely cast ?

I assumed every kUnaryPrefixExpression has the following form

Node @0 (tag: kUnaryPrefixExpression) { Leaf @0 /* ... */ Node @1 /* ... /*

So I guess that if we're sure the current node is a kUnaryPrefixExpression we can assume the cast will work always? Let me know if I'm wrong.

At the time of the writing of this comment, we only got verible::Symbol, so didn't know that yet.

hzeller · 2023-03-31T21:35:11Z

verilog/CST/expression.h

@@ -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


Please add tests for the new functions in expression_test.cc.

Done. Let me know if they're thorough enough.

hzeller · 2023-03-31T21:37:48Z

verilog/analysis/checkers/forbid_negative_array_dim.cc

+    const auto operand = verilog::GetUnaryPrefixOperand(symbol);
+
+    const bool is_constant = verilog::ConstantIntegerValue(*operand, &value);
+    if (is_constant > 0 && value > 0 && u_operator->text() == "-") {


is_constant is a bool, so using > on it is confusing.

Yes, sorry about that. Removed "value > 0" check, noticed it was needed and when I added it back I somehow made a mess.

hzeller · 2023-03-31T21:41:31Z

verilog/analysis/checkers/forbid_negative_array_dim.cc

+    // Extract operand and operator from kUnaryPrefixExpression
+    const auto u_operator = verilog::GetUnaryPrefixOperator(symbol);
+    const auto operand = verilog::GetUnaryPrefixOperand(symbol);
+


If we're at this point, did the matcher make sure that u_operator and operand are never nullptr ?

As the matcher ensures that symbol is a kUnaryPrefixExpression, I would expect that they would never be nullptr (if my asumption about the general format for every kUnaryPrefixExpression is correct). If I'm wrong, I would just add an early return if either operand or u_operator are nullptr.

I think it is sufficient if you add that as a comment; something like:

// symbol is a UnaryPrefixExpression, so operator and operand are defined

- Change auto for the actual types - Add tests for new functions for unary expressions - Add checks for correct input type for new functions. Return nullptr when appropiate - Remove typo when checking boolean value (redundant > 0)

hzeller

Nice. A few more comments.

hzeller · 2023-04-04T00:43:29Z

verilog/CST/expression.cc

+    const verible::Symbol& symbol) {
+  const auto& children = verible::SymbolCastToNode(symbol).children();
+  const auto leaf_symbol = children.front().get();
+  return &verible::down_cast<const verible::SyntaxTreeLeaf*>(leaf_symbol)


At the time of the writing of this comment, we only got verible::Symbol, so didn't know that yet.

hzeller · 2023-04-04T00:46:19Z

verilog/CST/expression.cc

@@ -98,6 +98,25 @@ const verible::Symbol* GetConditionExpressionFalseCase(
  return GetSubtreeAsSymbol(condition_expr, NodeEnum::kConditionExpression, 4);
 }

+const verible::TokenInfo* GetUnaryPrefixOperator(
+    const verible::Symbol& symbol) {
+  const SyntaxTreeNode* node = &verible::SymbolCastToNode(symbol);


this will crash if symbol is not a node.

Check added.

hzeller · 2023-04-04T00:48:30Z

verilog/CST/expression_test.cc

+    }
+  }
+}
+


Also add tests where Symbol is not a unary prefix expression. What if symbol is a leaf ?
Having test cases with broken input helps to make sure we handle that gracefully.

I added calls to the GetUnaryPrefix... functions with wrong input and check they return null pointer

hzeller · 2023-04-04T00:50:11Z

verilog/analysis/checkers/forbid_negative_array_dim.cc

+    // Extract operand and operator from kUnaryPrefixExpression
+    const auto u_operator = verilog::GetUnaryPrefixOperator(symbol);
+    const auto operand = verilog::GetUnaryPrefixOperand(symbol);
+


I think it is sufficient if you add that as a comment; something like:

// symbol is a UnaryPrefixExpression, so operator and operand are defined

hzeller · 2023-04-04T00:52:16Z

verilog/analysis/checkers/forbid_negative_array_dim.cc

+  }
+
+  verible::matcher::BoundSymbolManager manager;
+  if (UnaryPrefixExprMatcher().Matches(symbol, &manager)) {


If this is not matching, we can early out here. Saves one level of indentation and reduces the stack-size the reader of this code needs :)

if (!UnaryPrefixExprMatcher().Matches(symbol, &manager)) return;

... everything that follows implies that we have a unary prefix expression.

Done, thanks for pointing that out

- Perform check before doing SymbolCastToNode - Call function with broken input (leafs, ...) - Add comments, style cleanup.

hzeller · 2023-04-14T23:22:18Z

verilog/analysis/checkers/forbid_negative_array_dim_test.cc

+      /* Can't detect this at the moment */
+      {"logic l [+(-5):0];"},
+      /* Packed */
+      {"logic [", {kScalar, "-10"}, ":-0] l;"},


I am wondering, should there be a configuration for the packed and unpacked ranges ?
Maybe a particular style allows to set a bitwidth with negative values but not array sizes.

Something to discuss in #382, then a potential implementation that distinguishes the ranges in a follow-up PR though.

hzeller

LGTM, @fangism any additional comments ?

fangism

This looks great, thank you for your hard work!

IEncinas10 · 2023-04-14T23:41:53Z

Thank you both! My pleasure, I'll search for other issues feasible for my with my little knowledge of the codebase. Feel free to point me in any direction that might be of use to you!

Thanks again, enjoy the weekend and thanks for the great project you manage!

IEncinas10 force-pushed the forbid_negative_array_dim branch from f77f0ca to 9f96c9f Compare March 26, 2023 13:56

Add new lint rule for negative array dimensions

2e9e8d3

IEncinas10 force-pushed the forbid_negative_array_dim branch from 9f96c9f to 2e9e8d3 Compare March 26, 2023 14:23

Fix clang-tidy warning

d525d42

fangism requested changes Mar 31, 2023

View reviewed changes

IEncinas10 added 2 commits March 31, 2023 21:51

Address PR review

e98eff3

Add auxiliary functions for kUnaryExpressions Add more test cases Update copyright

Address PR review

b8e4591

Add auxiliary functions for kUnaryExpressions Add more test cases Update copyright for modified files

hzeller reviewed Mar 31, 2023

View reviewed changes

Address 2nd review

820e8f8

- Change auto for the actual types - Add tests for new functions for unary expressions - Add checks for correct input type for new functions. Return nullptr when appropiate - Remove typo when checking boolean value (redundant > 0)

IEncinas10 force-pushed the forbid_negative_array_dim branch from 431b9d1 to 820e8f8 Compare April 1, 2023 07:46

hzeller reviewed Apr 4, 2023

View reviewed changes

Address third row of comments:

ab991cf

- Perform check before doing SymbolCastToNode - Call function with broken input (leafs, ...) - Add comments, style cleanup.

hzeller reviewed Apr 14, 2023

View reviewed changes

hzeller requested a review from fangism April 14, 2023 23:23

fangism approved these changes Apr 14, 2023

View reviewed changes

hzeller merged commit 525ffaf into chipsalliance:master Apr 15, 2023

IEncinas10 deleted the forbid_negative_array_dim branch April 17, 2023 21:35

IEncinas10 mentioned this pull request Nov 21, 2023

How to use rule for having a begin and end #2040

Closed

Add new lint rule for negative array dimensions #1835

Add new lint rule for negative array dimensions #1835

Conversation

IEncinas10 commented Mar 26, 2023 • edited Loading

codecov-commenter commented Mar 26, 2023 • edited Loading

Codecov Report

fangism left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IEncinas10 Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzeller left a comment

Choose a reason for hiding this comment

fangism left a comment

Choose a reason for hiding this comment

IEncinas10 commented Apr 14, 2023

IEncinas10 commented Mar 26, 2023 •

edited

Loading

codecov-commenter commented Mar 26, 2023 •

edited

Loading

IEncinas10 Apr 4, 2023 •

edited

Loading