Merge pull request #2016 from IEncinas10/bugprone-suspicious-semicolon

linter: Add suspicious-semicolon rule

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 <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());

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",
+  ],
+)

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

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_

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

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),

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

verilog/tools/lint/testdata/suspicious_semicolon.sv

@@ -0,0 +1,6 @@
+module suspicious_semicolon ();
+  initial begin
+    if (x);
+      $display("Hi");
+  end
+endmodule