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.
diff --git a/common/analysis/linter_test_utils.h b/common/analysis/linter_test_utils.h index 7c7a0b0..88b401f 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 @@ template <class AnalyzerType, class RuleClass> void RunApplyFixCases(std::initializer_list<AutoFixInOut> tests, - absl::string_view configuration) { + absl::string_view configuration = "") { using rule_type = typename RuleClass::rule_type; auto rule_generator = [&configuration]() -> std::unique_ptr<rule_type> { std::unique_ptr<rule_type> instance(new RuleClass());
diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index 1b06db6..f61323d 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD
@@ -62,6 +62,7 @@ ":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 @@ "@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 0000000..043c21a --- /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 "verilog/analysis/checkers/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 0000000..a03a407 --- /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<verible::LintViolation> 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 0000000..3e422f5 --- /dev/null +++ b/verilog/analysis/checkers/suspicious_semicolon_rule_test.cc
@@ -0,0 +1,83 @@ +// 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<verible::LintTestCase> + 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 for(;;)", {kToken, ";"}, " end endmodule"}, + {"module m; initial begin foreach (array[i])", + {kToken, ";"}, + " end endmodule"}, + }; + + verible::RunLintTestCases<VerilogAnalyzer, SuspiciousSemicolon>( + kSuspiciousSemicolonTestCases); +} + +TEST(SuspiciousSemicolon, ShouldNotComplain) { + const std::initializer_list<verible::LintTestCase> + 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"}, + {"module m; endmodule;"}, + {"class c; int x;; endclass"}, + }; + + verible::RunLintTestCases<VerilogAnalyzer, SuspiciousSemicolon>( + kSuspiciousSemicolonTestCases); +} + +TEST(SuspiciousSemicolon, ApplyAutoFix) { + const std::initializer_list<verible::AutoFixInOut> + 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<VerilogAnalyzer, SuspiciousSemicolon>( + kSuspiciousSemicolonTestCases); +} + +} // namespace +} // namespace analysis +} // namespace verilog
diff --git a/verilog/tools/lint/BUILD b/verilog/tools/lint/BUILD index 13a4a24..04fd143 100644 --- a/verilog/tools/lint/BUILD +++ b/verilog/tools/lint/BUILD
@@ -69,6 +69,7 @@ ("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 273d124..492a415 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 0000000..ab49fb6 --- /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