Add autofixes for always_ff_non_blocking_rule

verilog/analysis/checkers/BUILD

@@ -1204,6 +1204,8 @@ cc_library(
         "//common/text:symbol",
         "//common/text:syntax-tree-context",
         "//common/util:casts",
+        "//verilog/CST:expression",
+        "//verilog/CST:statement",
         "//verilog/CST:verilog-matchers",
         "//verilog/analysis:descriptions",
         "//verilog/analysis:lint-rule-registry",

verilog/analysis/checkers/always_ff_non_blocking_rule.cc

@@ -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.
@@ -17,6 +17,7 @@
 #include <set>
 #include <string>
 
+#include "absl/strings/match.h"
 #include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
 #include "common/analysis/lint_rule_status.h"
@@ -27,13 +28,16 @@
 #include "common/text/symbol.h"
 #include "common/text/syntax_tree_context.h"
 #include "common/util/casts.h"
+#include "verilog/CST/expression.h"
+#include "verilog/CST/statement.h"
 #include "verilog/CST/verilog_matchers.h"  // IWYU pragma: keep
 #include "verilog/analysis/descriptions.h"
 #include "verilog/analysis/lint_rule_registry.h"
 
 namespace verilog {
 namespace analysis {
 
+using verible::AutoFix;
 using verible::LintRuleStatus;
 using verible::LintViolation;
 using verible::SearchSyntaxTree;
@@ -97,31 +101,74 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol,
   static const Matcher asgn_incdec_matcher{NodekIncrementDecrementExpression()};
   static const Matcher ident_matcher{NodekUnqualifiedId()};
 
+  std::vector<AutoFix> autofixes;
+
   // Rule may be waived if complete lhs consists of local variables
   //  -> determine root of lhs
   const verible::Symbol *check_root = nullptr;
+  absl::string_view lhs_id;
 
   verible::matcher::BoundSymbolManager symbol_man;
   if (asgn_blocking_matcher.Matches(symbol, &symbol_man)) {
-    if (const auto *const node =
-            verible::down_cast<const verible::SyntaxTreeNode *>(&symbol)) {
-      check_root =
-          /* lhs */ verible::down_cast<const verible::SyntaxTreeNode *>(
-              node->children()[0].get());
-    }
+    const verible::SyntaxTreeNode *node = &verible::SymbolCastToNode(symbol);
+    check_root = verilog::GetNetVariableAssignmentLhs(*node);
+    lhs_id = verible::StringSpanOfSymbol(*check_root);
+
+    const verible::SyntaxTreeLeaf *leaf =
+        verible::GetSubtreeAsLeaf(symbol, NodeEnum::kNetVariableAssignment, 1);
+
+    autofixes.emplace_back(AutoFix(
+        "Substitute blocking assignment '=' for nonblocking assignment '<='",
+        {leaf->get(), "<="}));
   } else {
     // Not interested in any other blocking assignments unless flagged
     if (!catch_modifying_assignments_) return;
 
+    // These autofixes require substituting the whole expression
+    absl::string_view original = verible::StringSpanOfSymbol(symbol);
     if (asgn_modify_matcher.Matches(symbol, &symbol_man)) {
-      if (const auto *const node =
-              verible::down_cast<const verible::SyntaxTreeNode *>(&symbol)) {
-        check_root =
-            /* lhs */ verible::down_cast<const verible::SyntaxTreeNode *>(
-                node->children()[0].get());
-      }
+      const verible::SyntaxTreeNode *node = &verible::SymbolCastToNode(symbol);
+      const verible::SyntaxTreeNode &lhs = *GetAssignModifyLhs(*node);
+      const verible::SyntaxTreeNode &rhs = *GetAssignModifyRhs(*node);
+      const verible::SyntaxTreeLeaf &operator_ =
+          *GetAssignModifyOperator(*node);
+      check_root = &lhs;
+
+      bool needs_parenthesis = NeedsParenthesis(rhs);
+
+      lhs_id = verible::StringSpanOfSymbol(lhs);
+      const std::string fix = absl::StrCat(
+          lhs_id, " <= ", lhs_id, " ",
+          verible::StringSpanOfSymbol(operator_).substr(0, 1), " ",
+          needs_parenthesis ? "(" : "", verible::StringSpanOfSymbol(rhs),
+          needs_parenthesis ? ")" : "", ";");
+
+      autofixes.emplace_back(
+          AutoFix("Substitute assignment operator for equivalent "
+                  "nonblocking assignment",
+                  {original, fix}));
     } else if (asgn_incdec_matcher.Matches(symbol, &symbol_man)) {
-      check_root = &symbol;
+      const verible::SyntaxTreeNode *operand =
+          GetIncrementDecrementOperand(symbol);
+      const verible::SyntaxTreeLeaf *operator_ =
+          GetIncrementDecrementOperator(symbol);
+      check_root = operand;
+
+      lhs_id = verible::StringSpanOfSymbol(*operand);
+      // Extract just the operation. Just '+' from '++'
+      const absl::string_view op =
+          verible::StringSpanOfSymbol(*operator_).substr(0, 1);
+
+      // Equivalent nonblocking assignment
+      // {'x++', '++x'} become 'x <= x + 1'
+      // {'x--', '--x'} become 'x <= x - 1'
+      const std::string fix =
+          absl::StrCat(lhs_id, " <= ", lhs_id, " ", op, " 1;");
+
+      autofixes.emplace_back(
+          AutoFix("Substitute increment/decrement operator for "
+                  "equivalent nonblocking assignment",
+                  {original, fix}));
     } else {
       // Not a blocking assignment
       return;
@@ -155,7 +202,13 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol,
   }
 
   // Enqueue detected violation unless waived
-  if (!waived) violations_.insert(LintViolation(symbol, kMessage, context));
+  if (waived) return;
+
+  if (IsAutoFixSafe(symbol, lhs_id)) {
+    violations_.insert(LintViolation(symbol, kMessage, context, autofixes));
+  } else {
+    violations_.insert(LintViolation(symbol, kMessage, context));
+  }
 }  // HandleSymbol()
 
 bool AlwaysFFNonBlockingRule::InsideBlock(const verible::Symbol &symbol,
@@ -180,6 +233,7 @@ bool AlwaysFFNonBlockingRule::InsideBlock(const verible::Symbol &symbol,
     if (always_ff_matcher.Matches(symbol, &symbol_man)) {
       VLOG(4) << "always_ff @DEPTH=" << depth << std::endl;
       inside_ = depth;
+      CollectLocalReferences(symbol);
     }
     return false;
   }
@@ -222,5 +276,94 @@ bool AlwaysFFNonBlockingRule::LocalDeclaration(const verible::Symbol &symbol) {
   return false;
 }  // LocalDeclaration()
 
+bool AlwaysFFNonBlockingRule::NeedsParenthesis(
+    const verible::Symbol &rhs) const {
+  // Avoid inserting parenthesis for simple expressions
+  // For example: x &= 1 -> x <= x & 1, and not x <= x & (1)
+  // This could be more precise, but checking every specific
+  // case where parenthesis are needed is hard. Adding them
+  // doesn't hurt and the user can remove them if needed.
+  bool complex_rhs_expr =
+      verible::GetLeftmostLeaf(rhs) != verible::GetRightmostLeaf(rhs);
+
+  // Check whether the rhs expression is already inside parenthesis
+  // Avoid searching for kParenGroups, this should be safe and faster
+  return complex_rhs_expr &&
+         !absl::StrContains(verible::StringSpanOfSymbol(rhs), '(');
+}
+
+void AlwaysFFNonBlockingRule::CollectLocalReferences(
+    const verible::Symbol &root) {
+  static const Matcher reference_matcher{NodekReference()};
+
+  const std::vector<verible::TreeSearchMatch> references_ =
+      verible::SearchSyntaxTree(root, reference_matcher);
+
+  // Precompute the StringSpan of every identifier referenced to.
+  // Avoid recomputing it several times
+  references.resize(references_.size());
+  std::transform(references_.cbegin(), references_.cend(), references.begin(),
+                 [](const verible::TreeSearchMatch &match) {
+                   return ReferenceWithId{
+                       match, verible::StringSpanOfSymbol(*match.match)};
+                 });
+}
+
+bool AlwaysFFNonBlockingRule::IsAutoFixSafe(
+    const verible::Symbol &faulting_assignment,
+    absl::string_view lhs_id) const {
+  std::vector<ReferenceWithId>::const_iterator itr = references.end();
+
+  // Let's assume that 'x' is the variable affected by the faulting
+  // assignment. In order to ensure that the autofix is safe we have
+  // to ensure that there is no later reference to 'x'
+  //
+  // Can't autofix        Can autofix
+  // begin                begin
+  //   x = x + 1;           x = x + 1;
+  //   y = x;               y <= y + 1;
+  // end                  end
+  //
+  // In practical terms: we'll scan the 'references' vector for kReferences
+  // to 'x' that appear after the faulting assignment in the always_ff block
+
+  static const Matcher reference_matcher{NodekReference()};
+
+  // Extract kReferences inside the faulting expression
+  const std::vector<verible::TreeSearchMatch> references_ =
+      verible::SearchSyntaxTree(faulting_assignment, reference_matcher);
+
+  // ref points to the latest reference to 'x' in our faulting expression
+  // 'x++', 'x = x + 1', 'x &= x + 1'
+  //  ^          ^             ^
+  //  We need this to know from where to start searching for later references
+  //  to 'x', and decide whether the AutoFix is safe or not
+  const verible::Symbol *ref =
+      std::find_if(std::rbegin(references_), std::rend(references_),
+                   [&](const verible::TreeSearchMatch &m) {
+                     return verible::StringSpanOfSymbol(*m.match) == lhs_id;
+                   })
+          ->match;
+
+  // Shouldn't happen, sanity check to avoid crashes
+  if (!ref) return false;
+
+  // Find the last reference to 'x' in the faulting assignment
+  itr = std::find_if(
+      std::begin(references), std::end(references),
+      [&](const ReferenceWithId &r) { return r.match.match == ref; });
+
+  // Search from the last reference to 'x' onwards. If there is any,
+  // we can't apply the autofix safely
+  itr = std::find_if(
+      std::next(itr), std::end(references),
+      [&](const ReferenceWithId &ref) { return ref.id == lhs_id; });
+
+  // Let's say 'x' is affected by a flagged operation { 'x = 1', 'x++', ... }
+  // We can safely autofix if after the flagged operation there are no
+  // more references to 'x'
+  return references.end() == itr;
+}
+
 }  // namespace analysis
 }  // namespace verilog

verilog/analysis/checkers/always_ff_non_blocking_rule.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.
@@ -15,12 +15,12 @@
 #ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_ALWAYS_FF_NON_BLOCKING_RULE_H_
 #define VERIBLE_VERILOG_ANALYSIS_CHECKERS_ALWAYS_FF_NON_BLOCKING_RULE_H_
 
-#include <set>
 #include <stack>
 #include <string>
 
 #include "common/analysis/lint_rule_status.h"
 #include "common/analysis/syntax_tree_lint_rule.h"
+#include "common/analysis/syntax_tree_search.h"
 #include "common/text/symbol.h"
 #include "common/text/syntax_tree_context.h"
 #include "verilog/analysis/descriptions.h"
@@ -45,6 +45,16 @@ class AlwaysFFNonBlockingRule : public verible::SyntaxTreeLintRule {
   bool InsideBlock(const verible::Symbol &symbol, int depth);
   // Processes local declarations
   bool LocalDeclaration(const verible::Symbol &symbol);
+  // Check the need of parenthesis in certain autofixes
+  // Might have false positives. Example:
+  // Fixing 'x *= y + 1' requires adding parenthesis
+  // 'x <= x & (y + 1)'
+  bool NeedsParenthesis(const verible::Symbol &rhs) const;
+  // Collect local references inside an always_ff block
+  void CollectLocalReferences(const verible::Symbol &root);
+  // Check if is safe to autofix
+  bool IsAutoFixSafe(const verible::Symbol &faulting_assignment,
+                     absl::string_view lhs_id) const;
 
  private:
   // Collected violations.
@@ -69,6 +79,16 @@ class AlwaysFFNonBlockingRule : public verible::SyntaxTreeLintRule {
 
   // In-order stack of local variable names
   std::vector<absl::string_view> locals_;
+
+  // NodekReference + string representation
+  // of the ID they're referring to
+  struct ReferenceWithId {
+    verible::TreeSearchMatch match;
+    absl::string_view id;
+  };
+  // Collection of references to variables
+  // assumed to be in program order
+  std::vector<ReferenceWithId> references;
 };
 
 }  // namespace analysis

verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc

@@ -30,6 +30,7 @@ namespace analysis {
 namespace {
 
 using verible::LintTestCase;
+using verible::RunApplyFixCases;
 using verible::RunConfiguredLintTestCases;
 using verible::RunLintTestCases;
 
@@ -120,13 +121,91 @@ TEST(AlwaysFFNonBlockingRule, LocalWaiving) {
        {kToken, "a"},
        "= b;\n"
        "end\nend\nendmodule"},
+      // Waive, 'a' is a local declaration
+      {"module m;\nalways_ff @(posedge c) begin static type(b) a; a = b; "
+       "end\nendmodule"},
+      {"module m;\nalways_ff @(posedge c) begin static type(b) a; a++; "
+       "end\nendmodule"},
+      {"module m;\nalways_ff @(posedge c) begin static type(b) a; ++a; "
+       "end\nendmodule"},
+      {"module m;\nalways_ff @(posedge c) begin static type(b) a; a &= b; "
+       "end\nendmodule"},
   };
 
   RunConfiguredLintTestCases<VerilogAnalyzer, AlwaysFFNonBlockingRule>(
       kAlwaysFFNonBlockingTestCases,
       "catch_modifying_assignments:on;waive_for_locals:on");
 }
 
+TEST(AlwaysFFNonBlockingRule, AutoFixDefault) {
+  const std::initializer_list<verible::AutoFixInOut> kTestCases = {
+      // Check we're not waiving local variables
+      {"module m;\nalways_ff begin reg k; k = 1; end\nendmodule",
+       "module m;\nalways_ff begin reg k; k <= 1; end\nendmodule"},
+      // Check we're ignoring modifying assignments
+      {"module m;\nalways_ff begin k &= 1; k = 1; end\nendmodule",
+       "module m;\nalways_ff begin k &= 1; k <= 1; end\nendmodule"},
+  };
+
+  RunApplyFixCases<VerilogAnalyzer, AlwaysFFNonBlockingRule>(kTestCases, "");
+}
+
+TEST(AlwaysFFNonBlockingRule, AutoFixCatchModifyingAssignments) {
+  const std::initializer_list<verible::AutoFixInOut> kTestCases = {
+      {"module m;\nalways_ff begin k &= 1; end\nendmodule",
+       "module m;\nalways_ff begin k <= k & 1; end\nendmodule"},
+      {"module m;\nalways_ff begin k &= a; end\nendmodule",
+       "module m;\nalways_ff begin k <= k & a; end\nendmodule"},
+      {"module m;\nalways_ff begin k |= (2 + 1); end\nendmodule",
+       "module m;\nalways_ff begin k <= k | (2 + 1); end\nendmodule"},
+      {"module m;\nalways_ff begin k *= 2 + 1; end\nendmodule",
+       "module m;\nalways_ff begin k <= k * (2 + 1); end\nendmodule"},
+      {"module m;\nalways_ff begin a++; end\nendmodule",
+       "module m;\nalways_ff begin a <= a + 1; end\nendmodule"},
+      {"module m;\nalways_ff begin ++a; end\nendmodule",
+       "module m;\nalways_ff begin a <= a + 1; end\nendmodule"},
+  };
+
+  RunApplyFixCases<VerilogAnalyzer, AlwaysFFNonBlockingRule>(
+      kTestCases, "catch_modifying_assignments:on");
+}
+
+TEST(AlwaysFFNonBlockingRule, AutoFixDontBreakCode) {
+  const std::initializer_list<verible::AutoFixInOut> kTestCases = {
+      // We can't fix 'k &= 1', because it would affect
+      // 'p <= k'
+      {"module m;\nalways_ff begin\n"
+       "k &= 1;\n"
+       "p <= k;\n"
+       "a++;"
+       "end\nendmodule",
+       "module m;\nalways_ff begin\n"
+       "k &= 1;\n"
+       "p <= k;\n"
+       "a <= a + 1;"
+       "end\nendmodule"},
+      // Correctly fix despide there is a reference to 'k' in the rhs
+      {"module m;\nalways_ff begin k = k + 1; end\nendmodule",
+       "module m;\nalways_ff begin k <= k + 1; end\nendmodule"},
+      // TODO: Add more tests
+  };
+
+  RunApplyFixCases<VerilogAnalyzer, AlwaysFFNonBlockingRule>(
+      kTestCases, "catch_modifying_assignments:on");
+}
+
+TEST(AlwaysFFNonBlockingRule, AutoFixWaiveLocals) {
+  const std::initializer_list<verible::AutoFixInOut> kTestCases = {
+      // Check that we're correctly waiving the 'p = 0' as it is a local
+      // variable
+      {"module m;\nalways_ff begin reg p; p = 0; k = 1; end\nendmodule",
+       "module m;\nalways_ff begin reg p; p = 0; k <= 1; end\nendmodule"},
+  };
+
+  RunApplyFixCases<VerilogAnalyzer, AlwaysFFNonBlockingRule>(
+      kTestCases, "waive_for_locals:on");
+}
+
 }  // namespace
 }  // namespace analysis
 }  // namespace verilog