diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index 3cc23395f..2fbe640d1 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -1229,13 +1229,17 @@ cc_library( "//common/text:config-utils", "//common/text:symbol", "//common/text:syntax-tree-context", + "//common/text:tree-utils", "//common/util:casts", "//common/util:logging", + "//verilog/CST:expression", + "//verilog/CST:statement", "//verilog/CST:verilog-matchers", "//verilog/CST:verilog-nonterminals", "//verilog/analysis:descriptions", "//verilog/analysis:lint-rule-registry", "@com_google_absl//absl/status", + "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:string_view", ], alwayslink = 1, diff --git a/verilog/analysis/checkers/always_ff_non_blocking_rule.cc b/verilog/analysis/checkers/always_ff_non_blocking_rule.cc index 67b833053..e4db3bd22 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. @@ -15,10 +15,14 @@ #include "verilog/analysis/checkers/always_ff_non_blocking_rule.h" #include +#include #include #include +#include +#include #include "absl/status/status.h" +#include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "common/analysis/lint_rule_status.h" #include "common/analysis/matcher/bound_symbol_manager.h" @@ -29,8 +33,11 @@ #include "common/text/config_utils.h" #include "common/text/symbol.h" #include "common/text/syntax_tree_context.h" +#include "common/text/tree_utils.h" #include "common/util/casts.h" #include "common/util/logging.h" +#include "verilog/CST/expression.h" +#include "verilog/CST/statement.h" #include "verilog/CST/verilog_matchers.h" #include "verilog/CST/verilog_nonterminals.h" #include "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 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->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(&symbol)) { - check_root = - /* lhs */ verible::down_cast( - 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 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 + + 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 b875d2bb5..78fd39aa4 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. @@ -24,6 +24,7 @@ #include "absl/strings/string_view.h" #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" @@ -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 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 098d90fac..c893a80f5 100644 --- a/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc +++ b/verilog/analysis/checkers/always_ff_non_blocking_rule_test.cc @@ -27,6 +27,7 @@ namespace analysis { namespace { using verible::LintTestCase; +using verible::RunApplyFixCases; using verible::RunConfiguredLintTestCases; using verible::RunLintTestCases; @@ -117,6 +118,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( @@ -124,6 +134,96 @@ 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 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", + 0, 1}, + {"module m;\nalways_ff begin\n" + "k &= 1;\n" // First violation, can't be autofixed + "p <= k;\n" + "a++;" + "end\nendmodule", + "module m;\nalways_ff begin\n" + "k &= 1;\n" + "p <= k;\n" + "a <= a + 1;" + "end\nendmodule", + verible::AutoFixInOut::NO_FIX_AVAILABLE, 0}, + // Correctly fix despite 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"}, + // Dont autofix inside expressions + {"module m;\nalways_ff begin k <= p++; p++; end\nendmodule", + "module m;\nalways_ff begin k <= p++; p <= p + 1; end\nendmodule", 0, 1}, + {"module m;\nalways_ff begin k <= p++; p++; end\nendmodule", + "module m;\nalways_ff begin k <= p++; p <= p + 1; end\nendmodule", + verible::AutoFixInOut::NO_FIX_AVAILABLE, 0}, + }; + + 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