From 0ca5570216fa35a9a63ed55eb0310fe3e421483e Mon Sep 17 00:00:00 2001 From: IEncinas10 Date: Sun, 7 May 2023 20:54:39 +0200 Subject: [PATCH] Add autofixes for always_ff_non_blocking_rule --- verilog/analysis/checkers/BUILD | 2 + .../checkers/always_ff_non_blocking_rule.cc | 173 ++++++++++++++++-- .../checkers/always_ff_non_blocking_rule.h | 24 ++- .../always_ff_non_blocking_rule_test.cc | 79 ++++++++ 4 files changed, 261 insertions(+), 17 deletions(-) diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index 1b06db65ab..797ec8d245 100644 --- a/verilog/analysis/checkers/BUILD +++ b/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", diff --git a/verilog/analysis/checkers/always_ff_non_blocking_rule.cc b/verilog/analysis/checkers/always_ff_non_blocking_rule.cc index 4d3fabb6ca..7191ce81e7 100644 --- a/verilog/analysis/checkers/always_ff_non_blocking_rule.cc +++ b/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 #include +#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,6 +28,8 @@ #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" @@ -34,6 +37,7 @@ 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 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(&symbol)) { - check_root = - /* lhs */ verible::down_cast( - 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(&symbol)) { - check_root = - /* lhs */ verible::down_cast( - 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 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::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 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 diff --git a/verilog/analysis/checkers/always_ff_non_blocking_rule.h b/verilog/analysis/checkers/always_ff_non_blocking_rule.h index 5e041bdc19..05d4a78600 100644 --- a/verilog/analysis/checkers/always_ff_non_blocking_rule.h +++ b/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 #include #include #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 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 references; }; } // namespace analysis diff --git a/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc b/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc index cec73323c8..094e530da8 100644 --- a/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc +++ b/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,6 +121,15 @@ 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( @@ -127,6 +137,75 @@ TEST(AlwaysFFNonBlockingRule, LocalWaiving) { "catch_modifying_assignments:on;waive_for_locals:on"); } +TEST(AlwaysFFNonBlockingRule, AutoFixDefault) { + const std::initializer_list 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(kTestCases, ""); +} + +TEST(AlwaysFFNonBlockingRule, AutoFixCatchModifyingAssignments) { + const std::initializer_list 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( + kTestCases, "catch_modifying_assignments:on"); +} + +TEST(AlwaysFFNonBlockingRule, AutoFixDontBreakCode) { + const std::initializer_list 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( + kTestCases, "catch_modifying_assignments:on"); +} + +TEST(AlwaysFFNonBlockingRule, AutoFixWaiveLocals) { + const std::initializer_list 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( + kTestCases, "waive_for_locals:on"); +} + } // namespace } // namespace analysis } // namespace verilog