From ccd529d693f458079fe58cbbb025c7e2024b1869 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Fri, 5 Apr 2024 23:16:59 +1100 Subject: [PATCH 01/12] Pull request https://github.com/chipsalliance/verible/pull/1336 seems to have stalled. Copied the code from https://github.com/suzizecat/verible/commit/0b087d72cfa58e9e8c34a164d4abaeda9880ca7d and rebased it to the head. Updated the tests and formatted. --- verilog/analysis/checkers/BUILD | 32 ++++ .../analysis/checkers/explicit_begin_rule.cc | 159 ++++++++++++++++++ .../analysis/checkers/explicit_begin_rule.h | 67 ++++++++ .../checkers/explicit_begin_rule_test.cc | 93 ++++++++++ 4 files changed, 351 insertions(+) create mode 100644 verilog/analysis/checkers/explicit_begin_rule.cc create mode 100644 verilog/analysis/checkers/explicit_begin_rule.h create mode 100644 verilog/analysis/checkers/explicit_begin_rule_test.cc diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index dc1177397..5ded24a4b 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -24,6 +24,7 @@ cc_library( ":disable-statement-rule", ":endif-comment-rule", ":enum-name-style-rule", + ":explicit-begin-rule", ":explicit-function-lifetime-rule", ":explicit-function-task-parameter-type-rule", ":explicit-parameter-storage-type-rule", @@ -1249,6 +1250,37 @@ cc_test( ], ) +cc_library( + name = "explicit-begin-rule", + srcs = ["explicit_begin_rule.cc"], + hdrs = ["explicit_begin_rule.h"], + deps = [ + "//common/analysis:lint-rule-status", + "//common/analysis:token-stream-lint-rule", + "//common/strings:comment-utils", + "//common/text:token-info", + "//verilog/analysis:descriptions", + "//verilog/analysis:lint-rule-registry", + "//verilog/parser:verilog-token-enum", + "@com_google_absl//absl/strings", + ], + alwayslink = 1, +) + +cc_test( + name = "explicit-begin-rule_test", + srcs = ["explicit_begin_rule_test.cc"], + deps = [ + ":explicit-begin-rule", + "//common/analysis:linter-test-utils", + "//common/analysis:token-stream-linter-test-utils", + "//common/text:symbol", + "//verilog/analysis:verilog-analyzer", + "//verilog/parser:verilog-token-enum", + "@com_google_googletest//:gtest_main", + ], +) + cc_library( name = "explicit-function-lifetime-rule", srcs = ["explicit_function_lifetime_rule.cc"], diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc new file mode 100644 index 000000000..bf1dde2d4 --- /dev/null +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -0,0 +1,159 @@ +// Copyright 2017-2020 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/explicit_begin_rule.h" + +#include +#include +#include +#include + +#include "absl/strings/str_cat.h" +#include "absl/strings/string_view.h" +#include "common/analysis/lint_rule_status.h" +#include "common/analysis/token_stream_lint_rule.h" +#include "common/strings/comment_utils.h" +#include "common/text/token_info.h" +#include "verilog/analysis/descriptions.h" +#include "verilog/analysis/lint_rule_registry.h" +#include "verilog/parser/verilog_token_enum.h" + +namespace verilog { +namespace analysis { + +using verible::AutoFix; +using verible::LintRuleStatus; +using verible::LintViolation; +using verible::TokenInfo; +using verible::TokenStreamLintRule; + +// Register the lint rule +VERILOG_REGISTER_LINT_RULE(ExplicitBeginRule); + +static const char kMessage[] = + "All block construct shall explicitely use begin/end."; + +const LintRuleDescriptor &ExplicitBeginRule::GetDescriptor() { + static const LintRuleDescriptor d{ + .name = "explicit-begin", + .topic = "explicit-begin", + .desc = + "Checks that a Verilog ``begin`` directive follows all " + "if, else and for loops.", + }; + return d; +} + +void ExplicitBeginRule::HandleToken(const TokenInfo &token) { + // Responds to a token by updating the state of the analysis. + bool raise_violation = false; + switch (state_) { + case State::kNormal: { + // On if/else/for tokens; + // Also, skip all conditional statement after a for or a if + // Then, expect a `begin` token or record a violation. + switch (token.token_enum()) { + case TK_if: + case TK_for: + condition_expr_level_ = 0; + last_condition_start_ = token; + state_ = State::kInCondition; + break; + case TK_else: + last_condition_start_ = token; + end_of_condition_statement_ = token; + state_ = State::kInElse; + break; + default: + break; + } // switch (token) + break; + } + case State::kInElse: { + // If we are in a else statement, we can either have a if (and need to + // skip cond. statement) Or directly wait for a begin. We make use of the + // boolean raise_violation in order to avoid code duplication. + switch (token.token_enum()) { + case TK_SPACE: // stay in the same state + break; + case TK_COMMENT_BLOCK: + case TK_EOL_COMMENT: + case TK_NEWLINE: + break; + case TK_if: + case TK_for: + condition_expr_level_ = 0; + last_condition_start_ = token; + state_ = State::kInCondition; + break; + case TK_begin: + state_ = State::kNormal; + break; + default: + raise_violation = true; + break; + } // switch (token) + break; + } + case State::kInCondition: { + if (token.text() == "(") { + condition_expr_level_++; + } else if (token.text() == ")") { + condition_expr_level_--; + if (condition_expr_level_ == 0) { + end_of_condition_statement_ = token; + state_ = State::kExpectBegin; + } + } + break; + } + case State::kExpectBegin: { + switch (token.token_enum()) { + case TK_SPACE: // stay in the same state + break; + case TK_COMMENT_BLOCK: + case TK_EOL_COMMENT: + case TK_NEWLINE: + break; + case TK_begin: + // If we got our begin token, we go back to normal status + state_ = State::kNormal; + break; + default: { + raise_violation = true; + break; + } + } // switch (token) + break; + } + } // switch (state_) + + if (raise_violation) { + violations_.insert(LintViolation( + last_condition_start_, + absl::StrCat(kMessage, " Expected begin, got ", token.text()))); + + // Once the violation is raised, we go back to a normal, default, state + condition_expr_level_ = 0; + state_ = State::kNormal; + raise_violation = false; + } +} + +LintRuleStatus ExplicitBeginRule::Report() const { + return LintRuleStatus(violations_, GetDescriptor()); +} + +} // namespace analysis +} // namespace verilog \ No newline at end of file diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h new file mode 100644 index 000000000..e90554c52 --- /dev/null +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -0,0 +1,67 @@ +// Copyright 2017-2020 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_EXPLICIT_BEGIN_RULE_H_ +#define VERIBLE_VERILOG_ANALYSIS_CHECKERS_EXPLICIT_BEGIN_RULE_H_ + +#include +#include +#include + +#include "common/analysis/lint_rule_status.h" +#include "common/analysis/token_stream_lint_rule.h" +#include "common/text/token_info.h" +#include "verilog/analysis/descriptions.h" + +namespace verilog { +namespace analysis { + +// Detects whether if and for loop statements use verilog block statements (begin/end) +class ExplicitBeginRule : public verible::TokenStreamLintRule { + public: + using rule_type = verible::TokenStreamLintRule; + + static const LintRuleDescriptor &GetDescriptor(); + + ExplicitBeginRule() : state_(State::kNormal), condition_expr_level_(0) {} + + void HandleToken(const verible::TokenInfo &token) final; + + verible::LintRuleStatus Report() const final; + + private: + // States of the internal token-based analysis. + enum class State { kNormal, kInCondition, kInElse, kExpectBegin }; + + // Internal lexical analysis state. + State state_; + + // Level of nested parenthesis when analizing conditional expressions. + int condition_expr_level_; + + // Token information for the last seen block opening (for/if/else). + verible::TokenInfo last_condition_start_ = verible::TokenInfo::EOFToken(); + verible::TokenInfo end_of_condition_statement_ = + verible::TokenInfo::EOFToken(); + + // Collection of found violations. + std::set violations_; + + void trigger_violation_(); +}; + +} // namespace analysis +} // namespace verilog + +#endif // VERIBLE_VERILOG_ANALYSIS_CHECKERS_ENDIF_COMMENT_RULE_H_ \ No newline at end of file diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc new file mode 100644 index 000000000..317ed863a --- /dev/null +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -0,0 +1,93 @@ +// Copyright 2017-2020 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/explicit_begin_rule.h" + +#include + +#include "common/analysis/linter_test_utils.h" +#include "common/analysis/token_stream_linter_test_utils.h" +#include "common/text/symbol.h" +#include "gtest/gtest.h" +#include "verilog/analysis/verilog_analyzer.h" +#include "verilog/parser/verilog_token_enum.h" + +namespace verilog { +namespace analysis { +namespace { + +using verible::LintTestCase; +using verible::RunApplyFixCases; +using verible::RunLintTestCases; + +// Tests that space-only text passes. +TEST(ExplicitBeginRuleTest, AcceptsBlank) { + const std::initializer_list kTestCases = { + {""}, + {" "}, + {"\n"}, + {" \n\n"}, + }; + RunLintTestCases(kTestCases); +} + +// Tests that properly matched if/begin passes. +TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { + const std::initializer_list kTestCases = { + {"if (FOO) begin"}, + {"if (FOO)\n begin"}, + {"if (FOO) //Comment\n begin"}, + {"else begin \n FOO"}, + {"else \nbegin \n FOO"}, + {"else //Comment\n begin \n FOO"}, + {"else \n //Comment\n begin \n FOO"}, + {"else if (FOO) begin"}, + {"else if (FOO)\n begin"}, + {"else if (FOO) //Comment\n begin"}, + {"else if (FOO)\n //Comment\n begin"}, + {"for(i = 0; i < N; i++) begin"}, + {"for(i = 0; i < N; i++)\nbegin"}, + {"for(i = 0; i < N; i++) // Comment\n begin"}, + {"for(i = 0; i < N; i++)\n // Comment\nbegin"}, + // {"forever begin"}, // Not supported + }; + RunLintTestCases(kTestCases); +} + +// Tests that unmatched block/begin fails is detected. +TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {{TK_if, "if"}, " (FOO)\n BAR"}, + {{TK_if, "if"}, " (FOO) //Comment\n BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_else, "else"}, " \n \n FOO"}, + {{TK_else, "else"}, " //Comment\n FOO"}, + {{TK_else, "else"}, " \n //Comment\n FOO"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO)\n BAR"}, + {"else ", {TK_if, "if"}, " (FOO) //Comment\n BAR"}, + {"else ", {TK_if, "if"}, " (FOO)\n //Comment\n BAR"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_for, "for"}, "(i = 0; i < N; i++)\n a <= 1'b1;"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) // Comment \n a <= 1'b1;"}, + {{TK_for, "for"}, "(i = 0; i < N; i++)\n // Comment\n a <= 1'b1;"}, + // {{TK_forever, "forever"}, "a <= 1'b1;"}, // Not supported + }; + RunLintTestCases(kTestCases); +} + +} // namespace +} // namespace analysis +} // namespace verilog \ No newline at end of file From cbced8ab5829b05a1e3531cf2a7accfe8e573401 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Fri, 12 Apr 2024 23:40:09 +1000 Subject: [PATCH 02/12] Added more common constructs that should have begin-end block statements. --- .../analysis/checkers/explicit_begin_rule.cc | 123 ++++++++++++------ .../analysis/checkers/explicit_begin_rule.h | 6 +- .../checkers/explicit_begin_rule_test.cc | 120 +++++++++++++++-- 3 files changed, 194 insertions(+), 55 deletions(-) diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc index bf1dde2d4..80ad13460 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.cc +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -42,7 +42,7 @@ using verible::TokenStreamLintRule; VERILOG_REGISTER_LINT_RULE(ExplicitBeginRule); static const char kMessage[] = - "All block construct shall explicitely use begin/end."; + " block constructs shall explicitly use begin/end."; const LintRuleDescriptor &ExplicitBeginRule::GetDescriptor() { static const LintRuleDescriptor d{ @@ -50,51 +50,93 @@ const LintRuleDescriptor &ExplicitBeginRule::GetDescriptor() { .topic = "explicit-begin", .desc = "Checks that a Verilog ``begin`` directive follows all " - "if, else and for loops.", + "if, else, always, always_comb, always_latch, always_ff," + " forever, initial, for, foreach and while statements.", }; return d; } void ExplicitBeginRule::HandleToken(const TokenInfo &token) { + // Ignore all white space and comments and return immediately + switch (token.token_enum()) { + case TK_SPACE: + case TK_NEWLINE: + case TK_COMMENT_BLOCK: + case TK_EOL_COMMENT: + return; + default: + break; + } + // Responds to a token by updating the state of the analysis. bool raise_violation = false; switch (state_) { - case State::kNormal: { - // On if/else/for tokens; - // Also, skip all conditional statement after a for or a if - // Then, expect a `begin` token or record a violation. + case State::kNormal: { switch (token.token_enum()) { - case TK_if: + // After token expect "begin" + case TK_always_comb: + case TK_always_latch: + case TK_forever: + case TK_initial: + start_token_ = token; + state_ = State::kExpectBegin; + break; + // After token expect a "condition" followed by "begin". NOTE: there may + // be tokens prior to the condition (like in an "always_ff" statement) + // and these are all ignored. + case TK_always_ff: + case TK_foreach: case TK_for: + case TK_if: + case TK_while: condition_expr_level_ = 0; - last_condition_start_ = token; + start_token_ = token; state_ = State::kInCondition; break; + // always gets special handling, as somtimes there is a "condition" or + // not before a "begin". + case TK_always: + condition_expr_level_ = 0; + start_token_ = token; + state_ = State::kInAlways; + break; + // else is also special as "if" or "begin" can follow case TK_else: - last_condition_start_ = token; - end_of_condition_statement_ = token; + start_token_ = token; state_ = State::kInElse; break; default: break; - } // switch (token) + } break; } - case State::kInElse: { - // If we are in a else statement, we can either have a if (and need to - // skip cond. statement) Or directly wait for a begin. We make use of the - // boolean raise_violation in order to avoid code duplication. + case State::kInAlways: { + // always is a little more complicated in that it can be imediattly + // followed by a "begin" or followed by some special characters ("@" or + // "*") and maybe a condition. switch (token.token_enum()) { - case TK_SPACE: // stay in the same state + case '@': + case '*': + break; + case TK_begin: + state_ = State::kNormal; break; - case TK_COMMENT_BLOCK: - case TK_EOL_COMMENT: - case TK_NEWLINE: + case '(': + condition_expr_level_ = 1; + state_ = State::kInCondition; + break; + default: + raise_violation = true; break; + } + break; + } + case State::kInElse: { + // An else statement can be followed by either a begin or an if. + switch (token.token_enum()) { case TK_if: - case TK_for: condition_expr_level_ = 0; - last_condition_start_ = token; + start_token_ = token; state_ = State::kInCondition; break; case TK_begin: @@ -103,29 +145,34 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { default: raise_violation = true; break; - } // switch (token) + } break; } case State::kInCondition: { - if (token.text() == "(") { - condition_expr_level_++; - } else if (token.text() == ")") { - condition_expr_level_--; - if (condition_expr_level_ == 0) { - end_of_condition_statement_ = token; - state_ = State::kExpectBegin; + // The last token expects a condition statement enclosed in a pair of + // parentheses "()". This process also ignores any tokens between the last + // token and the opening parentheses which simplifies "always_ff". + switch (token.token_enum()) { + case '(': { + condition_expr_level_++; + break; + } + case ')': { + condition_expr_level_--; + if (condition_expr_level_ == 0) { + state_ = State::kExpectBegin; + } + } + default: { + // throw away everything else + break; } } break; } case State::kExpectBegin: { + // The next token must be a "begin" switch (token.token_enum()) { - case TK_SPACE: // stay in the same state - break; - case TK_COMMENT_BLOCK: - case TK_EOL_COMMENT: - case TK_NEWLINE: - break; case TK_begin: // If we got our begin token, we go back to normal status state_ = State::kNormal; @@ -134,15 +181,15 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { raise_violation = true; break; } - } // switch (token) + } break; } } // switch (state_) if (raise_violation) { violations_.insert(LintViolation( - last_condition_start_, - absl::StrCat(kMessage, " Expected begin, got ", token.text()))); + start_token_, + absl::StrCat(start_token_.text(), kMessage, " Expected begin, got ", token.text()))); // Once the violation is raised, we go back to a normal, default, state condition_expr_level_ = 0; diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h index e90554c52..187452382 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.h +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -42,7 +42,7 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { private: // States of the internal token-based analysis. - enum class State { kNormal, kInCondition, kInElse, kExpectBegin }; + enum class State { kNormal, kInAlways, kInCondition, kInElse, kExpectBegin }; // Internal lexical analysis state. State state_; @@ -51,9 +51,7 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { int condition_expr_level_; // Token information for the last seen block opening (for/if/else). - verible::TokenInfo last_condition_start_ = verible::TokenInfo::EOFToken(); - verible::TokenInfo end_of_condition_statement_ = - verible::TokenInfo::EOFToken(); + verible::TokenInfo start_token_ = verible::TokenInfo::EOFToken(); // Collection of found violations. std::set violations_; diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc index 317ed863a..8ff18d0a3 100644 --- a/verilog/analysis/checkers/explicit_begin_rule_test.cc +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -45,22 +45,70 @@ TEST(ExplicitBeginRuleTest, AcceptsBlank) { // Tests that properly matched if/begin passes. TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { const std::initializer_list kTestCases = { - {"if (FOO) begin"}, - {"if (FOO)\n begin"}, - {"if (FOO) //Comment\n begin"}, + {"if (FOO) /*block comment */ begin a <= 1;"}, + {"if (FOO) begin a <= 1;"}, + {"if (FOO)begin : name_statement a <= 1;"}, + {"if (FOO)\n begin a <= 1;"}, + {"if (FOO) //Comment\n begin a <= 1;"}, + {"else begin \n FOO"}, {"else \nbegin \n FOO"}, {"else //Comment\n begin \n FOO"}, {"else \n //Comment\n begin \n FOO"}, - {"else if (FOO) begin"}, - {"else if (FOO)\n begin"}, - {"else if (FOO) //Comment\n begin"}, - {"else if (FOO)\n //Comment\n begin"}, - {"for(i = 0; i < N; i++) begin"}, - {"for(i = 0; i < N; i++)\nbegin"}, - {"for(i = 0; i < N; i++) // Comment\n begin"}, - {"for(i = 0; i < N; i++)\n // Comment\nbegin"}, - // {"forever begin"}, // Not supported + {"else if (FOO) begin a <= 1;"}, + {"else if (FOO)\n begin a <= 1;"}, + {"else if (FOO) //Comment\n begin a <= 1;"}, + {"else if (FOO)\n //Comment\n begin a <= 1;"}, + + {"for(i = 0; i < N; i++) begin a <= 1;"}, + {"for(i = 0; i < N; i++)\nbegin a <= 1;"}, + {"for(i = 0; i < N; i++) // Comment\n begin a <= 1;"}, + {"for(i = 0; i < N; i++)\n // Comment\nbegin a <= 1;"}, + + {"foreach(array[i]) begin a <= 1;"}, + {"foreach(array[i])\nbegin a <= 1;"}, + {"foreach(array[i]) // Comment\n begin a <= 1;"}, + {"foreach(array[i])\n // Comment\nbegin a <= 1;"}, + + {"while (a < 3) begin a = a + 1;"}, + {"while(a < 3)\nbegin a = a + 1;"}, + {"while (a < 3) // Comment\n begin a = a + 1;"}, + {"while(a < 3)\n // Comment\nbegin a = a + 1;"}, + + {"forever begin a <= 1;"}, + {"forever\nbegin a <= 1;"}, + {"forever // Comment\n begin a <= 1;"}, + {"forever\n // Comment\nbegin a <= 1;"}, + + {"initial begin a <= 1;"}, + {"initial\nbegin a <= 1;"}, + {"initial // Comment\n begin a <= 1;"}, + {"initial\n // Comment\nbegin a <= 1;"}, + + {"always_comb begin a = 1;"}, + {"always_comb\nbegin a = 1;"}, + {"always_comb // Comment\n begin a = 1;"}, + {"always_comb\n // Comment\nbegin a = 1;"}, + + {"always_latch begin a <= 1;"}, + {"always_latch\nbegin a <= 1;"}, + {"always_latch // Comment\n begin a <= 1;"}, + {"always_latch\n // Comment\nbegin a <= 1;"}, + + {"always_ff @( a or b) begin a <= 1;"}, + {"always_ff @ ( a or b)\nbegin a <= 1;"}, + {"always_ff @( (a) and b) // Comment\n begin a <= 1;"}, + {"always_ff @( a or ((b)))\n // Comment\nbegin a <= 1;"}, + + {"always @( a or b) begin a <= 1;"}, + {"always @ ( a or b)\nbegin a <= 1;"}, + {"always @( (a) and b) // Comment\n begin a <= 1;"}, + {"always @( a or ((b)))\n // Comment\nbegin a <= 1;"}, + {"always@* begin a = 1'b1;"}, + {"always@(*) begin a = 1'b1;"}, + {"always @* begin a = 1'b1;"}, + {"always begin a = 1'b1;"}, + {"always begin #10 a = 1'b1;"}, }; RunLintTestCases(kTestCases); } @@ -71,6 +119,7 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_if, "if"}, " (FOO) BAR"}, {{TK_if, "if"}, " (FOO)\n BAR"}, {{TK_if, "if"}, " (FOO) //Comment\n BAR"}, + {{TK_else, "else"}, " \n FOO"}, {{TK_else, "else"}, " \n \n FOO"}, {{TK_else, "else"}, " //Comment\n FOO"}, @@ -79,11 +128,56 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {"else ", {TK_if, "if"}, " (FOO)\n BAR"}, {"else ", {TK_if, "if"}, " (FOO) //Comment\n BAR"}, {"else ", {TK_if, "if"}, " (FOO)\n //Comment\n BAR"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++)\n a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++) // Comment \n a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++)\n // Comment\n a <= 1'b1;"}, - // {{TK_forever, "forever"}, "a <= 1'b1;"}, // Not supported + + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i])\n a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) // Comment \n a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i])\n // Comment\n a <= 1'b1;"}, + + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, " (array[i])\n a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) // Comment \n a <= 1'b1;"}, + {{TK_while, "while"}, " (array[i])\n // Comment\n a <= 1'b1;"}, + + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_forever, "forever"}, "\n a <= 1'b1;"}, + {{TK_forever, "forever"}, " // Comment \n a <= 1'b1;"}, + {{TK_forever, "forever"}, "\n // Comment\n a <= 1'b1;"}, + + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_initial, "initial"}, "\n a = 1'b1;"}, + {{TK_initial, "initial"}, " // Comment \n a = 1'b1;"}, + {{TK_initial, "initial"}, "\n // Comment\n a = 1'b1;"}, + + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, "\n a = 1'b1;"}, + {{TK_always_comb, "always_comb"}, " // Comment \n a = 1'b1;"}, + {{TK_always_comb, "always_comb"}, "\n // Comment\n a = 1'b1;"}, + + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, "\n a = 1'b1;"}, + {{TK_always_latch, "always_latch"}, " // Comment \n a = 1'b1;"}, + {{TK_always_latch, "always_latch"}, "\n // Comment\n a = 1'b1;"}, + + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, "@(a or b)\n a <= 1'b1;"}, + {{TK_always_ff, "always_ff"}, " @(posedge a or negedge b) // Comment \n a <= 1'b1;"}, + {{TK_always_ff, "always_ff"}, "@(a || b)\n // Comment\n a <= 1'b1;"}, + + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + {{TK_always, "always"}, "@(a or b)\n a = 1'b1;"}, + {{TK_always, "always"}, " @(posedge a or negedge b) // Comment \n a <= 1'b1;"}, + {{TK_always, "always"}, "@(a || b)\n // Comment\n <= 1'b1;"}, + {{TK_always, "always"}, "@* a = 1'b1;"}, + {{TK_always, "always"}, "@(*) a = 1'b1;"}, + {{TK_always, "always"}, " @* a = 1'b1;"}, + {{TK_always, "always"}, " a = 1'b1;"}, + {{TK_always, "always"}, " #10 a = 1'b1;"}, }; RunLintTestCases(kTestCases); } From 576dd700dbf9edcbe53023a81d47da6d682d56f3 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Sun, 14 Apr 2024 00:10:44 +1000 Subject: [PATCH 03/12] Added rule configuration, allowing users to disable explicit begin end blocks for each supported statement. --- verilog/analysis/checkers/BUILD | 1 + .../analysis/checkers/explicit_begin_rule.cc | 103 ++++++- .../analysis/checkers/explicit_begin_rule.h | 28 +- .../checkers/explicit_begin_rule_test.cc | 268 ++++++++++++++++-- 4 files changed, 373 insertions(+), 27 deletions(-) diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index 5ded24a4b..6d9293d5c 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -1258,6 +1258,7 @@ cc_library( "//common/analysis:lint-rule-status", "//common/analysis:token-stream-lint-rule", "//common/strings:comment-utils", + "//common/text:config-utils", "//common/text:token-info", "//verilog/analysis:descriptions", "//verilog/analysis:lint-rule-registry", diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc index 80ad13460..fbf401688 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.cc +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -24,6 +24,7 @@ #include "common/analysis/lint_rule_status.h" #include "common/analysis/token_stream_lint_rule.h" #include "common/strings/comment_utils.h" +#include "common/text/config_utils.h" #include "common/text/token_info.h" #include "verilog/analysis/descriptions.h" #include "verilog/analysis/lint_rule_registry.h" @@ -52,10 +53,91 @@ const LintRuleDescriptor &ExplicitBeginRule::GetDescriptor() { "Checks that a Verilog ``begin`` directive follows all " "if, else, always, always_comb, always_latch, always_ff," " forever, initial, for, foreach and while statements.", + .param = + { + {"if_enable", "true", + "All if statements require an explicit begin-end block"}, + {"else_enable", "true", + "All else statements require an explicit begin-end block"}, + {"always_enable", "true", + "All always statements require an explicit begin-end block"}, + {"always_comb_enable", "true", + "All always_comb statements require an explicit begin-end " + "block"}, + {"always_latch_enable", "true", + "All always_latch statements require an explicit begin-end " + "block"}, + {"always_ff_enable", "true", + "All always_ff statements require an explicit begin-end block"}, + {"forever_enable", "true", + "All forever statements require an explicit begin-end block"}, + {"initial_enable", "true", + "All initial statements require an explicit begin-end block"}, + {"for_enable", "true", + "All for statements require an explicit begin-end block"}, + {"foreach_enable", "true", + "All foreach statements require an explicit begin-end block"}, + {"while_enable", "true", + "All while statements require an explicit begin-end block"}, + }, }; return d; } +absl::Status ExplicitBeginRule::Configure(absl::string_view configuration) { + static const std::vector supported_statements = { + "if", "else", "always", "always_comb", + "always_latch", "always_ff", "forever", "initial", + "for", "foreach", "while"}; // same sequence as enum + // StyleChoicesBits + + using verible::config::SetBool; + return verible::ParseNameValues( + configuration, + { + {"if_enable", SetBool(&if_enable_)}, + {"else_enable", SetBool(&else_enable_)}, + {"always_enable", SetBool(&always_enable_)}, + {"always_comb_enable", SetBool(&always_comb_enable_)}, + {"always_latch_enable", SetBool(&always_latch_enable_)}, + {"always_ff_enable", SetBool(&always_ff_enable_)}, + {"forever_enable", SetBool(&forever_enable_)}, + {"initial_enable", SetBool(&initial_enable_)}, + {"for_enable", SetBool(&for_enable_)}, + {"foreach_enable", SetBool(&foreach_enable_)}, + {"while_enable", SetBool(&while_enable_)}, + }); +} + +bool ExplicitBeginRule::IsTokenEnabled(const TokenInfo &token) { + switch (token.token_enum()) { + case TK_always_comb: + return always_comb_enable_; + case TK_always_latch: + return always_latch_enable_; + case TK_forever: + return forever_enable_; + case TK_initial: + return initial_enable_; + case TK_always_ff: + return always_ff_enable_; + case TK_foreach: + return foreach_enable_; + case TK_for: + return for_enable_; + case TK_if: + return if_enable_; + case TK_while: + return while_enable_; + case TK_always: + return always_enable_; + case TK_else: + return else_enable_; + default: + return false; + } +} + void ExplicitBeginRule::HandleToken(const TokenInfo &token) { // Ignore all white space and comments and return immediately switch (token.token_enum()) { @@ -71,7 +153,11 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { // Responds to a token by updating the state of the analysis. bool raise_violation = false; switch (state_) { - case State::kNormal: { + case State::kNormal: { + if (!IsTokenEnabled(token)) { + return; + } + switch (token.token_enum()) { // After token expect "begin" case TK_always_comb: @@ -106,6 +192,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { state_ = State::kInElse; break; default: + // Do nothing break; } break; @@ -135,9 +222,13 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { // An else statement can be followed by either a begin or an if. switch (token.token_enum()) { case TK_if: - condition_expr_level_ = 0; - start_token_ = token; - state_ = State::kInCondition; + if (if_enable_) { + condition_expr_level_ = 0; + start_token_ = token; + state_ = State::kInCondition; + } else { + state_ = State::kNormal; + } break; case TK_begin: state_ = State::kNormal; @@ -188,8 +279,8 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { if (raise_violation) { violations_.insert(LintViolation( - start_token_, - absl::StrCat(start_token_.text(), kMessage, " Expected begin, got ", token.text()))); + start_token_, absl::StrCat(start_token_.text(), kMessage, + " Expected begin, got ", token.text()))); // Once the violation is raised, we go back to a normal, default, state condition_expr_level_ = 0; diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h index 187452382..2117a1b24 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.h +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -27,13 +27,18 @@ namespace verilog { namespace analysis { -// Detects whether if and for loop statements use verilog block statements (begin/end) +using verible::TokenInfo; + +// Detects whether if and for loop statements use verilog block statements +// (begin/end) class ExplicitBeginRule : public verible::TokenStreamLintRule { public: using rule_type = verible::TokenStreamLintRule; static const LintRuleDescriptor &GetDescriptor(); + absl::Status Configure(absl::string_view configuration) final; + ExplicitBeginRule() : state_(State::kNormal), condition_expr_level_(0) {} void HandleToken(const verible::TokenInfo &token) final; @@ -41,22 +46,35 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { verible::LintRuleStatus Report() const final; private: + bool IsTokenEnabled(const TokenInfo &token); + // States of the internal token-based analysis. enum class State { kNormal, kInAlways, kInCondition, kInElse, kExpectBegin }; // Internal lexical analysis state. State state_; - // Level of nested parenthesis when analizing conditional expressions. + // Level of nested parenthesis when analysing conditional expressions. int condition_expr_level_; - // Token information for the last seen block opening (for/if/else). + // Configuration + bool if_enable_ = true; + bool else_enable_ = true; + bool always_enable_ = true; + bool always_comb_enable_ = true; + bool always_latch_enable_ = true; + bool always_ff_enable_ = true; + bool forever_enable_ = true; + bool initial_enable_ = true; + bool for_enable_ = true; + bool foreach_enable_ = true; + bool while_enable_ = true; + + // Token that requires blocking statement. verible::TokenInfo start_token_ = verible::TokenInfo::EOFToken(); // Collection of found violations. std::set violations_; - - void trigger_violation_(); }; } // namespace analysis diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc index 8ff18d0a3..6c3bc5adb 100644 --- a/verilog/analysis/checkers/explicit_begin_rule_test.cc +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -29,6 +29,7 @@ namespace { using verible::LintTestCase; using verible::RunApplyFixCases; +using verible::RunConfiguredLintTestCases; using verible::RunLintTestCases; // Tests that space-only text passes. @@ -50,7 +51,7 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"if (FOO)begin : name_statement a <= 1;"}, {"if (FOO)\n begin a <= 1;"}, {"if (FOO) //Comment\n begin a <= 1;"}, - + {"else begin \n FOO"}, {"else \nbegin \n FOO"}, {"else //Comment\n begin \n FOO"}, @@ -59,12 +60,12 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"else if (FOO)\n begin a <= 1;"}, {"else if (FOO) //Comment\n begin a <= 1;"}, {"else if (FOO)\n //Comment\n begin a <= 1;"}, - + {"for(i = 0; i < N; i++) begin a <= 1;"}, {"for(i = 0; i < N; i++)\nbegin a <= 1;"}, {"for(i = 0; i < N; i++) // Comment\n begin a <= 1;"}, {"for(i = 0; i < N; i++)\n // Comment\nbegin a <= 1;"}, - + {"foreach(array[i]) begin a <= 1;"}, {"foreach(array[i])\nbegin a <= 1;"}, {"foreach(array[i]) // Comment\n begin a <= 1;"}, @@ -74,7 +75,7 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"while(a < 3)\nbegin a = a + 1;"}, {"while (a < 3) // Comment\n begin a = a + 1;"}, {"while(a < 3)\n // Comment\nbegin a = a + 1;"}, - + {"forever begin a <= 1;"}, {"forever\nbegin a <= 1;"}, {"forever // Comment\n begin a <= 1;"}, @@ -84,7 +85,7 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"initial\nbegin a <= 1;"}, {"initial // Comment\n begin a <= 1;"}, {"initial\n // Comment\nbegin a <= 1;"}, - + {"always_comb begin a = 1;"}, {"always_comb\nbegin a = 1;"}, {"always_comb // Comment\n begin a = 1;"}, @@ -94,7 +95,7 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"always_latch\nbegin a <= 1;"}, {"always_latch // Comment\n begin a <= 1;"}, {"always_latch\n // Comment\nbegin a <= 1;"}, - + {"always_ff @( a or b) begin a <= 1;"}, {"always_ff @ ( a or b)\nbegin a <= 1;"}, {"always_ff @( (a) and b) // Comment\n begin a <= 1;"}, @@ -110,6 +111,7 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"always begin a = 1'b1;"}, {"always begin #10 a = 1'b1;"}, }; + RunLintTestCases(kTestCases); } @@ -119,7 +121,7 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_if, "if"}, " (FOO) BAR"}, {{TK_if, "if"}, " (FOO)\n BAR"}, {{TK_if, "if"}, " (FOO) //Comment\n BAR"}, - + {{TK_else, "else"}, " \n FOO"}, {{TK_else, "else"}, " \n \n FOO"}, {{TK_else, "else"}, " //Comment\n FOO"}, @@ -128,12 +130,12 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {"else ", {TK_if, "if"}, " (FOO)\n BAR"}, {"else ", {TK_if, "if"}, " (FOO) //Comment\n BAR"}, {"else ", {TK_if, "if"}, " (FOO)\n //Comment\n BAR"}, - + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++)\n a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++) // Comment \n a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++)\n // Comment\n a <= 1'b1;"}, - + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i])\n a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i]) // Comment \n a <= 1'b1;"}, @@ -143,17 +145,17 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_while, "while"}, " (array[i])\n a <= 1'b1;"}, {{TK_while, "while"}, "(array[i]) // Comment \n a <= 1'b1;"}, {{TK_while, "while"}, " (array[i])\n // Comment\n a <= 1'b1;"}, - + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, {{TK_forever, "forever"}, "\n a <= 1'b1;"}, {{TK_forever, "forever"}, " // Comment \n a <= 1'b1;"}, {{TK_forever, "forever"}, "\n // Comment\n a <= 1'b1;"}, - + {{TK_initial, "initial"}, " a = 1'b1;\n"}, {{TK_initial, "initial"}, "\n a = 1'b1;"}, {{TK_initial, "initial"}, " // Comment \n a = 1'b1;"}, {{TK_initial, "initial"}, "\n // Comment\n a = 1'b1;"}, - + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, {{TK_always_comb, "always_comb"}, "\n a = 1'b1;"}, {{TK_always_comb, "always_comb"}, " // Comment \n a = 1'b1;"}, @@ -163,15 +165,17 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_always_latch, "always_latch"}, "\n a = 1'b1;"}, {{TK_always_latch, "always_latch"}, " // Comment \n a = 1'b1;"}, {{TK_always_latch, "always_latch"}, "\n // Comment\n a = 1'b1;"}, - + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, {{TK_always_ff, "always_ff"}, "@(a or b)\n a <= 1'b1;"}, - {{TK_always_ff, "always_ff"}, " @(posedge a or negedge b) // Comment \n a <= 1'b1;"}, + {{TK_always_ff, "always_ff"}, + " @(posedge a or negedge b) // Comment \n a <= 1'b1;"}, {{TK_always_ff, "always_ff"}, "@(a || b)\n // Comment\n a <= 1'b1;"}, - + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, {{TK_always, "always"}, "@(a or b)\n a = 1'b1;"}, - {{TK_always, "always"}, " @(posedge a or negedge b) // Comment \n a <= 1'b1;"}, + {{TK_always, "always"}, + " @(posedge a or negedge b) // Comment \n a <= 1'b1;"}, {{TK_always, "always"}, "@(a || b)\n // Comment\n <= 1'b1;"}, {{TK_always, "always"}, "@* a = 1'b1;"}, {{TK_always, "always"}, "@(*) a = 1'b1;"}, @@ -179,9 +183,241 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_always, "always"}, " a = 1'b1;"}, {{TK_always, "always"}, " #10 a = 1'b1;"}, }; + RunLintTestCases(kTestCases); } +// Tests that rule can be disabled for if statements +TEST(ExplicitBeginRuleTest, AcceptsIfBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {"if (FOO) BAR"}, + {"else if (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "if_enable:false"); +} + +// Tests that rule can be disabled for else statements +TEST(ExplicitBeginRuleTest, AcceptsElseBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {"else \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "else_enable:false"); +} + +// Tests that rule can be disabled for for statements +TEST(ExplicitBeginRuleTest, AcceptsForBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {"for(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "for_enable:false"); +} + +// Tests that rule can be disabled for foreach statements +TEST(ExplicitBeginRuleTest, AcceptsForeachBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {"foreach(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "foreach_enable:false"); +} + +// Tests that rule can be disabled for while statements +TEST(ExplicitBeginRuleTest, AcceptsWhileBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {"while(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "while_enable:false"); +} + +// Tests that rule can be disabled for forever statements +TEST(ExplicitBeginRuleTest, AcceptsForeverBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {"forever a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "forever_enable:false"); +} + +// Tests that rule can be disabled for initial statements +TEST(ExplicitBeginRuleTest, AcceptsInitialBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {"initial a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "initial_enable:false"); +} + +// Tests that rule can be disabled for always_comb statements +TEST(ExplicitBeginRuleTest, AcceptsAlwaysCombBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {"always_comb a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "always_comb_enable:false"); +} + +// Tests that rule can be disabled for always_latch statements +TEST(ExplicitBeginRuleTest, AcceptsAlwaysLatchBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {"always_latch a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "always_latch_enable:false"); +} + +// Tests that rule can be disabled for always_ff statements +TEST(ExplicitBeginRuleTest, AcceptsAlwaysFFBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {"always_ff @(a or b) a <= 1'b1;\n"}, + {{TK_always, "always"}, " @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "always_ff_enable:false"); +} + +// Tests that rule can be disabled for always statements +TEST(ExplicitBeginRuleTest, AcceptsAlwaysBlocksWithoutBeginConfigured) { + const std::initializer_list kTestCases = { + {{TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_for, "for"}, "(i = 0; i < N; i++) a <= 1'b1;"}, + {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, + {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, + {{TK_forever, "forever"}, " a <= 1'b1;\n"}, + {{TK_initial, "initial"}, " a = 1'b1;\n"}, + {{TK_always_comb, "always_comb"}, " a = 1'b1;\n"}, + {{TK_always_latch, "always_latch"}, " a = 1'b1;\n"}, + {{TK_always_ff, "always_ff"}, " @(a or b) a <= 1'b1;\n"}, + {"always @(a or b) a = 1'b1;\n"}, + }; + + RunConfiguredLintTestCases( + kTestCases, "always_enable:false"); +} + } // namespace } // namespace analysis } // namespace verilog \ No newline at end of file From a8178d656217fe71329b6a32c7968e1bbd20c8e6 Mon Sep 17 00:00:00 2001 From: Julien FAUCHER Date: Mon, 16 May 2022 15:42:45 +0200 Subject: [PATCH 04/12] [LINT] Add rule explicit-begin Add rule to check if the begin keyword always follows a if/else/for. Related to #1321 --- verilog/analysis/checkers/BUILD | 33 ++++ .../analysis/checkers/explicit_begin_rule.cc | 160 ++++++++++++++++++ .../analysis/checkers/explicit_begin_rule.h | 88 ++++++++++ 3 files changed, 281 insertions(+) create mode 100644 verilog/analysis/checkers/explicit_begin_rule.cc create mode 100644 verilog/analysis/checkers/explicit_begin_rule.h diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index dc1177397..6d9293d5c 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -24,6 +24,7 @@ cc_library( ":disable-statement-rule", ":endif-comment-rule", ":enum-name-style-rule", + ":explicit-begin-rule", ":explicit-function-lifetime-rule", ":explicit-function-task-parameter-type-rule", ":explicit-parameter-storage-type-rule", @@ -1249,6 +1250,38 @@ cc_test( ], ) +cc_library( + name = "explicit-begin-rule", + srcs = ["explicit_begin_rule.cc"], + hdrs = ["explicit_begin_rule.h"], + deps = [ + "//common/analysis:lint-rule-status", + "//common/analysis:token-stream-lint-rule", + "//common/strings:comment-utils", + "//common/text:config-utils", + "//common/text:token-info", + "//verilog/analysis:descriptions", + "//verilog/analysis:lint-rule-registry", + "//verilog/parser:verilog-token-enum", + "@com_google_absl//absl/strings", + ], + alwayslink = 1, +) + +cc_test( + name = "explicit-begin-rule_test", + srcs = ["explicit_begin_rule_test.cc"], + deps = [ + ":explicit-begin-rule", + "//common/analysis:linter-test-utils", + "//common/analysis:token-stream-linter-test-utils", + "//common/text:symbol", + "//verilog/analysis:verilog-analyzer", + "//verilog/parser:verilog-token-enum", + "@com_google_googletest//:gtest_main", + ], +) + cc_library( name = "explicit-function-lifetime-rule", srcs = ["explicit_function_lifetime_rule.cc"], diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc new file mode 100644 index 000000000..690e381ee --- /dev/null +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -0,0 +1,160 @@ +// Copyright 2017-2020 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/explicit_begin_rule.h" + +#include +#include +#include +#include + +#include "absl/strings/str_cat.h" +#include "absl/strings/string_view.h" +#include "common/analysis/lint_rule_status.h" +#include "common/analysis/token_stream_lint_rule.h" +#include "common/strings/comment_utils.h" +#include "common/text/token_info.h" +#include "verilog/analysis/descriptions.h" +#include "verilog/analysis/lint_rule_registry.h" +#include "verilog/parser/verilog_token_enum.h" + +namespace verilog { +namespace analysis { + +using verible::AutoFix; +using verible::LintRuleStatus; +using verible::LintViolation; +using verible::TokenInfo; +using verible::TokenStreamLintRule; + +// Register the lint rule +VERILOG_REGISTER_LINT_RULE(ExplicitBeginRule); + +static const char kMessage[] = + "All block construct shall explicitely use begin/end"; + +const LintRuleDescriptor& ExplicitBeginRule::GetDescriptor() { + static const LintRuleDescriptor d{ + .name = "explicit-begin", + .topic = "explicit-begin", + .desc = + "Checks that a Verilog ``begin`` directive follows all " + "if, else and for loops.", + }; + return d; +} + +void ExplicitBeginRule::HandleToken(const TokenInfo& token) { + // Responds to a token by updating the state of the analysis. + bool raise_violation = false; + switch (state_) { + case State::kNormal: { + // On if/else/for tokens; + // Also, skip all conditional statement after a for or a if + // Then, expect a `begin` token or record a violation. + switch (token.token_enum()) { + case TK_if: + case TK_for: + condition_expr_level_ = 0; + last_condition_start_ = token; + state_ = State::kInCondition; + break; + case TK_else: + last_condition_start_ = token; + end_of_condition_statement_ = token; + state_ = State::kInElse; + break; + default: + break; + } // switch (token) + break; + } + case State::kInElse: { + // If we are in a else statement, we can either have a if (and need to + // skip cond. statement) Or directly wait for a begin. We make use of the + // boolean raise_violation in order to avoid code duplication. + switch (token.token_enum()) { + case TK_SPACE: // stay in the same state + break; + case TK_COMMENT_BLOCK: + case TK_EOL_COMMENT: + break; + case TK_if: + case TK_for: + condition_expr_level_ = 0; + last_condition_start_ = token; + state_ = State::kInCondition; + break; + case TK_begin: + state_ = State::kNormal; + break; + default: + raise_violation = true; + break; + } // switch (token) + break; + } + case State::kInCondition: { + if (token.text() == "(") { + condition_expr_level_++; + } else if (token.text() == ")") { + condition_expr_level_--; + if (condition_expr_level_ == 0) { + end_of_condition_statement_ = token; + state_ = State::kExpectBegin; + } + } + break; + } + case State::kExpectBegin: { + switch (token.token_enum()) { + case TK_SPACE: // stay in the same state + break; + case TK_COMMENT_BLOCK: + case TK_EOL_COMMENT: + break; + case TK_begin: + // If we got our begin token, we go back to normal status + state_ = State::kNormal; + break; + default: { + raise_violation = true; + break; + } + } // switch (token) + break; + } + } // switch (state_) + + if (raise_violation) { + violations_.insert(LintViolation( + last_condition_start_, absl::StrCat(kMessage), + {AutoFix( + "Insert begin", + {end_of_condition_statement_, + absl::StrCat(end_of_condition_statement_.text(), " begin")})})); + + // Once the violation is raised, we go back to a normal, default, state + condition_expr_level_ = 0; + state_ = State::kNormal; + raise_violation = false; + } +} + +LintRuleStatus ExplicitBeginRule::Report() const { + return LintRuleStatus(violations_, GetDescriptor()); +} + +} // namespace analysis +} // namespace verilog diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h new file mode 100644 index 000000000..ad08a195f --- /dev/null +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -0,0 +1,88 @@ +// Copyright 2017-2020 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_EXPLICIT_BEGIN_RULE_H_ +#define VERIBLE_VERILOG_ANALYSIS_CHECKERS_EXPLICIT_BEGIN_RULE_H_ + +#include +#include +#include + +#include "common/analysis/lint_rule_status.h" +#include "common/analysis/token_stream_lint_rule.h" +#include "common/text/token_info.h" +#include "verilog/analysis/descriptions.h" + +namespace verilog { +namespace analysis { + +// Detects whether a Verilog `endif directive is followed by a comment that +// matches the opening `ifdef or `ifndef. +// +// Accepted examples: +// `ifdef FOO +// `endif // FOO +// +// `ifndef BAR +// `endif // BAR +// +// Rejected examples: +// `ifdef FOO +// `endif +// +// `ifdef FOO +// `endif // BAR +// +class ExplicitBeginRule : public verible::TokenStreamLintRule { + public: + using rule_type = verible::TokenStreamLintRule; + + static const LintRuleDescriptor& GetDescriptor(); + + ExplicitBeginRule() : state_(State::kNormal), condition_expr_level_(0) {} + + void HandleToken(const verible::TokenInfo& token) final; + + verible::LintRuleStatus Report() const final; + + private: + // States of the internal token-based analysis. + enum class State { + kNormal, + kInCondition, + kInElse, + kExpectBegin + }; + + // Internal lexical analysis state. + State state_; + + // Level of nested parenthesis when analizing conditional expressions. + int condition_expr_level_; + + // Token information for the last seen block opening (for/if/else). + verible::TokenInfo last_condition_start_ = verible::TokenInfo::EOFToken(); + verible::TokenInfo end_of_condition_statement_ = verible::TokenInfo::EOFToken(); + + + // Collection of found violations. + std::set violations_; + + void trigger_violation_(); +}; + +} // namespace analysis +} // namespace verilog + +#endif // VERIBLE_VERILOG_ANALYSIS_CHECKERS_ENDIF_COMMENT_RULE_H_ From 65823cf7ea828595cd122779c25c91b8f7b5f547 Mon Sep 17 00:00:00 2001 From: Julien FAUCHER Date: Mon, 16 May 2022 16:32:22 +0200 Subject: [PATCH 05/12] [LINT] Add explicit-begin tests Closes #1336 --- .../analysis/checkers/explicit_begin_rule.cc | 6 +- .../analysis/checkers/explicit_begin_rule.h | 36 ++++--- .../checkers/explicit_begin_rule_test.cc | 101 ++++++++++++++++++ 3 files changed, 125 insertions(+), 18 deletions(-) create mode 100644 verilog/analysis/checkers/explicit_begin_rule_test.cc diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc index 690e381ee..3f38494cf 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.cc +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -42,7 +42,7 @@ using verible::TokenStreamLintRule; VERILOG_REGISTER_LINT_RULE(ExplicitBeginRule); static const char kMessage[] = - "All block construct shall explicitely use begin/end"; + "All block construct shall explicitely use begin/end."; const LintRuleDescriptor& ExplicitBeginRule::GetDescriptor() { static const LintRuleDescriptor d{ @@ -89,6 +89,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo& token) { break; case TK_COMMENT_BLOCK: case TK_EOL_COMMENT: + case TK_NEWLINE: break; case TK_if: case TK_for: @@ -123,6 +124,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo& token) { break; case TK_COMMENT_BLOCK: case TK_EOL_COMMENT: + case TK_NEWLINE: break; case TK_begin: // If we got our begin token, we go back to normal status @@ -139,7 +141,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo& token) { if (raise_violation) { violations_.insert(LintViolation( - last_condition_start_, absl::StrCat(kMessage), + token, absl::StrCat(kMessage, " Expected begin, got ", token.text()), {AutoFix( "Insert begin", {end_of_condition_statement_, diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h index ad08a195f..ae1d67406 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.h +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -31,18 +31,27 @@ namespace analysis { // matches the opening `ifdef or `ifndef. // // Accepted examples: -// `ifdef FOO -// `endif // FOO +// if (FOO) begin // -// `ifndef BAR -// `endif // BAR +// else begin +// +// else if (FOO) begin +// +// for (FOO) begin // // Rejected examples: -// `ifdef FOO -// `endif +// if (FOO) +// BAR +// +// else +// BAR +// +// else if (FOO) +// BAR +// +// for (FOO) begin +// BAR // -// `ifdef FOO -// `endif // BAR // class ExplicitBeginRule : public verible::TokenStreamLintRule { public: @@ -58,12 +67,7 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { private: // States of the internal token-based analysis. - enum class State { - kNormal, - kInCondition, - kInElse, - kExpectBegin - }; + enum class State { kNormal, kInCondition, kInElse, kExpectBegin }; // Internal lexical analysis state. State state_; @@ -73,8 +77,8 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { // Token information for the last seen block opening (for/if/else). verible::TokenInfo last_condition_start_ = verible::TokenInfo::EOFToken(); - verible::TokenInfo end_of_condition_statement_ = verible::TokenInfo::EOFToken(); - + verible::TokenInfo end_of_condition_statement_ = + verible::TokenInfo::EOFToken(); // Collection of found violations. std::set violations_; diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc new file mode 100644 index 000000000..025678996 --- /dev/null +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -0,0 +1,101 @@ +// Copyright 2017-2020 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/explicit_begin_rule.h" + +#include + +#include "common/analysis/linter_test_utils.h" +#include "common/analysis/token_stream_linter_test_utils.h" +#include "common/text/symbol.h" +#include "gtest/gtest.h" +#include "verilog/analysis/verilog_analyzer.h" +#include "verilog/parser/verilog_token_enum.h" + +namespace verilog { +namespace analysis { +namespace { + +using verible::LintTestCase; +using verible::RunApplyFixCases; +using verible::RunLintTestCases; + +// Tests that space-only text passes. +TEST(ExplicitBeginRuleTest, AcceptsBlank) { + const std::initializer_list kTestCases = { + {""}, + {" "}, + {"\n"}, + {" \n\n"}, + }; + RunLintTestCases(kTestCases); +} + +// Tests that properly matched if/begin passes. +TEST(ExplicitBeginRuleTest, AcceptsEndifWithComment) { + const std::initializer_list kTestCases = { + {"if (FOO) begin"}, + {"if (FOO)\n begin"}, + {"if (FOO) //Comment\n begin"}, + {"else begin \n FOO"}, + {"else \nbegin \n FOO"}, + {"else //Comment\n begin \n FOO"}, + {"else \n //Comment\n begin \n FOO"}, + {"else if (FOO) begin"}, + {"else if (FOO)\n begin"}, + {"else if (FOO) //Comment\n begin"}, + {"else if (FOO)\n //Comment\n begin"}, + }; + RunLintTestCases(kTestCases); +} + +// Tests that unmatched block/begin fails is detected. +TEST(ExplicitBeginRuleTest, RejectsEndifWithoutComment) { + const std::initializer_list kTestCases = { + {"if (FOO) BAR"}, + {"if (FOO)\n BAR"}, + {"if (FOO) //Comment\n BAR"}, + {"else \n FOO"}, + {"else \n \n FOO"}, + {"else //Comment\n FOO"}, + {"else \n //Comment\n FOO"}, + {"else if (FOO) BAR"}, + {"else if (FOO)\n BAR"}, + {"else if (FOO) //Comment\n BAR"}, + {"else if (FOO)\n //Comment\n BAR"}, + }; + RunLintTestCases(kTestCases); +} + +TEST(ExplicitBeginRuleTest, ApplyAutoFix) { + // Alternatives the auto fix offers + const std::initializer_list kTestCases = { + {"if (FOO) BAR","if (FOO) begin BAR"}, + {"if (FOO)\n BAR","if (FOO) begin\n BAR"}, + {"if (FOO) //Comment\n BAR","if (FOO) begin //Comment\n BAR"}, + {"else \n FOO","else begin \n FOO"}, + {"else \n \n FOO","else begin \n \n FOO"}, + {"else //Comment\n FOO","else begin //Comment\n FOO"}, + {"else \n //Comment\n FOO","else begin \n //Comment\n FOO"}, + {"else if (FOO) BAR","else if (FOO) begin BAR"}, + {"else if (FOO)\n BAR","else if (FOO) begin\n BAR"}, + {"else if (FOO) //Comment\n BAR","else if (FOO) begin //Comment\n BAR"}, + {"else if (FOO)\n //Comment\n BAR","else if (FOO) begin\n //Comment\n BAR"}, + }; + RunApplyFixCases(kTestCases, ""); +} + +} // namespace +} // namespace analysis +} // namespace verilog From 3178218ea37749023e60b64d32b1b7ba8e681a3e Mon Sep 17 00:00:00 2001 From: Julien FAUCHER Date: Tue, 31 May 2022 16:07:18 +0200 Subject: [PATCH 06/12] [LINT] Update explicit-begin tests Remove the fix suggestion as invalid --- .../checkers/explicit_begin_rule_test.cc | 66 +++++++------------ 1 file changed, 24 insertions(+), 42 deletions(-) diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc index 025678996..6f64e40e8 100644 --- a/verilog/analysis/checkers/explicit_begin_rule_test.cc +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -43,59 +43,41 @@ TEST(ExplicitBeginRuleTest, AcceptsBlank) { } // Tests that properly matched if/begin passes. -TEST(ExplicitBeginRuleTest, AcceptsEndifWithComment) { +TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { const std::initializer_list kTestCases = { - {"if (FOO) begin"}, - {"if (FOO)\n begin"}, - {"if (FOO) //Comment\n begin"}, - {"else begin \n FOO"}, - {"else \nbegin \n FOO"}, - {"else //Comment\n begin \n FOO"}, - {"else \n //Comment\n begin \n FOO"}, - {"else if (FOO) begin"}, - {"else if (FOO)\n begin"}, - {"else if (FOO) //Comment\n begin"}, - {"else if (FOO)\n //Comment\n begin"}, + {"if (FOO) begin"}, + {"if (FOO)\n begin"}, + {"if (FOO) //Comment\n begin"}, + {"else begin \n FOO"}, + {"else \nbegin \n FOO"}, + {"else //Comment\n begin \n FOO"}, + {"else \n //Comment\n begin \n FOO"}, + {"else if (FOO) begin"}, + {"else if (FOO)\n begin"}, + {"else if (FOO) //Comment\n begin"}, + {"else if (FOO)\n //Comment\n begin"}, }; RunLintTestCases(kTestCases); } // Tests that unmatched block/begin fails is detected. -TEST(ExplicitBeginRuleTest, RejectsEndifWithoutComment) { +TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { const std::initializer_list kTestCases = { - {"if (FOO) BAR"}, - {"if (FOO)\n BAR"}, - {"if (FOO) //Comment\n BAR"}, - {"else \n FOO"}, - {"else \n \n FOO"}, - {"else //Comment\n FOO"}, - {"else \n //Comment\n FOO"}, - {"else if (FOO) BAR"}, - {"else if (FOO)\n BAR"}, - {"else if (FOO) //Comment\n BAR"}, - {"else if (FOO)\n //Comment\n BAR"}, + {{TK_if, "if"}, " (FOO) BAR"}, + {{TK_if, "if"}, " (FOO)\n BAR"}, + {{TK_if, "if"}, " (FOO) //Comment\n BAR"}, + {{TK_else, "else"}, " \n FOO"}, + {{TK_else, "else"}, " \n \n FOO"}, + {{TK_else, "else"}, " //Comment\n FOO"}, + {{TK_else, "else"}, " \n //Comment\n FOO"}, + {"else ", {TK_if, "if"}, " (FOO) BAR"}, + {"else ", {TK_if, "if"}, " (FOO)\n BAR"}, + {"else ", {TK_if, "if"}, " (FOO) //Comment\n BAR"}, + {"else ", {TK_if, "if"}, " (FOO)\n //Comment\n BAR"}, }; RunLintTestCases(kTestCases); } -TEST(ExplicitBeginRuleTest, ApplyAutoFix) { - // Alternatives the auto fix offers - const std::initializer_list kTestCases = { - {"if (FOO) BAR","if (FOO) begin BAR"}, - {"if (FOO)\n BAR","if (FOO) begin\n BAR"}, - {"if (FOO) //Comment\n BAR","if (FOO) begin //Comment\n BAR"}, - {"else \n FOO","else begin \n FOO"}, - {"else \n \n FOO","else begin \n \n FOO"}, - {"else //Comment\n FOO","else begin //Comment\n FOO"}, - {"else \n //Comment\n FOO","else begin \n //Comment\n FOO"}, - {"else if (FOO) BAR","else if (FOO) begin BAR"}, - {"else if (FOO)\n BAR","else if (FOO) begin\n BAR"}, - {"else if (FOO) //Comment\n BAR","else if (FOO) begin //Comment\n BAR"}, - {"else if (FOO)\n //Comment\n BAR","else if (FOO) begin\n //Comment\n BAR"}, - }; - RunApplyFixCases(kTestCases, ""); -} - } // namespace } // namespace analysis } // namespace verilog From 35e3bbf818e6215196cb3cece2df114ec19f4036 Mon Sep 17 00:00:00 2001 From: Julien FAUCHER Date: Tue, 31 May 2022 16:08:40 +0200 Subject: [PATCH 07/12] [LINT] Remove fix suggestion also move the error anchor to offending if/else/for in order to allow test to pass --- verilog/analysis/checkers/explicit_begin_rule.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc index 3f38494cf..4e83d4524 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.cc +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -141,11 +141,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo& token) { if (raise_violation) { violations_.insert(LintViolation( - token, absl::StrCat(kMessage, " Expected begin, got ", token.text()), - {AutoFix( - "Insert begin", - {end_of_condition_statement_, - absl::StrCat(end_of_condition_statement_.text(), " begin")})})); + last_condition_start_, absl::StrCat(kMessage, " Expected begin, got ", token.text()))); // Once the violation is raised, we go back to a normal, default, state condition_expr_level_ = 0; From 53c897655f26cbb48bbc8ec58f8ef06922a25222 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Sat, 27 Apr 2024 00:09:33 +1000 Subject: [PATCH 08/12] Handle consecutive statements missing begin tokens --- .../analysis/checkers/explicit_begin_rule.cc | 47 ++++++++++++------- .../analysis/checkers/explicit_begin_rule.h | 2 + .../checkers/explicit_begin_rule_test.cc | 22 +++++++-- 3 files changed, 50 insertions(+), 21 deletions(-) diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc index fbf401688..542ddd600 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.cc +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -138,24 +138,13 @@ bool ExplicitBeginRule::IsTokenEnabled(const TokenInfo &token) { } } -void ExplicitBeginRule::HandleToken(const TokenInfo &token) { - // Ignore all white space and comments and return immediately - switch (token.token_enum()) { - case TK_SPACE: - case TK_NEWLINE: - case TK_COMMENT_BLOCK: - case TK_EOL_COMMENT: - return; - default: - break; - } - +bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { // Responds to a token by updating the state of the analysis. bool raise_violation = false; switch (state_) { case State::kNormal: { if (!IsTokenEnabled(token)) { - return; + break; } switch (token.token_enum()) { @@ -198,7 +187,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { break; } case State::kInAlways: { - // always is a little more complicated in that it can be imediattly + // always is a little more complicated in that it can be immediately // followed by a "begin" or followed by some special characters ("@" or // "*") and maybe a condition. switch (token.token_enum()) { @@ -209,7 +198,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { state_ = State::kNormal; break; case '(': - condition_expr_level_ = 1; + condition_expr_level_++; state_ = State::kInCondition; break; default: @@ -283,9 +272,33 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { " Expected begin, got ", token.text()))); // Once the violation is raised, we go back to a normal, default, state - condition_expr_level_ = 0; state_ = State::kNormal; - raise_violation = false; + } + + return raise_violation; +} + +void ExplicitBeginRule::HandleToken(const TokenInfo &token) { + // Ignore all white space and comments and return immediately + switch (token.token_enum()) { + case TK_SPACE: + case TK_NEWLINE: + case TK_COMMENT_BLOCK: + case TK_EOL_COMMENT: + return; + default: + break; + } + + bool retry = HandleTokenStateMachine(token); + + // If this token raises a violation, it was because the statemachine was + // expecting a begin token. This token may expect a begin token too, so + // check it. + // Consider: forever if(a) #10; else #20; // 3 violations could be raised + // [forever, if, else]. + if (retry) { + HandleTokenStateMachine(token); } } diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h index 2117a1b24..005e20440 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.h +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -46,6 +46,8 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { verible::LintRuleStatus Report() const final; private: + bool HandleTokenStateMachine(const TokenInfo &token); + bool IsTokenEnabled(const TokenInfo &token); // States of the internal token-based analysis. diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc index 6c3bc5adb..3a99ce983 100644 --- a/verilog/analysis/checkers/explicit_begin_rule_test.cc +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -135,16 +135,20 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_for, "for"}, "(i = 0; i < N; i++)\n a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++) // Comment \n a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++)\n // Comment\n a <= 1'b1;"}, + {{TK_for, "for"}, + "(i = 0; i < N; i++)\n", + {TK_if, "if"}, + " (FOO) a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i])\n a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i]) // Comment \n a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i])\n // Comment\n a <= 1'b1;"}, - {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, - {{TK_while, "while"}, " (array[i])\n a <= 1'b1;"}, - {{TK_while, "while"}, "(array[i]) // Comment \n a <= 1'b1;"}, - {{TK_while, "while"}, " (array[i])\n // Comment\n a <= 1'b1;"}, + {{TK_while, "while"}, "(i < N) a <= 1'b1;"}, + {{TK_while, "while"}, " (i < N)\n a <= 1'b1;"}, + {{TK_while, "while"}, "(i < N) // Comment \n a <= 1'b1;"}, + {{TK_while, "while"}, " (i < N)\n // Comment\n a <= 1'b1;"}, {{TK_forever, "forever"}, " a <= 1'b1;\n"}, {{TK_forever, "forever"}, "\n a <= 1'b1;"}, @@ -182,6 +186,16 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_always, "always"}, " @* a = 1'b1;"}, {{TK_always, "always"}, " a = 1'b1;"}, {{TK_always, "always"}, " #10 a = 1'b1;"}, + + // Multiple consecutive failures + {{TK_if, "if"}, "(FOO) ", {TK_for, "for"}, "(i = 0; i < N; i++) a <= i;"}, + {{TK_if, "if"}, "(FOO) ", {TK_foreach, "foreach"}, "(array[i]) a <= i;"}, + {{TK_if, "if"}, "(FOO) ", {TK_while, "while"}, "(i < N) i++;"}, + {"generate ", + {TK_if, "if"}, + "(FOO):g_foo\n", + {TK_always_comb, "always_comb\n"}, + " a = 1'b1;\n endgenerate"}, }; RunLintTestCases(kTestCases); From 97a253e6d11c4bdf32b7a30e91fb9b132e4877b5 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Sat, 27 Apr 2024 00:09:33 +1000 Subject: [PATCH 09/12] Handle consecutive statements missing begin tokens --- .../analysis/checkers/explicit_begin_rule.cc | 47 +++++++++------ .../analysis/checkers/explicit_begin_rule.h | 2 + .../checkers/explicit_begin_rule_test.cc | 59 +++++++++++++++++-- 3 files changed, 87 insertions(+), 21 deletions(-) diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc index fbf401688..542ddd600 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.cc +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -138,24 +138,13 @@ bool ExplicitBeginRule::IsTokenEnabled(const TokenInfo &token) { } } -void ExplicitBeginRule::HandleToken(const TokenInfo &token) { - // Ignore all white space and comments and return immediately - switch (token.token_enum()) { - case TK_SPACE: - case TK_NEWLINE: - case TK_COMMENT_BLOCK: - case TK_EOL_COMMENT: - return; - default: - break; - } - +bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { // Responds to a token by updating the state of the analysis. bool raise_violation = false; switch (state_) { case State::kNormal: { if (!IsTokenEnabled(token)) { - return; + break; } switch (token.token_enum()) { @@ -198,7 +187,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { break; } case State::kInAlways: { - // always is a little more complicated in that it can be imediattly + // always is a little more complicated in that it can be immediately // followed by a "begin" or followed by some special characters ("@" or // "*") and maybe a condition. switch (token.token_enum()) { @@ -209,7 +198,7 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { state_ = State::kNormal; break; case '(': - condition_expr_level_ = 1; + condition_expr_level_++; state_ = State::kInCondition; break; default: @@ -283,9 +272,33 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { " Expected begin, got ", token.text()))); // Once the violation is raised, we go back to a normal, default, state - condition_expr_level_ = 0; state_ = State::kNormal; - raise_violation = false; + } + + return raise_violation; +} + +void ExplicitBeginRule::HandleToken(const TokenInfo &token) { + // Ignore all white space and comments and return immediately + switch (token.token_enum()) { + case TK_SPACE: + case TK_NEWLINE: + case TK_COMMENT_BLOCK: + case TK_EOL_COMMENT: + return; + default: + break; + } + + bool retry = HandleTokenStateMachine(token); + + // If this token raises a violation, it was because the statemachine was + // expecting a begin token. This token may expect a begin token too, so + // check it. + // Consider: forever if(a) #10; else #20; // 3 violations could be raised + // [forever, if, else]. + if (retry) { + HandleTokenStateMachine(token); } } diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h index 2117a1b24..005e20440 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.h +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -46,6 +46,8 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { verible::LintRuleStatus Report() const final; private: + bool HandleTokenStateMachine(const TokenInfo &token); + bool IsTokenEnabled(const TokenInfo &token); // States of the internal token-based analysis. diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc index 6c3bc5adb..35c33aae7 100644 --- a/verilog/analysis/checkers/explicit_begin_rule_test.cc +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -110,6 +110,18 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"always @* begin a = 1'b1;"}, {"always begin a = 1'b1;"}, {"always begin #10 a = 1'b1;"}, + + // Multiple consecutive failures + {"if(FOO) begin for(i = 0; i < N; i++) begin a <= i;"}, + {"if(FOO) begin foreach(array[i]) begin a <= i;"}, + {"if(FOO) begin while(i < N) begin i++;"}, + {"always @* begin if(FOO) begin a = 1; end else begin a = 0;"}, + {"always @(*) begin if(FOO) begin a = 1; end else begin a = 0;"}, + {"always @(posedge c) begin if(FOO) begin a <= 1; end else begin " + "a <= 0;"}, + {"always_comb begin if(FOO) begin a = 1; end else begin a = 0;"}, + {"always_ff @(posedge c) begin if(FOO) begin a <= 1; end else begin " + "a <= 0;"}, }; RunLintTestCases(kTestCases); @@ -135,16 +147,20 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_for, "for"}, "(i = 0; i < N; i++)\n a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++) // Comment \n a <= 1'b1;"}, {{TK_for, "for"}, "(i = 0; i < N; i++)\n // Comment\n a <= 1'b1;"}, + {{TK_for, "for"}, + "(i = 0; i < N; i++)\n", + {TK_if, "if"}, + " (FOO) a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i]) a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i])\n a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i]) // Comment \n a <= 1'b1;"}, {{TK_foreach, "foreach"}, "(array[i])\n // Comment\n a <= 1'b1;"}, - {{TK_while, "while"}, "(array[i]) a <= 1'b1;"}, - {{TK_while, "while"}, " (array[i])\n a <= 1'b1;"}, - {{TK_while, "while"}, "(array[i]) // Comment \n a <= 1'b1;"}, - {{TK_while, "while"}, " (array[i])\n // Comment\n a <= 1'b1;"}, + {{TK_while, "while"}, "(i < N) a <= 1'b1;"}, + {{TK_while, "while"}, " (i < N)\n a <= 1'b1;"}, + {{TK_while, "while"}, "(i < N) // Comment \n a <= 1'b1;"}, + {{TK_while, "while"}, " (i < N)\n // Comment\n a <= 1'b1;"}, {{TK_forever, "forever"}, " a <= 1'b1;\n"}, {{TK_forever, "forever"}, "\n a <= 1'b1;"}, @@ -182,6 +198,41 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { {{TK_always, "always"}, " @* a = 1'b1;"}, {{TK_always, "always"}, " a = 1'b1;"}, {{TK_always, "always"}, " #10 a = 1'b1;"}, + + // Multiple consecutive failures + {{TK_if, "if"}, "(FOO) ", {TK_for, "for"}, "(i = 0; i < N; i++) a <= i;"}, + {{TK_if, "if"}, "(FOO) ", {TK_foreach, "foreach"}, "(array[i]) a <= i;"}, + {{TK_if, "if"}, "(FOO) ", {TK_while, "while"}, "(i < N) i++;"}, + {{TK_always, "always"}, + " @* ", + {TK_if, "if"}, + "(FOO) a = 1;", + {TK_else, "else"}, + " a = 0;"}, + {{TK_always, "always"}, + " @(*) ", + {TK_if, "if"}, + "(FOO) a = 1;", + {TK_else, "else"}, + " a = 0;"}, + {{TK_always, "always"}, + " @(posedge c) ", + {TK_if, "if"}, + "(FOO) a <= 1;", + {TK_else, "else"}, + " a <= 0;"}, + {{TK_always_comb, "always_comb"}, + " ", + {TK_if, "if"}, + "(FOO) a = 1;", + {TK_else, "else"}, + " a = 0;"}, + {{TK_always_ff, "always_ff"}, + " @(posedge c) ", + {TK_if, "if"}, + "(FOO) a <= 1;", + {TK_else, "else"}, + " a <= 0;"}, }; RunLintTestCases(kTestCases); From 40b1497157ab219a52b5200ad5f70f4ec23bbd5b Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Tue, 30 Apr 2024 23:44:00 +1000 Subject: [PATCH 10/12] Fixed and added lint tool tests --- verilog/tools/lint/BUILD | 1 + verilog/tools/lint/testdata/always_comb_blocking.sv | 3 ++- verilog/tools/lint/testdata/always_ff_non_blocking.sv | 3 ++- verilog/tools/lint/testdata/explicit_begin.sv | 4 ++++ verilog/tools/lint/testdata/generate_begin_module.sv | 4 +++- verilog/tools/lint/testdata/plusarg_assignment.sv | 4 +++- verilog/tools/lint/testdata/suspicious_semicolon.sv | 1 + 7 files changed, 16 insertions(+), 4 deletions(-) create mode 100644 verilog/tools/lint/testdata/explicit_begin.sv diff --git a/verilog/tools/lint/BUILD b/verilog/tools/lint/BUILD index 75e2ca46a..aad9e55bd 100644 --- a/verilog/tools/lint/BUILD +++ b/verilog/tools/lint/BUILD @@ -61,6 +61,7 @@ _linter_test_configs = [ ("disable-statement", "disable_statement", False), ("endif-comment", "endif_comment", False), ("enum-name-style", "enum_name_style", True), + ("explicit-begin", "explicit_begin", False), ("explicit-function-lifetime", "explicit_function_lifetime", True), ("explicit-function-task-parameter-type", "explicit_function_parameter_type", True), ("explicit-function-task-parameter-type", "explicit_task_parameter_type", True), diff --git a/verilog/tools/lint/testdata/always_comb_blocking.sv b/verilog/tools/lint/testdata/always_comb_blocking.sv index 403cf28fb..e54eb4376 100644 --- a/verilog/tools/lint/testdata/always_comb_blocking.sv +++ b/verilog/tools/lint/testdata/always_comb_blocking.sv @@ -1,4 +1,5 @@ module always_comb_blocking; - always_comb + always_comb begin a <= b; // [Style: combinational-logic] [always-comb-blocking] + end endmodule diff --git a/verilog/tools/lint/testdata/always_ff_non_blocking.sv b/verilog/tools/lint/testdata/always_ff_non_blocking.sv index a699fb8ba..0a10182b5 100644 --- a/verilog/tools/lint/testdata/always_ff_non_blocking.sv +++ b/verilog/tools/lint/testdata/always_ff_non_blocking.sv @@ -1,4 +1,5 @@ module always_ff_non_blocking; - always_ff @(posedge c) + always_ff @(posedge c) begin a = b; // [Style: sequential-logic] [always-ff-non-blocking] + end endmodule diff --git a/verilog/tools/lint/testdata/explicit_begin.sv b/verilog/tools/lint/testdata/explicit_begin.sv new file mode 100644 index 000000000..3aadad445 --- /dev/null +++ b/verilog/tools/lint/testdata/explicit_begin.sv @@ -0,0 +1,4 @@ +module explicit_begin (); + always_comb + a = 1; +endmodule diff --git a/verilog/tools/lint/testdata/generate_begin_module.sv b/verilog/tools/lint/testdata/generate_begin_module.sv index 9cd984845..5ed49e144 100644 --- a/verilog/tools/lint/testdata/generate_begin_module.sv +++ b/verilog/tools/lint/testdata/generate_begin_module.sv @@ -2,7 +2,9 @@ module generate_begin_module; // verilog_lint: waive legacy-generate-region generate begin : gen_block1 - always @(posedge clk) foo <= bar; + always @(posedge clk) begin + foo <= bar; + end end endgenerate endmodule diff --git a/verilog/tools/lint/testdata/plusarg_assignment.sv b/verilog/tools/lint/testdata/plusarg_assignment.sv index 89f9f2435..af2210f93 100644 --- a/verilog/tools/lint/testdata/plusarg_assignment.sv +++ b/verilog/tools/lint/testdata/plusarg_assignment.sv @@ -1,6 +1,8 @@ class foo; function bar; // The use of $test$plusargs() is not allowed. - if ($test$plusargs("baz")) return 1; + if ($test$plusargs("baz")) begin + return 1; + end endfunction endclass diff --git a/verilog/tools/lint/testdata/suspicious_semicolon.sv b/verilog/tools/lint/testdata/suspicious_semicolon.sv index ab49fb6cc..45697b1ff 100644 --- a/verilog/tools/lint/testdata/suspicious_semicolon.sv +++ b/verilog/tools/lint/testdata/suspicious_semicolon.sv @@ -1,5 +1,6 @@ module suspicious_semicolon (); initial begin + // verilog_lint: waive explicit-begin if (x); $display("Hi"); end From 06bfc9338ca78486d4e3d29f5501ef9b6083b2a9 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Wed, 19 Jun 2024 22:40:12 +1000 Subject: [PATCH 11/12] Constraints can have if-else and foreach statements, but use curly braces instead of begin-end. To keep things simple, this change ignores all statements inside a constraint. --- .../analysis/checkers/explicit_begin_rule.cc | 49 +++++++++++++++++++ .../analysis/checkers/explicit_begin_rule.h | 15 +++++- .../checkers/explicit_begin_rule_test.cc | 19 +++++++ 3 files changed, 81 insertions(+), 2 deletions(-) diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc index e8faef13a..56fdf5d3a 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.cc +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -137,6 +137,20 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { bool raise_violation = false; switch (state_) { case State::kNormal: { + // Special handling for constraints + switch (token.token_enum()) { + case TK_constraint: { + constraint_expr_level_ = 0; + state_ = State::kConstraint; + return false; + } + case TK_with: { + constraint_expr_level_ = 0; + state_ = State::kInlineConstraint; + return false; + } + } + if (!IsTokenEnabled(token)) { break; } @@ -258,6 +272,41 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { } break; } + case State::kInlineConstraint: { + switch (token.token_enum()) { + case '{': { + // An InlineConstraint + constraint_expr_level_++; + state_ = State::kConstraint; + break; + } + default: { + // throw away everything else + state_ = State::kNormal; + break; + } + } + } + case State::kConstraint: { + // System Verilog constraints are special and use curly braces {} + // instead of begin-end. So ignore all constraints + switch (token.token_enum()) { + case '{': { + constraint_expr_level_++; + break; + } + case '}': { + constraint_expr_level_--; + if (constraint_expr_level_ == 0) { + state_ = State::kNormal; + } + } + default: { + // throw away everything else + break; + } + } + } } // switch (state_) if (raise_violation) { diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h index 4b41b2f09..5604e8e63 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.h +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -51,14 +51,25 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { bool IsTokenEnabled(const TokenInfo &token); // States of the internal token-based analysis. - enum class State { kNormal, kInAlways, kInCondition, kInElse, kExpectBegin }; + enum class State { + kNormal, + kInAlways, + kInCondition, + kInElse, + kExpectBegin, + kConstraint, + kInlineConstraint + }; // Internal lexical analysis state. State state_; - // Level of nested parenthesis when analysing conditional expressions. + // Level of nested parenthesis when analysing conditional expressions int condition_expr_level_; + // Level inside a constraint expression + int constraint_expr_level_; + // Configuration bool if_enable_ = true; bool else_enable_ = true; diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc index c9ab445ba..960e833a7 100644 --- a/verilog/analysis/checkers/explicit_begin_rule_test.cc +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -111,6 +111,16 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"always begin a = 1'b1;"}, {"always begin #10 a = 1'b1;"}, + // Ignore constraints + {"constraint c_array { foreach (array[i]) {array[i] == i;}}"}, + {"constraint c {if(a == 2){b == 1;}else{b == 2;}}"}, + + // Ignore inline constraints + {"task a(); std::randomize(b) with {foreach(b[i]){b[i] inside " + "{[0:1024]};}}; endtask"}, + {"task a(); std::randomize(b) with {if(a == 2){b == 1;}else{b == 2;}}; " + "endtask"}, + // Multiple consecutive failures {"if(FOO) begin for(i = 0; i < N; i++) begin a <= i;"}, {"if(FOO) begin foreach(array[i]) begin a <= i;"}, @@ -123,6 +133,10 @@ TEST(ExplicitBeginRuleTest, AcceptsBlocksWithBegin) { {"always_comb begin if(FOO) begin a = 1; end else begin a = 0;"}, {"always_ff @(posedge c) begin if(FOO) begin a <= 1; end else begin " "a <= 0;"}, + {"constraint c_array { foreach (array[i]) {array[i] == i;}}if(FOO) begin " + "a <= 1;end"}, + {"if(FOO) begin a <= 1;end constraint c {if(a == 2){b == 1;}else{b == " + "2;}}"}, }; RunLintTestCases(kTestCases); @@ -234,6 +248,11 @@ TEST(ExplicitBeginRuleTest, RejectBlocksWithoutBegin) { "(FOO) a <= 1;", {TK_else, "else"}, " a <= 0;"}, + {"constraint c_array { foreach (array[i]) array[i] == i;}", + {TK_if, "if"}, + "(FOO) a <= 1;"}, + {{TK_if, "if"}, + "(FOO) a <= 1; constraint c {if(a == 2) b == 1;else b == 2;}"}, }; RunLintTestCases(kTestCases); From 6da2fe5fc22fc13a4f58220b811b5abb544ce510 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Wed, 4 Sep 2024 23:34:29 +1000 Subject: [PATCH 12/12] Fixing blant, ClangTidy and fallthrough compile errors --- verilog/analysis/checkers/BUILD | 6 ++-- .../analysis/checkers/explicit_begin_rule.cc | 30 +++++++++++++--- .../analysis/checkers/explicit_begin_rule.h | 36 +++++++++---------- .../checkers/explicit_begin_rule_test.cc | 2 -- 4 files changed, 46 insertions(+), 28 deletions(-) diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index d5a59c728..760e7d538 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -1261,13 +1261,15 @@ cc_library( deps = [ "//common/analysis:lint-rule-status", "//common/analysis:token-stream-lint-rule", - "//common/strings:comment-utils", "//common/text:config-utils", "//common/text:token-info", "//verilog/analysis:descriptions", "//verilog/analysis:lint-rule-registry", "//verilog/parser:verilog-token-enum", + "@com_google_absl//absl/base:core_headers", + "@com_google_absl//absl/status", "@com_google_absl//absl/strings", + "@com_google_absl//absl/strings:string_view", ], alwayslink = 1, ) @@ -1279,9 +1281,9 @@ cc_test( ":explicit-begin-rule", "//common/analysis:linter-test-utils", "//common/analysis:token-stream-linter-test-utils", - "//common/text:symbol", "//verilog/analysis:verilog-analyzer", "//verilog/parser:verilog-token-enum", + "@com_google_googletest//:gtest", "@com_google_googletest//:gtest_main", ], ) diff --git a/verilog/analysis/checkers/explicit_begin_rule.cc b/verilog/analysis/checkers/explicit_begin_rule.cc index 56fdf5d3a..44fee0531 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.cc +++ b/verilog/analysis/checkers/explicit_begin_rule.cc @@ -14,16 +14,14 @@ #include "verilog/analysis/checkers/explicit_begin_rule.h" -#include #include -#include -#include +#include "absl/base/attributes.h" +#include "absl/status/status.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "common/analysis/lint_rule_status.h" #include "common/analysis/token_stream_lint_rule.h" -#include "common/strings/comment_utils.h" #include "common/text/config_utils.h" #include "common/text/token_info.h" #include "verilog/analysis/descriptions.h" @@ -33,7 +31,6 @@ namespace verilog { namespace analysis { -using verible::AutoFix; using verible::LintRuleStatus; using verible::LintViolation; using verible::TokenInfo; @@ -149,6 +146,10 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { state_ = State::kInlineConstraint; return false; } + default: { + // Do nothing + break; + } } if (!IsTokenEnabled(token)) { @@ -158,8 +159,11 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { switch (token.token_enum()) { // After token expect "begin" case TK_always_comb: + ABSL_FALLTHROUGH_INTENDED; case TK_always_latch: + ABSL_FALLTHROUGH_INTENDED; case TK_forever: + ABSL_FALLTHROUGH_INTENDED; case TK_initial: start_token_ = token; state_ = State::kExpectBegin; @@ -168,9 +172,13 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { // be tokens prior to the condition (like in an "always_ff" statement) // and these are all ignored. case TK_if: + ABSL_FALLTHROUGH_INTENDED; case TK_always_ff: + ABSL_FALLTHROUGH_INTENDED; case TK_for: + ABSL_FALLTHROUGH_INTENDED; case TK_foreach: + ABSL_FALLTHROUGH_INTENDED; case TK_while: condition_expr_level_ = 0; start_token_ = token; @@ -200,6 +208,7 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { // "*") and maybe a condition. switch (token.token_enum()) { case '@': + break; case '*': break; case TK_begin: @@ -250,6 +259,7 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { if (condition_expr_level_ == 0) { state_ = State::kExpectBegin; } + break; } default: { // throw away everything else @@ -287,6 +297,7 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { } } } + ABSL_FALLTHROUGH_INTENDED; case State::kConstraint: { // System Verilog constraints are special and use curly braces {} // instead of begin-end. So ignore all constraints @@ -300,12 +311,18 @@ bool ExplicitBeginRule::HandleTokenStateMachine(const TokenInfo &token) { if (constraint_expr_level_ == 0) { state_ = State::kNormal; } + break; } default: { // throw away everything else break; } } + break; + } + default: { + // Do nothing + break; } } // switch (state_) @@ -325,8 +342,11 @@ void ExplicitBeginRule::HandleToken(const TokenInfo &token) { // Ignore all white space and comments and return immediately switch (token.token_enum()) { case TK_SPACE: + return; case TK_NEWLINE: + return; case TK_COMMENT_BLOCK: + return; case TK_EOL_COMMENT: return; default: diff --git a/verilog/analysis/checkers/explicit_begin_rule.h b/verilog/analysis/checkers/explicit_begin_rule.h index 5604e8e63..a91205a45 100644 --- a/verilog/analysis/checkers/explicit_begin_rule.h +++ b/verilog/analysis/checkers/explicit_begin_rule.h @@ -16,9 +16,9 @@ #define VERIBLE_VERILOG_ANALYSIS_CHECKERS_EXPLICIT_BEGIN_RULE_H_ #include -#include -#include +#include "absl/status/status.h" +#include "absl/strings/string_view.h" #include "common/analysis/lint_rule_status.h" #include "common/analysis/token_stream_lint_rule.h" #include "common/text/token_info.h" @@ -39,8 +39,6 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { absl::Status Configure(absl::string_view configuration) final; - ExplicitBeginRule() : state_(State::kNormal), condition_expr_level_(0) {} - void HandleToken(const verible::TokenInfo &token) final; verible::LintRuleStatus Report() const final; @@ -62,29 +60,29 @@ class ExplicitBeginRule : public verible::TokenStreamLintRule { }; // Internal lexical analysis state. - State state_; + State state_{State::kNormal}; // Level of nested parenthesis when analysing conditional expressions - int condition_expr_level_; + int condition_expr_level_{0}; // Level inside a constraint expression - int constraint_expr_level_; + int constraint_expr_level_{0}; // Configuration - bool if_enable_ = true; - bool else_enable_ = true; - bool always_enable_ = true; - bool always_comb_enable_ = true; - bool always_latch_enable_ = true; - bool always_ff_enable_ = true; - bool for_enable_ = true; - bool forever_enable_ = true; - bool foreach_enable_ = true; - bool while_enable_ = true; - bool initial_enable_ = true; + bool if_enable_{true}; + bool else_enable_{true}; + bool always_enable_{true}; + bool always_comb_enable_{true}; + bool always_latch_enable_{true}; + bool always_ff_enable_{true}; + bool for_enable_{true}; + bool forever_enable_{true}; + bool foreach_enable_{true}; + bool while_enable_{true}; + bool initial_enable_{true}; // Token that requires blocking statement. - verible::TokenInfo start_token_ = verible::TokenInfo::EOFToken(); + verible::TokenInfo start_token_{verible::TokenInfo::EOFToken()}; // Collection of found violations. std::set violations_; diff --git a/verilog/analysis/checkers/explicit_begin_rule_test.cc b/verilog/analysis/checkers/explicit_begin_rule_test.cc index 960e833a7..50d0d6683 100644 --- a/verilog/analysis/checkers/explicit_begin_rule_test.cc +++ b/verilog/analysis/checkers/explicit_begin_rule_test.cc @@ -18,7 +18,6 @@ #include "common/analysis/linter_test_utils.h" #include "common/analysis/token_stream_linter_test_utils.h" -#include "common/text/symbol.h" #include "gtest/gtest.h" #include "verilog/analysis/verilog_analyzer.h" #include "verilog/parser/verilog_token_enum.h" @@ -28,7 +27,6 @@ namespace analysis { namespace { using verible::LintTestCase; -using verible::RunApplyFixCases; using verible::RunConfiguredLintTestCases; using verible::RunLintTestCases;