From fccb08b20ae416c0d8415b858bbf135dcd7cdd88 Mon Sep 17 00:00:00 2001 From: Ignacio Encinas Date: Fri, 22 Sep 2023 22:39:43 +0200 Subject: [PATCH] linter: Add suspicious-semicolon rule Inspired by clang-tidy's similar check. Things to note: verilog/tools/lint/testdata/forbid_consecutive_null_statement.sv has been modified so it doesn't fail under this rule too. --- common/analysis/linter_test_utils.h | 4 +- verilog/analysis/checkers/BUILD | 26 ++++++ .../checkers/suspicious_semicolon_rule.cc | 81 +++++++++++++++++++ .../checkers/suspicious_semicolon_rule.h | 62 ++++++++++++++ .../suspicious_semicolon_rule_test.cc | 81 +++++++++++++++++++ verilog/tools/lint/BUILD | 1 + .../forbid_consecutive_null_statements.sv | 1 - .../lint/testdata/suspicious_semicolon.sv | 6 ++ 8 files changed, 259 insertions(+), 3 deletions(-) create mode 100644 verilog/analysis/checkers/suspicious_semicolon_rule.cc create mode 100644 verilog/analysis/checkers/suspicious_semicolon_rule.h create mode 100644 verilog/analysis/checkers/suspicious_semicolon_rule_test.cc create mode 100644 verilog/tools/lint/testdata/suspicious_semicolon.sv diff --git a/common/analysis/linter_test_utils.h b/common/analysis/linter_test_utils.h index 7c7a0b07a7..88b401f543 100644 --- a/common/analysis/linter_test_utils.h +++ b/common/analysis/linter_test_utils.h @@ -1,4 +1,4 @@ -// Copyright 2017-2020 The Verible Authors. +// Copyright 2017-2023 The Verible Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -143,7 +143,7 @@ void RunLintAutoFixCase(const AutoFixInOut &test, template void RunApplyFixCases(std::initializer_list tests, - absl::string_view configuration) { + absl::string_view configuration = "") { using rule_type = typename RuleClass::rule_type; auto rule_generator = [&configuration]() -> std::unique_ptr { std::unique_ptr instance(new RuleClass()); diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index 1b06db65ab..f61323da48 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -62,6 +62,7 @@ cc_library( ":signal-name-style-rule", ":struct-union-name-style-rule", ":suggest-parentheses-rule", + ":suspicious-semicolon-rule", ":token-stream-lint-rule", ":truncated-numeric-literal-rule", ":undersized-binary-literal-rule", @@ -2123,3 +2124,28 @@ cc_test( "@com_google_googletest//:gtest_main", ], ) + +cc_library( + name = "suspicious-semicolon-rule", + srcs = ["suspicious_semicolon_rule.cc"], + hdrs = ["suspicious_semicolon_rule.h"], + deps = [ + "//common/analysis:lint-rule-status", + "//common/analysis:syntax-tree-lint-rule", + "//verilog/CST:verilog-matchers", + "//verilog/analysis:descriptions", + "//verilog/analysis:lint-rule-registry", + ], + alwayslink = 1, +) + +cc_test( + name = "suspicious-semicolon-rule_test", + srcs = ["suspicious_semicolon_rule_test.cc"], + deps = [ + ":suspicious-semicolon-rule", + "//common/analysis:syntax-tree-linter-test-utils", + "//verilog/analysis:verilog-analyzer", + "@com_google_googletest//:gtest_main", + ], +) diff --git a/verilog/analysis/checkers/suspicious_semicolon_rule.cc b/verilog/analysis/checkers/suspicious_semicolon_rule.cc new file mode 100644 index 0000000000..a8a8338965 --- /dev/null +++ b/verilog/analysis/checkers/suspicious_semicolon_rule.cc @@ -0,0 +1,81 @@ +// Copyright 2017-2023 The Verible Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "suspicious_semicolon_rule.h" + +#include "common/analysis/matcher/matcher.h" +#include "verilog/CST/verilog_matchers.h" +#include "verilog/CST/verilog_nonterminals.h" +#include "verilog/analysis/lint_rule_registry.h" + +namespace verilog { +namespace analysis { + +using verible::matcher::Matcher; + +VERILOG_REGISTER_LINT_RULE(SuspiciousSemicolon); + +static constexpr absl::string_view kMessage = + "Potentially unintended semicolon"; + +const LintRuleDescriptor &SuspiciousSemicolon::GetDescriptor() { + static const LintRuleDescriptor d{ + .name = "suspicious-semicolon", + .topic = "bugprone", + .desc = + "Checks that there are no suspicious semicolons that might affect " + "code behaviour but escape quick visual inspection"}; + return d; +} + +static const Matcher &NullStatementMatcher() { + static const Matcher matcher(NodekNullStatement()); + return matcher; +} + +void SuspiciousSemicolon::HandleNode( + const verible::SyntaxTreeNode &node, + const verible::SyntaxTreeContext &context) { + verible::matcher::BoundSymbolManager manager; + if (!NullStatementMatcher().Matches(node, &manager)) return; + + // Waive @(posedge clk); + // But catch always_ff @(posedge clk); + const bool parent_is_proc_timing_ctrl_statement = + context.DirectParentIs(NodeEnum::kProceduralTimingControlStatement); + if (!context.IsInside(NodeEnum::kAlwaysStatement) && + parent_is_proc_timing_ctrl_statement) { + return; + } + + if (!parent_is_proc_timing_ctrl_statement && + !context.DirectParentIsOneOf( + {NodeEnum::kForeachLoopStatement, NodeEnum::kWhileLoopStatement, + NodeEnum::kForLoopStatement, NodeEnum::kForeverLoopStatement, + NodeEnum::kIfBody, NodeEnum::kElseBody})) { + return; + } + + violations_.insert(verible::LintViolation( + node, kMessage, context, + {verible::AutoFix("Remove ';'", + {verible::StringSpanOfSymbol(node), ""})})); +} + +verible::LintRuleStatus SuspiciousSemicolon::Report() const { + return verible::LintRuleStatus(violations_, GetDescriptor()); +} + +} // namespace analysis +} // namespace verilog diff --git a/verilog/analysis/checkers/suspicious_semicolon_rule.h b/verilog/analysis/checkers/suspicious_semicolon_rule.h new file mode 100644 index 0000000000..a03a407e6a --- /dev/null +++ b/verilog/analysis/checkers/suspicious_semicolon_rule.h @@ -0,0 +1,62 @@ +// Copyright 2017-2023 The Verible Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_SUSPICIOUS_SEMICOLON_H_ +#define VERIBLE_VERILOG_ANALYSIS_CHECKERS_SUSPICIOUS_SEMICOLON_H_ + +#include "common/analysis/lint_rule_status.h" +#include "common/analysis/syntax_tree_lint_rule.h" +#include "verilog/analysis/descriptions.h" + +namespace verilog { +namespace analysis { + +/* + * Detect suspicious semicolons. Inspired by clang-tidy's + * bugprone-suspicious-semicolon check. + * + * This rule detects extra semicolons that modify code behaviour + * while having a good chance to escape quick visual inspection. + * + * A couple of examples: + * + * if (condition); + * `uvm_fatal(...); + * + * while (condition); begin + * doSomething(); + * end + * + * Reference: + * https://clang.llvm.org/extra/clang-tidy/checks/bugprone/suspicious-semicolon.html#bugprone-suspicious-semicolon + */ +class SuspiciousSemicolon : public verible::SyntaxTreeLintRule { + public: + using rule_type = verible::SyntaxTreeLintRule; + + static const LintRuleDescriptor &GetDescriptor(); + + void HandleNode(const verible::SyntaxTreeNode &node, + const verible::SyntaxTreeContext &context) final; + + verible::LintRuleStatus Report() const final; + + private: + std::set violations_; +}; + +} // namespace analysis +} // namespace verilog + +#endif // VERIBLE_VERILOG_ANALYSIS_CHECKERS_SUSPICIOUS_SEMICOLON_H_ diff --git a/verilog/analysis/checkers/suspicious_semicolon_rule_test.cc b/verilog/analysis/checkers/suspicious_semicolon_rule_test.cc new file mode 100644 index 0000000000..927550b0d1 --- /dev/null +++ b/verilog/analysis/checkers/suspicious_semicolon_rule_test.cc @@ -0,0 +1,81 @@ +// Copyright 2017-2023 The Verible Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "verilog/analysis/checkers/suspicious_semicolon_rule.h" + +#include "common/analysis/syntax_tree_linter_test_utils.h" +#include "verilog/analysis/verilog_analyzer.h" + +namespace verilog { +namespace analysis { +namespace { + +static constexpr int kToken = ';'; +TEST(SuspiciousSemicolon, DetectSuspiciousSemicolons) { + const std::initializer_list + kSuspiciousSemicolonTestCases = { + {"module m; initial begin if(x)", {kToken, ";"}, " end endmodule"}, + {"module m; initial begin if(x) x; else", + {kToken, ";"}, + " y; end endmodule"}, + {"module m; initial begin while(x)", {kToken, ";"}, " end endmodule"}, + {"module m; initial begin forever", {kToken, ";"}, " end endmodule"}, + {"module m; always_ff @(posedge clk)", {kToken, ";"}, " endmodule"}, + {"module m; initial begin foreach (array[i])", + {kToken, ";"}, + " end endmodule"}, + }; + + verible::RunLintTestCases( + kSuspiciousSemicolonTestCases); +} + +TEST(SuspiciousSemicolon, ShouldNotComplain) { + const std::initializer_list + kSuspiciousSemicolonTestCases = { + {""}, + {"module m; initial begin if(x) begin end end endmodule"}, + {"module m; @(posedge clk); endmodule"}, + {"module m; always_ff @(posedge clk) begin ; end endmodule"}, + {"class c; int x;; endclass"}, + }; + + verible::RunLintTestCases( + kSuspiciousSemicolonTestCases); +} + +TEST(SuspiciousSemicolon, ApplyAutoFix) { + const std::initializer_list + kSuspiciousSemicolonTestCases = { + {"module m; initial begin if(x); end endmodule", + "module m; initial begin if(x) end endmodule"}, + {"module m; initial begin if(x) x; else; y; end endmodule", + "module m; initial begin if(x) x; else y; end endmodule"}, + {"module m; initial begin while(x); end endmodule", + "module m; initial begin while(x) end endmodule"}, + {"module m; initial begin forever; end endmodule", + "module m; initial begin forever end endmodule"}, + {"module m; always_ff @(posedge clk); endmodule", + "module m; always_ff @(posedge clk) endmodule"}, + {"module m; initial begin foreach (array[i]); end endmodule", + "module m; initial begin foreach (array[i]) end endmodule"}, + }; + + verible::RunApplyFixCases( + kSuspiciousSemicolonTestCases); +} + +} // namespace +} // namespace analysis +} // namespace verilog diff --git a/verilog/tools/lint/BUILD b/verilog/tools/lint/BUILD index 13a4a24e9a..04fd143f65 100644 --- a/verilog/tools/lint/BUILD +++ b/verilog/tools/lint/BUILD @@ -69,6 +69,7 @@ _linter_test_configs = [ ("signal-name-style", "signal_name_style", False), ("struct-union-name-style", "struct_name_style", True), ("struct-union-name-style", "union_name_style", True), + ("suspicious-semicolon", "suspicious_semicolon", False), ("v2001-generate-begin", "generate_begin_module", True), ("void-cast", "void-cast", True), ("undersized-binary-literal", "undersized_binary_literal", True), diff --git a/verilog/tools/lint/testdata/forbid_consecutive_null_statements.sv b/verilog/tools/lint/testdata/forbid_consecutive_null_statements.sv index 273d12437d..492a415e1e 100644 --- a/verilog/tools/lint/testdata/forbid_consecutive_null_statements.sv +++ b/verilog/tools/lint/testdata/forbid_consecutive_null_statements.sv @@ -1,4 +1,3 @@ module forbid_consecutive_null_statements; - always_ff @(posedge foo) ;; // [Style: consecutive-null-statements] [forbid-consecutive-null-statements] endmodule diff --git a/verilog/tools/lint/testdata/suspicious_semicolon.sv b/verilog/tools/lint/testdata/suspicious_semicolon.sv new file mode 100644 index 0000000000..ab49fb6cc7 --- /dev/null +++ b/verilog/tools/lint/testdata/suspicious_semicolon.sv @@ -0,0 +1,6 @@ +module suspicious_semicolon (); + initial begin + if (x); + $display("Hi"); + end +endmodule