verilog: analysis: checkers: add autofixes for always_ff_non_blocking_rule

Offer autofixes without modifying program behaviour by taking care of
corner cases such as:

  always_ff @(posedge clk) begin
    x++;
    y <= x;
  end

which can't be converted to

  always_ff @(posedge clk) begin
    x <= x + 1;
    y <= x;
  end

Similarly, parenthesis are inserted where they might be needed:

  a *= b + 1;

becomes

  a <= b * (k + 1);

Author: IEncinas10

verible/verilog/analysis/checkers/BUILD

@@ -1229,13 +1229,17 @@ cc_library(
         "//verible/common/text:config-utils",
         "//verible/common/text:symbol",
         "//verible/common/text:syntax-tree-context",
+        "//verible/common/text:tree-utils",
         "//verible/common/util:casts",
         "//verible/common/util:logging",
+        "//verible/verilog/CST:expression",
+        "//verible/verilog/CST:statement",
         "//verible/verilog/CST:verilog-matchers",
         "//verible/verilog/CST:verilog-nonterminals",
         "//verible/verilog/analysis:descriptions",
         "//verible/verilog/analysis:lint-rule-registry",
         "@com_google_absl//absl/status",
+        "@com_google_absl//absl/strings",
         "@com_google_absl//absl/strings:string_view",
     ],
     alwayslink = 1,

verible/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.
@@ -15,10 +15,14 @@
 #include "verible/verilog/analysis/checkers/always-ff-non-blocking-rule.h"
 
 #include <algorithm>
+#include <iterator>
 #include <ostream>
 #include <set>
+#include <string>
+#include <vector>
 
 #include "absl/status/status.h"
+#include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
 #include "verible/common/analysis/lint-rule-status.h"
 #include "verible/common/analysis/matcher/bound-symbol-manager.h"
@@ -29,8 +33,11 @@
 #include "verible/common/text/config-utils.h"
 #include "verible/common/text/symbol.h"
 #include "verible/common/text/syntax-tree-context.h"
+#include "verible/common/text/tree_utils.h"
 #include "verible/common/util/casts.h"
 #include "verible/common/util/logging.h"
+#include "verible/verilog/CST/expression.h"
+#include "verible/verilog/CST/statement.h"
 #include "verible/verilog/CST/verilog-matchers.h"
 #include "verible/verilog/CST/verilog-nonterminals.h"
 #include "verible/verilog/analysis/descriptions.h"
@@ -39,6 +46,7 @@
 namespace verilog {
 namespace analysis {
 
+using verible::AutoFix;
 using verible::LintRuleStatus;
 using verible::LintViolation;
 using verible::SearchSyntaxTree;
@@ -102,31 +110,78 @@ 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->front().get());
-    }
+    const verible::SyntaxTreeNode *node = &verible::SymbolCastToNode(symbol);
+    check_root = verilog::GetNetVariableAssignmentLhs(*node);
+    lhs_id = verible::StringSpanOfSymbol(*check_root);
+
+    const verible::SyntaxTreeLeaf *equals =
+        verilog::GetNetVariableAssignmentOperator(*node);
+
+    autofixes.emplace_back(AutoFix(
+        "Substitute blocking assignment '=' for nonblocking assignment '<='",
+        {equals->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->front().get());
-      }
+      const verible::SyntaxTreeNode *node = &verible::SymbolCastToNode(symbol);
+      const verible::SyntaxTreeNode &lhs = *GetAssignModifyLhs(*node);
+      const verible::SyntaxTreeNode &rhs = *GetAssignModifyRhs(*node);
+
+      lhs_id = verible::StringSpanOfSymbol(lhs);
+      check_root = &lhs;
+
+      bool needs_parenthesis = NeedsParenthesis(rhs);
+      absl::string_view start_rhs_expr = needs_parenthesis ? " (" : " ";
+      absl::string_view end_rhs_expr = needs_parenthesis ? ");" : ";";
+
+      // Extract just the operation. Just '+' from '+='
+      const absl::string_view op =
+          verible::StringSpanOfSymbol(*GetAssignModifyOperator(*node))
+              .substr(0, 1);
+
+      const std::string fix =
+          absl::StrCat(lhs_id, " <= ", lhs_id, " ", op, start_rhs_expr,
+                       verible::StringSpanOfSymbol(rhs), end_rhs_expr);
+
+      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);
+
+      check_root = operand;
+      lhs_id = verible::StringSpanOfSymbol(*operand);
+
+      // Extract just the operation. Just '+' from '++'
+      const absl::string_view op =
+          verible::StringSpanOfSymbol(*GetIncrementDecrementOperator(symbol))
+              .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;
@@ -160,7 +215,16 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol,
   }
 
   // Enqueue detected violation unless waived
-  if (!waived) violations_.insert(LintViolation(symbol, kMessage, context));
+  if (waived) return;
+
+  // Dont autofix if the faulting expression is inside an expression
+  // Example: "p <= k++"
+  if (IsAutoFixSafe(symbol, lhs_id) &&
+      !context.IsInside(NodeEnum::kExpression)) {
+    violations_.insert(LintViolation(symbol, kMessage, context, autofixes));
+  } else {
+    violations_.insert(LintViolation(symbol, kMessage, context));
+  }
 }  // HandleSymbol()
 
 bool AlwaysFFNonBlockingRule::InsideBlock(const verible::Symbol &symbol,
@@ -185,6 +249,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;
   }
@@ -227,5 +292,96 @@ 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.
+  const bool complex_rhs_expr =
+      verible::GetLeftmostLeaf(rhs) != verible::GetRightmostLeaf(rhs);
+  if (!complex_rhs_expr) return false;
+
+  // Check if expression is wrapped in parenthesis
+  const verible::Symbol *inner = verilog::UnwrapExpression(rhs);
+  if (inner->Kind() == verible::SymbolKind::kLeaf) return false;
+
+  return !verible::SymbolCastToNode(*inner).MatchesTag(NodeEnum::kParenGroup);
+}
+
+void AlwaysFFNonBlockingRule::CollectLocalReferences(
+    const verible::Symbol &root) {
+  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
+
+  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

verible/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.
@@ -24,6 +24,7 @@
 #include "absl/strings/string_view.h"
 #include "verible/common/analysis/lint-rule-status.h"
 #include "verible/common/analysis/syntax-tree-lint-rule.h"
+#include "verible/common/analysis/syntax_tree_search.h"
 #include "verible/common/text/symbol.h"
 #include "verible/common/text/syntax-tree-context.h"
 #include "verible/verilog/analysis/descriptions.h"
@@ -48,6 +49,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.
@@ -72,6 +83,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